diff mbox series

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

Message ID 20180704112302.471456-3-vsementsov@virtuozzo.com
State New
Headers show
Series nbd base:allocation several extents | expand

Commit Message

Vladimir Sementsov-Ogievskiy July 4, 2018, 11:23 a.m. UTC
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(-)

Comments

Eric Blake July 5, 2018, 3:59 p.m. UTC | #1
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>
Vladimir Sementsov-Ogievskiy July 5, 2018, 4:18 p.m. UTC | #2
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!
Vladimir Sementsov-Ogievskiy July 10, 2018, 2:33 p.m. UTC | #3
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;
Eric Blake July 20, 2018, 8:42 p.m. UTC | #4
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.
Vladimir Sementsov-Ogievskiy Sept. 17, 2018, 4:15 p.m. UTC | #5
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 <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!
>

hmm, looks like lost patch :(
Eric Blake Sept. 27, 2018, 1:55 a.m. UTC | #6
On 9/17/18 11:15 AM, Vladimir Sementsov-Ogievskiy wrote:
> 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.
>>>

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

>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>
>>
>> Ok, thank you!
>>
> 
> hmm, looks like lost patch :(

Not lost, just extremely slow getting queued.  Now on my NBD queue.
diff mbox series

Patch

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;