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

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

08.10.2020 00:54, Eric Blake wrote:
> 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.
>>

it was only about the duplicated function

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

Honestly it wasn't.. So, I found some things to discuss, and it's possible that I've looked through with less attention. Hm. So, with v3 or without, I've have to lookthrough it again..

My suggested diff:

diff --git a/docs/interop/nbd.txt b/docs/interop/nbd.txt
index 56efec7aee..a0d91ff08b 100644
--- a/docs/interop/nbd.txt
+++ b/docs/interop/nbd.txt
@@ -36,7 +36,8 @@ the image, with a single context named:
  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
+    bit 0 set: NBD_STATE_DEPTH_LOCAL, the extent is allocated in the top
+               level image
      bit 1 set: NBD_STATE_DEPTH_BACKING, the extent is inherited from a
                 backing layer
  
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 06208bc250..2d7165960d 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -262,7 +262,7 @@ enum {
  /* Extent flags for qemu:allocation-depth in NBD_REPLY_TYPE_BLOCK_STATUS */
  #define NBD_STATE_DEPTH_UNALLOC (0 << 0)
  #define NBD_STATE_DEPTH_LOCAL   (1 << 0)
-#define NBD_STATE_DEPTH_BACKING (2 << 0)
+#define NBD_STATE_DEPTH_BACKING (1 << 1)
  #define NBD_STATE_DEPTH_MASK    (0x3)
  
  static inline bool nbd_reply_type_is_error(int type)
diff --git a/nbd/server.c b/nbd/server.c
index 830b21000b..3761ac180f 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -809,7 +809,7 @@ static bool nbd_meta_empty_or_pattern(NBDClient *client, const char *pattern,
  {
      if (!*query) {
          trace_nbd_negotiate_meta_query_parse("empty");
-        return !*pattern || client->opt == NBD_OPT_LIST_META_CONTEXT;
+        return client->opt == NBD_OPT_LIST_META_CONTEXT;
      }
      if (strcmp(query, pattern) == 0) {
          trace_nbd_negotiate_meta_query_parse(pattern);
@@ -874,11 +874,9 @@ static bool nbd_meta_qemu_query(NBDClient *client, NBDExportMetaContexts *meta,
          return true;
      }
  
-    if (nbd_strshift(&query, "allocation-depth")) {
+    if (!strcmp(query, "allocation-depth")) {
          trace_nbd_negotiate_meta_query_parse("allocation-depth");
-        if (nbd_meta_empty_or_pattern(client, "", query)) {
-            meta->allocation_depth = meta->exp->alloc_context;
-        }
+        meta->allocation_depth = meta->exp->alloc_context;
          return true;
      }
  
@@ -896,7 +894,7 @@ static bool nbd_meta_qemu_query(NBDClient *client, NBDExportMetaContexts *meta,
          return true;
      }
  
-    trace_nbd_negotiate_meta_query_skip("not dirty-bitmap");
+    trace_nbd_negotiate_meta_query_skip("unknown context in qemu: namespace");
      return true;
  }
  
@@ -2045,7 +2043,11 @@ static int nbd_co_send_extents(NBDClient *client, uint64_t handle,
      return nbd_co_send_iov(client, iov, 2, errp);
  }
  
-/* Get block status from the exported device and send it to the client */
+/*
+ * Get block status from the exported device and send it to the client,
+ * handling base:allocation or qemu:allocation-depth meta-context, based
+ * on @context_id value.
+ */
  static int nbd_co_send_block_status(NBDClient *client, uint64_t handle,
                                      BlockDriverState *bs, uint64_t offset,
                                      uint32_t length, bool dont_fragment,
@@ -2056,27 +2058,12 @@ static int nbd_co_send_block_status(NBDClient *client, uint64_t handle,
      unsigned int nb_extents = dont_fragment ? 1 : NBD_MAX_BLOCK_STATUS_EXTENTS;
      g_autoptr(NBDExtentArray) ea = nbd_extent_array_new(nb_extents);
  
-    ret = blockstatus_to_extents(bs, offset, length, ea);
-    if (ret < 0) {
-        return nbd_co_send_structured_error(
-                client, handle, -ret, "can't get block status", errp);
+    if (context_id == NBD_META_ID_BASE_ALLOCATION) {
+        ret = blockstatus_to_extents(bs, offset, length, ea);
+    } else {
+        assert(context_id == NBD_META_ID_ALLOCATION_DEPTH);
+        ret = blockalloc_to_extents(bs, offset, length, ea);
      }
-
-    return nbd_co_send_extents(client, handle, ea, last, context_id, errp);
-}
-
-/* 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);
@@ -2433,13 +2420,13 @@ static coroutine_fn int nbd_handle_request(NBDClient *client,
              }
  
              if (client->export_meta.allocation_depth) {
-                ret = nbd_co_send_block_alloc(client, request->handle,
-                                              blk_bs(exp->common.blk),
-                                              request->from, request->len,
-                                              dont_fragment,
-                                              !--contexts_remaining,
-                                              NBD_META_ID_ALLOCATION_DEPTH,
-                                              errp);
+                ret = nbd_co_send_block_status(client, request->handle,
+                                               blk_bs(exp->common.blk),
+                                               request->from, request->len,
+                                               dont_fragment,
+                                               !--contexts_remaining,
+                                               NBD_META_ID_ALLOCATION_DEPTH,
+                                               errp);
                  if (ret < 0) {
                      return ret;
                  }



with it:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

-- 
Best regards,
Vladimir