Re: [Qemu-devel] [PATCH v5 4/6] nbd/server: implement dirty bitmap export

[Eric Blake <eblake@redhat.com>]

On 06/20/2018 12:04 PM, Vladimir Sementsov-Ogievskiy wrote:
> 20.06.2018 19:27, Eric Blake wrote:
>> On 06/09/2018 10:17 AM, 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.
>>>

>>  static int nbd_co_send_extents(NBDClient *client, uint64_t handle,
>> -                               NBDExtent *extents, unsigned nb_extents,
>> +                               NBDExtent *extents, unsigned int 
>> nb_extents,
>> +                               uint64_t length, bool last,
>>                                 uint32_t context_id, Error **errp)
>>  {
>>      NBDStructuredMeta chunk;
>> @@ -1890,7 +1897,9 @@ static int nbd_co_send_extents(NBDClient 
>> *client, uint64_t handle,
>>          {.iov_base = extents, .iov_len = nb_extents * 
>> sizeof(extents[0])}
>>      };
>>
>> -    set_be_chunk(&chunk.h, 0, NBD_REPLY_TYPE_BLOCK_STATUS,
>> +    trace_nbd_co_send_extents(handle, nb_extents, context_id, length, 
>> last);
> 
> hm, length variable added only to be traced.. Ok, but a bit strange.

Yes. It doesn't affect what goes over the wire, but when it comes to 
tracing, knowing the sum of the extents can be quite helpful (especially 
knowing if the server's reply is shorter or longer than the client's 
request, and the fact that when two or more contexts are negotiated by 
the client, the different contexts can have different length replies)


>> +static unsigned int bitmap_to_extents(BdrvDirtyBitmap *bitmap, 
>> uint64_t offset,
>> +                                      uint64_t *length, NBDExtent 
>> *extents,
> 
> length - in-out? worth comment?

Sure.

> 
>> + unsigned int nb_extents,
>> +                                      bool dont_fragment)
>>  {
>>      uint64_t begin = offset, end;
>> -    uint64_t overall_end = offset + length;
>> -    unsigned i = 0;
>> +    uint64_t overall_end = offset + *length;
>> +    unsigned int i = 0;
>>      BdrvDirtyBitmapIter *it;
>>      bool dirty;
>>
>> @@ -1983,23 +1994,25 @@ static unsigned 
>> bitmap_to_extents(BdrvDirtyBitmap *bitmap, uint64_t offset,
>>
>>      bdrv_dirty_bitmap_unlock(bitmap);
>>
>> +    *length = end - begin;
> 
> hm, it will always be zero, as at the end of the previous loop we have 
> "begin = end;"

Ah, then it should be end - offset. Thanks for the careful review.

In fact, since ONLY the final extent can be larger than 2G (all earlier 
extents were short of the overall_end, and the incoming length is 
32-bit), but the NBD protocol says that at most one extent can go beyond 
the original request, we do NOT want to split a >2G extent into multiple 
extents, but rather cap it to just shy of 4G at the granularity offered 
by the bitmap itself.  At which point add_extents() never returns more 
than 1, and can just be inlined.

> 
>> return i;
>>  }
>>
>>  static int nbd_co_send_bitmap(NBDClient *client, uint64_t handle,
>>                                BdrvDirtyBitmap *bitmap, uint64_t offset,
>> -                              uint64_t length, bool dont_fragment,
>> +                              uint32_t length, bool dont_fragment,
>>                                uint32_t context_id, Error **errp)
>>  {
>>      int ret;
>> -    unsigned nb_extents = dont_fragment ? 1 : NBD_MAX_BITMAP_EXTENTS;
>> +    unsigned int nb_extents = dont_fragment ? 1 : 
>> NBD_MAX_BITMAP_EXTENTS;
>>      NBDExtent *extents = g_new(NBDExtent, nb_extents);
>> +    uint64_t final_length = length;
>>
>> -    nb_extents = bitmap_to_extents(bitmap, offset, length, extents, 
>> nb_extents,
>> -                                   dont_fragment);
>> +    nb_extents = bitmap_to_extents(bitmap, offset, &final_length, 
>> extents,
>> +                                   nb_extents, dont_fragment);
>>
>> -    ret = nbd_co_send_extents(client, handle, extents, nb_extents, 
>> context_id,
>> -                              errp);
>> +    ret = nbd_co_send_extents(client, handle, extents, nb_extents,
>> +                              final_length, true, context_id, errp);
> 
> hmm in-out pointer field only to trace final_length? may be just trace 
> it in bitmap_to_extents?

No, because base:allocation also benefits from tracing final_length.

> 
> also, it's not trivial, that the function now sends FLAG_DONE. I think, 
> worth add a comment, or
> a parameter like for nbd_co_send_block_status. It will simplify 
> introducing new contexts in future.

Do we anticipate adding any in the near future? But adding a parameter 
that is always true so that the callsite becomes more obvious on when to 
pass the done flag may indeed make future additions easier to spot in 
one place.

So here's what else I'll squash in:

diff --git i/nbd/server.c w/nbd/server.c
index e84aea6cf03..7a96b296839 100644
--- i/nbd/server.c
+++ w/nbd/server.c
@@ -1881,9 +1881,10 @@ static int 
blockstatus_to_extent_be(BlockDriverState *bs, uint64_t offset,

  /* nbd_co_send_extents
   *
- * @last controls whether NBD_REPLY_FLAG_DONE is sent.
- *
- * @extents should already be in big-endian format.
+ * @length is only for tracing purposes (and may be smaller or larger
+ * than the client's original request). @last controls whether
+ * NBD_REPLY_FLAG_DONE is sent. @extents should already be in
+ * big-endian format.
   */
  static int nbd_co_send_extents(NBDClient *client, uint64_t handle,
                                 NBDExtent *extents, unsigned int 
nb_extents,
@@ -1925,33 +1926,12 @@ static int nbd_co_send_block_status(NBDClient 
*client, uint64_t handle,
                                 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.
+/*
+ * Populate @extents from a dirty bitmap. Unless @dont_fragment, the
+ * final extent may exceed the original @length. Store in @length the
+ * byte length encoded (which may be smaller or larger than the
+ * original), and return the number of extents used.
   */
-static unsigned int add_extents(NBDExtent *extents, unsigned nb_extents,
-                                uint64_t length, uint32_t flags)
-{
-    unsigned int i = 0;
-    uint32_t max_extent = 0x80000000U;
-    uint32_t max_extent_be = cpu_to_be32(max_extent);
-    uint32_t flags_be = cpu_to_be32(flags);
-
-    for (i = 0; i < nb_extents && length > max_extent;
-         i++, length -= max_extent)
-    {
-        extents[i].length = max_extent_be;
-        extents[i].flags = flags_be;
-    }
-
-    if (length > 0 && i < nb_extents) {
-        extents[i].length = cpu_to_be32(length);
-        extents[i].flags = flags_be;
-        i++;
-    }
-
-    return i;
-}
-
  static unsigned int bitmap_to_extents(BdrvDirtyBitmap *bitmap, 
uint64_t offset,
                                        uint64_t *length, NBDExtent 
*extents,
                                        unsigned int nb_extents,
@@ -1976,16 +1956,18 @@ static unsigned int 
bitmap_to_extents(BdrvDirtyBitmap *bitmap, uint64_t offset,
              end = bdrv_dirty_iter_next(it);
          }
          if (end == -1) {
-            end = bdrv_dirty_bitmap_size(bitmap);
+            /* Cap to an aligned value < 4G beyond begin. */
+            end = MIN(bdrv_dirty_bitmap_size(bitmap),
+                      begin + 0x100000000ULL -
+                      bdrv_dirty_bitmap_granularity(bitmap));
          }
          if (dont_fragment && end > overall_end) {
-            /* We MUST not exceed requested region if 
NBD_CMD_FLAG_REQ_ONE flag
-             * is set */
              end = overall_end;
          }

-        i += add_extents(extents + i, nb_extents - i, end - begin,
-                         dirty ? NBD_STATE_DIRTY : 0);
+        extents[i].length = cpu_to_be32(end - begin);
+        extents[i].flags = cpu_to_be32(dirty ? NBD_STATE_DIRTY : 0);
+        i++;
          begin = end;
          dirty = !dirty;
      }
@@ -1994,13 +1976,13 @@ static unsigned int 
bitmap_to_extents(BdrvDirtyBitmap *bitmap, uint64_t offset,

      bdrv_dirty_bitmap_unlock(bitmap);

-    *length = end - begin;
+    *length = end - offset;
      return i;
  }

  static int nbd_co_send_bitmap(NBDClient *client, uint64_t handle,
                                BdrvDirtyBitmap *bitmap, uint64_t offset,
-                              uint32_t length, bool dont_fragment,
+                              uint32_t length, bool dont_fragment, bool 
last,
                                uint32_t context_id, Error **errp)
  {
      int ret;
@@ -2012,7 +1994,7 @@ static int nbd_co_send_bitmap(NBDClient *client, 
uint64_t handle,
                                     nb_extents, dont_fragment);

      ret = nbd_co_send_extents(client, handle, extents, nb_extents,
-                              final_length, true, context_id, errp);
+                              final_length, last, context_id, errp);

      g_free(extents);

@@ -2253,7 +2235,7 @@ static coroutine_fn int 
nbd_handle_request(NBDClient *client,
                                           client->exp->export_bitmap,
                                           request->from, request->len,
                                           request->flags & 
NBD_CMD_FLAG_REQ_ONE,
-                                         NBD_META_ID_DIRTY_BITMAP, errp);
+                                         true, 
NBD_META_ID_DIRTY_BITMAP, errp);
                  if (ret < 0) {
                      return ret;
                  }


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