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