On Thu, 10/13 19:34, Paolo Bonzini wrote:
> From: Fam Zheng <f...@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 <f...@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonz...@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
>
>
