[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 2/2] nbd/server: send more than one extent of ba

From: Vladimir Sementsov-Ogievskiy
Subject: Re: [Qemu-devel] [PATCH 2/2] nbd/server: send more than one extent of base:allocation context
Date: Mon, 17 Sep 2018 19:15:59 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0

05.07.2018 19:18, Vladimir Sementsov-Ogievskiy wrote:
05.07.2018 18:59, Eric Blake wrote:
On 07/04/2018 06:23 AM, Vladimir Sementsov-Ogievskiy wrote:
This is necessary for efficient block-status export, for clients which
support it.

qemu as client doesn't currently process additional information when querying block status.  Should it?

Are there other clients which DO make use of the additional information?

Actually, we have one in development. block status export is used for external full backups, like bitmap export - for external incremental backups.

This is a feature, so it is indeed 3.1 material.

Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
  nbd/server.c | 77 +++++++++++++++++++++++++++++++++++++++++++++---------------
  1 file changed, 58 insertions(+), 19 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index ea5fe0eb33..a6d013aec4 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1844,11 +1844,22 @@ static int coroutine_fn nbd_co_send_sparse_read(NBDClient *client,
      return ret;
  -static int blockstatus_to_extent_be(BlockDriverState *bs, uint64_t offset,
-                                    uint64_t bytes, NBDExtent *extent)
+ * Populate @extents from block status. Store in @bytes the byte length encoded + * (which may be smaller but (unlike bitmap_to_extents) _never_ larger than the
+ * original), and store in @nb_extents the number of extents used.

I don't think the comparison to bitmap_to_extents in a nested parenthetical buys us anything; the shorter:

(which may be smaller than the original)

would do just fine for the function contract.

+ *
+ * Returns zero on success and -errno on bdrv_block_status_above failure.

Is it worth returning a positive value for one less pointer parameter used for output?

I think, that if we have one such parameter anyway, let's use same path for very similar thing, for consistency.

+ */
+static int blockstatus_to_extents(BlockDriverState *bs, uint64_t offset,
+                                  uint64_t *bytes, NBDExtent *extents,
+                                  unsigned int *nb_extents)
-    uint64_t remaining_bytes = bytes;
+    uint64_t remaining_bytes = *bytes;
+    NBDExtent *extent = extents, *extents_end = extents + *nb_extents;

So both bytes and nb_extents are in-out.

@@ -1910,21 +1940,29 @@ static int nbd_co_send_extents(NBDClient *client, uint64_t handle,   /* Get block status from the exported device and send it to the client */   static int nbd_co_send_block_status(NBDClient *client, uint64_t handle,                                       BlockDriverState *bs, uint64_t offset,
-                                    uint32_t length, bool last,
-                                    uint32_t context_id, Error **errp)
+                                    uint32_t length, bool dont_fragment,
+                                    bool last, uint32_t context_id,
+                                    Error **errp)
      int ret;
-    NBDExtent extent;
+    unsigned int nb_extents = dont_fragment ? 1 : NBD_MAX_BITMAP_EXTENTS;
+    NBDExtent *extents = g_new(NBDExtent, nb_extents);
+    uint64_t final_length = length;
  -    ret = blockstatus_to_extent_be(bs, offset, length, &extent);
+    ret = blockstatus_to_extents(bs, offset, &final_length, extents,
+                                 &nb_extents);
      if (ret < 0) {
+        g_free(extents);
          return nbd_co_send_structured_error(
                  client, handle, -ret, "can't get block status", errp);
  -    return nbd_co_send_extents(client, handle, &extent, 1,
-                               be32_to_cpu(extent.length), last,
-                               context_id, errp);
+    ret = nbd_co_send_extents(client, handle, extents, nb_extents,
+                              final_length, last, context_id, errp);
+    g_free(extents);

At any rate, this conversion looks sane.

+    return ret;
@@ -2228,10 +2266,11 @@ static coroutine_fn int nbd_handle_request(NBDClient *client,
              (client->export_meta.base_allocation ||
+            bool dont_fragment = request->flags & NBD_CMD_FLAG_REQ_ONE;
              if (client->export_meta.base_allocation) {

Blank lines between declarations and statements can aid in legibility; I can add when queuing.

Reviewed-by: Eric Blake <address@hidden>

Ok, thank you!

hmm, looks like lost patch :(

Best regards,

reply via email to

[Prev in Thread] Current Thread [Next in Thread]