[RFC PATCH 0/5] Removal of AioContext lock, bs->parents and ->children: proof of concept

[RFC PATCH 0/5] Removal of AioContext lock, bs->parents and ->children: proof of concept

[Emanuele Giuseppe Esposito]

This serie tries to provide a proof of concept and a clear explanation
on why we need to use drains (and more precisely subtree_drains)
to replace the aiocontext lock, especially to protect BlockDriverState
->children and ->parent lists.

Just a small recap on the key concepts:
* We split block layer APIs in "global state" (GS), "I/O", and
"global state or I/O".
  GS are running in the main loop, under BQL, and are the only
  one allowed to modify the BlockDriverState graph.

  I/O APIs are thread safe and can run in any thread

  "global state or I/O" are essentially all APIs that use
  BDRV_POLL_WHILE. This is because there can be only 2 threads
  that can use BDRV_POLL_WHILE: main loop and the iothread that
  runs the aiocontext.

* Drains allow the caller (either main loop or iothread running
the context) to wait all in_flights requests and operations
of a BDS: normal drains target a given node and is parents, while
subtree ones also include the subgraph of the node. Siblings are
not affected by any of these two kind of drains.
After bdrv_drained_begin, no more request is allowed to come
from the affected nodes. Therefore the only actor left working
on a drained part of the graph should be the main loop.

What do we intend to do
-----------------------
We want to remove the AioContext lock. It is not 100% clear on how
many things we are protecting with it, and why.
As a starter, we want to protect BlockDriverState ->parents and
->children lists, since they are read by main loop and I/O but
only written by main loop under BQL. The function that modifies
these lists is bdrv_replace_child_common().

How do we want to do it
-----------------------
We individuated as ideal subtitute of AioContext lock
the subtree_drain API. The reason is simple: draining prevents the iothread to read or write the nodes, so once the main loop finishes
executing bdrv_drained_begin() on the interested graph, we are sure that
the iothread is not going to look or even interfere with that part of the graph.
We are also sure that the only two actors that can look at a specific
BlockDriverState in any given context are the main loop and the
iothread running the AioContext (ensured by "global state or IO" logic).

Why use _subtree_ instead of normal drain
-----------------------------------------
A simple drain "blocks" a given node and all its parents.
But it doesn't touch the child.
This means that if we use a simple drain, a child can always
keep processing requests, and eventually end up calling itself
bdrv_drained_begin, ending up reading the parents node while the main loop
is modifying them. Therefore a subtree drain is necessary.

Possible scenarios
-------------------
Keeping in mind that we can only have an iothread and the main loop
draining on a certain node, we could have:

main loop successfully drains and then iothread tries to drain:
  impossible scenario, as iothread is already stopped once main
  successfully drains.

iothread successfully drains and then main loop drains:
  should not be a problem, as:
  1) the iothread should be already "blocked" by its own drain
  2) main loop would still wait for it to completely block
  There is the issue of mirror overriding such scenario to avoid
  having deadlocks, but that is handled in the next section.

main loop and iothread try to drain together:
  As above, this case doens't really matter. As long as
  bdrv_drained_begin invariant is respected, the main loop will
  continue only once the iothread is "blocked" on that part of the graph.

A note on iothread draining
---------------------------
Theoretically draining from an iothread should not be possible,
as the iothread would be scheduling a bh in the main loop waiting
for itself to stop, even though it is not yet stopped since it is waiting for the bh.

This is what would happen in the tests in patch 5 if .drained_poll
was not implemented.

Therefore, one solution is to use .drained_poll callback in BlockJobDriver.
This callback overrides the default job poll() behavior, and
allows the polling condition to stop waiting for the job.
It is actually used only in mirror.
This however breaks bdrv_drained_begin invariant, because the
iothread is not really blocked on that node but continues running.
In order to fix this, patch 4 allows the polling condition to be
used only by the iothread, and not the main loop too, preventing
the drain to return before the iothread is effectively stopped.
This is also shown in the tests in patch 5. If the fix in patch
4 is removed, then the main loop drain will return earlier and
allow the iothread to run and drain together.

The other patches in this serie are cherry-picked from the various
series I already sent, and are included here just to allow
subtree_drained_begin/end_unlocked implementation.

Emanuele Giuseppe Esposito (5):
  aio-wait.h: introduce AIO_WAIT_WHILE_UNLOCKED
  introduce BDRV_POLL_WHILE_UNLOCKED
  block/io.c: introduce bdrv_subtree_drained_{begin/end}_unlocked
  child_job_drained_poll: override polling condition only when in home
    thread
  test-bdrv-drain: ensure draining from main loop stops iothreads

 block/io.c                   |  48 ++++++--
 blockjob.c                   |   3 +-
 include/block/aio-wait.h     |  15 ++-
 include/block/block.h        |   7 ++
 tests/unit/test-bdrv-drain.c | 218 +++++++++++++++++++++++++++++++++++
 5 files changed, 274 insertions(+), 17 deletions(-)

-- 
2.31.1

[RFC PATCH 1/5] aio-wait.h: introduce AIO_WAIT_WHILE_UNLOCKED

[Emanuele Giuseppe Esposito]

Same as AIO_WAIT_WHILE macro, but if we are in the Main loop
do not release and then acquire ctx_ 's aiocontext.

Once all Aiocontext locks go away, this macro will replace
AIO_WAIT_WHILE.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 include/block/aio-wait.h | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/include/block/aio-wait.h b/include/block/aio-wait.h
index b39eefb38d..ff27fe4eab 100644
--- a/include/block/aio-wait.h
+++ b/include/block/aio-wait.h
@@ -59,10 +59,11 @@ typedef struct {
 extern AioWait global_aio_wait;
 
 /**
- * AIO_WAIT_WHILE:
+ * _AIO_WAIT_WHILE:
  * @ctx: the aio context, or NULL if multiple aio contexts (for which the
  *       caller does not hold a lock) are involved in the polling condition.
  * @cond: wait while this conditional expression is true
+ * @unlock: whether to unlock and then lock again @ctx
  *
  * Wait while a condition is true.  Use this to implement synchronous
  * operations that require event loop activity.
@@ -75,7 +76,7 @@ extern AioWait global_aio_wait;
  * wait on conditions between two IOThreads since that could lead to deadlock,
  * go via the main loop instead.
  */
-#define AIO_WAIT_WHILE(ctx, cond) ({                               \
+#define _AIO_WAIT_WHILE(ctx, cond, unlock) ({                      \
     bool waited_ = false;                                          \
     AioWait *wait_ = &global_aio_wait;                             \
     AioContext *ctx_ = (ctx);                                      \
@@ -90,11 +91,11 @@ extern AioWait global_aio_wait;
         assert(qemu_get_current_aio_context() ==                   \
                qemu_get_aio_context());                            \
         while ((cond)) {                                           \
-            if (ctx_) {                                            \
+            if (unlock && ctx_) {                                  \
                 aio_context_release(ctx_);                         \
             }                                                      \
             aio_poll(qemu_get_aio_context(), true);                \
-            if (ctx_) {                                            \
+            if (unlock && ctx_) {                                  \
                 aio_context_acquire(ctx_);                         \
             }                                                      \
             waited_ = true;                                        \
@@ -103,6 +104,12 @@ extern AioWait global_aio_wait;
     qatomic_dec(&wait_->num_waiters);                              \
     waited_; })
 
+#define AIO_WAIT_WHILE(ctx, cond)                                  \
+    _AIO_WAIT_WHILE(ctx, cond, true)
+
+#define AIO_WAIT_WHILE_UNLOCKED(ctx, cond)                         \
+    _AIO_WAIT_WHILE(ctx, cond, false)
+
 /**
  * aio_wait_kick:
  * Wake up the main thread if it is waiting on AIO_WAIT_WHILE().  During
-- 
2.31.1

Re: [RFC PATCH 1/5] aio-wait.h: introduce AIO_WAIT_WHILE_UNLOCKED

[Stefan Hajnoczi]

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

On Tue, Mar 01, 2022 at 09:21:09AM -0500, Emanuele Giuseppe Esposito wrote:
> Same as AIO_WAIT_WHILE macro, but if we are in the Main loop
> do not release and then acquire ctx_ 's aiocontext.
> 
> Once all Aiocontext locks go away, this macro will replace
> AIO_WAIT_WHILE.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>  include/block/aio-wait.h | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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

[RFC PATCH 2/5] introduce BDRV_POLL_WHILE_UNLOCKED

[Emanuele Giuseppe Esposito]

Same as BDRV_POLL_WHILE, but uses AIO_WAIT_WHILE_UNLOCKED.
See doc comment for more info.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 include/block/block.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/block/block.h b/include/block/block.h
index e1713ee306..5a7a850c16 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -512,6 +512,11 @@ void bdrv_drain_all(void);
     AIO_WAIT_WHILE(bdrv_get_aio_context(bs_),              \
                    cond); })
 
+#define BDRV_POLL_WHILE_UNLOCKED(bs, cond) ({              \
+    BlockDriverState *bs_ = (bs);                          \
+    AIO_WAIT_WHILE_UNLOCKED(bdrv_get_aio_context(bs_),     \
+                            cond); })
+
 int generated_co_wrapper bdrv_pdiscard(BdrvChild *child, int64_t offset,
                                        int64_t bytes);
 int bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int64_t bytes);
-- 
2.31.1

Re: [RFC PATCH 2/5] introduce BDRV_POLL_WHILE_UNLOCKED

[Stefan Hajnoczi]

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

On Tue, Mar 01, 2022 at 09:21:10AM -0500, Emanuele Giuseppe Esposito wrote:
> Same as BDRV_POLL_WHILE, but uses AIO_WAIT_WHILE_UNLOCKED.
> See doc comment for more info.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>  include/block/block.h | 5 +++++
>  1 file changed, 5 insertions(+)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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

Re: [RFC PATCH 2/5] introduce BDRV_POLL_WHILE_UNLOCKED

[Eric Blake]

On Tue, Mar 01, 2022 at 09:21:10AM -0500, Emanuele Giuseppe Esposito wrote:
> Same as BDRV_POLL_WHILE, but uses AIO_WAIT_WHILE_UNLOCKED.
> See doc comment for more info.

This sentence implies there is a doc comment...

> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>  include/block/block.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/include/block/block.h b/include/block/block.h
> index e1713ee306..5a7a850c16 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -512,6 +512,11 @@ void bdrv_drain_all(void);
>      AIO_WAIT_WHILE(bdrv_get_aio_context(bs_),              \
>                     cond); })
>  
> +#define BDRV_POLL_WHILE_UNLOCKED(bs, cond) ({              \
> +    BlockDriverState *bs_ = (bs);                          \
> +    AIO_WAIT_WHILE_UNLOCKED(bdrv_get_aio_context(bs_),     \
> +                            cond); })
> +

but none is added here.  I'm presuming that the comment is already in
the file (not shown in the limited context view here), but it may be
worth tweaking the commit message to mention that.

>  int generated_co_wrapper bdrv_pdiscard(BdrvChild *child, int64_t offset,
>                                         int64_t bytes);
>  int bdrv_co_pdiscard(BdrvChild *child, int64_t offset, int64_t bytes);
> -- 
> 2.31.1
> 
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

[RFC PATCH 3/5] block/io.c: introduce bdrv_subtree_drained_{begin/end}_unlocked

[Emanuele Giuseppe Esposito]

Same as the locked version, but use BDRV_POLL_UNLOCKED.
We are going to add drains to all graph modifications, and they are
generally performed without the AioContext lock taken.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 block/io.c            | 48 ++++++++++++++++++++++++++++++++-----------
 include/block/block.h |  2 ++
 2 files changed, 38 insertions(+), 12 deletions(-)

diff --git a/block/io.c b/block/io.c
index 4e4cb556c5..d474449d2d 100644
--- a/block/io.c
+++ b/block/io.c
@@ -242,6 +242,7 @@ typedef struct {
     bool begin;
     bool recursive;
     bool poll;
+    bool unlocked;
     BdrvChild *parent;
     bool ignore_bds_parents;
     int *drained_end_counter;
@@ -332,7 +333,7 @@ static bool bdrv_drain_poll_top_level(BlockDriverState *bs, bool recursive,
 
 static void bdrv_do_drained_begin(BlockDriverState *bs, bool recursive,
                                   BdrvChild *parent, bool ignore_bds_parents,
-                                  bool poll);
+                                  bool poll, bool unlocked);
 static void bdrv_do_drained_end(BlockDriverState *bs, bool recursive,
                                 BdrvChild *parent, bool ignore_bds_parents,
                                 int *drained_end_counter);
@@ -350,7 +351,8 @@ static void bdrv_co_drain_bh_cb(void *opaque)
         if (data->begin) {
             assert(!data->drained_end_counter);
             bdrv_do_drained_begin(bs, data->recursive, data->parent,
-                                  data->ignore_bds_parents, data->poll);
+                                  data->ignore_bds_parents, data->poll,
+                                  data->unlocked);
         } else {
             assert(!data->poll);
             bdrv_do_drained_end(bs, data->recursive, data->parent,
@@ -372,6 +374,7 @@ static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs,
                                                 BdrvChild *parent,
                                                 bool ignore_bds_parents,
                                                 bool poll,
+                                                bool unlocked,
                                                 int *drained_end_counter)
 {
     BdrvCoDrainData data;
@@ -392,6 +395,7 @@ static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs,
         .parent = parent,
         .ignore_bds_parents = ignore_bds_parents,
         .poll = poll,
+        .unlocked = unlocked,
         .drained_end_counter = drained_end_counter,
     };
 
@@ -439,13 +443,13 @@ void bdrv_do_drained_begin_quiesce(BlockDriverState *bs,
 
 static void bdrv_do_drained_begin(BlockDriverState *bs, bool recursive,
                                   BdrvChild *parent, bool ignore_bds_parents,
-                                  bool poll)
+                                  bool poll, bool unlocked)
 {
     BdrvChild *child, *next;
 
     if (qemu_in_coroutine()) {
         bdrv_co_yield_to_drain(bs, true, recursive, parent, ignore_bds_parents,
-                               poll, NULL);
+                               poll, unlocked, NULL);
         return;
     }
 
@@ -456,7 +460,7 @@ static void bdrv_do_drained_begin(BlockDriverState *bs, bool recursive,
         bs->recursive_quiesce_counter++;
         QLIST_FOREACH_SAFE(child, &bs->children, next, next) {
             bdrv_do_drained_begin(child->bs, true, child, ignore_bds_parents,
-                                  false);
+                                  false, false);
         }
     }
 
@@ -471,18 +475,30 @@ static void bdrv_do_drained_begin(BlockDriverState *bs, bool recursive,
      */
     if (poll) {
         assert(!ignore_bds_parents);
-        BDRV_POLL_WHILE(bs, bdrv_drain_poll_top_level(bs, recursive, parent));
+        if (unlocked) {
+            BDRV_POLL_WHILE_UNLOCKED(bs,
+                                     bdrv_drain_poll_top_level(bs, recursive,
+                                                               parent));
+        } else {
+            BDRV_POLL_WHILE(bs,
+                            bdrv_drain_poll_top_level(bs, recursive, parent));
+        }
     }
 }
 
 void bdrv_drained_begin(BlockDriverState *bs)
 {
-    bdrv_do_drained_begin(bs, false, NULL, false, true);
+    bdrv_do_drained_begin(bs, false, NULL, false, true, false);
 }
 
 void bdrv_subtree_drained_begin(BlockDriverState *bs)
 {
-    bdrv_do_drained_begin(bs, true, NULL, false, true);
+    bdrv_do_drained_begin(bs, true, NULL, false, true, false);
+}
+
+void bdrv_subtree_drained_begin_unlocked(BlockDriverState *bs)
+{
+    bdrv_do_drained_begin(bs, true, NULL, false, true, true);
 }
 
 /**
@@ -510,7 +526,7 @@ static void bdrv_do_drained_end(BlockDriverState *bs, bool recursive,
 
     if (qemu_in_coroutine()) {
         bdrv_co_yield_to_drain(bs, false, recursive, parent, ignore_bds_parents,
-                               false, drained_end_counter);
+                               false, false, drained_end_counter);
         return;
     }
     assert(bs->quiesce_counter > 0);
@@ -554,12 +570,19 @@ void bdrv_subtree_drained_end(BlockDriverState *bs)
     BDRV_POLL_WHILE(bs, qatomic_read(&drained_end_counter) > 0);
 }
 
+void bdrv_subtree_drained_end_unlocked(BlockDriverState *bs)
+{
+    int drained_end_counter = 0;
+    bdrv_do_drained_end(bs, true, NULL, false, &drained_end_counter);
+    BDRV_POLL_WHILE_UNLOCKED(bs, qatomic_read(&drained_end_counter) > 0);
+}
+
 void bdrv_apply_subtree_drain(BdrvChild *child, BlockDriverState *new_parent)
 {
     int i;
 
     for (i = 0; i < new_parent->recursive_quiesce_counter; i++) {
-        bdrv_do_drained_begin(child->bs, true, child, false, true);
+        bdrv_do_drained_begin(child->bs, true, child, false, true, false);
     }
 }
 
@@ -642,7 +665,8 @@ void bdrv_drain_all_begin(void)
     BlockDriverState *bs = NULL;
 
     if (qemu_in_coroutine()) {
-        bdrv_co_yield_to_drain(NULL, true, false, NULL, true, true, NULL);
+        bdrv_co_yield_to_drain(NULL, true, false, NULL, true, true, false,
+                               NULL);
         return;
     }
 
@@ -667,7 +691,7 @@ void bdrv_drain_all_begin(void)
         AioContext *aio_context = bdrv_get_aio_context(bs);
 
         aio_context_acquire(aio_context);
-        bdrv_do_drained_begin(bs, false, NULL, true, false);
+        bdrv_do_drained_begin(bs, false, NULL, true, false, false);
         aio_context_release(aio_context);
     }
 
diff --git a/include/block/block.h b/include/block/block.h
index 5a7a850c16..072bde370f 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -780,6 +780,7 @@ void bdrv_do_drained_begin_quiesce(BlockDriverState *bs,
  * exclusive access to all child nodes as well.
  */
 void bdrv_subtree_drained_begin(BlockDriverState *bs);
+void bdrv_subtree_drained_begin_unlocked(BlockDriverState *bs);
 
 /**
  * bdrv_drained_end:
@@ -812,6 +813,7 @@ void bdrv_drained_end_no_poll(BlockDriverState *bs, int *drained_end_counter);
  * End a quiescent section started by bdrv_subtree_drained_begin().
  */
 void bdrv_subtree_drained_end(BlockDriverState *bs);
+void bdrv_subtree_drained_end_unlocked(BlockDriverState *bs);
 
 void bdrv_add_child(BlockDriverState *parent, BlockDriverState *child,
                     Error **errp);
-- 
2.31.1

Re: [RFC PATCH 3/5] block/io.c: introduce bdrv_subtree_drained_{begin/end}_unlocked

[Stefan Hajnoczi]

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

On Tue, Mar 01, 2022 at 09:21:11AM -0500, Emanuele Giuseppe Esposito wrote:
> Same as the locked version, but use BDRV_POLL_UNLOCKED.
> We are going to add drains to all graph modifications, and they are
> generally performed without the AioContext lock taken.
> 
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>  block/io.c            | 48 ++++++++++++++++++++++++++++++++-----------
>  include/block/block.h |  2 ++
>  2 files changed, 38 insertions(+), 12 deletions(-)

Looks okay but I hope we can get rid of the non-_unlocked versions soon
to avoid adding extra APIs and supporting both locked and unlocked code
paths.

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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

[RFC PATCH 4/5] child_job_drained_poll: override polling condition only when in home thread

[Emanuele Giuseppe Esposito]

drv->drained_poll() is only implemented in mirror, and allows
it to drain from the coroutine. The mirror implementation uses
in_drain flag to recognize when it is draining from coroutine,
and consequently avoid deadlocking (wait the poll condition in
child_job_drained_poll to wait for itself).

The problem is that this flag is dangerous, because it breaks
bdrv_drained_begin() invariants: once drained_begin ends, all
jobs, in_flight requests, and anything running in the iothread
are blocked.

This can be broken in such way:
iothread(mirror): s->in_drain = true; // mirror.c:1112
main loop: bdrv_drained_begin(mirror_bs);
/*
 * drained_begin wait for bdrv_drain_poll_top_level() condition,
 * that translates in child_job_drained_poll() for jobs, but
 * mirror implements drv->drained_poll() so it returns
 * !!in_flight_requests, which his 0 (assertion in mirror.c:1105).
 */
main loop: thinks iothread is stopped and is modifying the graph...
iothread(mirror): *continues*, as nothing is stopping it
iothread(mirror): bdrv_drained_begin(bs);
/* draining reads the graph while it is modified!! */
main loop: done modifying the graph...

In order to fix this, we can simply allow drv->drained_poll()
to be called only by the iothread, and not the main loop.
We distinguish it by using in_aio_context_home_thread(), that
returns false if @ctx is not the same as the thread that runs it.

Co-Developed-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 blockjob.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/blockjob.c b/blockjob.c
index 10815a89fe..e132d9587e 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -107,6 +107,7 @@ static bool child_job_drained_poll(BdrvChild *c)
     BlockJob *bjob = c->opaque;
     Job *job = &bjob->job;
     const BlockJobDriver *drv = block_job_driver(bjob);
+    AioContext *ctx = block_job_get_aio_context(bjob);
 
     /* An inactive or completed job doesn't have any pending requests. Jobs
      * with !job->busy are either already paused or have a pause point after
@@ -117,7 +118,7 @@ static bool child_job_drained_poll(BdrvChild *c)
 
     /* Otherwise, assume that it isn't fully stopped yet, but allow the job to
      * override this assumption. */
-    if (drv->drained_poll) {
+    if (in_aio_context_home_thread(ctx) && drv->drained_poll) {
         return drv->drained_poll(bjob);
     } else {
         return true;
-- 
2.31.1

Re: [RFC PATCH 4/5] child_job_drained_poll: override polling condition only when in home thread

[Stefan Hajnoczi]

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

On Tue, Mar 01, 2022 at 09:21:12AM -0500, Emanuele Giuseppe Esposito wrote:
> drv->drained_poll() is only implemented in mirror, and allows
> it to drain from the coroutine. The mirror implementation uses
> in_drain flag to recognize when it is draining from coroutine,
> and consequently avoid deadlocking (wait the poll condition in
> child_job_drained_poll to wait for itself).
> 
> The problem is that this flag is dangerous, because it breaks
> bdrv_drained_begin() invariants: once drained_begin ends, all
> jobs, in_flight requests, and anything running in the iothread
> are blocked.
> 
> This can be broken in such way:
> iothread(mirror): s->in_drain = true; // mirror.c:1112
> main loop: bdrv_drained_begin(mirror_bs);
> /*
>  * drained_begin wait for bdrv_drain_poll_top_level() condition,
>  * that translates in child_job_drained_poll() for jobs, but
>  * mirror implements drv->drained_poll() so it returns
>  * !!in_flight_requests, which his 0 (assertion in mirror.c:1105).
>  */
> main loop: thinks iothread is stopped and is modifying the graph...
> iothread(mirror): *continues*, as nothing is stopping it
> iothread(mirror): bdrv_drained_begin(bs);
> /* draining reads the graph while it is modified!! */
> main loop: done modifying the graph...
> 
> In order to fix this, we can simply allow drv->drained_poll()
> to be called only by the iothread, and not the main loop.
> We distinguish it by using in_aio_context_home_thread(), that
> returns false if @ctx is not the same as the thread that runs it.
> 
> Co-Developed-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
> ---
>  blockjob.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/blockjob.c b/blockjob.c
> index 10815a89fe..e132d9587e 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -107,6 +107,7 @@ static bool child_job_drained_poll(BdrvChild *c)
>      BlockJob *bjob = c->opaque;
>      Job *job = &bjob->job;
>      const BlockJobDriver *drv = block_job_driver(bjob);
> +    AioContext *ctx = block_job_get_aio_context(bjob);
>  
>      /* An inactive or completed job doesn't have any pending requests. Jobs
>       * with !job->busy are either already paused or have a pause point after
> @@ -117,7 +118,7 @@ static bool child_job_drained_poll(BdrvChild *c)
>  
>      /* Otherwise, assume that it isn't fully stopped yet, but allow the job to
>       * override this assumption. */
> -    if (drv->drained_poll) {
> +    if (in_aio_context_home_thread(ctx) && drv->drained_poll) {
>          return drv->drained_poll(bjob);

The purpose of in_aio_context_home_thread() is not obvious from the
code. Please add a comment explaining this case.

Thanks,
Stefan

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

[RFC PATCH 5/5] test-bdrv-drain: ensure draining from main loop stops iothreads

[Emanuele Giuseppe Esposito]

Add 2 tests: test_main_and_then_iothread_drain ensures that if the
main thread drains, the iothread cannot drain (and thus read
the graph). test_main_and_iothread_drain instead lets main loop
and iothread to drain together, and makes sure that no drain
happens in parallel.

Note that we are using bdrv_subtree_drained_{begin/end}_unlocked
to try avoid using the aiocontext lock as much as possible, since
it will eventually go away.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 tests/unit/test-bdrv-drain.c | 218 +++++++++++++++++++++++++++++++++++
 1 file changed, 218 insertions(+)

diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c
index 36be84ae55..bf7fdcb568 100644
--- a/tests/unit/test-bdrv-drain.c
+++ b/tests/unit/test-bdrv-drain.c
@@ -1534,6 +1534,219 @@ static void test_set_aio_context(void)
     iothread_join(b);
 }
 
+typedef struct ParallelDrainJob {
+    BlockJob common;
+    BlockBackend *blk;
+    BlockDriverState *bs;
+    IOThread *a;
+    bool should_fail;
+    bool allowed_to_run;
+    bool conclude_test;
+    bool job_started;
+} ParallelDrainJob;
+
+typedef struct BDRVParallelTestState {
+    ParallelDrainJob *job;
+} BDRVParallelTestState;
+
+static void coroutine_fn bdrv_test_par_co_drain(BlockDriverState *bs)
+{
+    BDRVParallelTestState *s = bs->opaque;
+    ParallelDrainJob *job = s->job;
+    assert(!qatomic_read(&job->should_fail));
+}
+
+static int parallel_run_test(Job *job, Error **errp)
+{
+    ParallelDrainJob *s = container_of(job, ParallelDrainJob, common.job);
+    s->job_started = true;
+
+    while (!s->conclude_test) {
+        if (s->allowed_to_run) {
+            bdrv_subtree_drained_begin_unlocked(s->bs);
+            bdrv_subtree_drained_end_unlocked(s->bs);
+        }
+        job_pause_point(&s->common.job);
+    }
+    s->job_started = false;
+
+    return 0;
+}
+
+static BlockDriver bdrv_test_parallel = {
+    .format_name            = "test",
+    .instance_size          = sizeof(BDRVParallelTestState),
+    .supports_backing       = true,
+
+    .bdrv_co_drain_begin    = bdrv_test_par_co_drain,
+    .bdrv_co_drain_end      = bdrv_test_par_co_drain,
+
+    .bdrv_child_perm        = bdrv_default_perms,
+};
+
+static bool parallel_drained_poll(BlockJob *job)
+{
+    /* need to return false otherwise a drain in coroutine deadlocks */
+    return false;
+}
+
+static const BlockJobDriver test_block_job_driver_parallel = {
+    .job_driver = {
+        .instance_size = sizeof(ParallelDrainJob),
+        .run           = parallel_run_test,
+        .user_resume   = block_job_user_resume,
+        .free          = block_job_free,
+    },
+    .drained_poll      = parallel_drained_poll,
+};
+
+static ParallelDrainJob *setup_job_test(void)
+{
+    BlockBackend *blk;
+    BlockDriverState *bs;
+    Error *err = NULL;
+    IOThread *a = iothread_new();
+    AioContext *ctx_a = iothread_get_aio_context(a);
+    ParallelDrainJob *s;
+    BDRVParallelTestState *p;
+    int ret;
+
+    blk = blk_new(qemu_get_aio_context(),
+                         BLK_PERM_CONSISTENT_READ, BLK_PERM_ALL);
+    blk_set_allow_aio_context_change(blk, true);
+
+    bs = bdrv_new_open_driver(&bdrv_test_parallel, "parent", 0,
+                                     &error_abort);
+    p = bs->opaque;
+
+    ret = blk_insert_bs(blk, bs, &error_abort);
+    assert(ret == 0);
+
+    s = block_job_create("job1", &test_block_job_driver_parallel,
+                         NULL, blk_bs(blk), 0, BLK_PERM_ALL, 0, JOB_DEFAULT,
+                         NULL, NULL, &err);
+    s->bs = bs;
+    s->a = a;
+    s->blk = blk;
+    p->job = s;
+
+    ret = bdrv_try_set_aio_context(bs, ctx_a, &error_abort);
+    assert(ret == 0);
+    assert(blk_get_aio_context(blk) == ctx_a);
+    assert(s->common.job.aio_context == ctx_a);
+
+    return s;
+}
+
+static void stop_and_tear_down_test(ParallelDrainJob *s)
+{
+    AioContext *ctx = iothread_get_aio_context(s->a);
+
+    /* stop iothread */
+    s->conclude_test = true;
+
+    /* wait that it's stopped */
+    while (s->job_started) {
+        aio_poll(qemu_get_current_aio_context(), false);
+    }
+
+    aio_context_acquire(ctx);
+    bdrv_unref(s->bs);
+    blk_unref(s->blk);
+    aio_context_release(ctx);
+    iothread_join(s->a);
+}
+
+/**
+ * test_main_and_then_iothread_drain: test that if the main
+ * loop drains, iothread cannot possibly drain.
+ *
+ * In this test, make sure that the main loop has firstly
+ * drained, and then allow the iothread to try and drain.
+ *
+ * Therefore, if the main loop drains, there is no way that
+ * the graph can be read or written by the iothread.
+ */
+static void test_main_and_then_iothread_drain(void)
+{
+    ParallelDrainJob *s = setup_job_test();
+
+    s->allowed_to_run = false;
+    job_start(&s->common.job);
+
+    /*
+     * Wait that the iothread starts, even though it just
+     * loops without doing anything (allowed_to_run is false)
+     */
+    while (!s->job_started) {
+        aio_poll(qemu_get_current_aio_context(), false);
+    }
+
+    /*
+     * Drain in the main loop and ensure that no drain
+     * is performed by the iothread.
+     */
+    bdrv_subtree_drained_begin_unlocked(s->bs);
+    /* iothread should be put */
+    qatomic_set(&s->should_fail, true);
+    /* let the iothread do drains */
+    s->allowed_to_run = true;
+
+    /* [perform graph modifications here], as iothread is stopped */
+    sleep(3);
+
+    /* done with modifications, let the iothread drain once it resumes */
+    qatomic_set(&s->should_fail, false);
+
+    /* drained_end should resume the iothread */
+    bdrv_subtree_drained_end_unlocked(s->bs);
+
+    stop_and_tear_down_test(s);
+}
+
+/**
+ * test_main_and_iothread_drain: test that if the main
+ * loop drains, iothread cannot possibly drain.
+ *
+ * In this test, simply let iothread and main loop concurrenly
+ * loop and drain together, making sure that iothread never
+ * reads the graph while main loop is draining.
+ *
+ * Therefore, if the main loop drains, there is no way that
+ * the graph can be read or written by the iothread.
+ */
+static void test_main_and_iothread_drain(void)
+{
+    ParallelDrainJob *s = setup_job_test();
+    int i;
+
+    /* let the iothread do drains from beginning */
+    s->allowed_to_run = true;
+    job_start(&s->common.job);
+
+    /* wait that the iothread starts and begins with drains. */
+    while (!s->job_started) {
+        aio_poll(qemu_get_current_aio_context(), false);
+    }
+
+    /* at the same time, also main loop performs drains */
+    for (i = 0; i < 1000; i++) {
+        bdrv_subtree_drained_begin_unlocked(s->bs);
+        /* iothread should be put, regardless of when it drained */
+        qatomic_set(&s->should_fail, true);
+
+        /* [perform graph modifications here] */
+
+        /* done with modifications, let the iothread drain once it resumes */
+        qatomic_set(&s->should_fail, false);
+
+        /* drained_end should resume the iothread */
+        bdrv_subtree_drained_end_unlocked(s->bs);
+    }
+
+    stop_and_tear_down_test(s);
+}
+
 
 typedef struct TestDropBackingBlockJob {
     BlockJob common;
@@ -2185,6 +2398,11 @@ int main(int argc, char **argv)
     g_test_add_func("/bdrv-drain/iothread/drain_subtree",
                     test_iothread_drain_subtree);
 
+    g_test_add_func("/bdrv-drain/iothread/drain_main_and_then_iothread",
+                    test_main_and_then_iothread_drain);
+    g_test_add_func("/bdrv-drain/iothread/drain_parallel",
+                    test_main_and_iothread_drain);
+
     g_test_add_func("/bdrv-drain/blockjob/drain_all", test_blockjob_drain_all);
     g_test_add_func("/bdrv-drain/blockjob/drain", test_blockjob_drain);
     g_test_add_func("/bdrv-drain/blockjob/drain_subtree",
-- 
2.31.1

Re: [RFC PATCH 0/5] Removal of AioContext lock, bs->parents and ->children: proof of concept

[Emanuele Giuseppe Esposito]

I would really love to hear opinions on this, since we already had some
discussions on other similar patches.

Thank you,
Emanuele

On 01/03/2022 15:21, Emanuele Giuseppe Esposito wrote:
> This serie tries to provide a proof of concept and a clear explanation
> on why we need to use drains (and more precisely subtree_drains)
> to replace the aiocontext lock, especially to protect BlockDriverState
> ->children and ->parent lists.
> 
> Just a small recap on the key concepts:
> * We split block layer APIs in "global state" (GS), "I/O", and
> "global state or I/O".
>   GS are running in the main loop, under BQL, and are the only
>   one allowed to modify the BlockDriverState graph.
> 
>   I/O APIs are thread safe and can run in any thread
> 
>   "global state or I/O" are essentially all APIs that use
>   BDRV_POLL_WHILE. This is because there can be only 2 threads
>   that can use BDRV_POLL_WHILE: main loop and the iothread that
>   runs the aiocontext.
> 
> * Drains allow the caller (either main loop or iothread running
> the context) to wait all in_flights requests and operations
> of a BDS: normal drains target a given node and is parents, while
> subtree ones also include the subgraph of the node. Siblings are
> not affected by any of these two kind of drains.
> After bdrv_drained_begin, no more request is allowed to come
> from the affected nodes. Therefore the only actor left working
> on a drained part of the graph should be the main loop.
> 
> What do we intend to do
> -----------------------
> We want to remove the AioContext lock. It is not 100% clear on how
> many things we are protecting with it, and why.
> As a starter, we want to protect BlockDriverState ->parents and
> ->children lists, since they are read by main loop and I/O but
> only written by main loop under BQL. The function that modifies
> these lists is bdrv_replace_child_common().
> 
> How do we want to do it
> -----------------------
> We individuated as ideal subtitute of AioContext lock
> the subtree_drain API. The reason is simple: draining prevents the iothread to read or write the nodes, so once the main loop finishes
> executing bdrv_drained_begin() on the interested graph, we are sure that
> the iothread is not going to look or even interfere with that part of the graph.
> We are also sure that the only two actors that can look at a specific
> BlockDriverState in any given context are the main loop and the
> iothread running the AioContext (ensured by "global state or IO" logic).
> 
> Why use _subtree_ instead of normal drain
> -----------------------------------------
> A simple drain "blocks" a given node and all its parents.
> But it doesn't touch the child.
> This means that if we use a simple drain, a child can always
> keep processing requests, and eventually end up calling itself
> bdrv_drained_begin, ending up reading the parents node while the main loop
> is modifying them. Therefore a subtree drain is necessary.
> 
> Possible scenarios
> -------------------
> Keeping in mind that we can only have an iothread and the main loop
> draining on a certain node, we could have:
> 
> main loop successfully drains and then iothread tries to drain:
>   impossible scenario, as iothread is already stopped once main
>   successfully drains.
> 
> iothread successfully drains and then main loop drains:
>   should not be a problem, as:
>   1) the iothread should be already "blocked" by its own drain
>   2) main loop would still wait for it to completely block
>   There is the issue of mirror overriding such scenario to avoid
>   having deadlocks, but that is handled in the next section.
> 
> main loop and iothread try to drain together:
>   As above, this case doens't really matter. As long as
>   bdrv_drained_begin invariant is respected, the main loop will
>   continue only once the iothread is "blocked" on that part of the graph.
> 
> A note on iothread draining
> ---------------------------
> Theoretically draining from an iothread should not be possible,
> as the iothread would be scheduling a bh in the main loop waiting
> for itself to stop, even though it is not yet stopped since it is waiting for the bh.
> 
> This is what would happen in the tests in patch 5 if .drained_poll
> was not implemented.
> 
> Therefore, one solution is to use .drained_poll callback in BlockJobDriver.
> This callback overrides the default job poll() behavior, and
> allows the polling condition to stop waiting for the job.
> It is actually used only in mirror.
> This however breaks bdrv_drained_begin invariant, because the
> iothread is not really blocked on that node but continues running.
> In order to fix this, patch 4 allows the polling condition to be
> used only by the iothread, and not the main loop too, preventing
> the drain to return before the iothread is effectively stopped.
> This is also shown in the tests in patch 5. If the fix in patch
> 4 is removed, then the main loop drain will return earlier and
> allow the iothread to run and drain together.
> 
> The other patches in this serie are cherry-picked from the various
> series I already sent, and are included here just to allow
> subtree_drained_begin/end_unlocked implementation.
> 
> Emanuele Giuseppe Esposito (5):
>   aio-wait.h: introduce AIO_WAIT_WHILE_UNLOCKED
>   introduce BDRV_POLL_WHILE_UNLOCKED
>   block/io.c: introduce bdrv_subtree_drained_{begin/end}_unlocked
>   child_job_drained_poll: override polling condition only when in home
>     thread
>   test-bdrv-drain: ensure draining from main loop stops iothreads
> 
>  block/io.c                   |  48 ++++++--
>  blockjob.c                   |   3 +-
>  include/block/aio-wait.h     |  15 ++-
>  include/block/block.h        |   7 ++
>  tests/unit/test-bdrv-drain.c | 218 +++++++++++++++++++++++++++++++++++
>  5 files changed, 274 insertions(+), 17 deletions(-)
> 

Re: [RFC PATCH 0/5] Removal of AioContext lock, bs->parents and ->children: proof of concept

[Stefan Hajnoczi]

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

On Tue, Mar 01, 2022 at 09:21:08AM -0500, Emanuele Giuseppe Esposito wrote:
> This serie tries to provide a proof of concept and a clear explanation
> on why we need to use drains (and more precisely subtree_drains)
> to replace the aiocontext lock, especially to protect BlockDriverState
> ->children and ->parent lists.
> 
> Just a small recap on the key concepts:
> * We split block layer APIs in "global state" (GS), "I/O", and
> "global state or I/O".
>   GS are running in the main loop, under BQL, and are the only
>   one allowed to modify the BlockDriverState graph.
> 
>   I/O APIs are thread safe and can run in any thread
> 
>   "global state or I/O" are essentially all APIs that use
>   BDRV_POLL_WHILE. This is because there can be only 2 threads
>   that can use BDRV_POLL_WHILE: main loop and the iothread that
>   runs the aiocontext.
> 
> * Drains allow the caller (either main loop or iothread running
> the context) to wait all in_flights requests and operations
> of a BDS: normal drains target a given node and is parents, while
> subtree ones also include the subgraph of the node. Siblings are
> not affected by any of these two kind of drains.

Siblings are drained to the extent required for their parent node to
reach in_flight == 0.

I haven't checked the code but I guess the case you're alluding to is
that siblings with multiple parents could have other I/O in flight that
will not be drained and further I/O can be submitted after the parent
has drained?

> After bdrv_drained_begin, no more request is allowed to come
> from the affected nodes. Therefore the only actor left working
> on a drained part of the graph should be the main loop.

It's worth remembering that this invariant is not enforced. Draining is
a cooperative scheme. Everything *should* be notified and stop
submitting I/O, but there is no assertion or return -EBUSY if a request
is submitted while the BDS is drained.

If the thread that drained the BDS wants, I think it can (legally)
submit I/O within the drained section.

> What do we intend to do
> -----------------------
> We want to remove the AioContext lock. It is not 100% clear on how
> many things we are protecting with it, and why.
> As a starter, we want to protect BlockDriverState ->parents and
> ->children lists, since they are read by main loop and I/O but
> only written by main loop under BQL. The function that modifies
> these lists is bdrv_replace_child_common().
> 
> How do we want to do it
> -----------------------
> We individuated as ideal subtitute of AioContext lock
> the subtree_drain API. The reason is simple: draining prevents the iothread to read or write the nodes, so once the main loop finishes
> executing bdrv_drained_begin() on the interested graph, we are sure that
> the iothread is not going to look or even interfere with that part of the graph.
> We are also sure that the only two actors that can look at a specific
> BlockDriverState in any given context are the main loop and the
> iothread running the AioContext (ensured by "global state or IO" logic).
> 
> Why use _subtree_ instead of normal drain
> -----------------------------------------
> A simple drain "blocks" a given node and all its parents.
> But it doesn't touch the child.
> This means that if we use a simple drain, a child can always
> keep processing requests, and eventually end up calling itself
> bdrv_drained_begin, ending up reading the parents node while the main loop
> is modifying them. Therefore a subtree drain is necessary.
> 
> Possible scenarios
> -------------------
> Keeping in mind that we can only have an iothread and the main loop
> draining on a certain node, we could have:
> 
> main loop successfully drains and then iothread tries to drain:
>   impossible scenario, as iothread is already stopped once main
>   successfully drains.
> 
> iothread successfully drains and then main loop drains:
>   should not be a problem, as:
>   1) the iothread should be already "blocked" by its own drain

Once drained_begin() returns in the IOThread, the IOThread can do
anything it wants, including more submitting I/O. I don't consider that
"blocked", so I'm not sure what this sentence means?

The way the main loop thread protects itself against the IOThread is via
the aio "external" handler concept and block job drain callbacks, which
are activated by drained_begin(). They ensure that the IOThread will not
perform further processing that submits I/O, but the IOThread code that
invoked drained_begin() can still do anything it wants.

>   2) main loop would still wait for it to completely block
>   There is the issue of mirror overriding such scenario to avoid
>   having deadlocks, but that is handled in the next section.
> 
> main loop and iothread try to drain together:
>   As above, this case doens't really matter. As long as
>   bdrv_drained_begin invariant is respected, the main loop will
>   continue only once the iothread is "blocked" on that part of the graph.

I'm not sure about this, see above.

> 
> A note on iothread draining
> ---------------------------
> Theoretically draining from an iothread should not be possible,
> as the iothread would be scheduling a bh in the main loop waiting
> for itself to stop, even though it is not yet stopped since it is waiting for the bh.

I'm not sure what you mean. Which function schedules this BH?

> 
> This is what would happen in the tests in patch 5 if .drained_poll
> was not implemented.
> 
> Therefore, one solution is to use .drained_poll callback in BlockJobDriver.
> This callback overrides the default job poll() behavior, and
> allows the polling condition to stop waiting for the job.
> It is actually used only in mirror.
> This however breaks bdrv_drained_begin invariant, because the
> iothread is not really blocked on that node but continues running.
> In order to fix this, patch 4 allows the polling condition to be
> used only by the iothread, and not the main loop too, preventing
> the drain to return before the iothread is effectively stopped.
> This is also shown in the tests in patch 5. If the fix in patch
> 4 is removed, then the main loop drain will return earlier and
> allow the iothread to run and drain together.
> 
> The other patches in this serie are cherry-picked from the various
> series I already sent, and are included here just to allow
> subtree_drained_begin/end_unlocked implementation.
> 
> Emanuele Giuseppe Esposito (5):
>   aio-wait.h: introduce AIO_WAIT_WHILE_UNLOCKED
>   introduce BDRV_POLL_WHILE_UNLOCKED
>   block/io.c: introduce bdrv_subtree_drained_{begin/end}_unlocked
>   child_job_drained_poll: override polling condition only when in home
>     thread
>   test-bdrv-drain: ensure draining from main loop stops iothreads
> 
>  block/io.c                   |  48 ++++++--
>  blockjob.c                   |   3 +-
>  include/block/aio-wait.h     |  15 ++-
>  include/block/block.h        |   7 ++
>  tests/unit/test-bdrv-drain.c | 218 +++++++++++++++++++++++++++++++++++
>  5 files changed, 274 insertions(+), 17 deletions(-)
> 
> -- 
> 2.31.1
> 

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

Re: [RFC PATCH 0/5] Removal of AioContext lock, bs->parents and ->children: proof of concept

[Emanuele Giuseppe Esposito]

Am 02/03/2022 um 10:47 schrieb Stefan Hajnoczi:
> On Tue, Mar 01, 2022 at 09:21:08AM -0500, Emanuele Giuseppe Esposito wrote:
>> This serie tries to provide a proof of concept and a clear explanation
>> on why we need to use drains (and more precisely subtree_drains)
>> to replace the aiocontext lock, especially to protect BlockDriverState
>> ->children and ->parent lists.
>>
>> Just a small recap on the key concepts:
>> * We split block layer APIs in "global state" (GS), "I/O", and
>> "global state or I/O".
>>   GS are running in the main loop, under BQL, and are the only
>>   one allowed to modify the BlockDriverState graph.
>>
>>   I/O APIs are thread safe and can run in any thread
>>
>>   "global state or I/O" are essentially all APIs that use
>>   BDRV_POLL_WHILE. This is because there can be only 2 threads
>>   that can use BDRV_POLL_WHILE: main loop and the iothread that
>>   runs the aiocontext.
>>
>> * Drains allow the caller (either main loop or iothread running
>> the context) to wait all in_flights requests and operations
>> of a BDS: normal drains target a given node and is parents, while
>> subtree ones also include the subgraph of the node. Siblings are
>> not affected by any of these two kind of drains.
> 
> Siblings are drained to the extent required for their parent node to
> reach in_flight == 0.
> 
> I haven't checked the code but I guess the case you're alluding to is
> that siblings with multiple parents could have other I/O in flight that
> will not be drained and further I/O can be submitted after the parent
> has drained?

Yes, this in theory can happen. I don't really know if this happens
practically, and how likely is to happen.

The alternative would be to make a drain that blocks the whole graph,
siblings included, but that would probably be an overkill.

> 
>> After bdrv_drained_begin, no more request is allowed to come
>> from the affected nodes. Therefore the only actor left working
>> on a drained part of the graph should be the main loop.
> 
> It's worth remembering that this invariant is not enforced. Draining is
> a cooperative scheme. Everything *should* be notified and stop
> submitting I/O, but there is no assertion or return -EBUSY if a request
> is submitted while the BDS is drained.

Yes, it is a cooperative scheme and all except from mirror (fixed in the
last patch) seem to follow the invariant.
> 
> If the thread that drained the BDS wants, I think it can (legally)
> submit I/O within the drained section.
> 

Yes but at some point the main loop drains too before changing the
graph. Then the thread must be stopped, according with the invariant
above. So it doesn't matter if the thread has already drained or not.

>> What do we intend to do
>> -----------------------
>> We want to remove the AioContext lock. It is not 100% clear on how
>> many things we are protecting with it, and why.
>> As a starter, we want to protect BlockDriverState ->parents and
>> ->children lists, since they are read by main loop and I/O but
>> only written by main loop under BQL. The function that modifies
>> these lists is bdrv_replace_child_common().
>>
>> How do we want to do it
>> -----------------------
>> We individuated as ideal subtitute of AioContext lock
>> the subtree_drain API. The reason is simple: draining prevents the iothread to read or write the nodes, so once the main loop finishes
>> executing bdrv_drained_begin() on the interested graph, we are sure that
>> the iothread is not going to look or even interfere with that part of the graph.
>> We are also sure that the only two actors that can look at a specific
>> BlockDriverState in any given context are the main loop and the
>> iothread running the AioContext (ensured by "global state or IO" logic).
>>
>> Why use _subtree_ instead of normal drain
>> -----------------------------------------
>> A simple drain "blocks" a given node and all its parents.
>> But it doesn't touch the child.
>> This means that if we use a simple drain, a child can always
>> keep processing requests, and eventually end up calling itself
>> bdrv_drained_begin, ending up reading the parents node while the main loop
>> is modifying them. Therefore a subtree drain is necessary.
>>
>> Possible scenarios
>> -------------------
>> Keeping in mind that we can only have an iothread and the main loop
>> draining on a certain node, we could have:
>>
>> main loop successfully drains and then iothread tries to drain:
>>   impossible scenario, as iothread is already stopped once main
>>   successfully drains.
>>
>> iothread successfully drains and then main loop drains:
>>   should not be a problem, as:
>>   1) the iothread should be already "blocked" by its own drain
> 
> Once drained_begin() returns in the IOThread, the IOThread can do
> anything it wants, including more submitting I/O. I don't consider that
> "blocked", so I'm not sure what this sentence means?
> 
> The way the main loop thread protects itself against the IOThread is via
> the aio "external" handler concept and block job drain callbacks, which
> are activated by drained_begin(). They ensure that the IOThread will not
> perform further processing that submits I/O, but the IOThread code that
> invoked drained_begin() can still do anything it wants.

As above I think that regardless on what the iothread is doing, once the
main loop has finished executing bdrv_drained_begin the iothread should
not be doing anything related to the nodes that have been drained.

> 
>>   2) main loop would still wait for it to completely block
>>   There is the issue of mirror overriding such scenario to avoid
>>   having deadlocks, but that is handled in the next section.
>>
>> main loop and iothread try to drain together:
>>   As above, this case doens't really matter. As long as
>>   bdrv_drained_begin invariant is respected, the main loop will
>>   continue only once the iothread is "blocked" on that part of the graph.
> 
> I'm not sure about this, see above.
> 
>>
>> A note on iothread draining
>> ---------------------------
>> Theoretically draining from an iothread should not be possible,
>> as the iothread would be scheduling a bh in the main loop waiting
>> for itself to stop, even though it is not yet stopped since it is waiting for the bh.
> 
> I'm not sure what you mean. Which function schedules this BH?
bdrv_drained_begin and _end schedule a bh to run the draining code in
the main loop, if they are in a coroutine. That's what I meant here :)
> 
>>
>> This is what would happen in the tests in patch 5 if .drained_poll
>> was not implemented.
>>
>> Therefore, one solution is to use .drained_poll callback in BlockJobDriver.
>> This callback overrides the default job poll() behavior, and
>> allows the polling condition to stop waiting for the job.
>> It is actually used only in mirror.
>> This however breaks bdrv_drained_begin invariant, because the
>> iothread is not really blocked on that node but continues running.
>> In order to fix this, patch 4 allows the polling condition to be
>> used only by the iothread, and not the main loop too, preventing
>> the drain to return before the iothread is effectively stopped.
>> This is also shown in the tests in patch 5. If the fix in patch
>> 4 is removed, then the main loop drain will return earlier and
>> allow the iothread to run and drain together.
>>
>> The other patches in this serie are cherry-picked from the various
>> series I already sent, and are included here just to allow
>> subtree_drained_begin/end_unlocked implementation.
>>
>> Emanuele Giuseppe Esposito (5):
>>   aio-wait.h: introduce AIO_WAIT_WHILE_UNLOCKED
>>   introduce BDRV_POLL_WHILE_UNLOCKED
>>   block/io.c: introduce bdrv_subtree_drained_{begin/end}_unlocked
>>   child_job_drained_poll: override polling condition only when in home
>>     thread
>>   test-bdrv-drain: ensure draining from main loop stops iothreads
>>
>>  block/io.c                   |  48 ++++++--
>>  blockjob.c                   |   3 +-
>>  include/block/aio-wait.h     |  15 ++-
>>  include/block/block.h        |   7 ++
>>  tests/unit/test-bdrv-drain.c | 218 +++++++++++++++++++++++++++++++++++
>>  5 files changed, 274 insertions(+), 17 deletions(-)
>>
>> -- 
>> 2.31.1
>>

Re: [RFC PATCH 0/5] Removal of AioContext lock, bs->parents and ->children: proof of concept

[Stefan Hajnoczi]

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

On Wed, Mar 09, 2022 at 02:26:28PM +0100, Emanuele Giuseppe Esposito wrote:
> Am 02/03/2022 um 10:47 schrieb Stefan Hajnoczi:
> > On Tue, Mar 01, 2022 at 09:21:08AM -0500, Emanuele Giuseppe Esposito wrote:
> >> Possible scenarios
> >> -------------------
> >> Keeping in mind that we can only have an iothread and the main loop
> >> draining on a certain node, we could have:
> >>
> >> main loop successfully drains and then iothread tries to drain:
> >>   impossible scenario, as iothread is already stopped once main
> >>   successfully drains.
> >>
> >> iothread successfully drains and then main loop drains:
> >>   should not be a problem, as:
> >>   1) the iothread should be already "blocked" by its own drain
> > 
> > Once drained_begin() returns in the IOThread, the IOThread can do
> > anything it wants, including more submitting I/O. I don't consider that
> > "blocked", so I'm not sure what this sentence means?
> > 
> > The way the main loop thread protects itself against the IOThread is via
> > the aio "external" handler concept and block job drain callbacks, which
> > are activated by drained_begin(). They ensure that the IOThread will not
> > perform further processing that submits I/O, but the IOThread code that
> > invoked drained_begin() can still do anything it wants.
> 
> As above I think that regardless on what the iothread is doing, once the
> main loop has finished executing bdrv_drained_begin the iothread should
> not be doing anything related to the nodes that have been drained.

I agree. What I wanted to highlight is that waiting for requests to
complete is not what stops the IOThread, it's the "external" AIO handler
mechanism.

Stefan

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

Re: [RFC PATCH 0/5] Removal of AioContext lock, bs->parents and ->children: proof of concept

[Emanuele Giuseppe Esposito]

Am 09/03/2022 um 14:26 schrieb Emanuele Giuseppe Esposito:
>>> * Drains allow the caller (either main loop or iothread running
>>> the context) to wait all in_flights requests and operations
>>> of a BDS: normal drains target a given node and is parents, while
>>> subtree ones also include the subgraph of the node. Siblings are
>>> not affected by any of these two kind of drains.
>> Siblings are drained to the extent required for their parent node to
>> reach in_flight == 0.
>>
>> I haven't checked the code but I guess the case you're alluding to is
>> that siblings with multiple parents could have other I/O in flight that
>> will not be drained and further I/O can be submitted after the parent
>> has drained?
> Yes, this in theory can happen. I don't really know if this happens
> practically, and how likely is to happen.
> 
> The alternative would be to make a drain that blocks the whole graph,
> siblings included, but that would probably be an overkill.
> 

So I have thought about this, and I think maybe this is not a concrete
problem.
Suppose we have a graph where "parent" has 2 children: "child" and
"sibling". "sibling" also has a blockjob.

Now, main loop wants to modify parent-child relation and maybe detach
child from parent.

1st wrong assumption: the sibling is not drained. Actually my strategy
takes into account draining both nodes, also because parent could be in
another graph. Therefore sibling is drained.

But let's assume "sibling" is the sibling of the parent.

Therefore we have
"child" -> "parent" -> "grandparent"
and
"blockjob" -> "sibling" -> "grandparent"

The issue is the following: main loop can't drain "sibling", because
subtree_drained does not reach it. Therefore blockjob can still run
while main loop modifies "child" -> "parent". Blockjob can either:
1) drain, but this won't affect "child" -> "parent"
2) read the graph in other ways different from drain, for example
.set_aio_context recursively touches the whole graph.
3) write the graph.

3) can be only performed in the main loop, because it's a graph
operation. It means that the blockjob runs when the graph modifying
coroutine/bh is not running. They never run together.
The safety of this operation relies on where the drains are and will be
inserted. If you do like in my patch "block.c:
bdrv_replace_child_noperm: first call ->attach(), and then add child\x0f",
then we would have problem, because we drain between two writes, and the
blockjob will find an inconsistent graph. If we do it as we seem to do
it so far, then we won't really have any problem.

2) is a read, and can theoretically be performed by another thread. But
is there a function that does that? .set_aio_context for example is a GS
function, so we will fall back to case 3) and nothing bad would happen.

Is there a counter example for this?

-----------

Talking about something else, I discussed with Kevin what *seems* to be
an alternative way to do this, instead of adding drains everywhere.
His idea is to replicate what blk_wait_while_drained() currently does
but on a larger scale. It is something in between this subtree_drains
logic and a rwlock.

Basically if I understood correctly, we could implement
bdrv_wait_while_drained(), and put in all places where we would put a
read lock: all the reads to ->parents and ->children.
This function detects if the bdrv is under drain, and if so it will stop
and wait that the drain finishes (ie the graph modification).
On the other side, each write would just need to drain probably both
nodes (simple drain), to signal that we are modifying the graph. Once
bdrv_drained_begin() finishes, we are sure all coroutines are stopped.
Once bdrv_drained_end() finishes, we automatically let all coroutine
restart, and continue where they left off.

Seems a good compromise between drains and rwlock. What do you think?

I am not sure how painful it will be to implement though.

Thank you,
Emanuele

Re: [RFC PATCH 0/5] Removal of AioContext lock, bs->parents and ->children: proof of concept

[Hanna Reitz]

On 17.03.22 17:23, Emanuele Giuseppe Esposito wrote:
>
> Am 09/03/2022 um 14:26 schrieb Emanuele Giuseppe Esposito:
>>>> * Drains allow the caller (either main loop or iothread running
>>>> the context) to wait all in_flights requests and operations
>>>> of a BDS: normal drains target a given node and is parents, while
>>>> subtree ones also include the subgraph of the node. Siblings are
>>>> not affected by any of these two kind of drains.
>>> Siblings are drained to the extent required for their parent node to
>>> reach in_flight == 0.
>>>
>>> I haven't checked the code but I guess the case you're alluding to is
>>> that siblings with multiple parents could have other I/O in flight that
>>> will not be drained and further I/O can be submitted after the parent
>>> has drained?
>> Yes, this in theory can happen. I don't really know if this happens
>> practically, and how likely is to happen.
>>
>> The alternative would be to make a drain that blocks the whole graph,
>> siblings included, but that would probably be an overkill.
>>
> So I have thought about this, and I think maybe this is not a concrete
> problem.
> Suppose we have a graph where "parent" has 2 children: "child" and
> "sibling". "sibling" also has a blockjob.
>
> Now, main loop wants to modify parent-child relation and maybe detach
> child from parent.
>
> 1st wrong assumption: the sibling is not drained. Actually my strategy
> takes into account draining both nodes, also because parent could be in
> another graph. Therefore sibling is drained.
>
> But let's assume "sibling" is the sibling of the parent.
>
> Therefore we have
> "child" -> "parent" -> "grandparent"
> and
> "blockjob" -> "sibling" -> "grandparent"
>
> The issue is the following: main loop can't drain "sibling", because
> subtree_drained does not reach it. Therefore blockjob can still run
> while main loop modifies "child" -> "parent". Blockjob can either:
> 1) drain, but this won't affect "child" -> "parent"
> 2) read the graph in other ways different from drain, for example
> .set_aio_context recursively touches the whole graph.
> 3) write the graph.

I don’t really understand the problem here.  If the block job only 
operates on the sibling subgraph, why would it care what’s going on in 
the other subgraph?

Block jobs should own all nodes that are associated with them (e.g. 
because they intend to drop or replace them when the job is done), so 
when part of the graph is drained, all jobs that could modify that part 
should be drained, too.

> 3) can be only performed in the main loop, because it's a graph
> operation. It means that the blockjob runs when the graph modifying
> coroutine/bh is not running. They never run together.
> The safety of this operation relies on where the drains are and will be
> inserted. If you do like in my patch "block.c:
> bdrv_replace_child_noperm: first call ->attach(), and then add child\x0f",
> then we would have problem, because we drain between two writes, and the
> blockjob will find an inconsistent graph. If we do it as we seem to do
> it so far, then we won't really have any problem.
>
> 2) is a read, and can theoretically be performed by another thread. But
> is there a function that does that? .set_aio_context for example is a GS
> function, so we will fall back to case 3) and nothing bad would happen.
>
> Is there a counter example for this?
>
> -----------
>
> Talking about something else, I discussed with Kevin what *seems* to be
> an alternative way to do this, instead of adding drains everywhere.
> His idea is to replicate what blk_wait_while_drained() currently does
> but on a larger scale. It is something in between this subtree_drains
> logic and a rwlock.
>
> Basically if I understood correctly, we could implement
> bdrv_wait_while_drained(), and put in all places where we would put a
> read lock: all the reads to ->parents and ->children.
> This function detects if the bdrv is under drain, and if so it will stop
> and wait that the drain finishes (ie the graph modification).
> On the other side, each write would just need to drain probably both
> nodes (simple drain), to signal that we are modifying the graph. Once
> bdrv_drained_begin() finishes, we are sure all coroutines are stopped.
> Once bdrv_drained_end() finishes, we automatically let all coroutine
> restart, and continue where they left off.
>
> Seems a good compromise between drains and rwlock. What do you think?

Well, sounds complicated.  So I’m asking myself whether this would be 
noticeably better than just an RwLock for graph modifications, like the 
global lock Vladimir has proposed.

Hanna

Re: [RFC PATCH 0/5] Removal of AioContext lock, bs->parents and ->children: proof of concept

[Emanuele Giuseppe Esposito]

Am 30/03/2022 um 12:53 schrieb Hanna Reitz:
> On 17.03.22 17:23, Emanuele Giuseppe Esposito wrote:
>>
>> Am 09/03/2022 um 14:26 schrieb Emanuele Giuseppe Esposito:
>>>>> * Drains allow the caller (either main loop or iothread running
>>>>> the context) to wait all in_flights requests and operations
>>>>> of a BDS: normal drains target a given node and is parents, while
>>>>> subtree ones also include the subgraph of the node. Siblings are
>>>>> not affected by any of these two kind of drains.
>>>> Siblings are drained to the extent required for their parent node to
>>>> reach in_flight == 0.
>>>>
>>>> I haven't checked the code but I guess the case you're alluding to is
>>>> that siblings with multiple parents could have other I/O in flight that
>>>> will not be drained and further I/O can be submitted after the parent
>>>> has drained?
>>> Yes, this in theory can happen. I don't really know if this happens
>>> practically, and how likely is to happen.
>>>
>>> The alternative would be to make a drain that blocks the whole graph,
>>> siblings included, but that would probably be an overkill.
>>>
>> So I have thought about this, and I think maybe this is not a concrete
>> problem.
>> Suppose we have a graph where "parent" has 2 children: "child" and
>> "sibling". "sibling" also has a blockjob.
>>
>> Now, main loop wants to modify parent-child relation and maybe detach
>> child from parent.
>>
>> 1st wrong assumption: the sibling is not drained. Actually my strategy
>> takes into account draining both nodes, also because parent could be in
>> another graph. Therefore sibling is drained.
>>
>> But let's assume "sibling" is the sibling of the parent.
>>
>> Therefore we have
>> "child" -> "parent" -> "grandparent"
>> and
>> "blockjob" -> "sibling" -> "grandparent"
>>
>> The issue is the following: main loop can't drain "sibling", because
>> subtree_drained does not reach it. Therefore blockjob can still run
>> while main loop modifies "child" -> "parent". Blockjob can either:
>> 1) drain, but this won't affect "child" -> "parent"
>> 2) read the graph in other ways different from drain, for example
>> .set_aio_context recursively touches the whole graph.
>> 3) write the graph.
> 
> I don’t really understand the problem here.  If the block job only
> operates on the sibling subgraph, why would it care what’s going on in
> the other subgraph?

We are talking about something that probably does not happen, but what
if it calls a callback similar to .set_aio_context that goes through the
whole graph?

Even though the first question is: is there such callback?

Second even more irrealistic case is when a job randomly looks for a bs
in another connectivity component and for example drains it.
Again probably impossible.

> Block jobs should own all nodes that are associated with them (e.g.
> because they intend to drop or replace them when the job is done), so
> when part of the graph is drained, all jobs that could modify that part
> should be drained, too.

What do you mean with "own"?

> 
>> 3) can be only performed in the main loop, because it's a graph
>> operation. It means that the blockjob runs when the graph modifying
>> coroutine/bh is not running. They never run together.
>> The safety of this operation relies on where the drains are and will be
>> inserted. If you do like in my patch "block.c:
>> bdrv_replace_child_noperm: first call ->attach(), and then add child\x0f",
>> then we would have problem, because we drain between two writes, and the
>> blockjob will find an inconsistent graph. If we do it as we seem to do
>> it so far, then we won't really have any problem.
>>
>> 2) is a read, and can theoretically be performed by another thread. But
>> is there a function that does that? .set_aio_context for example is a GS
>> function, so we will fall back to case 3) and nothing bad would happen.
>>
>> Is there a counter example for this?
>>
>> -----------
>>
>> Talking about something else, I discussed with Kevin what *seems* to be
>> an alternative way to do this, instead of adding drains everywhere.
>> His idea is to replicate what blk_wait_while_drained() currently does
>> but on a larger scale. It is something in between this subtree_drains
>> logic and a rwlock.
>>
>> Basically if I understood correctly, we could implement
>> bdrv_wait_while_drained(), and put in all places where we would put a
>> read lock: all the reads to ->parents and ->children.
>> This function detects if the bdrv is under drain, and if so it will stop
>> and wait that the drain finishes (ie the graph modification).
>> On the other side, each write would just need to drain probably both
>> nodes (simple drain), to signal that we are modifying the graph. Once
>> bdrv_drained_begin() finishes, we are sure all coroutines are stopped.
>> Once bdrv_drained_end() finishes, we automatically let all coroutine
>> restart, and continue where they left off.
>>
>> Seems a good compromise between drains and rwlock. What do you think?
> 
> Well, sounds complicated.  So I’m asking myself whether this would be
> noticeably better than just an RwLock for graph modifications, like the
> global lock Vladimir has proposed.

But the point is then: aren't we re-inventing an AioContext lock?
the lock will protect not only ->parents and ->child, but also other
bdrv fields that are concurrently read/written.

I don't know, it seems to me that there is a lot of uncertainty on which
way to take...

Emanuele

Re: [RFC PATCH 0/5] Removal of AioContext lock, bs->parents and ->children: proof of concept

[Hanna Reitz]

On 30.03.22 13:55, Emanuele Giuseppe Esposito wrote:
>
> Am 30/03/2022 um 12:53 schrieb Hanna Reitz:
>> On 17.03.22 17:23, Emanuele Giuseppe Esposito wrote:
>>> Am 09/03/2022 um 14:26 schrieb Emanuele Giuseppe Esposito:
>>>>>> * Drains allow the caller (either main loop or iothread running
>>>>>> the context) to wait all in_flights requests and operations
>>>>>> of a BDS: normal drains target a given node and is parents, while
>>>>>> subtree ones also include the subgraph of the node. Siblings are
>>>>>> not affected by any of these two kind of drains.
>>>>> Siblings are drained to the extent required for their parent node to
>>>>> reach in_flight == 0.
>>>>>
>>>>> I haven't checked the code but I guess the case you're alluding to is
>>>>> that siblings with multiple parents could have other I/O in flight that
>>>>> will not be drained and further I/O can be submitted after the parent
>>>>> has drained?
>>>> Yes, this in theory can happen. I don't really know if this happens
>>>> practically, and how likely is to happen.
>>>>
>>>> The alternative would be to make a drain that blocks the whole graph,
>>>> siblings included, but that would probably be an overkill.
>>>>
>>> So I have thought about this, and I think maybe this is not a concrete
>>> problem.
>>> Suppose we have a graph where "parent" has 2 children: "child" and
>>> "sibling". "sibling" also has a blockjob.
>>>
>>> Now, main loop wants to modify parent-child relation and maybe detach
>>> child from parent.
>>>
>>> 1st wrong assumption: the sibling is not drained. Actually my strategy
>>> takes into account draining both nodes, also because parent could be in
>>> another graph. Therefore sibling is drained.
>>>
>>> But let's assume "sibling" is the sibling of the parent.
>>>
>>> Therefore we have
>>> "child" -> "parent" -> "grandparent"
>>> and
>>> "blockjob" -> "sibling" -> "grandparent"
>>>
>>> The issue is the following: main loop can't drain "sibling", because
>>> subtree_drained does not reach it. Therefore blockjob can still run
>>> while main loop modifies "child" -> "parent". Blockjob can either:
>>> 1) drain, but this won't affect "child" -> "parent"
>>> 2) read the graph in other ways different from drain, for example
>>> .set_aio_context recursively touches the whole graph.
>>> 3) write the graph.
>> I don’t really understand the problem here.  If the block job only
>> operates on the sibling subgraph, why would it care what’s going on in
>> the other subgraph?
> We are talking about something that probably does not happen, but what
> if it calls a callback similar to .set_aio_context that goes through the
> whole graph?

Hm.  Quite unfortunate if such a callback can operate on drained nodes, 
I’d say.  Ideally callbacks wouldn’t do that, but probably they will. :/

> Even though the first question is: is there such callback?

I mean, you could say any callback qualifies.  Draining a node will only 
drain its recursive parents, so siblings are not affected.  If the 
sibling issues the callback on its parent...  (E.g. changes in the 
backing chain requiring a qcow2 parent node to change the backing file 
string in its image file)

> Second even more irrealistic case is when a job randomly looks for a bs
> in another connectivity component and for example drains it.
> Again probably impossible.

I hope so, but the block layer sure likes to surprise me.

>> Block jobs should own all nodes that are associated with them (e.g.
>> because they intend to drop or replace them when the job is done), so
>> when part of the graph is drained, all jobs that could modify that part
>> should be drained, too.
> What do you mean with "own"?

They’re added with block_job_add_bdrv(), and then are children of the 
BlockJob object.

>>> 3) can be only performed in the main loop, because it's a graph
>>> operation. It means that the blockjob runs when the graph modifying
>>> coroutine/bh is not running. They never run together.
>>> The safety of this operation relies on where the drains are and will be
>>> inserted. If you do like in my patch "block.c:
>>> bdrv_replace_child_noperm: first call ->attach(), and then add child\x0f",
>>> then we would have problem, because we drain between two writes, and the
>>> blockjob will find an inconsistent graph. If we do it as we seem to do
>>> it so far, then we won't really have any problem.
>>>
>>> 2) is a read, and can theoretically be performed by another thread. But
>>> is there a function that does that? .set_aio_context for example is a GS
>>> function, so we will fall back to case 3) and nothing bad would happen.
>>>
>>> Is there a counter example for this?
>>>
>>> -----------
>>>
>>> Talking about something else, I discussed with Kevin what *seems* to be
>>> an alternative way to do this, instead of adding drains everywhere.
>>> His idea is to replicate what blk_wait_while_drained() currently does
>>> but on a larger scale. It is something in between this subtree_drains
>>> logic and a rwlock.
>>>
>>> Basically if I understood correctly, we could implement
>>> bdrv_wait_while_drained(), and put in all places where we would put a
>>> read lock: all the reads to ->parents and ->children.
>>> This function detects if the bdrv is under drain, and if so it will stop
>>> and wait that the drain finishes (ie the graph modification).
>>> On the other side, each write would just need to drain probably both
>>> nodes (simple drain), to signal that we are modifying the graph. Once
>>> bdrv_drained_begin() finishes, we are sure all coroutines are stopped.
>>> Once bdrv_drained_end() finishes, we automatically let all coroutine
>>> restart, and continue where they left off.
>>>
>>> Seems a good compromise between drains and rwlock. What do you think?
>> Well, sounds complicated.  So I’m asking myself whether this would be
>> noticeably better than just an RwLock for graph modifications, like the
>> global lock Vladimir has proposed.
> But the point is then: aren't we re-inventing an AioContext lock?

I don’t know how AioContext locks would even help with graph changes.  
If I want to change a block subgraph that’s in a different I/O thread, 
locking that thread isn’t enough (I would’ve thought); because I have no 
idea what the thread is doing when I’m locking it.  Perhaps it’s 
iterating through ->children right now (with some yields in between), 
and by pausing it, changing the graph, and then resuming it, it’ll still 
cause problems.

> the lock will protect not only ->parents and ->child, but also other
> bdrv fields that are concurrently read/written.

I would’ve thought this lock should only protect ->parents and ->children.

> I don't know, it seems to me that there is a lot of uncertainty on which
> way to take...

Definitely. :)

I wouldn’t call that a bad thing, necessarily.  Let’s look at the 
positive side: There are many ideas!

Hanna

Re: [RFC PATCH 0/5] Removal of AioContext lock, bs->parents and ->children: proof of concept

[Paolo Bonzini]

On 3/30/22 12:53, Hanna Reitz wrote:
>>
>> Seems a good compromise between drains and rwlock. What do you think?
> 
> Well, sounds complicated.  So I’m asking myself whether this would be 
> noticeably better than just an RwLock for graph modifications, like the 
> global lock Vladimir has proposed.

A global lock would have to be taken by all iothreads on every I/O 
operation, even a read-write lock would scale because it has a global 
CoMutex inside and rdlock/unlock both take it.  Even if the critical 
section is small, the cacheline bumping would be pretty bad.

Perhaps we could reuse the code in cpus-common.c, which relies on a list 
of possible readers and is quite cheap (two memory barriers basically) 
for writers.  Here we would have a list of AioContexts as the possible 
readers.

The slow path uses a QemuMutex, a QemuCond for the readers and a 
QemuCond for the writers.  The reader QemuCond can be replaced by a 
CoQueue, I think.

Paolo

Re: [RFC PATCH 0/5] Removal of AioContext lock, bs->parents and ->children: proof of concept

[Paolo Bonzini]

On 3/30/22 18:02, Paolo Bonzini wrote:
> On 3/30/22 12:53, Hanna Reitz wrote:
>>>
>>> Seems a good compromise between drains and rwlock. What do you think?
>>
>> Well, sounds complicated.  So I’m asking myself whether this would be 
>> noticeably better than just an RwLock for graph modifications, like 
>> the global lock Vladimir has proposed.

[try again, this time with brain connected]

A global lock would have to be taken by all iothreads on every I/O
operation, even a CoRwLock would not scale because it has a global
CoMutex inside and rdlock/unlock both take it.  Even if the critical
section is small, the cacheline bumping would be pretty bad.

Perhaps we could reuse the code in cpus-common.c, which relies on a list
of possible readers and is quite cheap (two memory barriers basically)
for readers.  Here we would have a list of AioContexts (or perhaps 
BlockDriverStates?) as the possible readers.

The slow path uses a QemuMutex, a QemuCond for the readers and a
QemuCond for the writers.  The reader QemuCond can be replaced by a
CoQueue, I think.

Paolo

Re: [RFC PATCH 0/5] Removal of AioContext lock, bs->parents and ->children: proof of concept

[Emanuele Giuseppe Esposito]

Am 31/03/2022 um 11:59 schrieb Paolo Bonzini:
> On 3/30/22 18:02, Paolo Bonzini wrote:
>> On 3/30/22 12:53, Hanna Reitz wrote:
>>>>
>>>> Seems a good compromise between drains and rwlock. What do you think?
>>>
>>> Well, sounds complicated.  So I’m asking myself whether this would be
>>> noticeably better than just an RwLock for graph modifications, like
>>> the global lock Vladimir has proposed.
> 
> [try again, this time with brain connected]
> 
> A global lock would have to be taken by all iothreads on every I/O
> operation, even a CoRwLock would not scale because it has a global
> CoMutex inside and rdlock/unlock both take it.  Even if the critical
> section is small, the cacheline bumping would be pretty bad.
> 
> Perhaps we could reuse the code in cpus-common.c, which relies on a list
> of possible readers and is quite cheap (two memory barriers basically)
> for readers.  Here we would have a list of AioContexts (or perhaps
> BlockDriverStates?) as the possible readers.
> 
> The slow path uses a QemuMutex, a QemuCond for the readers and a
> QemuCond for the writers.  The reader QemuCond can be replaced by a
> CoQueue, I think.
> 

It would be something like this:
Essentially move graph_bdrv_states as public, so that we can iterate
through all bs as cpu-common.c does with cpus, and then duplicate the
already existing API in cpu-common.c:

bdrv_graph_list_wrlock <-> start_exclusive
bdrv_graph_list_wrunlock <-> end_exclusive
bdrv_graph_list_rdlock <-> cpu_exec_start
bdrv_graph_list_rdunlock <-> cpu_exec_end

The only difference is that there is no qemu_cpu_kick(), but I don't
think it matters.

What do you think?

diff --git a/block.c b/block.c
index 718e4cae8b..af8dd37101 100644
--- a/block.c
+++ b/block.c
@@ -52,6 +52,7 @@
 #include "qemu/range.h"
 #include "qemu/rcu.h"
 #include "block/coroutines.h"
+#include "block/block-list.h"

 #ifdef CONFIG_BSD
 #include <sys/ioctl.h>
@@ -67,10 +68,6 @@

 #define NOT_DONE 0x7fffffff /* used while emulated sync operation in
progress */

-/* Protected by BQL */
-static QTAILQ_HEAD(, BlockDriverState) graph_bdrv_states =
-    QTAILQ_HEAD_INITIALIZER(graph_bdrv_states);
-
 /* Protected by BQL */
 static QTAILQ_HEAD(, BlockDriverState) all_bdrv_states =
     QTAILQ_HEAD_INITIALIZER(all_bdrv_states);
diff --git a/include/block/block-list.h b/include/block/block-list.h
new file mode 100644
index 0000000000..d55a056938
--- /dev/null
+++ b/include/block/block-list.h
@@ -0,0 +1,54 @@
+#ifndef BLOCK_LIST_H
+#define BLOCK_LIST_H
+
+#include "qemu/osdep.h"
+
+
+/* Protected by BQL */
+QTAILQ_HEAD(,BlockDriverState) graph_bdrv_states =
QTAILQ_HEAD_INITIALIZER(graph_bdrv_states);
+
+void bdrv_init_graph_list_lock(void);
+
+/*
+ * bdrv_graph_list_wrlock:
+ * Modify the graph. Nobody else is allowed to access the graph.
+ * Set pending op >= 1, so that the next readers will wait in a
coroutine queue.
+ * Then keep track of the running readers using bs->has_writer,
+ * and wait until they finish.
+ */
+void bdrv_graph_list_wrlock(void);
+
+/*
+ * bdrv_graph_list_wrunlock:
+ * Write finished, reset pending operations to 0 and restart
+ * all readers that are waiting.
+ */
+void bdrv_graph_list_wrunlock(void);
+
+/*
+ * bdrv_graph_list_rdlock:
+ * Read the bs graph. Set bs->reading_graph true, and if there are pending
+ * operations, it means that the main loop is modifying the graph,
+ * therefore wait in exclusive_resume coroutine queue.
+ * If this read is coming while the writer has already marked the
+ * running read requests and it's waiting for them to finish,
+ * wait that the write finishes first.
+ * Main loop will then wake this coroutine once it is done.
+ */
+void bdrv_graph_list_rdlock(BlockDriverState *bs);
+
+/*
+ * bdrv_graph_list_rdunlock:
+ * Read terminated, decrease the count of pending operations.
+ * If it's the last read that the writer is waiting for, signal
+ * the writer that we are done and let it continue.
+ */
+void bdrv_graph_list_rdunlock(BlockDriverState *bs);
+
+
+
+
+#define BS_GRAPH_READER(bs) /* in main loop OR bs->reading_graph */
+#define BS_GRAPH_WRITER(bs) /* in main loop AND bs->bs_graph_pending_op
> 0 */
+#endif /* BLOCK_LIST_H */
+
diff --git a/include/block/block_int-common.h
b/include/block/block_int-common.h
index 5a04c778e4..0266876a59 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -985,6 +985,20 @@ struct BlockDriverState {
     bool force_share; /* if true, always allow all shared permissions */
     bool implicit;  /* if true, this filter node was automatically
inserted */

+    /*
+     * If true, the bs is reading the graph.
+     * No write should happen in the meanwhile.
+     * Atomic.
+     */
+    bool reading_graph;
+
+    /*
+     * If true, the main loop is modifying the graph.
+     * bs cannot read the graph.
+     * Protected by bs_graph_list_lock.
+     */
+    bool has_writer;
+
     BlockDriver *drv; /* NULL means no media */
     void *opaque;

Re: [RFC PATCH 0/5] Removal of AioContext lock, bs->parents and ->children: proof of concept

[Paolo Bonzini]

On 3/31/22 15:51, Emanuele Giuseppe Esposito wrote:
> 
> bdrv_graph_list_wrlock <-> start_exclusive
> bdrv_graph_list_wrunlock <-> end_exclusive
> bdrv_graph_list_rdlock <-> cpu_exec_start
> bdrv_graph_list_rdunlock <-> cpu_exec_end

This wouldn't protect the list but the whole graph, i.e. the parents and 
children of all BDSes.  So the functions would be:

   bdrv_graph_wrlock <-> start_exclusive
   bdrv_graph_wrunlock <-> end_exclusive
   bdrv_graph_rdlock <-> cpu_exec_start
   bdrv_graph_rdunlock <-> cpu_exec_end


The list itself would be used internally to implement the write-side 
lock and unlock primitives, but it would not be protected by the above 
functions.  So there would be a couple additional functions:

   bdrv_graph_list_lock <-> cpu_list_lock
   bdrv_graph_list_unlock <-> cpu_list_unlock

> +void bdrv_graph_list_rdlock(BlockDriverState *bs);
> +void bdrv_graph_list_rdunlock(BlockDriverState *bs);

Apart from the naming change, these two would be coroutine_fn.

> +#define BS_GRAPH_READER(bs) /* in main loop OR bs->reading_graph */
> +#define BS_GRAPH_WRITER(bs) /* in main loop AND bs->bs_graph_pending_op

bs_graph_pending_op is not part of bs->, it is a global variable 
(corresponding to pending_cpus in cpus-common.c).  I would call it 
bs_graph_pending_reader since you have "has_writer" below.

Also, this second #define does not need an argument, and is really the 
same as assert_bdrv_graph_writable(bs).  So perhaps you can rename the 
first one to assert_bdrv_graph_readable(bs).

> 
> +    /*
> +     * If true, the main loop is modifying the graph.
> +     * bs cannot read the graph.
> +     * Protected by bs_graph_list_lock.
> +     */
> +    bool has_writer;

Note that it's "has_waiter" in cpus-common.c. :)  has_writer is fine too.

Paolo

Re: [RFC PATCH 0/5] Removal of AioContext lock, bs->parents and ->children: proof of concept

[Emanuele Giuseppe Esposito]

Am 31/03/2022 um 18:40 schrieb Paolo Bonzini:
> On 3/31/22 15:51, Emanuele Giuseppe Esposito wrote:
>>
>> bdrv_graph_list_wrlock <-> start_exclusive
>> bdrv_graph_list_wrunlock <-> end_exclusive
>> bdrv_graph_list_rdlock <-> cpu_exec_start
>> bdrv_graph_list_rdunlock <-> cpu_exec_end
> 
> This wouldn't protect the list but the whole graph, i.e. the parents and
> children of all BDSes.  So the functions would be:
> 
>   bdrv_graph_wrlock <-> start_exclusive
>   bdrv_graph_wrunlock <-> end_exclusive
>   bdrv_graph_rdlock <-> cpu_exec_start
>   bdrv_graph_rdunlock <-> cpu_exec_end
> 
> 
> The list itself would be used internally to implement the write-side
> lock and unlock primitives, but it would not be protected by the above
> functions.  So there would be a couple additional functions:
> 
>   bdrv_graph_list_lock <-> cpu_list_lock
>   bdrv_graph_list_unlock <-> cpu_list_unlock

The list would be graph_bdrv_states, why do we need to protect it with a
lock? Currently it is protected by BQL, and theoretically only
bdrv_graph_wrlock iterates on it. And as we defined in the assertion
below, wrlock is always in the main loop too.

> 
>> +void bdrv_graph_list_rdlock(BlockDriverState *bs);
>> +void bdrv_graph_list_rdunlock(BlockDriverState *bs);
> 
> Apart from the naming change, these two would be coroutine_fn.
> 
>> +#define BS_GRAPH_READER(bs) /* in main loop OR bs->reading_graph */
>> +#define BS_GRAPH_WRITER(bs) /* in main loop AND bs->bs_graph_pending_op
> 
> bs_graph_pending_op is not part of bs->, it is a global variable
> (corresponding to pending_cpus in cpus-common.c).  I would call it
> bs_graph_pending_reader since you have "has_writer" below.

Hmm maybe it is better to go with has_waiter and pending_op. This is
because as "pending" we always have 1 write and zero or more reads.

Rest makes sense.

Emanuele
> 
> Also, this second #define does not need an argument, and is really the
> same as assert_bdrv_graph_writable(bs).  So perhaps you can rename the
> first one to assert_bdrv_graph_readable(bs).
> 
>>
>> +    /*
>> +     * If true, the main loop is modifying the graph.
>> +     * bs cannot read the graph.
>> +     * Protected by bs_graph_list_lock.
>> +     */
>> +    bool has_writer;
> 
> Note that it's "has_waiter" in cpus-common.c. :)  has_writer is fine too.
> 
> Paolo
> 

Re: [RFC PATCH 0/5] Removal of AioContext lock, bs->parents and ->children: proof of concept

[Paolo Bonzini]

On 4/1/22 10:05, Emanuele Giuseppe Esposito wrote:
>> The list itself would be used internally to implement the write-side
>> lock and unlock primitives, but it would not be protected by the above
>> functions.  So there would be a couple additional functions:
>>
>>    bdrv_graph_list_lock <-> cpu_list_lock
>>    bdrv_graph_list_unlock <-> cpu_list_unlock
> 
> The list would be graph_bdrv_states, why do we need to protect it with a
> lock? Currently it is protected by BQL, and theoretically only
> bdrv_graph_wrlock iterates on it. And as we defined in the assertion
> below, wrlock is always in the main loop too.

You're right, CPU_FOREACH only appears in start_exclusive; so likewise 
you only need to walk the list in bdrv_graph_wrlock, i.e. only under BQL.

My thought was that, within the implementation, you'll need a mutex to 
protect has_waiter, and protecting the list with the same mutex made 
sense to me.  But indeed it's not necessary.

Paolo

>>> +void bdrv_graph_list_rdlock(BlockDriverState *bs);
>>> +void bdrv_graph_list_rdunlock(BlockDriverState *bs);
>>
>> Apart from the naming change, these two would be coroutine_fn.
>>
>>> +#define BS_GRAPH_READER(bs) /* in main loop OR bs->reading_graph */
>>> +#define BS_GRAPH_WRITER(bs) /* in main loop AND bs->bs_graph_pending_op
>>
>> bs_graph_pending_op is not part of bs->, it is a global variable
>> (corresponding to pending_cpus in cpus-common.c).  I would call it
>> bs_graph_pending_reader since you have "has_writer" below.

Re: [RFC PATCH 0/5] Removal of AioContext lock, bs->parents and ->children: proof of concept

[Stefan Hajnoczi]

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

On Fri, Apr 01, 2022 at 01:01:53PM +0200, Paolo Bonzini wrote:
> On 4/1/22 10:05, Emanuele Giuseppe Esposito wrote:
> > > The list itself would be used internally to implement the write-side
> > > lock and unlock primitives, but it would not be protected by the above
> > > functions.  So there would be a couple additional functions:
> > > 
> > >    bdrv_graph_list_lock <-> cpu_list_lock
> > >    bdrv_graph_list_unlock <-> cpu_list_unlock
> > 
> > The list would be graph_bdrv_states, why do we need to protect it with a
> > lock? Currently it is protected by BQL, and theoretically only
> > bdrv_graph_wrlock iterates on it. And as we defined in the assertion
> > below, wrlock is always in the main loop too.
> 
> You're right, CPU_FOREACH only appears in start_exclusive; so likewise you
> only need to walk the list in bdrv_graph_wrlock, i.e. only under BQL.
> 
> My thought was that, within the implementation, you'll need a mutex to
> protect has_waiter, and protecting the list with the same mutex made sense
> to me.  But indeed it's not necessary.

What is the relationship between this new API and aio_set_fd_handler()'s
is_external?

A few thoughts:

- The new API doesn't stop more I/O requests from being submitted, it
  just blocks the current coroutine so request processing is deferred.

- In other words, is_external is a flow control API whereas the new API
  queues up request coroutines without notifying the caller.

- The new API still needs to be combined with bdrv_drained_begin/end()
  to ensure in-flight requests are done.

- It's not obvious to me whether the new API obsoletes is_external. I
  think it probably doesn't.

Stefan

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

Re: [RFC PATCH 0/5] Removal of AioContext lock, bs->parents and ->children: proof of concept

[Paolo Bonzini]

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

On Mon, Apr 4, 2022 at 11:25 AM Stefan Hajnoczi <stefanha@redhat.com> wrote:

> - The new API doesn't stop more I/O requests from being submitted, it
>   just blocks the current coroutine so request processing is deferred.
>

New I/O requests would not complete until the write-side critical section
ends. However they would still be accepted: from the point of view of the
guest, the "consumed" index of the virtio ring would move forward, unlike
bdrv_drained_begin/end().

- In other words, is_external is a flow control API whereas the new API
>   queues up request coroutines without notifying the caller.
>

Yes, I think this is the same I wrote above.

> - The new API still needs to be combined with bdrv_drained_begin/end()
>   to ensure in-flight requests are done.
>

I don't think so, because in-flight requests would take the lock for
reading. The write side would not start until those in-flight requests
release the lock.

- It's not obvious to me whether the new API obsoletes is_external. I think
> it probably doesn't.
>

I agree that it doesn't. This new lock is only protecting ->parents and
->children. bdrv_drained_begin()/end() remains necessary, for example, when
you need to send a request during the drained section. An example is
block_resize.

In addition, bdrv_drained_begin()/end() ensures that the callback of
blk_aio_*() functions has been invoked (see commit 46aaf2a566,
"block-backend: Decrease in_flight only after callback", 2018-09-25).  This
new lock would not ensure that.

As an aside, instead of is_external, QEMU could remove/add the ioeventfd
handler in the blk->dev_ops->drained_begin and blk->dev_ops->drained_end
callbacks respectively. But that's just a code cleanup.

Paolo

[-- Attachment #2: Type: text/html, Size: 4063 bytes --]

Re: [RFC PATCH 0/5] Removal of AioContext lock, bs->parents and ->children: proof of concept

[Emanuele Giuseppe Esposito]

Am 04/04/2022 um 11:41 schrieb Paolo Bonzini:
> 
> 
> On Mon, Apr 4, 2022 at 11:25 AM Stefan Hajnoczi <stefanha@redhat.com
> <mailto:stefanha@redhat.com>> wrote:
> 
>     - The new API doesn't stop more I/O requests from being submitted, it
>       just blocks the current coroutine so request processing is deferred.
> 
> 
> New I/O requests would not complete until the write-side critical
> section ends. However they would still be accepted: from the point of
> view of the guest, the "consumed" index of the virtio ring would move
> forward, unlike bdrv_drained_begin/end().
> 
>     - In other words, is_external is a flow control API whereas the new API
>       queues up request coroutines without notifying the caller.
> 
> 
> Yes, I think this is the same I wrote above.
> 
>     - The new API still needs to be combined with bdrv_drained_begin/end()
>       to ensure in-flight requests are done.
> 
> 
> I don't think so, because in-flight requests would take the lock for
> reading. The write side would not start until those in-flight requests
> release the lock.
> 
>     - It's not obvious to me whether the new API obsoletes is_external.
>     I think it probably doesn't.
> 
> 
> I agree that it doesn't. This new lock is only protecting ->parents and
> ->children. 

Side note: it will also be used to protect other fields, like
.aio_context I think. I haven't checked if there is something else we
might want to protect that is currently protected by AioContext lock.

At least, I think we are going to use the same lock, right?

Emanuele

bdrv_drained_begin()/end() remains necessary, for example,
> when you need to send a request during the drained section. An example
> is block_resize.
> 
> In addition, bdrv_drained_begin()/end() ensures that the callback of
> blk_aio_*() functions has been invoked (see commit 46aaf2a566,
> "block-backend: Decrease in_flight only after callback", 2018-09-25). 
> This new lock would not ensure that.
> 
> As an aside, instead of is_external, QEMU could remove/add the ioeventfd
> handler in the blk->dev_ops->drained_begin and blk->dev_ops->drained_end
> callbacks respectively. But that's just a code cleanup.
> 
> Paolo

Re: [RFC PATCH 0/5] Removal of AioContext lock, bs->parents and ->children: proof of concept

[Paolo Bonzini]

On 4/4/22 11:51, Emanuele Giuseppe Esposito wrote:
>>
>> I agree that it doesn't. This new lock is only protecting ->parents and
>> ->children. 
> Side note: it will also be used to protect other fields, like
> .aio_context I think. I haven't checked if there is something else we
> might want to protect that is currently protected by AioContext lock.
> 
> At least, I think we are going to use the same lock, right?

I have no idea honestly.  It can make sense for anything that is changed
very rarely and read during requests.

.aio_context has the .detach/.attach callbacks and I wonder if there
should be any reason to access it outside the callbacks.  A lot of uses
of .aio_context (for example for aio_bh_new or
aio_bh_schedule_oneshot/replay_bh_schedule_oneshot_event) can, and
perhaps should, be changed to just qemu_get_current_aio_context().  For
multiqueue we probably want the same BlockDriverState to use the
AioContext corresponding to a virtio queue, rather than always the same one.

Paolo

Re: [RFC PATCH 0/5] Removal of AioContext lock, bs->parents and ->children: proof of concept

[Stefan Hajnoczi]

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

On Mon, Apr 04, 2022 at 11:41:04AM +0200, Paolo Bonzini wrote:
> On Mon, Apr 4, 2022 at 11:25 AM Stefan Hajnoczi <stefanha@redhat.com> wrote:
> > - The new API still needs to be combined with bdrv_drained_begin/end()
> >   to ensure in-flight requests are done.
> >
> 
> I don't think so, because in-flight requests would take the lock for
> reading. The write side would not start until those in-flight requests
> release the lock.

Good point!

> - It's not obvious to me whether the new API obsoletes is_external. I think
> > it probably doesn't.
> >
> 
> I agree that it doesn't. This new lock is only protecting ->parents and
> ->children. bdrv_drained_begin()/end() remains necessary, for example, when
> you need to send a request during the drained section. An example is
> block_resize.
> 
> In addition, bdrv_drained_begin()/end() ensures that the callback of
> blk_aio_*() functions has been invoked (see commit 46aaf2a566,
> "block-backend: Decrease in_flight only after callback", 2018-09-25).  This
> new lock would not ensure that.
> 
> As an aside, instead of is_external, QEMU could remove/add the ioeventfd
> handler in the blk->dev_ops->drained_begin and blk->dev_ops->drained_end
> callbacks respectively. But that's just a code cleanup.

Interesting idea. If is_external is a block layer-specific feature that
nothing else in QEMU uses then I like the idea because it's cleaner and
more obvious than is_external.

Stefan

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

Re: [RFC PATCH 0/5] Removal of AioContext lock, bs->parents and ->children: proof of concept

[Kevin Wolf]

Am 04.04.2022 um 11:41 hat Paolo Bonzini geschrieben:
> As an aside, instead of is_external, QEMU could remove/add the ioeventfd
> handler in the blk->dev_ops->drained_begin and blk->dev_ops->drained_end
> callbacks respectively. But that's just a code cleanup.

Yes, this is the proper way to do it that we intended to implement
"sometime later". aio_disable_external() is really an ugly hack.

I assume that with multiqueue, we'll have to rework this anyway because
there won't be a single AioContext to disable any more - and I'm not
sure if a node will even know from which AioContexts requests could
potentially be sent to it. So we may have to go through the proper
callback mechanisms at that point.

Kevin

Re: [RFC PATCH 0/5] Removal of AioContext lock, bs->parents and ->children: proof of concept

[Emanuele Giuseppe Esposito]

So this is a more concrete and up-to-date header.

Few things to notice:
- we have a list of AioContext. They are registered once an aiocontext
is created, and deleted when it is destroyed.
This list is helpful because each aiocontext can only modify its own
number of readers, avoiding unnecessary cacheline bouncing

- if a coroutine changes aiocontext, it's ok with regards to the
per-aiocontext reader counter. As long as the sum is correct, there's no
issue. The problem comes only once the original aiocontext is deleted,
and at that point we need to move the count it held to a shared global
variable, otherwise we risk to lose track of readers.

- All synchronization between the flags explained in this header is of
course handled in the implementation. But for now it would be nice to
have a feedback on the idea/API.

So in short we need:
- per-aiocontext counter
- global list of aiocontext
- global additional reader counter (in case an aiocontext is deleted)
- global CoQueue
- global has_writer flag
- global QemuMutex to protect the list access

Emanuele

#ifndef BLOCK_LOCK_H
#define BLOCK_LOCK_H

#include "qemu/osdep.h"

/*
 * register_aiocontext:
 * Add AioContext @ctx to the list of AioContext.
 * This list is used to obtain the total number of readers
 * currently running the graph.
 */
void register_aiocontext(AioContext *ctx);

/*
 * unregister_aiocontext:
 * Removes AioContext @ctx to the list of AioContext.
 */
void unregister_aiocontext(AioContext *ctx);

/*
 * bdrv_graph_wrlock:
 * Modify the graph. Nobody else is allowed to access the graph.
 * Set global has_writer to 1, so that the next readers will wait
 * that writer is done in a coroutine queue.
 * Then keep track of the running readers by counting what is the total
 * amount of readers (sum of all aiocontext readers), and wait until
 * they all finish with AIO_WAIT_WHILE.
 */
void bdrv_graph_wrlock(void);

/*
 * bdrv_graph_wrunlock:
 * Write finished, reset global has_writer to 0 and restart
 * all readers that are waiting.
 */
void bdrv_graph_wrunlock(void);

/*
 * bdrv_graph_co_rdlock:
 * Read the bs graph. Increases the reader counter of the current
aiocontext,
 * and if has_writer is set, it means that the writer is modifying
 * the graph, therefore wait in a coroutine queue.
 * The writer will then wake this coroutine once it is done.
 *
 * This lock cannot be taken recursively.
 */
void coroutine_fn bdrv_graph_co_rdlock(void);

/*
 * bdrv_graph_rdunlock:
 * Read terminated, decrease the count of readers in the current aiocontext.
 * If the writer is waiting for reads to finish (has_writer == 1), signal
 * the writer that we are done via aio_wait_kick() to let it continue.
 */
void coroutine_fn bdrv_graph_co_rdunlock(void);

#endif /* BLOCK_LOCK_H */

Re: [RFC PATCH 0/5] Removal of AioContext lock, bs->parents and ->children: proof of concept

[Kevin Wolf]

Am 13.04.2022 um 15:43 hat Emanuele Giuseppe Esposito geschrieben:
> So this is a more concrete and up-to-date header.
> 
> Few things to notice:
> - we have a list of AioContext. They are registered once an aiocontext
> is created, and deleted when it is destroyed.
> This list is helpful because each aiocontext can only modify its own
> number of readers, avoiding unnecessary cacheline bouncing
> 
> - if a coroutine changes aiocontext, it's ok with regards to the
> per-aiocontext reader counter. As long as the sum is correct, there's no
> issue. The problem comes only once the original aiocontext is deleted,
> and at that point we need to move the count it held to a shared global
> variable, otherwise we risk to lose track of readers.

So the idea is that we can do bdrv_graph_co_rdlock() in one thread and
the corresponding bdrv_graph_co_rdunlock() in a different thread?

Would the unlock somehow remember the original thread, or do you use the
"sum is correct" argument and allow negative counter values, so you can
end up having count +1 in A and -1 in B to represent "no active
readers"? If this happens, it's likely to happen many times, so do we
have to take integer overflows into account then?

> - All synchronization between the flags explained in this header is of
> course handled in the implementation. But for now it would be nice to
> have a feedback on the idea/API.
> 
> So in short we need:
> - per-aiocontext counter
> - global list of aiocontext
> - global additional reader counter (in case an aiocontext is deleted)
> - global CoQueue
> - global has_writer flag
> - global QemuMutex to protect the list access
> 
> Emanuele
> 
> #ifndef BLOCK_LOCK_H
> #define BLOCK_LOCK_H
> 
> #include "qemu/osdep.h"
> 
> /*
>  * register_aiocontext:
>  * Add AioContext @ctx to the list of AioContext.
>  * This list is used to obtain the total number of readers
>  * currently running the graph.
>  */
> void register_aiocontext(AioContext *ctx);
> 
> /*
>  * unregister_aiocontext:
>  * Removes AioContext @ctx to the list of AioContext.
>  */
> void unregister_aiocontext(AioContext *ctx);
> 
> /*
>  * bdrv_graph_wrlock:
>  * Modify the graph. Nobody else is allowed to access the graph.
>  * Set global has_writer to 1, so that the next readers will wait
>  * that writer is done in a coroutine queue.
>  * Then keep track of the running readers by counting what is the total
>  * amount of readers (sum of all aiocontext readers), and wait until
>  * they all finish with AIO_WAIT_WHILE.
>  */
> void bdrv_graph_wrlock(void);

Do we need a coroutine version that yields instead of using
AIO_WAIT_WHILE() or are we sure this will only ever be called from
non-coroutine contexts?

> /*
>  * bdrv_graph_wrunlock:
>  * Write finished, reset global has_writer to 0 and restart
>  * all readers that are waiting.
>  */
> void bdrv_graph_wrunlock(void);
> 
> /*
>  * bdrv_graph_co_rdlock:
>  * Read the bs graph. Increases the reader counter of the current
> aiocontext,
>  * and if has_writer is set, it means that the writer is modifying
>  * the graph, therefore wait in a coroutine queue.
>  * The writer will then wake this coroutine once it is done.
>  *
>  * This lock cannot be taken recursively.
>  */
> void coroutine_fn bdrv_graph_co_rdlock(void);

What prevents it from being taken recursively when it's just a counter?
(I do see however, that you can't take a reader lock while you have the
writer lock or vice versa because it would deadlock.)

Does this being a coroutine_fn mean that we would have to convert QMP
command handlers to coroutines so that they can take the rdlock while
they don't expect the graph to change? Or should we have a non-coroutine
version, too, that works with AIO_WAIT_WHILE()?

Or should this only be taken for very small pieces of code directly
accessing the BdrvChild objects, and high-level users like QMP commands
shouldn't even consider themselves readers?

> /*
>  * bdrv_graph_rdunlock:
>  * Read terminated, decrease the count of readers in the current aiocontext.
>  * If the writer is waiting for reads to finish (has_writer == 1), signal
>  * the writer that we are done via aio_wait_kick() to let it continue.
>  */
> void coroutine_fn bdrv_graph_co_rdunlock(void);
> 
> #endif /* BLOCK_LOCK_H */

I expect that in the final version, we might want to have some sugar
like a WITH_BDRV_GRAPH_RDLOCK_GUARD() macro, but obviously that doesn't
affect the fundamental design.

Kevin

Re: [RFC PATCH 0/5] Removal of AioContext lock, bs->parents and ->children: proof of concept

[Emanuele Giuseppe Esposito]

Am 13/04/2022 um 16:51 schrieb Kevin Wolf:
> Am 13.04.2022 um 15:43 hat Emanuele Giuseppe Esposito geschrieben:
>> So this is a more concrete and up-to-date header.
>>
>> Few things to notice:
>> - we have a list of AioContext. They are registered once an aiocontext
>> is created, and deleted when it is destroyed.
>> This list is helpful because each aiocontext can only modify its own
>> number of readers, avoiding unnecessary cacheline bouncing
>>
>> - if a coroutine changes aiocontext, it's ok with regards to the
>> per-aiocontext reader counter. As long as the sum is correct, there's no
>> issue. The problem comes only once the original aiocontext is deleted,
>> and at that point we need to move the count it held to a shared global
>> variable, otherwise we risk to lose track of readers.
> 
> So the idea is that we can do bdrv_graph_co_rdlock() in one thread and
> the corresponding bdrv_graph_co_rdunlock() in a different thread?
> 
> Would the unlock somehow remember the original thread, or do you use the
> "sum is correct" argument and allow negative counter values, so you can
> end up having count +1 in A and -1 in B to represent "no active
> readers"? If this happens, it's likely to happen many times, so do we
> have to take integer overflows into account then?
> 
>> - All synchronization between the flags explained in this header is of
>> course handled in the implementation. But for now it would be nice to
>> have a feedback on the idea/API.
>>
>> So in short we need:
>> - per-aiocontext counter
>> - global list of aiocontext
>> - global additional reader counter (in case an aiocontext is deleted)
>> - global CoQueue
>> - global has_writer flag
>> - global QemuMutex to protect the list access
>>
>> Emanuele
>>
>> #ifndef BLOCK_LOCK_H
>> #define BLOCK_LOCK_H
>>
>> #include "qemu/osdep.h"
>>
>> /*
>>  * register_aiocontext:
>>  * Add AioContext @ctx to the list of AioContext.
>>  * This list is used to obtain the total number of readers
>>  * currently running the graph.
>>  */
>> void register_aiocontext(AioContext *ctx);
>>
>> /*
>>  * unregister_aiocontext:
>>  * Removes AioContext @ctx to the list of AioContext.
>>  */
>> void unregister_aiocontext(AioContext *ctx);
>>
>> /*
>>  * bdrv_graph_wrlock:
>>  * Modify the graph. Nobody else is allowed to access the graph.
>>  * Set global has_writer to 1, so that the next readers will wait
>>  * that writer is done in a coroutine queue.
>>  * Then keep track of the running readers by counting what is the total
>>  * amount of readers (sum of all aiocontext readers), and wait until
>>  * they all finish with AIO_WAIT_WHILE.
>>  */
>> void bdrv_graph_wrlock(void);
> 
> Do we need a coroutine version that yields instead of using
> AIO_WAIT_WHILE() or are we sure this will only ever be called from
> non-coroutine contexts?

writes (graph modifications) are always done under BQL in the main loop.
Except an unit test, I don't think a coroutine ever does that.

> 
>> /*
>>  * bdrv_graph_wrunlock:
>>  * Write finished, reset global has_writer to 0 and restart
>>  * all readers that are waiting.
>>  */
>> void bdrv_graph_wrunlock(void);
>>
>> /*
>>  * bdrv_graph_co_rdlock:
>>  * Read the bs graph. Increases the reader counter of the current
>> aiocontext,
>>  * and if has_writer is set, it means that the writer is modifying
>>  * the graph, therefore wait in a coroutine queue.
>>  * The writer will then wake this coroutine once it is done.
>>  *
>>  * This lock cannot be taken recursively.
>>  */
>> void coroutine_fn bdrv_graph_co_rdlock(void);
> 
> What prevents it from being taken recursively when it's just a counter?
> (I do see however, that you can't take a reader lock while you have the
> writer lock or vice versa because it would deadlock.)
> 
I actually didn't add the assertion to prevent it from being recoursive
yet, but I think it simplifies everything if it's not recoursive

> Does this being a coroutine_fn mean that we would have to convert QMP
> command handlers to coroutines so that they can take the rdlock while
> they don't expect the graph to change? Or should we have a non-coroutine
> version, too, that works with AIO_WAIT_WHILE()?

Why convert the QMP command handlers? coroutine_fn was just to signal
that it can also be called from coroutines, like the ones created by the
blk_* API.
A reader does not have to be a coroutine. AIO_WAIT_WHILE is not
mandatory to allow it to finish, it helps to ensure progress in case
some reader is waiting for something, but other than that is not
necessary IMO.

> Or should this only be taken for very small pieces of code directly
> accessing the BdrvChild objects, and high-level users like QMP commands
> shouldn't even consider themselves readers?
> 

No I think if we focus on small pieces of code we end up having a
million lock/unlock pairs.

>> /*
>>  * bdrv_graph_rdunlock:
>>  * Read terminated, decrease the count of readers in the current aiocontext.
>>  * If the writer is waiting for reads to finish (has_writer == 1), signal
>>  * the writer that we are done via aio_wait_kick() to let it continue.
>>  */
>> void coroutine_fn bdrv_graph_co_rdunlock(void);
>>
>> #endif /* BLOCK_LOCK_H */
> 
> I expect that in the final version, we might want to have some sugar
> like a WITH_BDRV_GRAPH_RDLOCK_GUARD() macro, but obviously that doesn't
> affect the fundamental design.

Yeah I will ping you once I get to that point ;)

Emanuele
> 
> Kevin
> 

Re: [RFC PATCH 0/5] Removal of AioContext lock, bs->parents and ->children: proof of concept

[Emanuele Giuseppe Esposito]

Am 13/04/2022 um 17:14 schrieb Emanuele Giuseppe Esposito:
> 
> 
> Am 13/04/2022 um 16:51 schrieb Kevin Wolf:
>> Am 13.04.2022 um 15:43 hat Emanuele Giuseppe Esposito geschrieben:
>>> So this is a more concrete and up-to-date header.
>>>
>>> Few things to notice:
>>> - we have a list of AioContext. They are registered once an aiocontext
>>> is created, and deleted when it is destroyed.
>>> This list is helpful because each aiocontext can only modify its own
>>> number of readers, avoiding unnecessary cacheline bouncing
>>>
>>> - if a coroutine changes aiocontext, it's ok with regards to the
>>> per-aiocontext reader counter. As long as the sum is correct, there's no
>>> issue. The problem comes only once the original aiocontext is deleted,
>>> and at that point we need to move the count it held to a shared global
>>> variable, otherwise we risk to lose track of readers.
>>
>> So the idea is that we can do bdrv_graph_co_rdlock() in one thread and
>> the corresponding bdrv_graph_co_rdunlock() in a different thread?
>>
>> Would the unlock somehow remember the original thread, or do you use the
>> "sum is correct" argument and allow negative counter values, so you can
>> end up having count +1 in A and -1 in B to represent "no active
>> readers"? If this happens, it's likely to happen many times, so do we
>> have to take integer overflows into account then?
>>
>>> - All synchronization between the flags explained in this header is of
>>> course handled in the implementation. But for now it would be nice to
>>> have a feedback on the idea/API.
>>>
>>> So in short we need:
>>> - per-aiocontext counter
>>> - global list of aiocontext
>>> - global additional reader counter (in case an aiocontext is deleted)
>>> - global CoQueue
>>> - global has_writer flag
>>> - global QemuMutex to protect the list access
>>>
>>> Emanuele
>>>
>>> #ifndef BLOCK_LOCK_H
>>> #define BLOCK_LOCK_H
>>>
>>> #include "qemu/osdep.h"
>>>
>>> /*
>>>  * register_aiocontext:
>>>  * Add AioContext @ctx to the list of AioContext.
>>>  * This list is used to obtain the total number of readers
>>>  * currently running the graph.
>>>  */
>>> void register_aiocontext(AioContext *ctx);
>>>
>>> /*
>>>  * unregister_aiocontext:
>>>  * Removes AioContext @ctx to the list of AioContext.
>>>  */
>>> void unregister_aiocontext(AioContext *ctx);
>>>
>>> /*
>>>  * bdrv_graph_wrlock:
>>>  * Modify the graph. Nobody else is allowed to access the graph.
>>>  * Set global has_writer to 1, so that the next readers will wait
>>>  * that writer is done in a coroutine queue.
>>>  * Then keep track of the running readers by counting what is the total
>>>  * amount of readers (sum of all aiocontext readers), and wait until
>>>  * they all finish with AIO_WAIT_WHILE.
>>>  */
>>> void bdrv_graph_wrlock(void);
>>
>> Do we need a coroutine version that yields instead of using
>> AIO_WAIT_WHILE() or are we sure this will only ever be called from
>> non-coroutine contexts?
> 
> writes (graph modifications) are always done under BQL in the main loop.
> Except an unit test, I don't think a coroutine ever does that.

Additional point (1): I am also prepraring a serie with all "helpful
fixes" I got through all other discarderd/obsolete series, like
subtree_drain and similar.

So except for the job patches, you can discard all other series.

> 
>>
>>> /*
>>>  * bdrv_graph_wrunlock:
>>>  * Write finished, reset global has_writer to 0 and restart
>>>  * all readers that are waiting.
>>>  */
>>> void bdrv_graph_wrunlock(void);
>>>
>>> /*
>>>  * bdrv_graph_co_rdlock:
>>>  * Read the bs graph. Increases the reader counter of the current
>>> aiocontext,
>>>  * and if has_writer is set, it means that the writer is modifying
>>>  * the graph, therefore wait in a coroutine queue.
>>>  * The writer will then wake this coroutine once it is done.
>>>  *
>>>  * This lock cannot be taken recursively.
>>>  */
>>> void coroutine_fn bdrv_graph_co_rdlock(void);
>>
>> What prevents it from being taken recursively when it's just a counter?
>> (I do see however, that you can't take a reader lock while you have the
>> writer lock or vice versa because it would deadlock.)
>>
> I actually didn't add the assertion to prevent it from being recoursive
> yet, but I think it simplifies everything if it's not recoursive


Additional point (2): I forgot that with counters there's no easy way to
avoid recursion, so yeah theoretically it can be recursive. Still,
better avoid doing it intentionally though.

> 
>> Does this being a coroutine_fn mean that we would have to convert QMP
>> command handlers to coroutines so that they can take the rdlock while
>> they don't expect the graph to change? Or should we have a non-coroutine
>> version, too, that works with AIO_WAIT_WHILE()?
> 
> Why convert the QMP command handlers? coroutine_fn was just to signal
> that it can also be called from coroutines, like the ones created by the
> blk_* API.
> A reader does not have to be a coroutine. AIO_WAIT_WHILE is not
> mandatory to allow it to finish, it helps to ensure progress in case
> some reader is waiting for something, but other than that is not
> necessary IMO.
> 
>> Or should this only be taken for very small pieces of code directly
>> accessing the BdrvChild objects, and high-level users like QMP commands
>> shouldn't even consider themselves readers?
>>
> 
> No I think if we focus on small pieces of code we end up having a
> million lock/unlock pairs.
> 
>>> /*
>>>  * bdrv_graph_rdunlock:
>>>  * Read terminated, decrease the count of readers in the current aiocontext.
>>>  * If the writer is waiting for reads to finish (has_writer == 1), signal
>>>  * the writer that we are done via aio_wait_kick() to let it continue.
>>>  */
>>> void coroutine_fn bdrv_graph_co_rdunlock(void);
>>>
>>> #endif /* BLOCK_LOCK_H */
>>
>> I expect that in the final version, we might want to have some sugar
>> like a WITH_BDRV_GRAPH_RDLOCK_GUARD() macro, but obviously that doesn't
>> affect the fundamental design.
> 
> Yeah I will ping you once I get to that point ;)
> 
> Emanuele
>>
>> Kevin
>>

Re: [RFC PATCH 0/5] Removal of AioContext lock, bs->parents and ->children: proof of concept

[Kevin Wolf]

Am 13.04.2022 um 17:14 hat Emanuele Giuseppe Esposito geschrieben:
> Am 13/04/2022 um 16:51 schrieb Kevin Wolf:
> > Am 13.04.2022 um 15:43 hat Emanuele Giuseppe Esposito geschrieben:
> >> So this is a more concrete and up-to-date header.
> >>
> >> Few things to notice:
> >> - we have a list of AioContext. They are registered once an aiocontext
> >> is created, and deleted when it is destroyed.
> >> This list is helpful because each aiocontext can only modify its own
> >> number of readers, avoiding unnecessary cacheline bouncing
> >>
> >> - if a coroutine changes aiocontext, it's ok with regards to the
> >> per-aiocontext reader counter. As long as the sum is correct, there's no
> >> issue. The problem comes only once the original aiocontext is deleted,
> >> and at that point we need to move the count it held to a shared global
> >> variable, otherwise we risk to lose track of readers.
> > 
> > So the idea is that we can do bdrv_graph_co_rdlock() in one thread and
> > the corresponding bdrv_graph_co_rdunlock() in a different thread?
> > 
> > Would the unlock somehow remember the original thread, or do you use the
> > "sum is correct" argument and allow negative counter values, so you can
> > end up having count +1 in A and -1 in B to represent "no active
> > readers"? If this happens, it's likely to happen many times, so do we
> > have to take integer overflows into account then?
> > 
> >> - All synchronization between the flags explained in this header is of
> >> course handled in the implementation. But for now it would be nice to
> >> have a feedback on the idea/API.
> >>
> >> So in short we need:
> >> - per-aiocontext counter
> >> - global list of aiocontext
> >> - global additional reader counter (in case an aiocontext is deleted)
> >> - global CoQueue
> >> - global has_writer flag
> >> - global QemuMutex to protect the list access
> >>
> >> Emanuele
> >>
> >> #ifndef BLOCK_LOCK_H
> >> #define BLOCK_LOCK_H
> >>
> >> #include "qemu/osdep.h"
> >>
> >> /*
> >>  * register_aiocontext:
> >>  * Add AioContext @ctx to the list of AioContext.
> >>  * This list is used to obtain the total number of readers
> >>  * currently running the graph.
> >>  */
> >> void register_aiocontext(AioContext *ctx);
> >>
> >> /*
> >>  * unregister_aiocontext:
> >>  * Removes AioContext @ctx to the list of AioContext.
> >>  */
> >> void unregister_aiocontext(AioContext *ctx);
> >>
> >> /*
> >>  * bdrv_graph_wrlock:
> >>  * Modify the graph. Nobody else is allowed to access the graph.
> >>  * Set global has_writer to 1, so that the next readers will wait
> >>  * that writer is done in a coroutine queue.
> >>  * Then keep track of the running readers by counting what is the total
> >>  * amount of readers (sum of all aiocontext readers), and wait until
> >>  * they all finish with AIO_WAIT_WHILE.
> >>  */
> >> void bdrv_graph_wrlock(void);
> > 
> > Do we need a coroutine version that yields instead of using
> > AIO_WAIT_WHILE() or are we sure this will only ever be called from
> > non-coroutine contexts?
> 
> writes (graph modifications) are always done under BQL in the main loop.

Yes, I think we're fairly certain about this part.

> Except an unit test, I don't think a coroutine ever does that.

I'm not sure about this one, though. Didn't you have cases where
bdrv_replace_child_noperm() was called in coroutine context? Or maybe
I'm mixing up things here.

> >> /*
> >>  * bdrv_graph_wrunlock:
> >>  * Write finished, reset global has_writer to 0 and restart
> >>  * all readers that are waiting.
> >>  */
> >> void bdrv_graph_wrunlock(void);
> >>
> >> /*
> >>  * bdrv_graph_co_rdlock:
> >>  * Read the bs graph. Increases the reader counter of the current
> >> aiocontext,
> >>  * and if has_writer is set, it means that the writer is modifying
> >>  * the graph, therefore wait in a coroutine queue.
> >>  * The writer will then wake this coroutine once it is done.
> >>  *
> >>  * This lock cannot be taken recursively.
> >>  */
> >> void coroutine_fn bdrv_graph_co_rdlock(void);
> > 
> > What prevents it from being taken recursively when it's just a counter?
> > (I do see however, that you can't take a reader lock while you have the
> > writer lock or vice versa because it would deadlock.)
> > 
> I actually didn't add the assertion to prevent it from being recoursive
> yet, but I think it simplifies everything if it's not recoursive
> 
> > Does this being a coroutine_fn mean that we would have to convert QMP
> > command handlers to coroutines so that they can take the rdlock while
> > they don't expect the graph to change? Or should we have a non-coroutine
> > version, too, that works with AIO_WAIT_WHILE()?
> 
> Why convert the QMP command handlers? coroutine_fn was just to signal
> that it can also be called from coroutines, like the ones created by the
> blk_* API.

coroutine_fn means that it can _only_ be called from coroutines (because
it will yield, which doesn't work outside of a coroutine - not sure what
happens, probably just a crash).

> A reader does not have to be a coroutine. AIO_WAIT_WHILE is not
> mandatory to allow it to finish, it helps to ensure progress in case
> some reader is waiting for something, but other than that is not
> necessary IMO.

When it's outside of a coroutine, how would you implement waiting for a
writer to finish if not with AIO_WAIT_WHILE()?

> > Or should this only be taken for very small pieces of code directly
> > accessing the BdrvChild objects, and high-level users like QMP commands
> > shouldn't even consider themselves readers?
> > 
> 
> No I think if we focus on small pieces of code we end up having a
> million lock/unlock pairs.

Yes, I agree. On the other hand, if we're taking the locks in high-level
outer operations, avoiding to take the lock recursively might become
harder. I guess we'll see how it works out when we actually introduce
callers.

Kevin

Re: [RFC PATCH 0/5] Removal of AioContext lock, bs->parents and ->children: proof of concept

[Paolo Bonzini]

On 4/13/22 18:29, Kevin Wolf wrote:
>> A reader does not have to be a coroutine. AIO_WAIT_WHILE is not
>> mandatory to allow it to finish, it helps to ensure progress in case
>> some reader is waiting for something, but other than that is not
>> necessary IMO.
> When it's outside of a coroutine, how would you implement waiting for a
> writer to finish if not with AIO_WAIT_WHILE()?
> 

In the main thread a non-coroutine can always read the graph though, 
because the only writer can be the main thread.

If the critical sections are large enough, I don't think rdlock needs to 
be taken outside a coroutine in the iothread, e.g. in a bottom half.

>> No I think if we focus on small pieces of code we end up having a
>> million lock/unlock pairs.
> 
> Yes, I agree. On the other hand, if we're taking the locks in high-level
> outer operations, avoiding to take the lock recursively might become
> harder. I guess we'll see how it works out when we actually introduce
> callers.

My "hope" is that taking the locks in blk_* functions covers most of the 
calls, and then only a few (dozens) direct uses of bdrv_* remain.

Unfortunately, adding assertions is not easy because "is there a reader" 
cannot be easily answered.  But I think Emanuele has a debug mode that 
can enable the assertions at a performance cost.

Paolo

Re: [RFC PATCH 0/5] Removal of AioContext lock, bs->parents and ->children: proof of concept

[Paolo Bonzini]

On 4/13/22 16:51, Kevin Wolf wrote:
> So the idea is that we can do bdrv_graph_co_rdlock() in one thread and
> the corresponding bdrv_graph_co_rdunlock() in a different thread?
> 
> Would the unlock somehow remember the original thread, or do you use the
> "sum is correct" argument and allow negative counter values, so you can
> end up having count +1 in A and -1 in B to represent "no active
> readers"? If this happens, it's likely to happen many times, so do we
> have to take integer overflows into account then?

The counter cannot be negative, so you can use uint32_t and sum modulo 
2^32.  You might have a thread with counter 2^31+1 which is negative in 
twos complement; and a thread with counter -2^31-1 which is positive in 
twos complement; but their sum cancels out correctly.

Paolo

Re: [RFC PATCH 0/5] Removal of AioContext lock, bs->parents and ->children: proof of concept

[Vladimir Sementsov-Ogievskiy]

01.03.2022 17:21, Emanuele Giuseppe Esposito wrote:
> This serie tries to provide a proof of concept and a clear explanation
> on why we need to use drains (and more precisely subtree_drains)
> to replace the aiocontext lock, especially to protect BlockDriverState
> ->children and ->parent lists.
> 
> Just a small recap on the key concepts:
> * We split block layer APIs in "global state" (GS), "I/O", and
> "global state or I/O".
>    GS are running in the main loop, under BQL, and are the only
>    one allowed to modify the BlockDriverState graph.
> 
>    I/O APIs are thread safe and can run in any thread
> 
>    "global state or I/O" are essentially all APIs that use
>    BDRV_POLL_WHILE. This is because there can be only 2 threads
>    that can use BDRV_POLL_WHILE: main loop and the iothread that
>    runs the aiocontext.
> 
> * Drains allow the caller (either main loop or iothread running
> the context) to wait all in_flights requests and operations
> of a BDS: normal drains target a given node and is parents, while
> subtree ones also include the subgraph of the node. Siblings are
> not affected by any of these two kind of drains.
> After bdrv_drained_begin, no more request is allowed to come
> from the affected nodes. Therefore the only actor left working
> on a drained part of the graph should be the main loop.
> 
> What do we intend to do
> -----------------------
> We want to remove the AioContext lock. It is not 100% clear on how
> many things we are protecting with it, and why.
> As a starter, we want to protect BlockDriverState ->parents and
> ->children lists, since they are read by main loop and I/O but
> only written by main loop under BQL. The function that modifies
> these lists is bdrv_replace_child_common().
> 
> How do we want to do it
> -----------------------
> We individuated as ideal subtitute of AioContext lock
> the subtree_drain API. The reason is simple: draining prevents the iothread to read or write the nodes, so once the main loop finishes

I'm not sure it's ideal. Unfortunately I'm not really good in all that BQL, AioContext locks and drains. So, I can't give good advice. But here are my doubts:

Draining is very restrictive measure: even if drained section is very short, at least on bdrv_drained_begin() we have to wait for all current requests, and don't start new ones. That slows down the guest. In the same time there are operations that don't require to stop guest IO requests. For example manipulation with dirty bitmaps - qmp commands block-dirty-bitmap-add block-dirty-bitmap-remove. Or different query requests..

I see only two real cases, where we do need drain:

1. When we need a consistent "point-in-time". For example, when we start backup in transaction with some dirty-bitmap manipulation commands.

2. When we need to modify block-graph: if we are going to break relation A -> B, there must not be any in-flight request that want to use this relation.

All other operations, for which we want some kind of lock (like AioContext lock or something) we actually don't want to stop guest IO.


Next, I have a problem in mind, that in past lead to a lot of iotest 30 failures. Next there were different fixes and improvements, but the core problem (as far as I understand) is still here: nothing protects us when we are in some graph modification process (for example block-job finalization) do yield, switch to other coroutine and enter another graph modification process (for example, another block-job finaliztion)..
(for details look at my old "[PATCH RFC 0/5] Fix accidental crash in iotest 30"  https://lists.nongnu.org/archive/html/qemu-devel/2020-11/msg05290.html , where I suggested to add a global graph_modify_mutex CoMutex, to be held during graph-modifying process that may yield)..
Does your proposal solve this problem?


> executing bdrv_drained_begin() on the interested graph, we are sure that
> the iothread is not going to look or even interfere with that part of the graph.
> We are also sure that the only two actors that can look at a specific
> BlockDriverState in any given context are the main loop and the
> iothread running the AioContext (ensured by "global state or IO" logic).
> 
> Why use _subtree_ instead of normal drain
> -----------------------------------------
> A simple drain "blocks" a given node and all its parents.
> But it doesn't touch the child.
> This means that if we use a simple drain, a child can always
> keep processing requests, and eventually end up calling itself
> bdrv_drained_begin, ending up reading the parents node while the main loop
> is modifying them. Therefore a subtree drain is necessary.

I'm not sure I understand.. Could you give a more concrete example of what may happen if we use simple drain?
Who will call bdrv_drained_begin() you say about? Some IO path in parallel thread? Do we really have an IO path that calls bdrv_drained_begin()?

> 
> Possible scenarios
> -------------------
> Keeping in mind that we can only have an iothread and the main loop
> draining on a certain node, we could have:
> 
> main loop successfully drains and then iothread tries to drain:
>    impossible scenario, as iothread is already stopped once main
>    successfully drains.
> 
> iothread successfully drains and then main loop drains:
>    should not be a problem, as:
>    1) the iothread should be already "blocked" by its own drain
>    2) main loop would still wait for it to completely block
>    There is the issue of mirror overriding such scenario to avoid
>    having deadlocks, but that is handled in the next section.
> 
> main loop and iothread try to drain together:
>    As above, this case doens't really matter. As long as
>    bdrv_drained_begin invariant is respected, the main loop will
>    continue only once the iothread is "blocked" on that part of the graph.
> 
> A note on iothread draining
> ---------------------------
> Theoretically draining from an iothread should not be possible,
> as the iothread would be scheduling a bh in the main loop waiting
> for itself to stop, even though it is not yet stopped since it is waiting for the bh.
> 
> This is what would happen in the tests in patch 5 if .drained_poll
> was not implemented.
> 
> Therefore, one solution is to use .drained_poll callback in BlockJobDriver.
> This callback overrides the default job poll() behavior, and
> allows the polling condition to stop waiting for the job.
> It is actually used only in mirror.
> This however breaks bdrv_drained_begin invariant, because the
> iothread is not really blocked on that node but continues running.
> In order to fix this, patch 4 allows the polling condition to be
> used only by the iothread, and not the main loop too, preventing
> the drain to return before the iothread is effectively stopped.
> This is also shown in the tests in patch 5. If the fix in patch
> 4 is removed, then the main loop drain will return earlier and
> allow the iothread to run and drain together.
> 
> The other patches in this serie are cherry-picked from the various
> series I already sent, and are included here just to allow
> subtree_drained_begin/end_unlocked implementation.
> 
> Emanuele Giuseppe Esposito (5):
>    aio-wait.h: introduce AIO_WAIT_WHILE_UNLOCKED
>    introduce BDRV_POLL_WHILE_UNLOCKED
>    block/io.c: introduce bdrv_subtree_drained_{begin/end}_unlocked
>    child_job_drained_poll: override polling condition only when in home
>      thread
>    test-bdrv-drain: ensure draining from main loop stops iothreads
> 
>   block/io.c                   |  48 ++++++--
>   blockjob.c                   |   3 +-
>   include/block/aio-wait.h     |  15 ++-
>   include/block/block.h        |   7 ++
>   tests/unit/test-bdrv-drain.c | 218 +++++++++++++++++++++++++++++++++++
>   5 files changed, 274 insertions(+), 17 deletions(-)
> 


-- 
Best regards,
Vladimir

Re: [RFC PATCH 0/5] Removal of AioContext lock, bs->parents and ->children: proof of concept

[Stefan Hajnoczi]

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

On Wed, Mar 02, 2022 at 02:07:30PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 01.03.2022 17:21, Emanuele Giuseppe Esposito wrote:
> > This serie tries to provide a proof of concept and a clear explanation
> > on why we need to use drains (and more precisely subtree_drains)
> > to replace the aiocontext lock, especially to protect BlockDriverState
> > ->children and ->parent lists.
> > 
> > Just a small recap on the key concepts:
> > * We split block layer APIs in "global state" (GS), "I/O", and
> > "global state or I/O".
> >    GS are running in the main loop, under BQL, and are the only
> >    one allowed to modify the BlockDriverState graph.
> > 
> >    I/O APIs are thread safe and can run in any thread
> > 
> >    "global state or I/O" are essentially all APIs that use
> >    BDRV_POLL_WHILE. This is because there can be only 2 threads
> >    that can use BDRV_POLL_WHILE: main loop and the iothread that
> >    runs the aiocontext.
> > 
> > * Drains allow the caller (either main loop or iothread running
> > the context) to wait all in_flights requests and operations
> > of a BDS: normal drains target a given node and is parents, while
> > subtree ones also include the subgraph of the node. Siblings are
> > not affected by any of these two kind of drains.
> > After bdrv_drained_begin, no more request is allowed to come
> > from the affected nodes. Therefore the only actor left working
> > on a drained part of the graph should be the main loop.
> > 
> > What do we intend to do
> > -----------------------
> > We want to remove the AioContext lock. It is not 100% clear on how
> > many things we are protecting with it, and why.
> > As a starter, we want to protect BlockDriverState ->parents and
> > ->children lists, since they are read by main loop and I/O but
> > only written by main loop under BQL. The function that modifies
> > these lists is bdrv_replace_child_common().
> > 
> > How do we want to do it
> > -----------------------
> > We individuated as ideal subtitute of AioContext lock
> > the subtree_drain API. The reason is simple: draining prevents the iothread to read or write the nodes, so once the main loop finishes
> 
> I'm not sure it's ideal. Unfortunately I'm not really good in all that BQL, AioContext locks and drains. So, I can't give good advice. But here are my doubts:
> 
> Draining is very restrictive measure: even if drained section is very short, at least on bdrv_drained_begin() we have to wait for all current requests, and don't start new ones. That slows down the guest. In the same time there are operations that don't require to stop guest IO requests. For example manipulation with dirty bitmaps - qmp commands block-dirty-bitmap-add block-dirty-bitmap-remove. Or different query requests..
> 
> I see only two real cases, where we do need drain:
> 
> 1. When we need a consistent "point-in-time". For example, when we start backup in transaction with some dirty-bitmap manipulation commands.
> 
> 2. When we need to modify block-graph: if we are going to break relation A -> B, there must not be any in-flight request that want to use this relation.
> 
> All other operations, for which we want some kind of lock (like AioContext lock or something) we actually don't want to stop guest IO.

Yes, I think so too.

Block drivers should already be at a point where block-dirty-bitmap-add
and similar commands can be synchronized with just the qcow2 CoMutex.

Not every aio_context_acquire() needs to be replace by a drained
section. Some can safely be dropped because there are fine-grained locks
in place that provide sufficient protection.

Stefan

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

Re: [RFC PATCH 0/5] Removal of AioContext lock, bs->parents and ->children: proof of concept

[Emanuele Giuseppe Esposito]

Am 02/03/2022 um 12:07 schrieb Vladimir Sementsov-Ogievskiy:
> 01.03.2022 17:21, Emanuele Giuseppe Esposito wrote:
>> This serie tries to provide a proof of concept and a clear explanation
>> on why we need to use drains (and more precisely subtree_drains)
>> to replace the aiocontext lock, especially to protect BlockDriverState
>> ->children and ->parent lists.
>>
>> Just a small recap on the key concepts:
>> * We split block layer APIs in "global state" (GS), "I/O", and
>> "global state or I/O".
>>    GS are running in the main loop, under BQL, and are the only
>>    one allowed to modify the BlockDriverState graph.
>>
>>    I/O APIs are thread safe and can run in any thread
>>
>>    "global state or I/O" are essentially all APIs that use
>>    BDRV_POLL_WHILE. This is because there can be only 2 threads
>>    that can use BDRV_POLL_WHILE: main loop and the iothread that
>>    runs the aiocontext.
>>
>> * Drains allow the caller (either main loop or iothread running
>> the context) to wait all in_flights requests and operations
>> of a BDS: normal drains target a given node and is parents, while
>> subtree ones also include the subgraph of the node. Siblings are
>> not affected by any of these two kind of drains.
>> After bdrv_drained_begin, no more request is allowed to come
>> from the affected nodes. Therefore the only actor left working
>> on a drained part of the graph should be the main loop.
>>
>> What do we intend to do
>> -----------------------
>> We want to remove the AioContext lock. It is not 100% clear on how
>> many things we are protecting with it, and why.
>> As a starter, we want to protect BlockDriverState ->parents and
>> ->children lists, since they are read by main loop and I/O but
>> only written by main loop under BQL. The function that modifies
>> these lists is bdrv_replace_child_common().
>>
>> How do we want to do it
>> -----------------------
>> We individuated as ideal subtitute of AioContext lock
>> the subtree_drain API. The reason is simple: draining prevents the
>> iothread to read or write the nodes, so once the main loop finishes
> 
> I'm not sure it's ideal. Unfortunately I'm not really good in all that
> BQL, AioContext locks and drains. So, I can't give good advice. But here
> are my doubts:
> 
> Draining is very restrictive measure: even if drained section is very
> short, at least on bdrv_drained_begin() we have to wait for all current
> requests, and don't start new ones. That slows down the guest. 

I don't think we are in a critical path where performance is important here.

In the
> same time there are operations that don't require to stop guest IO
> requests. For example manipulation with dirty bitmaps - qmp commands
> block-dirty-bitmap-add block-dirty-bitmap-remove. Or different query
> requests..
>

Maybe you misunderstood or I was not 100% clear, but I am talking about replacing the AioContext lock for the ->parents and ->children instance. Not everywhere. This is the first step, and then we will see if the additional things that it protects can use drain or something else

 
> I see only two real cases, where we do need drain:
> 
> 1. When we need a consistent "point-in-time". For example, when we start
> backup in transaction with some dirty-bitmap manipulation commands.
> 
> 2. When we need to modify block-graph: if we are going to break relation
> A -> B, there must not be any in-flight request that want to use this
> relation.

That's the use case I am considering.
> 
> All other operations, for which we want some kind of lock (like
> AioContext lock or something) we actually don't want to stop guest IO.

Yes, they have to be analyzed case by case.
> 
> 
> Next, I have a problem in mind, that in past lead to a lot of iotest 30
> failures. Next there were different fixes and improvements, but the core
> problem (as far as I understand) is still here: nothing protects us when
> we are in some graph modification process (for example block-job
> finalization) do yield, switch to other coroutine and enter another
> graph modification process (for example, another block-job finaliztion)..

That's another point to consider. I don't really have a solution for this.

> (for details look at my old "[PATCH RFC 0/5] Fix accidental crash in
> iotest 30" 
> https://lists.nongnu.org/archive/html/qemu-devel/2020-11/msg05290.html ,
> where I suggested to add a global graph_modify_mutex CoMutex, to be held
> during graph-modifying process that may yield)..
> Does your proposal solve this problem?
> 
> 
>> executing bdrv_drained_begin() on the interested graph, we are sure that
>> the iothread is not going to look or even interfere with that part of
>> the graph.
>> We are also sure that the only two actors that can look at a specific
>> BlockDriverState in any given context are the main loop and the
>> iothread running the AioContext (ensured by "global state or IO" logic).
>>
>> Why use _subtree_ instead of normal drain
>> -----------------------------------------
>> A simple drain "blocks" a given node and all its parents.
>> But it doesn't touch the child.
>> This means that if we use a simple drain, a child can always
>> keep processing requests, and eventually end up calling itself
>> bdrv_drained_begin, ending up reading the parents node while the main
>> loop
>> is modifying them. Therefore a subtree drain is necessary.
> 
> I'm not sure I understand.. Could you give a more concrete example of
> what may happen if we use simple drain?
> Who will call bdrv_drained_begin() you say about? Some IO path in
> parallel thread? Do we really have an IO path that calls
> bdrv_drained_begin()?

It is already being done in mirror, where it drains while running the .run() Job callback.

I made an unit test for this:
https://gitlab.com/eesposit/qemu/-/commit/1ffd7193ed441020f51aeeb49c3b07edb6949760

assume that you have the following graph:

blk (blk) -> overlay (bs)
overlay (bs) --- backed by ---> backing (bs) 
job1 (blockjob) --> backing

Then the main loop calls bdrv_drained_begin(overlay)
This means overlay and bs are under drain, but backing and most importantly the job are not.
At this point, the job run() function decides to drain. It should not be
allowed to read overlay parents list for example, but instead there is nothing
to prevent it from doing that, and thus we see drains_done >0.

On the other side, when we subtree_drain, also the job is drained and thus it goes in
pause.

Makes sense?
> 
>>
>> Possible scenarios
>> -------------------
>> Keeping in mind that we can only have an iothread and the main loop
>> draining on a certain node, we could have:
>>
>> main loop successfully drains and then iothread tries to drain:
>>    impossible scenario, as iothread is already stopped once main
>>    successfully drains.
>>
>> iothread successfully drains and then main loop drains:
>>    should not be a problem, as:
>>    1) the iothread should be already "blocked" by its own drain
>>    2) main loop would still wait for it to completely block
>>    There is the issue of mirror overriding such scenario to avoid
>>    having deadlocks, but that is handled in the next section.
>>
>> main loop and iothread try to drain together:
>>    As above, this case doens't really matter. As long as
>>    bdrv_drained_begin invariant is respected, the main loop will
>>    continue only once the iothread is "blocked" on that part of the
>> graph.
>>
>> A note on iothread draining
>> ---------------------------
>> Theoretically draining from an iothread should not be possible,
>> as the iothread would be scheduling a bh in the main loop waiting
>> for itself to stop, even though it is not yet stopped since it is
>> waiting for the bh.
>>
>> This is what would happen in the tests in patch 5 if .drained_poll
>> was not implemented.
>>
>> Therefore, one solution is to use .drained_poll callback in
>> BlockJobDriver.
>> This callback overrides the default job poll() behavior, and
>> allows the polling condition to stop waiting for the job.
>> It is actually used only in mirror.
>> This however breaks bdrv_drained_begin invariant, because the
>> iothread is not really blocked on that node but continues running.
>> In order to fix this, patch 4 allows the polling condition to be
>> used only by the iothread, and not the main loop too, preventing
>> the drain to return before the iothread is effectively stopped.
>> This is also shown in the tests in patch 5. If the fix in patch
>> 4 is removed, then the main loop drain will return earlier and
>> allow the iothread to run and drain together.
>>
>> The other patches in this serie are cherry-picked from the various
>> series I already sent, and are included here just to allow
>> subtree_drained_begin/end_unlocked implementation.
>>
>> Emanuele Giuseppe Esposito (5):
>>    aio-wait.h: introduce AIO_WAIT_WHILE_UNLOCKED
>>    introduce BDRV_POLL_WHILE_UNLOCKED
>>    block/io.c: introduce bdrv_subtree_drained_{begin/end}_unlocked
>>    child_job_drained_poll: override polling condition only when in home
>>      thread
>>    test-bdrv-drain: ensure draining from main loop stops iothreads
>>
>>   block/io.c                   |  48 ++++++--
>>   blockjob.c                   |   3 +-
>>   include/block/aio-wait.h     |  15 ++-
>>   include/block/block.h        |   7 ++
>>   tests/unit/test-bdrv-drain.c | 218 +++++++++++++++++++++++++++++++++++
>>   5 files changed, 274 insertions(+), 17 deletions(-)
>>
> 
> 

Re: [RFC PATCH 0/5] Removal of AioContext lock, bs->parents and ->children: proof of concept

[Emanuele Giuseppe Esposito]

Am 09/03/2022 um 14:26 schrieb Emanuele Giuseppe Esposito:
>> Next, I have a problem in mind, that in past lead to a lot of iotest 30
>> failures. Next there were different fixes and improvements, but the core
>> problem (as far as I understand) is still here: nothing protects us when
>> we are in some graph modification process (for example block-job
>> finalization) do yield, switch to other coroutine and enter another
>> graph modification process (for example, another block-job finaliztion)..
> That's another point to consider. I don't really have a solution for this.
> 
On a side note, that might not be a real problem.
If I understand correctly, your fear is that we are doing something like
parent->children[x] = new_node // partial graph operation
/* yield to another coroutine */
coroutine reads/writes parent->children[x] and/or new_node->parents[y]
/* yield back */
new_node->parents[y] = parent // end of the initial graph operation

Is that what you are pointing out here?
If so, is there a concrete example for this? Because yields and drains
(that eventually can poll) seem to be put either before or after the
whole graph modification section. In other words, even if a coroutine
enters, it will be always before or after the _whole_ graph modification
is performed.

Thank you,
Emanuele

Re: [RFC PATCH 0/5] Removal of AioContext lock, bs->parents and ->children: proof of concept

[Vladimir Sementsov-Ogievskiy]

17.03.2022 00:55, Emanuele Giuseppe Esposito wrote:
> 
> 
> Am 09/03/2022 um 14:26 schrieb Emanuele Giuseppe Esposito:
>>> Next, I have a problem in mind, that in past lead to a lot of iotest 30
>>> failures. Next there were different fixes and improvements, but the core
>>> problem (as far as I understand) is still here: nothing protects us when
>>> we are in some graph modification process (for example block-job
>>> finalization) do yield, switch to other coroutine and enter another
>>> graph modification process (for example, another block-job finaliztion)..
>> That's another point to consider. I don't really have a solution for this.
>>
> On a side note, that might not be a real problem.
> If I understand correctly, your fear is that we are doing something like
> parent->children[x] = new_node // partial graph operation
> /* yield to another coroutine */
> coroutine reads/writes parent->children[x] and/or new_node->parents[y]
> /* yield back */
> new_node->parents[y] = parent // end of the initial graph operation
> 
> Is that what you are pointing out here?
> If so, is there a concrete example for this? Because yields and drains
> (that eventually can poll) seem to be put either before or after the
> whole graph modification section. In other words, even if a coroutine
> enters, it will be always before or after the _whole_ graph modification
> is performed.
> 

The old example was here: https://lists.gnu.org/archive/html/qemu-devel/2020-11/msg05212.html  - not sure how much is it applicable now.

Another example - look at bdrv_drop_intermediate() in block.c and at TODO comments in it.

In both cases the problem is we want to update some metadata in qcow2 (backing file name) as part of block-graph modification. But this update does write to qcow2 header which may yield and switch to some another block-graph modification code.


-- 
Best regards,
Vladimir

Re: [RFC PATCH 0/5] Removal of AioContext lock, bs->parents and ->children: proof of concept

[Vladimir Sementsov-Ogievskiy]

09.03.2022 16:26, Emanuele Giuseppe Esposito wrote:
> 
> 
> Am 02/03/2022 um 12:07 schrieb Vladimir Sementsov-Ogievskiy:
>> 01.03.2022 17:21, Emanuele Giuseppe Esposito wrote:
>>> This serie tries to provide a proof of concept and a clear explanation
>>> on why we need to use drains (and more precisely subtree_drains)
>>> to replace the aiocontext lock, especially to protect BlockDriverState
>>> ->children and ->parent lists.
>>>
>>> Just a small recap on the key concepts:
>>> * We split block layer APIs in "global state" (GS), "I/O", and
>>> "global state or I/O".
>>>     GS are running in the main loop, under BQL, and are the only
>>>     one allowed to modify the BlockDriverState graph.
>>>
>>>     I/O APIs are thread safe and can run in any thread
>>>
>>>     "global state or I/O" are essentially all APIs that use
>>>     BDRV_POLL_WHILE. This is because there can be only 2 threads
>>>     that can use BDRV_POLL_WHILE: main loop and the iothread that
>>>     runs the aiocontext.
>>>
>>> * Drains allow the caller (either main loop or iothread running
>>> the context) to wait all in_flights requests and operations
>>> of a BDS: normal drains target a given node and is parents, while
>>> subtree ones also include the subgraph of the node. Siblings are
>>> not affected by any of these two kind of drains.
>>> After bdrv_drained_begin, no more request is allowed to come
>>> from the affected nodes. Therefore the only actor left working
>>> on a drained part of the graph should be the main loop.
>>>
>>> What do we intend to do
>>> -----------------------
>>> We want to remove the AioContext lock. It is not 100% clear on how
>>> many things we are protecting with it, and why.
>>> As a starter, we want to protect BlockDriverState ->parents and
>>> ->children lists, since they are read by main loop and I/O but
>>> only written by main loop under BQL. The function that modifies
>>> these lists is bdrv_replace_child_common().
>>>
>>> How do we want to do it
>>> -----------------------
>>> We individuated as ideal subtitute of AioContext lock
>>> the subtree_drain API. The reason is simple: draining prevents the
>>> iothread to read or write the nodes, so once the main loop finishes
>>
>> I'm not sure it's ideal. Unfortunately I'm not really good in all that
>> BQL, AioContext locks and drains. So, I can't give good advice. But here
>> are my doubts:
>>
>> Draining is very restrictive measure: even if drained section is very
>> short, at least on bdrv_drained_begin() we have to wait for all current
>> requests, and don't start new ones. That slows down the guest.
> 
> I don't think we are in a critical path where performance is important here.
> 
> In the
>> same time there are operations that don't require to stop guest IO
>> requests. For example manipulation with dirty bitmaps - qmp commands
>> block-dirty-bitmap-add block-dirty-bitmap-remove. Or different query
>> requests..
>>
> 
> Maybe you misunderstood or I was not 100% clear, but I am talking about replacing the AioContext lock for the ->parents and ->children instance. Not everywhere. This is the first step, and then we will see if the additional things that it protects can use drain or something else

Ok, if we are only about graph modification that's not a critical performance path.

> 
>   
>> I see only two real cases, where we do need drain:
>>
>> 1. When we need a consistent "point-in-time". For example, when we start
>> backup in transaction with some dirty-bitmap manipulation commands.
>>
>> 2. When we need to modify block-graph: if we are going to break relation
>> A -> B, there must not be any in-flight request that want to use this
>> relation.
> 
> That's the use case I am considering.
>>
>> All other operations, for which we want some kind of lock (like
>> AioContext lock or something) we actually don't want to stop guest IO.
> 
> Yes, they have to be analyzed case by case.
>>
>>
>> Next, I have a problem in mind, that in past lead to a lot of iotest 30
>> failures. Next there were different fixes and improvements, but the core
>> problem (as far as I understand) is still here: nothing protects us when
>> we are in some graph modification process (for example block-job
>> finalization) do yield, switch to other coroutine and enter another
>> graph modification process (for example, another block-job finaliztion)..
> 
> That's another point to consider. I don't really have a solution for this.
> 
>> (for details look at my old "[PATCH RFC 0/5] Fix accidental crash in
>> iotest 30"
>> https://lists.nongnu.org/archive/html/qemu-devel/2020-11/msg05290.html ,
>> where I suggested to add a global graph_modify_mutex CoMutex, to be held
>> during graph-modifying process that may yield)..
>> Does your proposal solve this problem?
>>
>>
>>> executing bdrv_drained_begin() on the interested graph, we are sure that
>>> the iothread is not going to look or even interfere with that part of
>>> the graph.
>>> We are also sure that the only two actors that can look at a specific
>>> BlockDriverState in any given context are the main loop and the
>>> iothread running the AioContext (ensured by "global state or IO" logic).
>>>
>>> Why use _subtree_ instead of normal drain
>>> -----------------------------------------
>>> A simple drain "blocks" a given node and all its parents.
>>> But it doesn't touch the child.
>>> This means that if we use a simple drain, a child can always
>>> keep processing requests, and eventually end up calling itself
>>> bdrv_drained_begin, ending up reading the parents node while the main
>>> loop
>>> is modifying them. Therefore a subtree drain is necessary.
>>
>> I'm not sure I understand.. Could you give a more concrete example of
>> what may happen if we use simple drain?
>> Who will call bdrv_drained_begin() you say about? Some IO path in
>> parallel thread? Do we really have an IO path that calls
>> bdrv_drained_begin()?
> 
> It is already being done in mirror, where it drains while running the .run() Job callback.
> 
> I made an unit test for this:
> https://gitlab.com/eesposit/qemu/-/commit/1ffd7193ed441020f51aeeb49c3b07edb6949760
> 
> assume that you have the following graph:
> 
> blk (blk) -> overlay (bs)
> overlay (bs) --- backed by ---> backing (bs)
> job1 (blockjob) --> backing
> 
> Then the main loop calls bdrv_drained_begin(overlay)
> This means overlay and bs are under drain, but backing and most importantly the job are not.
> At this point, the job run() function decides to drain. It should not be
> allowed to read overlay parents list for example, but instead there is nothing
> to prevent it from doing that, and thus we see drains_done >0.
> 
> On the other side, when we subtree_drain, also the job is drained and thus it goes in
> pause.
> 
> Makes sense?

Ah seems I understand what you mean.

One of my arguments is that "drain" - is not a lock against other clients who want to modify the graph. Because, drained section allows nested drained sections.

And you try to solve it, by draining more things, this way, we'll drain also the job, which is a possible client, who may want to modify the graph in parallel.

So, in other words, when we want to modify the graph, we drain the whole connectivity component of the graph. And we think that we are safe from other graph modifications because all related jobs are drained.
Interesting, is that possible that some not drained job from another connectivity component will want to connect some node from our drained component?

I just still feel that draining is a wrong mechanism to avoid interaction with other clients who want to modify the graph, because:

1. we stop the whole IO on all subgraph which is not necessary
2. draining is not a mutex, it allows nesting and it's ok when two different clients drain same nodes. Draining is just a requirement to do no IO at these nodes.

And in your way, it seems that to be absolutely safe we'll need to drain everything..

In my feeling it's better to keep draining what it is now: requirement to have no IO requests. And to isolate graph modifications from each other make a new synchronization mechanism, something like a global queue, where clients who want to get an access to graph modifications wait for their turn.

>>
>>>
>>> Possible scenarios
>>> -------------------
>>> Keeping in mind that we can only have an iothread and the main loop
>>> draining on a certain node, we could have:
>>>
>>> main loop successfully drains and then iothread tries to drain:
>>>     impossible scenario, as iothread is already stopped once main
>>>     successfully drains.
>>>
>>> iothread successfully drains and then main loop drains:
>>>     should not be a problem, as:
>>>     1) the iothread should be already "blocked" by its own drain
>>>     2) main loop would still wait for it to completely block
>>>     There is the issue of mirror overriding such scenario to avoid
>>>     having deadlocks, but that is handled in the next section.
>>>
>>> main loop and iothread try to drain together:
>>>     As above, this case doens't really matter. As long as
>>>     bdrv_drained_begin invariant is respected, the main loop will
>>>     continue only once the iothread is "blocked" on that part of the
>>> graph.
>>>
>>> A note on iothread draining
>>> ---------------------------
>>> Theoretically draining from an iothread should not be possible,
>>> as the iothread would be scheduling a bh in the main loop waiting
>>> for itself to stop, even though it is not yet stopped since it is
>>> waiting for the bh.
>>>
>>> This is what would happen in the tests in patch 5 if .drained_poll
>>> was not implemented.
>>>
>>> Therefore, one solution is to use .drained_poll callback in
>>> BlockJobDriver.
>>> This callback overrides the default job poll() behavior, and
>>> allows the polling condition to stop waiting for the job.
>>> It is actually used only in mirror.
>>> This however breaks bdrv_drained_begin invariant, because the
>>> iothread is not really blocked on that node but continues running.
>>> In order to fix this, patch 4 allows the polling condition to be
>>> used only by the iothread, and not the main loop too, preventing
>>> the drain to return before the iothread is effectively stopped.
>>> This is also shown in the tests in patch 5. If the fix in patch
>>> 4 is removed, then the main loop drain will return earlier and
>>> allow the iothread to run and drain together.
>>>
>>> The other patches in this serie are cherry-picked from the various
>>> series I already sent, and are included here just to allow
>>> subtree_drained_begin/end_unlocked implementation.
>>>
>>> Emanuele Giuseppe Esposito (5):
>>>     aio-wait.h: introduce AIO_WAIT_WHILE_UNLOCKED
>>>     introduce BDRV_POLL_WHILE_UNLOCKED
>>>     block/io.c: introduce bdrv_subtree_drained_{begin/end}_unlocked
>>>     child_job_drained_poll: override polling condition only when in home
>>>       thread
>>>     test-bdrv-drain: ensure draining from main loop stops iothreads
>>>
>>>    block/io.c                   |  48 ++++++--
>>>    blockjob.c                   |   3 +-
>>>    include/block/aio-wait.h     |  15 ++-
>>>    include/block/block.h        |   7 ++
>>>    tests/unit/test-bdrv-drain.c | 218 +++++++++++++++++++++++++++++++++++
>>>    5 files changed, 274 insertions(+), 17 deletions(-)
>>>
>>
>>
> 


-- 
Best regards,
Vladimir

Re: [RFC PATCH 0/5] Removal of AioContext lock, bs->parents and ->children: proof of concept

[Vladimir Sementsov-Ogievskiy]

09.03.2022 16:26, Emanuele Giuseppe Esposito wrote:
> 
> Am 02/03/2022 um 12:07 schrieb Vladimir Sementsov-Ogievskiy:
>> 01.03.2022 17:21, Emanuele Giuseppe Esposito wrote:
>>> This serie tries to provide a proof of concept and a clear explanation
>>> on why we need to use drains (and more precisely subtree_drains)
>>> to replace the aiocontext lock, especially to protect BlockDriverState
>>> ->children and ->parent lists.
>>>
>>> Just a small recap on the key concepts:
>>> * We split block layer APIs in "global state" (GS), "I/O", and
>>> "global state or I/O".
>>>     GS are running in the main loop, under BQL, and are the only
>>>     one allowed to modify the BlockDriverState graph.
>>>
>>>     I/O APIs are thread safe and can run in any thread
>>>
>>>     "global state or I/O" are essentially all APIs that use
>>>     BDRV_POLL_WHILE. This is because there can be only 2 threads
>>>     that can use BDRV_POLL_WHILE: main loop and the iothread that
>>>     runs the aiocontext.
>>>
>>> * Drains allow the caller (either main loop or iothread running
>>> the context) to wait all in_flights requests and operations
>>> of a BDS: normal drains target a given node and is parents, while
>>> subtree ones also include the subgraph of the node. Siblings are
>>> not affected by any of these two kind of drains.
>>> After bdrv_drained_begin, no more request is allowed to come
>>> from the affected nodes. Therefore the only actor left working
>>> on a drained part of the graph should be the main loop.
>>>
>>> What do we intend to do
>>> -----------------------
>>> We want to remove the AioContext lock. It is not 100% clear on how
>>> many things we are protecting with it, and why.
>>> As a starter, we want to protect BlockDriverState ->parents and
>>> ->children lists, since they are read by main loop and I/O but
>>> only written by main loop under BQL. The function that modifies
>>> these lists is bdrv_replace_child_common().
>>>
>>> How do we want to do it
>>> -----------------------
>>> We individuated as ideal subtitute of AioContext lock
>>> the subtree_drain API. The reason is simple: draining prevents the
>>> iothread to read or write the nodes, so once the main loop finishes
>> I'm not sure it's ideal. Unfortunately I'm not really good in all that
>> BQL, AioContext locks and drains. So, I can't give good advice. But here
>> are my doubts:
>>
>> Draining is very restrictive measure: even if drained section is very
>> short, at least on bdrv_drained_begin() we have to wait for all current
>> requests, and don't start new ones. That slows down the guest.
> I don't think we are in a critical path where performance is important here.
> 
> In the
>> same time there are operations that don't require to stop guest IO
>> requests. For example manipulation with dirty bitmaps - qmp commands
>> block-dirty-bitmap-add block-dirty-bitmap-remove. Or different query
>> requests..
>>
> Maybe you misunderstood or I was not 100% clear, but I am talking about replacing the AioContext lock for the ->parents and ->children instance. Not everywhere. This is the first step, and then we will see if the additional things that it protects can use drain or something else
> 
>   
>> I see only two real cases, where we do need drain:
>>
>> 1. When we need a consistent "point-in-time". For example, when we start
>> backup in transaction with some dirty-bitmap manipulation commands.
>>
>> 2. When we need to modify block-graph: if we are going to break relation
>> A -> B, there must not be any in-flight request that want to use this
>> relation.
> That's the use case I am considering.
>> All other operations, for which we want some kind of lock (like
>> AioContext lock or something) we actually don't want to stop guest IO.
> Yes, they have to be analyzed case by case.
>>
>> Next, I have a problem in mind, that in past lead to a lot of iotest 30
>> failures. Next there were different fixes and improvements, but the core
>> problem (as far as I understand) is still here: nothing protects us when
>> we are in some graph modification process (for example block-job
>> finalization) do yield, switch to other coroutine and enter another
>> graph modification process (for example, another block-job finaliztion)..
> That's another point to consider. I don't really have a solution for this.
> 
>> (for details look at my old "[PATCH RFC 0/5] Fix accidental crash in
>> iotest 30"
>> https://lists.nongnu.org/archive/html/qemu-devel/2020-11/msg05290.html  ,
>> where I suggested to add a global graph_modify_mutex CoMutex, to be held
>> during graph-modifying process that may yield)..
>> Does your proposal solve this problem?
>>
>>
>>> executing bdrv_drained_begin() on the interested graph, we are sure that
>>> the iothread is not going to look or even interfere with that part of
>>> the graph.
>>> We are also sure that the only two actors that can look at a specific
>>> BlockDriverState in any given context are the main loop and the
>>> iothread running the AioContext (ensured by "global state or IO" logic).
>>>
>>> Why use_subtree_  instead of normal drain
>>> -----------------------------------------
>>> A simple drain "blocks" a given node and all its parents.
>>> But it doesn't touch the child.
>>> This means that if we use a simple drain, a child can always
>>> keep processing requests, and eventually end up calling itself
>>> bdrv_drained_begin, ending up reading the parents node while the main
>>> loop
>>> is modifying them. Therefore a subtree drain is necessary.
>> I'm not sure I understand.. Could you give a more concrete example of
>> what may happen if we use simple drain?
>> Who will call bdrv_drained_begin() you say about? Some IO path in
>> parallel thread? Do we really have an IO path that calls
>> bdrv_drained_begin()?
> It is already being done in mirror, where it drains while running the .run() Job callback.
> 
> I made an unit test for this:
> https://gitlab.com/eesposit/qemu/-/commit/1ffd7193ed441020f51aeeb49c3b07edb6949760
> 
> assume that you have the following graph:
> 
> blk (blk) -> overlay (bs)
> overlay (bs) --- backed by ---> backing (bs)
> job1 (blockjob) --> backing
> 
> Then the main loop calls bdrv_drained_begin(overlay)
> This means overlay and bs are under drain, but backing and most importantly the job are not.
> At this point, the job run() function decides to drain. It should not be
> allowed to read overlay parents list for example, but instead there is nothing
> to prevent it from doing that, and thus we see drains_done >0.
> 
> On the other side, when we subtree_drain, also the job is drained and thus it goes in
> pause.
> 
> Makes sense?


As I understand:

You want to make drained section to be a kind of lock, so that if we take this lock, we can modify the graph and we are sure that no other client will modify it in parallel.

But drained sections can be nested. So to solve the problem you try to drain more nodes: include subtree for example, or may be we need to drain the whole graph connectivity component, or (to be more safe) the whole block layer (to be sure that during drained section in one connectivity component some not-drained block-job from another connectivity component will not try to attach some node from our drained connectivity component)..

I still feel that draining is wrong tool for isolating graph modifying operations from each other:

1. Drained sections can be nested, and natively that's not a kind of lock. That's just a requirement to have no IO requests. There may be several clients that want this condition on same set of nodes.

2. Blocking IO on the whole connected subgraph or even on the whole block layer graph is not necessary, so that's an extra blocking.


Could we instead do the following:

1. Keep draining as is - a mechanism to stop IO on some nodes

2. To isolate graph-modifying operations implement another mechanism: something like a global queue, where clients wait until they gen an access to modify block layer.


This way, any graph modifying process would look like this:

1. drained_begin(only where necessary, not the whole subgraph in general)

2. wait in the global queue

3. Ok, now we can do all the modifications

4. Kick the global queue, so that next client will get an access

5. drained_end()


-- 
Best regards,
Vladimir

Re: [RFC PATCH 0/5] Removal of AioContext lock, bs->parents and ->children: proof of concept

[Emanuele Giuseppe Esposito]

>
> Ah seems I understand what you mean.
>
> One of my arguments is that "drain" - is not a lock against other
> clients who want to modify the graph. Because, drained section allows
> nested drained sections.
>
> And you try to solve it, by draining more things, this way, we'll drain
> also the job, which is a possible client, who may want to modify the
> graph in parallel.
>
> So, in other words, when we want to modify the graph, we drain the whole
> connectivity component of the graph. And we think that we are safe from
> other graph modifications because all related jobs are drained.
> Interesting, is that possible that some not drained job from another
> connectivity component will want to connect some node from our drained
> component?

You mean another job or whathever calling bdrv_find_node() on a random
graph? Yes that is not protected. But can this happen?

That's the question. What are the invariants here? Can anything happen?

>
> I just still feel that draining is a wrong mechanism to avoid
> interaction with other clients who want to modify the graph, because:
>
> 1. we stop the whole IO on all subgraph which is not necessary
> 2. draining is not a mutex, it allows nesting and it's ok when two
> different clients drain same nodes. Draining is just a requirement to do
> no IO at these nodes.
>
> And in your way, it seems that to be absolutely safe we'll need to drain
> everything..
>
> In my feeling it's better to keep draining what it is now: requirement
> to have no IO requests. And to isolate graph modifications from each
> other make a new synchronization mechanism, something like a global
> queue, where clients who want to get an access to graph modifications
> wait for their turn.

This is a matter of definitions. Subtree drains can theoretically work,
I managed to answer to my own doubts in the last email I sent.

Yes, there is still some completely random case like the one I wrote
above, but I think it is more a matter of what we want to use and what
meaning we want to give to drains.

Global queue is what Kevin proposes, I will try to implement it.

> 
> 
> As I understand:
> 
> You want to make drained section to be a kind of lock, so that if we
> take this lock, we can modify the graph and we are sure that no other
> client will modify it in parallel.

Yes

> 
> But drained sections can be nested. So to solve the problem you try to
> drain more nodes: include subtree for example, or may be we need to
> drain the whole graph connectivity component, or (to be more safe) the
> whole block layer (to be sure that during drained section in one
> connectivity component some not-drained block-job from another
> connectivity component will not try to attach some node from our drained
> connectivity component)..
> 
> I still feel that draining is wrong tool for isolating graph modifying
> operations from each other:
> 
> 1. Drained sections can be nested, and natively that's not a kind of
> lock. That's just a requirement to have no IO requests. There may be
> several clients that want this condition on same set of nodes.
> 
> 2. Blocking IO on the whole connected subgraph or even on the whole
> block layer graph is not necessary, so that's an extra blocking.
> 
> 
> Could we instead do the following:
> 
> 1. Keep draining as is - a mechanism to stop IO on some nodes
> 
> 2. To isolate graph-modifying operations implement another mechanism:
> something like a global queue, where clients wait until they gen an
> access to modify block layer.
> 
> 
> This way, any graph modifying process would look like this:
> 
> 1. drained_begin(only where necessary, not the whole subgraph in general)
> 
> 2. wait in the global queue
> 
> 3. Ok, now we can do all the modifications
> 
> 4. Kick the global queue, so that next client will get an access
> 
> 5. drained_end()
> 
> 

Please give a look at what Kevin (described by me) proposed. I think
it's the same as you are suggesting. I am pasting it below.
I will try to implement this and see if it is doable or not.

I think the advantage of drains is that it isn't so complicated and
doesn't add any complication to the existing code.
But we'll see how it goes with this global queue.

> His idea is to replicate what blk_wait_while_drained() currently does
> but on a larger scale. It is something in between this subtree_drains
> logic and a rwlock.
> 
> Basically if I understood correctly, we could implement
> bdrv_wait_while_drained(), and put in all places where we would put a
> read lock: all the reads to ->parents and ->children.
> This function detects if the bdrv is under drain, and if so it will stop
> and wait that the drain finishes (ie the graph modification).
> On the other side, each write would just need to drain probably both
> nodes (simple drain), to signal that we are modifying the graph. Once
> bdrv_drained_begin() finishes, we are sure all coroutines are stopped.
> Once bdrv_drained_end() finishes, we automatically let all coroutine
> restart, and continue where they left off.
> 
> Seems a good compromise between drains and rwlock. What do you think?
> 
> I am not sure how painful it will be to implement though.

Emanuele

Re: [RFC PATCH 0/5] Removal of AioContext lock, bs->parents and ->children: proof of concept

[Vladimir Sementsov-Ogievskiy]

30.03.2022 12:09, Emanuele Giuseppe Esposito wrote:
>>
>> Ah seems I understand what you mean.
>>
>> One of my arguments is that "drain" - is not a lock against other
>> clients who want to modify the graph. Because, drained section allows
>> nested drained sections.
>>
>> And you try to solve it, by draining more things, this way, we'll drain
>> also the job, which is a possible client, who may want to modify the
>> graph in parallel.
>>
>> So, in other words, when we want to modify the graph, we drain the whole
>> connectivity component of the graph. And we think that we are safe from
>> other graph modifications because all related jobs are drained.
>> Interesting, is that possible that some not drained job from another
>> connectivity component will want to connect some node from our drained
>> component?
> 
> You mean another job or whathever calling bdrv_find_node() on a random
> graph? Yes that is not protected. But can this happen?
> 
> That's the question. What are the invariants here? Can anything happen?
> 
>>
>> I just still feel that draining is a wrong mechanism to avoid
>> interaction with other clients who want to modify the graph, because:
>>
>> 1. we stop the whole IO on all subgraph which is not necessary
>> 2. draining is not a mutex, it allows nesting and it's ok when two
>> different clients drain same nodes. Draining is just a requirement to do
>> no IO at these nodes.
>>
>> And in your way, it seems that to be absolutely safe we'll need to drain
>> everything..
>>
>> In my feeling it's better to keep draining what it is now: requirement
>> to have no IO requests. And to isolate graph modifications from each
>> other make a new synchronization mechanism, something like a global
>> queue, where clients who want to get an access to graph modifications
>> wait for their turn.
> 
> This is a matter of definitions. Subtree drains can theoretically work,
> I managed to answer to my own doubts in the last email I sent.
> 
> Yes, there is still some completely random case like the one I wrote
> above, but I think it is more a matter of what we want to use and what
> meaning we want to give to drains.
> 
> Global queue is what Kevin proposes, I will try to implement it.
> 
>>
>>
>> As I understand:
>>
>> You want to make drained section to be a kind of lock, so that if we
>> take this lock, we can modify the graph and we are sure that no other
>> client will modify it in parallel.
> 
> Yes
> 
>>
>> But drained sections can be nested. So to solve the problem you try to
>> drain more nodes: include subtree for example, or may be we need to
>> drain the whole graph connectivity component, or (to be more safe) the
>> whole block layer (to be sure that during drained section in one
>> connectivity component some not-drained block-job from another
>> connectivity component will not try to attach some node from our drained
>> connectivity component)..
>>
>> I still feel that draining is wrong tool for isolating graph modifying
>> operations from each other:
>>
>> 1. Drained sections can be nested, and natively that's not a kind of
>> lock. That's just a requirement to have no IO requests. There may be
>> several clients that want this condition on same set of nodes.
>>
>> 2. Blocking IO on the whole connected subgraph or even on the whole
>> block layer graph is not necessary, so that's an extra blocking.
>>
>>
>> Could we instead do the following:
>>
>> 1. Keep draining as is - a mechanism to stop IO on some nodes
>>
>> 2. To isolate graph-modifying operations implement another mechanism:
>> something like a global queue, where clients wait until they gen an
>> access to modify block layer.
>>
>>
>> This way, any graph modifying process would look like this:
>>
>> 1. drained_begin(only where necessary, not the whole subgraph in general)
>>
>> 2. wait in the global queue
>>
>> 3. Ok, now we can do all the modifications
>>
>> 4. Kick the global queue, so that next client will get an access
>>
>> 5. drained_end()
>>
>>
> 
> Please give a look at what Kevin (described by me) proposed. I think
> it's the same as you are suggesting. I am pasting it below.
> I will try to implement this and see if it is doable or not.
> 
> I think the advantage of drains is that it isn't so complicated and
> doesn't add any complication to the existing code.
> But we'll see how it goes with this global queue.
> 
>> His idea is to replicate what blk_wait_while_drained() currently does
>> but on a larger scale. It is something in between this subtree_drains
>> logic and a rwlock.
>>
>> Basically if I understood correctly, we could implement
>> bdrv_wait_while_drained(), and put in all places where we would put a
>> read lock: all the reads to ->parents and ->children.
>> This function detects if the bdrv is under drain, and if so it will stop
>> and wait that the drain finishes (ie the graph modification).
>> On the other side, each write would just need to drain probably both
>> nodes (simple drain), to signal that we are modifying the graph. Once
>> bdrv_drained_begin() finishes, we are sure all coroutines are stopped.
>> Once bdrv_drained_end() finishes, we automatically let all coroutine
>> restart, and continue where they left off.
>>
>> Seems a good compromise between drains and rwlock. What do you think?
>>
>> I am not sure how painful it will be to implement though.
> 

Hm, I don't see, where is global queue here? Or bdrv_wait_while_drained() is global and has no bs arguement?




-- 
Best regards,
Vladimir

Re: [RFC PATCH 0/5] Removal of AioContext lock, bs->parents and ->children: proof of concept

[Emanuele Giuseppe Esposito]

Am 30/03/2022 um 11:52 schrieb Vladimir Sementsov-Ogievskiy:
> 30.03.2022 12:09, Emanuele Giuseppe Esposito wrote:
>>>
>>> Ah seems I understand what you mean.
>>>
>>> One of my arguments is that "drain" - is not a lock against other
>>> clients who want to modify the graph. Because, drained section allows
>>> nested drained sections.
>>>
>>> And you try to solve it, by draining more things, this way, we'll drain
>>> also the job, which is a possible client, who may want to modify the
>>> graph in parallel.
>>>
>>> So, in other words, when we want to modify the graph, we drain the whole
>>> connectivity component of the graph. And we think that we are safe from
>>> other graph modifications because all related jobs are drained.
>>> Interesting, is that possible that some not drained job from another
>>> connectivity component will want to connect some node from our drained
>>> component?
>>
>> You mean another job or whathever calling bdrv_find_node() on a random
>> graph? Yes that is not protected. But can this happen?
>>
>> That's the question. What are the invariants here? Can anything happen?
>>
>>>
>>> I just still feel that draining is a wrong mechanism to avoid
>>> interaction with other clients who want to modify the graph, because:
>>>
>>> 1. we stop the whole IO on all subgraph which is not necessary
>>> 2. draining is not a mutex, it allows nesting and it's ok when two
>>> different clients drain same nodes. Draining is just a requirement to do
>>> no IO at these nodes.
>>>
>>> And in your way, it seems that to be absolutely safe we'll need to drain
>>> everything..
>>>
>>> In my feeling it's better to keep draining what it is now: requirement
>>> to have no IO requests. And to isolate graph modifications from each
>>> other make a new synchronization mechanism, something like a global
>>> queue, where clients who want to get an access to graph modifications
>>> wait for their turn.
>>
>> This is a matter of definitions. Subtree drains can theoretically work,
>> I managed to answer to my own doubts in the last email I sent.
>>
>> Yes, there is still some completely random case like the one I wrote
>> above, but I think it is more a matter of what we want to use and what
>> meaning we want to give to drains.
>>
>> Global queue is what Kevin proposes, I will try to implement it.
>>
>>>
>>>
>>> As I understand:
>>>
>>> You want to make drained section to be a kind of lock, so that if we
>>> take this lock, we can modify the graph and we are sure that no other
>>> client will modify it in parallel.
>>
>> Yes
>>
>>>
>>> But drained sections can be nested. So to solve the problem you try to
>>> drain more nodes: include subtree for example, or may be we need to
>>> drain the whole graph connectivity component, or (to be more safe) the
>>> whole block layer (to be sure that during drained section in one
>>> connectivity component some not-drained block-job from another
>>> connectivity component will not try to attach some node from our drained
>>> connectivity component)..
>>>
>>> I still feel that draining is wrong tool for isolating graph modifying
>>> operations from each other:
>>>
>>> 1. Drained sections can be nested, and natively that's not a kind of
>>> lock. That's just a requirement to have no IO requests. There may be
>>> several clients that want this condition on same set of nodes.
>>>
>>> 2. Blocking IO on the whole connected subgraph or even on the whole
>>> block layer graph is not necessary, so that's an extra blocking.
>>>
>>>
>>> Could we instead do the following:
>>>
>>> 1. Keep draining as is - a mechanism to stop IO on some nodes
>>>
>>> 2. To isolate graph-modifying operations implement another mechanism:
>>> something like a global queue, where clients wait until they gen an
>>> access to modify block layer.
>>>
>>>
>>> This way, any graph modifying process would look like this:
>>>
>>> 1. drained_begin(only where necessary, not the whole subgraph in
>>> general)
>>>
>>> 2. wait in the global queue
>>>
>>> 3. Ok, now we can do all the modifications
>>>
>>> 4. Kick the global queue, so that next client will get an access
>>>
>>> 5. drained_end()
>>>
>>>
>>
>> Please give a look at what Kevin (described by me) proposed. I think
>> it's the same as you are suggesting. I am pasting it below.
>> I will try to implement this and see if it is doable or not.
>>
>> I think the advantage of drains is that it isn't so complicated and
>> doesn't add any complication to the existing code.
>> But we'll see how it goes with this global queue.
>>
>>> His idea is to replicate what blk_wait_while_drained() currently does
>>> but on a larger scale. It is something in between this subtree_drains
>>> logic and a rwlock.
>>>
>>> Basically if I understood correctly, we could implement
>>> bdrv_wait_while_drained(), and put in all places where we would put a
>>> read lock: all the reads to ->parents and ->children.
>>> This function detects if the bdrv is under drain, and if so it will stop
>>> and wait that the drain finishes (ie the graph modification).
>>> On the other side, each write would just need to drain probably both
>>> nodes (simple drain), to signal that we are modifying the graph. Once
>>> bdrv_drained_begin() finishes, we are sure all coroutines are stopped.
>>> Once bdrv_drained_end() finishes, we automatically let all coroutine
>>> restart, and continue where they left off.
>>>
>>> Seems a good compromise between drains and rwlock. What do you think?
>>>
>>> I am not sure how painful it will be to implement though.
>>
> 
> Hm, I don't see, where is global queue here? Or
> bdrv_wait_while_drained() is global and has no bs arguement?
> 
> 

From what I understand, blk_wait_while_drained has a queue internally.
Yes, the queue would be global, and all coroutines that want to perform
a read will have to wait until the modification is ended.

Whether to wake the queue up with a drain or a write lock is also
another point worth discussion maybe.

Re: [RFC PATCH 0/5] Removal of AioContext lock, bs->parents and ->children: proof of concept

[Kevin Wolf]

Am 30.03.2022 um 11:58 hat Emanuele Giuseppe Esposito geschrieben:
> 
> 
> Am 30/03/2022 um 11:52 schrieb Vladimir Sementsov-Ogievskiy:
> > 30.03.2022 12:09, Emanuele Giuseppe Esposito wrote:
> >>>
> >>> Ah seems I understand what you mean.
> >>>
> >>> One of my arguments is that "drain" - is not a lock against other
> >>> clients who want to modify the graph. Because, drained section allows
> >>> nested drained sections.
> >>>
> >>> And you try to solve it, by draining more things, this way, we'll drain
> >>> also the job, which is a possible client, who may want to modify the
> >>> graph in parallel.
> >>>
> >>> So, in other words, when we want to modify the graph, we drain the whole
> >>> connectivity component of the graph. And we think that we are safe from
> >>> other graph modifications because all related jobs are drained.
> >>> Interesting, is that possible that some not drained job from another
> >>> connectivity component will want to connect some node from our drained
> >>> component?
> >>
> >> You mean another job or whathever calling bdrv_find_node() on a random
> >> graph? Yes that is not protected. But can this happen?
> >>
> >> That's the question. What are the invariants here? Can anything happen?
> >>
> >>>
> >>> I just still feel that draining is a wrong mechanism to avoid
> >>> interaction with other clients who want to modify the graph, because:
> >>>
> >>> 1. we stop the whole IO on all subgraph which is not necessary
> >>> 2. draining is not a mutex, it allows nesting and it's ok when two
> >>> different clients drain same nodes. Draining is just a requirement to do
> >>> no IO at these nodes.
> >>>
> >>> And in your way, it seems that to be absolutely safe we'll need to drain
> >>> everything..
> >>>
> >>> In my feeling it's better to keep draining what it is now: requirement
> >>> to have no IO requests. And to isolate graph modifications from each
> >>> other make a new synchronization mechanism, something like a global
> >>> queue, where clients who want to get an access to graph modifications
> >>> wait for their turn.
> >>
> >> This is a matter of definitions. Subtree drains can theoretically work,
> >> I managed to answer to my own doubts in the last email I sent.
> >>
> >> Yes, there is still some completely random case like the one I wrote
> >> above, but I think it is more a matter of what we want to use and what
> >> meaning we want to give to drains.
> >>
> >> Global queue is what Kevin proposes, I will try to implement it.
> >>
> >>>
> >>>
> >>> As I understand:
> >>>
> >>> You want to make drained section to be a kind of lock, so that if we
> >>> take this lock, we can modify the graph and we are sure that no other
> >>> client will modify it in parallel.
> >>
> >> Yes
> >>
> >>>
> >>> But drained sections can be nested. So to solve the problem you try to
> >>> drain more nodes: include subtree for example, or may be we need to
> >>> drain the whole graph connectivity component, or (to be more safe) the
> >>> whole block layer (to be sure that during drained section in one
> >>> connectivity component some not-drained block-job from another
> >>> connectivity component will not try to attach some node from our drained
> >>> connectivity component)..
> >>>
> >>> I still feel that draining is wrong tool for isolating graph modifying
> >>> operations from each other:
> >>>
> >>> 1. Drained sections can be nested, and natively that's not a kind of
> >>> lock. That's just a requirement to have no IO requests. There may be
> >>> several clients that want this condition on same set of nodes.
> >>>
> >>> 2. Blocking IO on the whole connected subgraph or even on the whole
> >>> block layer graph is not necessary, so that's an extra blocking.
> >>>
> >>>
> >>> Could we instead do the following:
> >>>
> >>> 1. Keep draining as is - a mechanism to stop IO on some nodes
> >>>
> >>> 2. To isolate graph-modifying operations implement another mechanism:
> >>> something like a global queue, where clients wait until they gen an
> >>> access to modify block layer.
> >>>
> >>>
> >>> This way, any graph modifying process would look like this:
> >>>
> >>> 1. drained_begin(only where necessary, not the whole subgraph in
> >>> general)
> >>>
> >>> 2. wait in the global queue
> >>>
> >>> 3. Ok, now we can do all the modifications
> >>>
> >>> 4. Kick the global queue, so that next client will get an access
> >>>
> >>> 5. drained_end()
> >>>
> >>>
> >>
> >> Please give a look at what Kevin (described by me) proposed. I think
> >> it's the same as you are suggesting. I am pasting it below.
> >> I will try to implement this and see if it is doable or not.
> >>
> >> I think the advantage of drains is that it isn't so complicated and
> >> doesn't add any complication to the existing code.
> >> But we'll see how it goes with this global queue.
> >>
> >>> His idea is to replicate what blk_wait_while_drained() currently does
> >>> but on a larger scale. It is something in between this subtree_drains
> >>> logic and a rwlock.
> >>>
> >>> Basically if I understood correctly, we could implement
> >>> bdrv_wait_while_drained(), and put in all places where we would put a
> >>> read lock: all the reads to ->parents and ->children.
> >>> This function detects if the bdrv is under drain, and if so it will stop
> >>> and wait that the drain finishes (ie the graph modification).
> >>> On the other side, each write would just need to drain probably both
> >>> nodes (simple drain), to signal that we are modifying the graph. Once
> >>> bdrv_drained_begin() finishes, we are sure all coroutines are stopped.
> >>> Once bdrv_drained_end() finishes, we automatically let all coroutine
> >>> restart, and continue where they left off.
> >>>
> >>> Seems a good compromise between drains and rwlock. What do you think?
> >>>
> >>> I am not sure how painful it will be to implement though.
> >>
> > 
> > Hm, I don't see, where is global queue here? Or
> > bdrv_wait_while_drained() is global and has no bs arguement?
> > 
> > 
> 
> From what I understand, blk_wait_while_drained has a queue internally.
> Yes, the queue would be global, and all coroutines that want to perform
> a read will have to wait until the modification is ended.
> 
> Whether to wake the queue up with a drain or a write lock is also
> another point worth discussion maybe.

I think I intended it to be per-BDS (just like blk_wait_while_drained()
uses a per-BB queue). Obviously, you can always replace a finer grained
lock with a coarse one without losing correctness. I'm not sure if that
simplifies things enough to justify using a single global lock (which
would be a step backwards even from AioContext locks). On the other
hand, graph modifications are rare, so maybe it's not too bad.

Kevin