Re: [PATCH v2 4/5] nbd: Add new qemu:allocation-depth metacontext

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5528 bytes --]

On 10/7/20 8:40 AM, Vladimir Sementsov-Ogievskiy wrote:
> 30.09.2020 15:11, Eric Blake wrote:
>> 'qemu-img map' provides a way to determine which extents of an image
>> come from the top layer vs. inherited from a backing chain.  This is
>> useful information worth exposing over NBD.  There is a proposal to
>> add a QMP command block-dirty-bitmap-populate which can create a dirty
>> bitmap that reflects allocation information, at which point
>> qemu:dirty-bitmap:NAME can expose that information via the creation of
>> a temporary bitmap, but we can shorten the effort by adding a new
>> qemu:allocation-depth context that does the same thing without an
>> intermediate bitmap (this patch does not eliminate the need for that
>> proposal, as it will have other uses as well).
>>
>> For this patch, I just encoded a tri-state value (unallocated, from
>> this layer, from any of the backing layers); an obvious extension
>> would be to provide the actual depth in bits 31-4 while keeping bits
>> 1-0 as a tri-state (leaving bits 3-2 unused, for ease of reading depth
>> from a hex number).  But this extension would require
>> bdrv_is_allocated_above to return a depth number.
>>
>> Note that this patch does not actually enable any way to request a
>> server to enable this context; that will come in the next patch.
>>

>> +In the allocation depth context, bits 0 and 1 form a tri-state value:
>> +
>> +    bits 0-1 clear: NBD_STATE_DEPTH_UNALLOC, means the extent is
>> unallocated
>> +    bit 0 set: NBD_STATE_DEPTH_LOCAL, the extent is allocated in this
>> image
> 
> Maybe, s/this/the top level/, as, what is "this" for NBD client?

Sure.


>> @@ -864,12 +867,21 @@ static bool nbd_meta_qemu_query(NBDClient
>> *client, NBDExportMetaContexts *meta,
>>
>>       if (!*query) {

We get here for "qemu:".

>>           if (client->opt == NBD_OPT_LIST_META_CONTEXT) {
>> +            meta->allocation_depth = meta->exp->alloc_context;
>>               meta->bitmap = !!meta->exp->export_bitmap;
>>           }
>>           trace_nbd_negotiate_meta_query_parse("empty");
>>           return true;
>>       }
>>
>> +    if (nbd_strshift(&query, "allocation-depth")) {

We get here for "qemu:allocation-depth", and as you pointed out,
"qemu:allocation-depth-extended".

>> +        trace_nbd_negotiate_meta_query_parse("allocation-depth");
>> +        if (nbd_meta_empty_or_pattern(client, "", query)) {
> 
> How much it differs from "if (!*query) {", I don't see...

The nbd_meta_empty_or_pattern returns false for
"qemu:allocation-depth-extended"; it only returns true for
"qemu:allocation-depth".  But, as you pointed out,

> 
>> +            meta->allocation_depth = meta->exp->alloc_context;
>> +        }
>> +        return true;
>> +    }
> 
> why not just
> 
>  if (!strcmp(query, "allocation-depth")) {
>     meta->allocation_depth = meta->exp->alloc_context;
>     return true;
>  }
> 
> ?

that does seem shorter.

> 
> With your code you also parse something like
> "allocation-depth-extended".. Is it on purpose?

The string is parsed, but does not affect meta->XXX, which means nothing
gets advertised to the client.  The trace messages might differ, but
behavior is correct.

> 
>> +
>>       if (nbd_strshift(&query, "dirty-bitmap:")) {
>>           trace_nbd_negotiate_meta_query_parse("dirty-bitmap:");
>>           if (!meta->exp->export_bitmap) {
> 
> 
> Also, "trace_nbd_negotiate_meta_query_skip("not dirty-bitmap"); " at
> function end should
> now be something like trace_nbd_negotiate_meta_query_skip("unknown
> context in qemu: namespace");

Good idea.


>> +/* Get allocation depth from the exported device and send it to the
>> client */
>> +static int nbd_co_send_block_alloc(NBDClient *client, uint64_t handle,
>> +                                   BlockDriverState *bs, uint64_t
>> offset,
>> +                                   uint32_t length, bool dont_fragment,
>> +                                   bool last, uint32_t context_id,
>> +                                   Error **errp)
>> +{
>> +    int ret;
>> +    unsigned int nb_extents = dont_fragment ? 1 :
>> NBD_MAX_BLOCK_STATUS_EXTENTS;
>> +    g_autoptr(NBDExtentArray) ea = nbd_extent_array_new(nb_extents);
>> +
>> +    ret = blockalloc_to_extents(bs, offset, length, ea);
>> +    if (ret < 0) {
>> +        return nbd_co_send_structured_error(
>> +                client, handle, -ret, "can't get block status", errp);
>> +    }
>> +
>> +    return nbd_co_send_extents(client, handle, ea, last, context_id,
>> errp);
>> +}
> 
> 
> The function is a copy of nbd_co_send_block_status()..
> 
> Probably, we can reuse nbd_co_send_block_status(), and just call
> blockstatus_to_extents()
> or blockalloc_to_extents() depending on context_id parameter.

Nice idea to reduce the duplication.

> 
> Still, I'm OK with it as is.
> 

So is that Reviewed-by:, or do I need to post v3 with my tweaks in
response to your comments?

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]