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

[Vladimir Sementsov-Ogievskiy]

18.04.2018 17:05, Eric Blake wrote:
> 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?

reasonable.

> > @@ -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?

ok, missed it.

--
Best regards,
Vladimir