Re: [Qemu-devel] [PATCH 06/18] qed: Implement .bdrv_drain

[Fam Zheng <famz@redhat.com>]

On Thu, 10/13 19:34, Paolo Bonzini wrote:
> From: Fam Zheng <famz@redhat.com>
> 
> The "need_check_timer" is used to clear the "NEED_CHECK" flag in the
> image header after a grace period once metadata update has finished. To
> comply with the bdrv_drain semantics, we should make sure it remains
> deleted once .bdrv_drain is called.
> 
> The change to qed_need_check_timer_cb is needed because bdrv_qed_drain
> is called after s->bs has been drained, and should not operate on it;
> instead it should operate on the BdrvChild-ren exclusively.  Doing so
> is easy because QED does not have a bdrv_co_flush_to_os callback, hence
> all that is needed to flush it is to ensure writes have reached the disk.
> 
> Based on commit df9a681dc9a

Much is changed so you should already take the authorship, otherwise I cannot
rev-by it. :)

> (which however included some unrelated
> hunks, possibly due to a merge failure or an overlooked squash).
> The patch was reverted because at the time bdrv_qed_drain could call
> qed_plug_allocating_write_reqs while an allocating write was queued.
> This however is not possible anymore after the previous patch;
> .bdrv_drain is only called after all writes have completed at the
> QED level, and its purpose is to trigger metadata writes in bs->file.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block/qed.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/block/qed.c b/block/qed.c
> index 3ee879b..1a7ef0a 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> @@ -336,7 +336,7 @@ static void qed_need_check_timer_cb(void *opaque)
>      qed_plug_allocating_write_reqs(s);
>  
>      /* Ensure writes are on disk before clearing flag */
> -    bdrv_aio_flush(s->bs, qed_clear_need_check, s);
> +    bdrv_aio_flush(s->bs->file->bs, qed_clear_need_check, s);

If this one has to change, what about the other bdrv_aio_flush(s->bs, ...) down
in this call path:

    qed_need_check_timer_cb
        qed_clear_need_check
            qed_write_header
                qed_flush_after_clear_need_check

?

>  }
>  
>  static void qed_start_need_check_timer(BDRVQEDState *s)
> @@ -378,6 +378,19 @@ static void bdrv_qed_attach_aio_context(BlockDriverState *bs,
>      }
>  }
>  
> +static void bdrv_qed_drain(BlockDriverState *bs)
> +{
> +    BDRVQEDState *s = bs->opaque;
> +
> +    /* Fire the timer immediately in order to start doing I/O as soon as the
> +     * header is flushed.
> +     */
> +    if (s->need_check_timer && timer_pending(s->need_check_timer)) {
> +        qed_cancel_need_check_timer(s);
> +        qed_need_check_timer_cb(s);
> +    }
> +}
> +
>  static int bdrv_qed_open(BlockDriverState *bs, QDict *options, int flags,
>                           Error **errp)
>  {
> @@ -1668,6 +1681,7 @@ static BlockDriver bdrv_qed = {
>      .bdrv_check               = bdrv_qed_check,
>      .bdrv_detach_aio_context  = bdrv_qed_detach_aio_context,
>      .bdrv_attach_aio_context  = bdrv_qed_attach_aio_context,
> +    .bdrv_drain               = bdrv_qed_drain,
>  };
>  
>  static void bdrv_qed_init(void)
> -- 
> 2.7.4
> 
>