Re: [Qemu-devel] [PATCH 03/18] mirror: use bdrv_drained_begin/bdrv_drained_end

[Fam Zheng <famz@redhat.com>]

On Thu, 10/13 19:34, Paolo Bonzini wrote:
> Ensure that there are no changes between the last check to
> bdrv_get_dirty_count and the switch to the target.
> 
> There is already a bdrv_drained_end call, we only need to ensure
> that bdrv_drained_begin is not called twice.
> 
> Cc: qemu-stable@nongnu.org

Cc stable? I don't see an existing bug here, can you explain?

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block/mirror.c | 35 +++++++++++++++++++++++------------
>  1 file changed, 23 insertions(+), 12 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index bd1963d..8cd69aa 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -622,6 +622,7 @@ static void coroutine_fn mirror_run(void *opaque)
>      MirrorExitData *data;
>      BlockDriverState *bs = blk_bs(s->common.blk);
>      BlockDriverState *target_bs = blk_bs(s->target);
> +    bool need_drain = true;
>      int64_t length;
>      BlockDriverInfo bdi;
>      char backing_filename[2]; /* we only need 2 characters because we are only
> @@ -756,11 +757,26 @@ static void coroutine_fn mirror_run(void *opaque)
>               * source has dirty data to copy!
>               *
>               * Note that I/O can be submitted by the guest while
> -             * mirror_populate runs.
> +             * mirror_populate runs, so pause it now.  Before deciding
> +             * whether to switch to target check one last time if I/O has
> +             * come in the meanwhile, and if not flush the data to disk.
>               */
>              trace_mirror_before_drain(s, cnt);
> -            bdrv_co_drain(bs);
> +
> +            bdrv_drained_begin(bs);
>              cnt = bdrv_get_dirty_count(s->dirty_bitmap);
> +            if (cnt > 0) {
> +                bdrv_drained_end(bs);
> +                continue;
> +            }
> +
> +            /* The two disks are in sync.  Exit and report successful
> +             * completion.
> +             */
> +            assert(QLIST_EMPTY(&bs->tracked_requests));
> +            s->common.cancelled = false;
> +            need_drain = false;
> +            break;
>          }
>  
>          ret = 0;
> @@ -773,13 +789,6 @@ static void coroutine_fn mirror_run(void *opaque)
>          } else if (!should_complete) {
>              delay_ns = (s->in_flight == 0 && cnt == 0 ? SLICE_TIME : 0);
>              block_job_sleep_ns(&s->common, QEMU_CLOCK_REALTIME, delay_ns);
> -        } else if (cnt == 0) {
> -            /* The two disks are in sync.  Exit and report successful
> -             * completion.
> -             */
> -            assert(QLIST_EMPTY(&bs->tracked_requests));
> -            s->common.cancelled = false;
> -            break;
>          }
>          s->last_pause_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
>      }
> @@ -791,6 +800,7 @@ immediate_exit:
>           * the target is a copy of the source.
>           */
>          assert(ret < 0 || (!s->synced && block_job_is_cancelled(&s->common)));
> +        assert(need_drain);
>          mirror_wait_for_all_io(s);
>      }
>  
> @@ -802,9 +812,10 @@ immediate_exit:
>  
>      data = g_malloc(sizeof(*data));
>      data->ret = ret;
> -    /* Before we switch to target in mirror_exit, make sure data doesn't
> -     * change. */
> -    bdrv_drained_begin(bs);
> +
> +    if (need_drain) {

Not sure whether this if block is necessary (i.e. when !(cnt == 0 &&
should_complete)), but it certainly doesn't hurt.

> +        bdrv_drained_begin(bs);
> +    }
>      block_job_defer_to_main_loop(&s->common, mirror_exit, data);
>  }
>  
> -- 
> 2.7.4
> 
> 

Fam