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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 8280 bytes --]

On 17.08.20 16:32, Kevin Wolf wrote:
> Am 17.08.2020 um 15:51 hat Max Reitz geschrieben:
>> 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.
> 
> Makes things a lot easier.
> 
> Maybe I should try to move patch 15 earlier. The series is mostly just
> in the order that I wrote things, but there were also a few nasty
> dependencies in the part the generalises things from NBD to BlockExport.
> So I'm not sure if this is a patch that can be moved.
> 
>>>> 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.
> 
> So should I or not? :-)

Can we delay it until after this series?  I.e., as long as it retains
the name “writable”, would pulling it up into BlockExportOptions a
compatible change?

If so, then I suppose we could do it afterwards.  But I think it does
make the most sense to “just” do it as part of this series.

>> 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.)
> 
> This makes sense. Nobody likes having to rewrite their RFC series.
> 
> But there is one thing I dread even more: Polishing the RFC series for
> another week until I can send it out as non-RFC and _then_ having to
> rewrite it.

Yes.  Especially bad with tests.

>> 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.)
> 
> Basically just telling you that I was already interested in your input
> for this point specifically when I sent the series.

OK :)

>> 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...)
> 
> I usually send RFC patches when I know that I wouldn't consider them
> mergable yet, but I don't want to invest the time to polish them before
> I know that other people agree with the approach and the time won't be
> wasted.
> 
>> 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?
> 
> In many cases (and I think this is one of them in large parts), I only
> really learn what the series will look like when I write it.

That’s true.  With a pure design RFC, it’s often difficult to know even
the scope of the design until you’ve begun to write code.  So there’s a
danger of just writing a bunch of uncontroversial basic design stuff
because one has no idea of what may actually become problematic and
questionable. :/

> I could have sent a design RFC for the QAPI part, but I didn't expect
> this to be contentious because it's just the normal add/del/query thing
> that exists for pretty much everything else, too.

Yeah, the functions themselves are clear.

Hm.  Perhaps software engineering just is actually difficult, and
there’s no way around it.

>>>> 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.
> 
> I can try. I seem to remember that you had a reason not to do this the
> last time we discussed generalised exports, but I'm not sure what it
> was.
> 
> The obvious one could be that the block export layer doesn't know which
> permissions are needed. But it can always start with minimal permissions
> and let the driver do a blk_set_perm() if it needs more.

Trying sounds good.  Since there shouldn’t be consequences for the QMP
interface, we™ can always try again later (i.e., when adding more export
types).

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]