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

[Eric Blake <eblake@redhat.com>]

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.

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

> +     */
> +    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?

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.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org