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

[Kevin Wolf]

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? :-)

> 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.

> 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.

> 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.

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.

> >> 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.

Kevin

signature.asc
Description: PGP signature