[Qemu-devel] [PATCH 0/2] nbd base:allocation several extents

[Qemu-devel] [PATCH 0/2] nbd base:allocation several extents

[Vladimir Sementsov-Ogievskiy]

Hi all!

Patch 01 is a fix an should definitely go to 3.0. Patch 02 is an
improvement, so as you want, 3.1 is ok.

The idea is send more than one extent of base:allocation context, if
possible, which is necessary for efficient block-status export, for
clients which support it.

Vladimir Sementsov-Ogievskiy (2):
  nbd/server: fix nbd_co_send_block_status
  nbd/server: send more than one extent of base:allocation context

 nbd/server.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 58 insertions(+), 18 deletions(-)

-- 
2.11.1

[Qemu-devel] [PATCH 1/2] nbd/server: fix nbd_co_send_block_status

[Vladimir Sementsov-Ogievskiy]

Call nbd_co_send_extents() with correct length parameter
(extent.length may be smaller than original length).

Also, switch length parameter type to uint32_t, to correspond with
request->len and similar nbd_co_send_bitmap().

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 nbd/server.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c
index e52b76bd1a..ea5fe0eb33 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1910,7 +1910,7 @@ 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,
-                                    uint64_t length, bool last,
+                                    uint32_t length, bool last,
                                     uint32_t context_id, Error **errp)
 {
     int ret;
@@ -1922,7 +1922,8 @@ static int nbd_co_send_block_status(NBDClient *client, uint64_t handle,
                 client, handle, -ret, "can't get block status", errp);
     }
 
-    return nbd_co_send_extents(client, handle, &extent, 1, length, last,
+    return nbd_co_send_extents(client, handle, &extent, 1,
+                               be32_to_cpu(extent.length), last,
                                context_id, errp);
 }
 
-- 
2.11.1

[Qemu-devel] [PATCH 2/2] nbd/server: send more than one extent of base:allocation context

[Vladimir Sementsov-Ogievskiy]

This is necessary for efficient block-status export, for clients which
support it.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 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.
+ *
+ * Returns zero on success and -errno on bdrv_block_status_above failure.
+ */
+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;
+    bool first_extent = true;
 
+    assert(*nb_extents);
     while (remaining_bytes) {
         uint32_t flags;
         int64_t num;
@@ -1860,21 +1871,40 @@ static int blockstatus_to_extent_be(BlockDriverState *bs, uint64_t offset,
 
         flags = (ret & BDRV_BLOCK_ALLOCATED ? 0 : NBD_STATE_HOLE) |
                 (ret & BDRV_BLOCK_ZERO      ? NBD_STATE_ZERO : 0);
+        offset += num;
+        remaining_bytes -= num;
 
-        if (remaining_bytes == bytes) {
+        if (first_extent) {
             extent->flags = flags;
+            extent->length = num;
+            first_extent = false;
+            continue;
         }
 
-        if (flags != extent->flags) {
-            break;
+        if (flags == extent->flags) {
+            /* extend current extent */
+            extent->length += num;
+        } else {
+            if (extent + 1 == extents_end) {
+                break;
+            }
+
+            /* start new extent */
+            extent++;
+            extent->flags = flags;
+            extent->length = num;
         }
+    }
 
-        offset += num;
-        remaining_bytes -= num;
+    extents_end = extent + 1;
+
+    for (extent = extents; extent < extents_end; extent++) {
+        cpu_to_be32s(&extent->flags);
+        cpu_to_be32s(&extent->length);
     }
 
-    cpu_to_be32s(&extent->flags);
-    extent->length = cpu_to_be32(bytes - remaining_bytes);
+    *bytes -= remaining_bytes;
+    *nb_extents = extents_end - extents;
 
     return 0;
 }
@@ -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);
+
+    return ret;
 }
 
 /*
@@ -2228,10 +2266,11 @@ static coroutine_fn int nbd_handle_request(NBDClient *client,
             (client->export_meta.base_allocation ||
              client->export_meta.bitmap))
         {
+            bool dont_fragment = request->flags & NBD_CMD_FLAG_REQ_ONE;
             if (client->export_meta.base_allocation) {
                 ret = nbd_co_send_block_status(client, request->handle,
                                                blk_bs(exp->blk), request->from,
-                                               request->len,
+                                               request->len, dont_fragment,
                                                !client->export_meta.bitmap,
                                                NBD_META_ID_BASE_ALLOCATION,
                                                errp);
@@ -2244,7 +2283,7 @@ static coroutine_fn int nbd_handle_request(NBDClient *client,
                 ret = nbd_co_send_bitmap(client, request->handle,
                                          client->exp->export_bitmap,
                                          request->from, request->len,
-                                         request->flags & NBD_CMD_FLAG_REQ_ONE,
+                                         dont_fragment,
                                          true, NBD_META_ID_DIRTY_BITMAP, errp);
                 if (ret < 0) {
                     return ret;
-- 
2.11.1

Re: [Qemu-devel] [PATCH 1/2] nbd/server: fix nbd_co_send_block_status

[Eric Blake]

On 07/04/2018 06:23 AM, Vladimir Sementsov-Ogievskiy wrote:
> Call nbd_co_send_extents() with correct length parameter
> (extent.length may be smaller than original length).

Only matters for tracing purposes, but indeed worth an accurate trace.

> 
> Also, switch length parameter type to uint32_t, to correspond with
> request->len and similar nbd_co_send_bitmap().

No semantic impact.

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   nbd/server.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/nbd/server.c b/nbd/server.c
> index e52b76bd1a..ea5fe0eb33 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -1910,7 +1910,7 @@ 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,
> -                                    uint64_t length, bool last,
> +                                    uint32_t length, bool last,
>                                       uint32_t context_id, Error **errp)
>   {
>       int ret;
> @@ -1922,7 +1922,8 @@ static int nbd_co_send_block_status(NBDClient *client, uint64_t handle,
>                   client, handle, -ret, "can't get block status", errp);
>       }
>   
> -    return nbd_co_send_extents(client, handle, &extent, 1, length, last,
> +    return nbd_co_send_extents(client, handle, &extent, 1,
> +                               be32_to_cpu(extent.length), last,

be32_to_cpu() is necessary to undo the fact that extent is already 
encoded for transmission over the wire.  Alternatively, 
blockstatus_to_extent_be() could be modified to return negative on 
error, or the total length encoded on success, or set the final length 
via a pointer parameter.  But the next patch will probably deal with 
that more directly.

This one is fine for 3.0,
Reviewed-by: Eric Blake <eblake@redhat.com>

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

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

[Eric Blake]

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?

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

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   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?

> + */
> +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 ||
>                client->export_meta.bitmap))
>           {
> +            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 <eblake@redhat.com>

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

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

[Vladimir Sementsov-Ogievskiy]

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 <vsementsov@virtuozzo.com>
>> ---
>>   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 ||
>>                client->export_meta.bitmap))
>>           {
>> +            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 <eblake@redhat.com>
>

Ok, thank you!

-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

04.07.2018 14:23, Vladimir Sementsov-Ogievskiy wrote:
> This is necessary for efficient block-status export, for clients which
> support it.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   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.
> + *
> + * Returns zero on success and -errno on bdrv_block_status_above failure.
> + */
> +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;
> +    bool first_extent = true;
>   
> +    assert(*nb_extents);
>       while (remaining_bytes) {
>           uint32_t flags;
>           int64_t num;
> @@ -1860,21 +1871,40 @@ static int blockstatus_to_extent_be(BlockDriverState *bs, uint64_t offset,

after bdrv_block_status_above, is there a guarantee that num > 0? Should 
we add an assertion?

>   
>           flags = (ret & BDRV_BLOCK_ALLOCATED ? 0 : NBD_STATE_HOLE) |
>                   (ret & BDRV_BLOCK_ZERO      ? NBD_STATE_ZERO : 0);
> +        offset += num;
> +        remaining_bytes -= num;
>   
> -        if (remaining_bytes == bytes) {
> +        if (first_extent) {
>               extent->flags = flags;
> +            extent->length = num;
> +            first_extent = false;
> +            continue;
>           }
>   
> -        if (flags != extent->flags) {
> -            break;
> +        if (flags == extent->flags) {
> +            /* extend current extent */
> +            extent->length += num;
> +        } else {
> +            if (extent + 1 == extents_end) {
> +                break;
> +            }
> +
> +            /* start new extent */
> +            extent++;
> +            extent->flags = flags;
> +            extent->length = num;
>           }
> +    }
>   
> -        offset += num;
> -        remaining_bytes -= num;
> +    extents_end = extent + 1;
> +
> +    for (extent = extents; extent < extents_end; extent++) {
> +        cpu_to_be32s(&extent->flags);
> +        cpu_to_be32s(&extent->length);
>       }
>   
> -    cpu_to_be32s(&extent->flags);
> -    extent->length = cpu_to_be32(bytes - remaining_bytes);
> +    *bytes -= remaining_bytes;
> +    *nb_extents = extents_end - extents;
>   
>       return 0;
>   }
> @@ -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);
> +
> +    return ret;
>   }
>   
>   /*
> @@ -2228,10 +2266,11 @@ static coroutine_fn int nbd_handle_request(NBDClient *client,
>               (client->export_meta.base_allocation ||
>                client->export_meta.bitmap))
>           {
> +            bool dont_fragment = request->flags & NBD_CMD_FLAG_REQ_ONE;
>               if (client->export_meta.base_allocation) {
>                   ret = nbd_co_send_block_status(client, request->handle,
>                                                  blk_bs(exp->blk), request->from,
> -                                               request->len,
> +                                               request->len, dont_fragment,
>                                                  !client->export_meta.bitmap,
>                                                  NBD_META_ID_BASE_ALLOCATION,
>                                                  errp);
> @@ -2244,7 +2283,7 @@ static coroutine_fn int nbd_handle_request(NBDClient *client,
>                   ret = nbd_co_send_bitmap(client, request->handle,
>                                            client->exp->export_bitmap,
>                                            request->from, request->len,
> -                                         request->flags & NBD_CMD_FLAG_REQ_ONE,
> +                                         dont_fragment,
>                                            true, NBD_META_ID_DIRTY_BITMAP, errp);
>                   if (ret < 0) {
>                       return ret;


-- 
Best regards,
Vladimir

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

[Eric Blake]

On 07/10/2018 09:33 AM, Vladimir Sementsov-Ogievskiy wrote:
> 04.07.2018 14:23, Vladimir Sementsov-Ogievskiy wrote:
>> This is necessary for efficient block-status export, for clients which
>> support it.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---

>> +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;
>> +    bool first_extent = true;
>> +    assert(*nb_extents);
>>       while (remaining_bytes) {
>>           uint32_t flags;
>>           int64_t num;
>> @@ -1860,21 +1871,40 @@ static int 
>> blockstatus_to_extent_be(BlockDriverState *bs, uint64_t offset,
> 
> after bdrv_block_status_above, is there a guarantee that num > 0? Should 
> we add an assertion?

bdrv_block_status_above() asserts that drivers set *pnum to non-zero on 
success (that is, a driver's .bdrv_co_block_status must make progress or 
fail); however, a caller can still get *pnum == 0 on the corner case of 
requesting status at or beyond the end-of-file boundary (where 
bdrv_block_status_above() does not call into a driver's 
.bdrv_co_block_status callback).  Since the NBD code already validated 
that the client's code does not exceed EOF, and the block layer 
guaranteed that an in-range request made progress on success, you should 
be fine with an assertion that num > 0.

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