Re: [RFC PATCH 07/22] block/export: Remove magic from block-export-add

[Kevin Wolf]

Am 19.08.2020 um 21:50 hat Eric Blake geschrieben:
> cc: Peter Krempa
> 
> On 8/13/20 11:29 AM, Kevin Wolf wrote:
> > nbd-server-add tries to be convenient and adds two questionable
> > features that we don't want to share in block-export-add, even for NBD
> > exports:
> > 
> > 1. When requesting a writable export of a read-only device, the export
> >     is silently downgraded to read-only. This should be an error in the
> >     context of block-export-add.
> 
> I'd be happy for this to be an error even with nbd-export-add; I don't think
> it would harm any of libvirt's existing usage (either for storage migration,
> or for incremental backups).
> 
> Side note: In the past, I had a proposal to enhance the NBD Protocol to
> allow a client to advertise to the server its intent on being a read-only or
> read-write client.  Not relevant to this patch, but this part of the commit
> message reminds me that I should revisit that topic (Rich and I recently hit
> another case in nbdkit where such an extension would be nice, when it comes
> to using NBD's multi-conn for better performance on a read-only connection,
> but only if the server knows the client intends to be read-only)
> 
> > 
> > 2. When using a BlockBackend name, unplugging the device from the guest
> >     will automatically stop the NBD server, too. This may sometimes be
> >     what you want, but it could also be very surprising. Let's keep
> >     things explicit with block-export-add. If the user wants to stop the
> >     export, they should tell us so.
> 
> Here, keeping the nbd command different from the block-export command seems
> tolerable.  On the other hand, I wonder if Peter needs to change anything in
> libvirt's incremental backup code to handle this sudden disappearance of an
> NBD device during a disk hot-unplug (that is, either the presence of an
> ongoing pull-mode backup should block disk unplug, or libvirt needs a way to
> guarantee that an ongoing backup NBD device remains in spite of subsequent
> disk actions on the guest).  Depending on libvirt's needs, we may want to
> revisit the nbd command to have the same policy as block-export-add, plus an
> introspectible feature notation.

As long as we can keep the compatibility code local to qmp_nbd_*(), I
don't think it's too bad. In particular because it's already written.

Instead of adjusting libvirt to changes in the nbd-* commands, I'd
rather have it change over to block-export-*. I would like to see the
nbd-server-add/remove commands deprecated soon after we have the
replacements.

> > 
> > Move these things into the nbd-server-add QMP command handler so that
> > they apply only there.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >   include/block/nbd.h   |  3 ++-
> 
> > +void qmp_block_export_add(BlockExportOptions *export, Error **errp)
> > +{
> > +    blk_exp_add(export, errp);
> >   }
> >   void qmp_nbd_server_add(BlockExportOptionsNbd *arg, Error **errp)
> >   {
> > -    BlockExportOptions export = {
> > +    BlockExport *export;
> > +    BlockDriverState *bs;
> > +    BlockBackend *on_eject_blk;
> > +
> > +    BlockExportOptions export_opts = {
> >           .type = BLOCK_EXPORT_TYPE_NBD,
> >           .u.nbd = *arg,
> >       };
> > -    qmp_block_export_add(&export, errp);
> > +
> > +    /*
> > +     * nbd-server-add doesn't complain when a read-only device should be
> > +     * exported as writable, but simply downgrades it. This is an error 
> > with
> > +     * block-export-add.
> 
> I'd be happy with either marking this deprecated now (and fixing it in two
> releases), or declaring it a bug in nbd-server-add now (and fixing it
> outright).

How about deprecating nbd-server-add completely?

> > +     */
> > +    bs = bdrv_lookup_bs(arg->device, arg->device, NULL);
> > +    if (bs && bdrv_is_read_only(bs)) {
> > +        arg->writable = false;
> > +    }
> > +
> > +    export = blk_exp_add(&export_opts, errp);
> > +    if (!export) {
> > +        return;
> > +    }
> > +
> > +    /*
> > +     * nbd-server-add removes the export when the named BlockBackend used 
> > for
> > +     * @device goes away.
> > +     */
> > +    on_eject_blk = blk_by_name(arg->device);
> > +    if (on_eject_blk) {
> > +        nbd_export_set_on_eject_blk(export, on_eject_blk);
> > +    }
> 
> Wait - is the magic export removal tied only to exporting a drive name, and
> not a node name?  So as long as libvirt is using only node names whwen
> adding exports, a drive being unplugged won't interfere?

Yes, seems so. It's the existing behaviour, I'm only moving the code
around.

> Overall, the change makes sense to me, although I'd love to see if we could
> go further on the writable vs. read-only issue.

If nbd-server-add will be going away relatively soon, it's probably not
worth the trouble. But if you have reasons to keep it, maybe we should
consider it.

Kevin