qemu-block
[Top][All Lists]
Advanced

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

Re: [Qemu-block] [Qemu-devel] [PATCH v4 5/8] block/mirror: fix and impro


From: John Snow
Subject: Re: [Qemu-block] [Qemu-devel] [PATCH v4 5/8] block/mirror: fix and improve do_sync_target_write
Date: Mon, 17 Sep 2018 17:16:50 -0400
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1


On 09/17/2018 10:57 AM, Vladimir Sementsov-Ogievskiy wrote:
> Use bdrv_dirty_bitmap_next_dirty_area() instead of
> bdrv_dirty_iter_next_area(), because of the following problems of
> bdrv_dirty_iter_next_area():
> 
> 1. Using HBitmap iterators we should carefully handle unaligned offset,
> as first call to hbitmap_iter_next() may return a value less than
> original offset (actually, it will be original offset rounded down to
> bitmap granularity). This handling is not done in
> do_sync_target_write().
> 
> 2. bdrv_dirty_iter_next_area() handles unaligned max_offset
> incorrectly:
> 
> look at the code:
>     if (max_offset == iter->bitmap->size) {
>         /* If max_offset points to the image end, round it up by the
>          * bitmap granularity */
>         gran_max_offset = ROUND_UP(max_offset, granularity);
>     } else {
>         gran_max_offset = max_offset;
>     }
> 
>     ret = hbitmap_iter_next(&iter->hbi, false);
>     if (ret < 0 || ret + granularity > gran_max_offset) {
>         return false;
>     }
> 
> and assume that max_offset != iter->bitmap->size but still unaligned.
> if 0 < ret < max_offset we found dirty area, but the function can
> return false in this case (if ret + granularity > max_offset).
> 
> 3. bdrv_dirty_iter_next_area() uses inefficient loop to find the end of
> the dirty area. Let's use more efficient hbitmap_next_zero instead
> (bdrv_dirty_bitmap_next_dirty_area() do so)
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>  block/mirror.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 6cc10df5c9..b7565a6c1c 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -1167,25 +1167,23 @@ static void do_sync_target_write(MirrorBlockJob *job, 
> MirrorMethod method,
>                                   uint64_t offset, uint64_t bytes,
>                                   QEMUIOVector *qiov, int flags)
>  {
> -    BdrvDirtyBitmapIter *iter;
>      QEMUIOVector target_qiov;
> -    uint64_t dirty_offset;
> -    int dirty_bytes;
> +    uint64_t dirty_offset = offset;
> +    uint64_t dirty_bytes;
>  
>      if (qiov) {
>          qemu_iovec_init(&target_qiov, qiov->niov);
>      }
>  
> -    iter = bdrv_dirty_iter_new(job->dirty_bitmap);
> -    bdrv_set_dirty_iter(iter, offset);
> -
>      while (true) {
>          bool valid_area;
>          int ret;
>  
>          bdrv_dirty_bitmap_lock(job->dirty_bitmap);
> -        valid_area = bdrv_dirty_iter_next_area(iter, offset + bytes,
> -                                               &dirty_offset, &dirty_bytes);
> +        dirty_bytes = MIN(offset + bytes - dirty_offset, INT_MAX);
> +        valid_area = bdrv_dirty_bitmap_next_dirty_area(job->dirty_bitmap,
> +                                                       &dirty_offset,
> +                                                       &dirty_bytes);
>          if (!valid_area) {
>              bdrv_dirty_bitmap_unlock(job->dirty_bitmap);
>              break;
> @@ -1241,9 +1239,10 @@ static void do_sync_target_write(MirrorBlockJob *job, 
> MirrorMethod method,
>                  break;
>              }
>          }
> +
> +        dirty_offset += dirty_bytes;
>      }
>  
> -    bdrv_dirty_iter_free(iter);
>      if (qiov) {
>          qemu_iovec_destroy(&target_qiov);
>      }
> 

1-5 look good, with the variable name change in 3.

Reviewed-by: John Snow <address@hidden>

1-8; Eric: you can stage these behind your NBD patch if you are happy
with them.

Acked-by: John Snow <address@hidden>



reply via email to

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