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
signature.asc
Description: OpenPGP digital signature
