Re: [RFC PATCH 09/22] nbd: Add writethrough to block-export-add

[Max Reitz]

On 17.08.20 15:13, Kevin Wolf wrote:
> Am 17.08.2020 um 14:56 hat Max Reitz geschrieben:
>> On 13.08.20 18:29, Kevin Wolf wrote:
>>> qemu-nbd allows use of writethrough cache modes, which mean that write
>>> requests made through NBD will cause a flush before they complete.
>>> Expose the same functionality in block-export-add.
>>>
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> ---
>>>  qapi/block-export.json | 7 ++++++-
>>>  blockdev-nbd.c         | 2 +-
>>>  2 files changed, 7 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/qapi/block-export.json b/qapi/block-export.json
>>> index 1fdc55c53a..4ce163411f 100644
>>> --- a/qapi/block-export.json
>>> +++ b/qapi/block-export.json
>>> @@ -167,10 +167,15 @@
>>>  # Describes a block export, i.e. how single node should be exported on an
>>>  # external interface.
>>>  #
>>> +# @writethrough: If true, caches are flushed after every write request to 
>>> the
>>> +#                export before completion is signalled. (since: 5.2;
>>> +#                default: false)
>>> +#
>>>  # Since: 4.2
>>>  ##
>>>  { 'union': 'BlockExportOptions',
>>> -  'base': { 'type': 'BlockExportType' },
>>> +  'base': { 'type': 'BlockExportType',
>>> +            '*writethrough': 'bool' },
>>>    'discriminator': 'type',
>>>    'data': {
>>>        'nbd': 'BlockExportOptionsNbd'
>>
>> Hm.  I find it weird to have @writethrough in the base but @device in
>> the specialized class.
>>
>> I think everything that will be common to all block exports should be in
>> the base, and that probably includes a node-name.  I’m aware that will
>> make things more tedious in the code, but perhaps it would be a nicer
>> interface in the end.  Or is the real problem that that would create
>> problems in the storage daemon’s command line interface, because then
>> the specialized (legacy) NBD interface would no longer be compatible
>> with the new generalized block export interface?
> 
> Indeed. I think patch 15 has what you're looking for.

Great. :)

Discussions where both participants have the same opinion from the start
are the best ones.

>> Anyway, @writable might be a similar story.  A @read-only may make sense
>> in general, I think.
> 
> Pulling @writable up is easier than a @read-only, but that's a naming
> detail.

Sure.

> In general I agree, but this part isn't addressed in this series yet.
> Part of the reason why this is an RFC was to find out if I should
> include things like this or if we'll do it when we add another export
> type or common functionality that needs the same option.

Sure, sure.


Meta: I personally don’t like RFC patches very much.  RFC to me means
everything is fair game, and reviewers should be free to let their
thoughts wander and come up with perhaps wild ideas, just trying to
gauge what everyone thinks.

When I’m the submitter, I tend to get defensive then, because I’ve
invested time in writing the code already, so I tend to be biased
against fundamental changes.  (Horrible personal trait.  I’m working on it.)

As a reviewer, the code and thus some fleshed out design is there
already, so it’s difficult to break free from that and find completely
different solutions to the original problem.
(I kind of ventured in that direction for this patch, and it seems like
you immediately noticed that my response was different from usual and
pointed out the RFC status, perhaps to make me feel more comfortable in
questioning the fundamental design more.  Which I noticed, hence this
wall of text.)

Perhaps I’m wrong.  Perhaps it’s just myself (the points I’ve just
listed are definitely my own personal weaknesses), but I can’t help but
project and assume that others may feel similar, at least in part.
So I feel like RFCs that consist of patches tend to at least lock me in
to the solution that’s present.  I find them difficult to handle, both
as a submitter and as a reviewer.

All in all, that means on either side I tend to handle patch RFCs as
“Standard series, just tests missing”.  Not sure if that’s ideal.  Or
maybe that’s exactly what patch RFCs are?

(Of course, it can and should be argued that even for standard series, I
shouldn’t be afraid of questioning the fundamental design still.  But
that’s hard...)


But, well.  The alternative is writing pure design RFCs, and then you
tend to get weeks of slow discussion, drawing everything out.  Which
isn’t ideal either.  Or is that just a baseless prejudice I have?

>> Basically, I think that the export code should be separate from the code
>> setting up the BlockBackend that should be exported, so all options
>> regarding that BB should be common; and those options are @node-name,
>> @writethrough, and @read-only.  (And perhaps other things like
>> @resizable, too, even though that isn’t something to consider for NBD.)
> 
> Do you mean that the BlockBackend should already be created by the
> generic block export layer?

It would certainly be nice, if it were feasible, don’t you think?

We don’t have to bend backwards for it, but maybe it would force us to
bring the natural separation of block device and export parameters to
the interface.

Max

signature.asc
Description: OpenPGP digital signature