Re: [PATCH 09/10] nbd/server: use bdrv_dirty_bitmap_next_dirty_area

[Eric Blake <eblake@redhat.com>]

On 9/30/19 10:15 AM, Vladimir Sementsov-Ogievskiy wrote:
> Use bdrv_dirty_bitmap_next_dirty_area for bitmap_to_extents. Since
> bdrv_dirty_bitmap_next_dirty_area is very accurate in its interface,
> we'll never exceed requested region with last chunk. So, we don't need
> dont_fragment, and bitmap_to_extents() interface becomes clean enough
> to not require any comment.

Comments are a useful style, even if functions seem trivial.

When req_one is in effect, we have to stop at the requested length. 
When req_one is not in effect, the NBD spec does not require us to stop 
until the next change in extent status, but also does not force us to 
continue past.  So this change is fine from the protocol standpoint.

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   nbd/server.c | 58 ++++++++++++++++------------------------------------
>   1 file changed, 18 insertions(+), 40 deletions(-)
> 
> diff --git a/nbd/server.c b/nbd/server.c
> index cc63d8ad21..edbdb1b6b6 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -2023,57 +2023,35 @@ static int nbd_co_send_block_status(NBDClient *client, uint64_t handle,
>       return nbd_co_send_extents(client, handle, ea, last, context_id, errp);
>   }
>   
> -/*
> - * Populate @ea from a dirty bitmap. Unless @dont_fragment, the
> - * final extent may exceed the original @length.
> - */

I would have kept the first sentence, and dropped only the second.

>   static void bitmap_to_extents(BdrvDirtyBitmap *bitmap,
>                                 uint64_t offset, uint64_t length,
> -                              NBDExtentArray *ea, bool dont_fragment)
> +                              NBDExtentArray *es)
>   {
> -    uint64_t begin = offset, end = offset;
> -    uint64_t overall_end = offset + length;
> -    BdrvDirtyBitmapIter *it;
> -    bool dirty;
> +    int64_t start, dirty_start, dirty_count;
> +    int64_t end = offset + length;
> +    bool full = false;
>   
>       bdrv_dirty_bitmap_lock(bitmap);

> +    for (start = offset;
> +         bdrv_dirty_bitmap_next_dirty_area(bitmap, start, end, INT32_MAX,
> +                                           &dirty_start, &dirty_count);
> +         start = dirty_start + dirty_count)
> +    {
> +        if ((nbd_extent_array_add(es, dirty_start - start, 0) < 0) ||
> +            (nbd_extent_array_add(es, dirty_count, NBD_STATE_DIRTY) < 0))

As long as bdrv_dirty_bitmap_next_dirty_area works correctly, this 
should work regardless of whether start is dirty or clean (if dirty, the 
first call will be a 0-length no-op).


> +        {
> +            full = true;
>               break;
>           }
> -        begin = end;
> -        dirty = next_dirty;
>       }
>   
> -    bdrv_dirty_iter_free(it);
> +    if (!full) {
> +        /* last non dirty extent */
> +        nbd_extent_array_add(es, end - start, 0);
> +    }

Losing the possibility of reporting beyond the end of the original 
request (when req_one is not in force) is not fatal (it might make some 
clients less efficient when walking the entire disk, but qemu as a 
client isn't currently taking advantage of NBD's permission to return 
extra length).

Reviewed-by: Eric Blake <eblake@redhat.com>

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