Re: [Qemu-devel] [PATCH for-2.6 1/2] block/mirror: Revive dead yielding code

[Fam Zheng <famz@redhat.com>]

On Wed, 04/20 00:59, Max Reitz wrote:
> mirror_iteration() is supposed to wait if the current chunk is subject
> to a still in-flight mirroring operation. However, it mixed checking
> this conflict situation with checking the dirty status of a chunk. A
> simplification for the latter condition (the first chunk encountered is
> always dirty) led to neglecting the former: We just skip the first chunk
> and thus never test whether it conflicts with an in-flight operation.
> 
> To fix this, pull out the code which waits for in-flight operations on
> the first chunk of the range to be mirrored to settle.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/mirror.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index c2cfc1a..2714a77 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -298,7 +298,7 @@ static void mirror_do_zero_or_discard(MirrorBlockJob *s,
>  static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
>  {
>      BlockDriverState *source = s->common.bs;
> -    int64_t sector_num;
> +    int64_t sector_num, first_chunk;
>      uint64_t delay_ns = 0;
>      /* At least the first dirty chunk is mirrored in one iteration. */
>      int nb_chunks = 1;
> @@ -313,6 +313,12 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
>          assert(sector_num >= 0);
>      }
>  
> +    first_chunk = sector_num / sectors_per_chunk;
> +    while (test_bit(first_chunk, s->in_flight_bitmap)) {
> +        trace_mirror_yield_in_flight(s, first_chunk, s->in_flight);
> +        mirror_wait_for_io(s);
> +    }
> +
>      /* Find the number of consective dirty chunks following the first dirty
>       * one, and wait for in flight requests in them. */
>      while (nb_chunks * sectors_per_chunk < (s->buf_size >> BDRV_SECTOR_BITS)) {
> @@ -324,17 +330,12 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
>              break;
>          }
>          if (test_bit(next_chunk, s->in_flight_bitmap)) {

Now this can be combiled into the previous if block.

> -            if (nb_chunks > 0) {
> -                break;
> -            }
> -            trace_mirror_yield_in_flight(s, next_sector, s->in_flight);
> -            mirror_wait_for_io(s);
> -            /* Now retry.  */
> -        } else {
> -            hbitmap_next = hbitmap_iter_next(&s->hbi);
> -            assert(hbitmap_next == next_sector);
> -            nb_chunks++;
> +            break;
>          }
> +
> +        hbitmap_next = hbitmap_iter_next(&s->hbi);
> +        assert(hbitmap_next == next_sector);
> +        nb_chunks++;
>      }
>  
>      /* Clear dirty bits before querying the block status, because
> -- 
> 2.8.0
> 

Reviewed-by: Fam Zheng <famz@redhat.com>