Re: [Qemu-devel] [PATCH v3 0/2] block: Quiesce old aio context during bdrv_set_aio_context

[Fam Zheng <famz@redhat.com>]

On Thu, 04/13 14:51, Paolo Bonzini wrote:
> 
> 
> On 11/04/2017 00:04, Stefan Hajnoczi wrote:
> > On Mon, Apr 10, 2017 at 10:26:34AM +0800, Fam Zheng wrote:
> >> v3: Use bdrv_parent_drained_begin/end. [Kevin]
> >>     Do it before releasing new_context. [Stefan]
> >>
> >> Fam Zheng (2):
> >>   block: Make bdrv_parent_drained_begin/end public
> >>   block: Quiesce old aio context during bdrv_set_aio_context
> >>
> >>  block.c               |  7 +++++--
> >>  block/io.c            |  4 ++--
> >>  include/block/block.h | 16 ++++++++++++++++
> >>  3 files changed, 23 insertions(+), 4 deletions(-)
> > 
> > Thanks, applied to my block tree for 2.10:
> > https://github.com/stefanha/qemu/commits/block
> > 
> > If you'd like this in a stable release please CC qemu-stable.
> 
> Stefan, can you squash this documentation patch in please?
> 
> diff --git a/block.c b/block.c
> index 3b20a29..cb1c5c9 100644
> --- a/block.c
> +++ b/block.c
> @@ -4392,6 +4392,18 @@ void bdrv_set_aio_context(BlockDriverState *bs,
> AioContext *new_context)
>  {
>      AioContext *ctx = bdrv_get_aio_context(bs);
> 
> +    /* FIXME: This is the same as bdrv_drained_begin, only done
> +     * manually.  At the end of this function, aio_enable_external
> +     * must be done on ctx, not on new_context, so we cannot call
> +     * bdrv_drained_end there.  And because bs->quiesce_counter will
> +     * not be decreased there, we have to "explode" bdrv_drained_begin
> +     * here too.
> +     *
> +     * Once aio_disable_external and aio_enable_external are replaced
> +     * by BlockBackend device ops (called by .drained_begin/end
> +     * BdrvChildRole callbacks), this can be replaced by a simple
> +     * bdrv_drained_begin/end pair.
> +     */
>      aio_disable_external(ctx);
>      bdrv_parent_drained_begin(bs);
>      bdrv_drain(bs); /* ensure there are no in-flight requests */

This patch was necessary to fix the earlier crash so it is already merged with
my last pull request for -rc4:

commit aabf591007a83dc6520329bce929570115849286
Author: Fam Zheng <famz@redhat.com>
Date:   Wed Apr 5 14:44:24 2017 +0800

    block: Quiesce old aio context during bdrv_set_aio_context
    
    The fact that the bs->aio_context is changing can confuse the dataplane
    iothread, because of the now fine granularity aio context lock.
    bdrv_drain should rather be a bdrv_drained_begin/end pair, but since
    bs->aio_context is changing, we can just use aio_disable_external and
    bdrv_parent_drained_begin.
    
    Reported-by: Ed Swierk <eswierk@skyportsystems.com>
    Signed-off-by: Fam Zheng <famz@redhat.com>
    Acked-by: Stefan Hajnoczi <stefanha@redhat.com>
    Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
    Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Paolo, maybe you want to submit a formal patch for 2.10? (Sorry for the
confusion, these series of mine went a bit messy.)

Fam