Re: [Qemu-devel] [PATCH v2 2/3] nbd/server: implement dirty bitmap export

[Eric Blake]

On 04/13/2018 01:14 PM, Vladimir Sementsov-Ogievskiy wrote:
> Handle new NBD meta namespace: "qemu", and corresponding queries:
> "qemu:dirty-bitmap:<export bitmap name>".
> 
> With new metadata context negotiated, BLOCK_STATUS query will reply
> with dirty-bitmap data, converted to extents. New public function
> nbd_export_bitmap selects bitmap to export. For now, only one bitmap
> may be exported.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>  include/block/nbd.h |   6 ++
>  nbd/server.c        | 234 
> ++++++++++++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 224 insertions(+), 16 deletions(-)
> 

>  
> +/* nbd_meta_bitmap_query
> + *
> + * Handle query to 'qemu-dirty-bitmap' namespace.

Stale comment

> + * 'len' is the amount of text remaining to be read from the current name, 
> after
> + * the 'qemu-dirty-bitmap:' portion has been stripped.

and again

> + *
> + * Return -errno on I/O error, 0 if option was completely handled by
> + * sending a reply about inconsistent lengths, or 1 on success. */
> +static int nbd_meta_bitmap_query(NBDClient *client, NBDExportMetaContexts 
> *meta,
> +                                 uint32_t len, Error **errp)
> +{
> +    if (!client->exp->export_bitmap) {
> +        return nbd_opt_skip(client, len, errp);
> +    }
> +
> +    return nbd_meta_single_query(
> +            client, client->exp->export_bitmap_context + strlen("qemu:"), 
> len,
> +            &meta->dirty_bitmap, errp);

Wait, so client->exp->export_bitmap_context is stored WITH the "qemu:"
prefix as part of the name?

During _LIST, you are allowing a query of "qemu:" to list all
qemu-specific contexts (namely, qemu:dirty-bitmap:<export-name>); but
should we also make it work that if you do "qemu:dirty-bitmap:", you get
a list of just dirty-bitmap contexts even if "qemu:" has added other
contexts in the meantime?

> @@ -1800,6 +1851,94 @@ static int nbd_co_send_block_status(NBDClient *client, 
> uint64_t handle,
>      return nbd_co_send_extents(client, handle, &extent, 1, context_id, errp);
>  }
>  
> +/* Set several extents, describing region of given @length with given @flags.
> + * Do not set more than @nb_extents, return number of set extents.
> + */
> +static unsigned add_extents(NBDExtent *extents, unsigned nb_extents,
> +                            uint64_t length, uint32_t flags)
> +{
> +    unsigned i = 0;
> +    uint32_t max_extent = QEMU_ALIGN_DOWN(INT32_MAX, 512);
> +    /* TODO: should be somehow in agreement with blocksize of OPT_GO,
> +     * and this should be documented in NBD proto. */

nbd.git commit 218c446 documented that descriptor lengths MUST be an
integer multiple of minimum block size; as for maximum, I see no reason
why BLOCK_STATUS can't report larger extents than what NBD_CMD_READ can
do.  Do we need further doc tweaks to nbd.git?

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

signature.asc
Description: OpenPGP digital signature