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

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

[Fam Zheng]

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(-)

-- 
2.9.3

[Qemu-devel] [PATCH v3 1/2] block: Make bdrv_parent_drained_begin/end public

[Fam Zheng]

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/io.c            |  4 ++--
 include/block/block.h | 16 ++++++++++++++++
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/block/io.c b/block/io.c
index 7321dda..9598646 100644
--- a/block/io.c
+++ b/block/io.c
@@ -44,7 +44,7 @@ static void coroutine_fn bdrv_co_do_rw(void *opaque);
 static int coroutine_fn bdrv_co_do_pwrite_zeroes(BlockDriverState *bs,
     int64_t offset, int count, BdrvRequestFlags flags);
 
-static void bdrv_parent_drained_begin(BlockDriverState *bs)
+void bdrv_parent_drained_begin(BlockDriverState *bs)
 {
     BdrvChild *c;
 
@@ -55,7 +55,7 @@ static void bdrv_parent_drained_begin(BlockDriverState *bs)
     }
 }
 
-static void bdrv_parent_drained_end(BlockDriverState *bs)
+void bdrv_parent_drained_end(BlockDriverState *bs)
 {
     BdrvChild *c;
 
diff --git a/include/block/block.h b/include/block/block.h
index 3e09222..488a07e 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -573,6 +573,22 @@ void bdrv_io_plug(BlockDriverState *bs);
 void bdrv_io_unplug(BlockDriverState *bs);
 
 /**
+ * bdrv_parent_drained_begin:
+ *
+ * Begin a quiesced section of all users of @bs. This is part of
+ * bdrv_drained_begin.
+ */
+void bdrv_parent_drained_begin(BlockDriverState *bs);
+
+/**
+ * bdrv_parent_drained_end:
+ *
+ * End a quiesced section of all users of @bs. This is part of
+ * bdrv_drained_end.
+ */
+void bdrv_parent_drained_end(BlockDriverState *bs);
+
+/**
  * bdrv_drained_begin:
  *
  * Begin a quiesced section for exclusive access to the BDS, by disabling
-- 
2.9.3

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

[Fam Zheng]

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>
---
 block.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index b8a3011..a995a8e 100644
--- a/block.c
+++ b/block.c
@@ -4396,11 +4396,12 @@ void bdrv_attach_aio_context(BlockDriverState *bs,
 
 void bdrv_set_aio_context(BlockDriverState *bs, AioContext *new_context)
 {
-    AioContext *ctx;
+    AioContext *ctx = bdrv_get_aio_context(bs);
 
+    aio_disable_external(ctx);
+    bdrv_parent_drained_begin(bs);
     bdrv_drain(bs); /* ensure there are no in-flight requests */
 
-    ctx = bdrv_get_aio_context(bs);
     while (aio_poll(ctx, false)) {
         /* wait for all bottom halves to execute */
     }
@@ -4412,6 +4413,8 @@ void bdrv_set_aio_context(BlockDriverState *bs, AioContext *new_context)
      */
     aio_context_acquire(new_context);
     bdrv_attach_aio_context(bs, new_context);
+    bdrv_parent_drained_end(bs);
+    aio_enable_external(ctx);
     aio_context_release(new_context);
 }
 
-- 
2.9.3

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 652 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

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

[Paolo Bonzini]

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 */

Thanks,

Paolo

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

[Fam Zheng]

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

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

[Paolo Bonzini]

On 13/04/2017 15:21, Fam Zheng wrote:
> Paolo, maybe you want to submit a formal patch for 2.10? (Sorry for the
> confusion, these series of mine went a bit messy.)

Yes, will do.  Or just submit the alternative fix that is mentioned in
the comment.

Paolo