[PATCH v2 0/6] block: switch to AIO_WAIT_WHILE_UNLOCKED() where possible

[PATCH v2 0/6] block: switch to AIO_WAIT_WHILE_UNLOCKED() where possible

[Stefan Hajnoczi]

v2:
- Clarify NULL ctx argument in Patch 1 commit description [Kevin]

AIO_WAIT_WHILE_UNLOCKED() is the future replacement for AIO_WAIT_WHILE(). Most
callers haven't been converted yet because they rely on the AioContext lock. I
looked through the code and found the easy cases that can be converted today.

Stefan Hajnoczi (6):
  block: don't acquire AioContext lock in bdrv_drain_all()
  block: convert blk_exp_close_all_type() to AIO_WAIT_WHILE_UNLOCKED()
  block: convert bdrv_graph_wrlock() to AIO_WAIT_WHILE_UNLOCKED()
  block: convert bdrv_drain_all_begin() to AIO_WAIT_WHILE_UNLOCKED()
  hmp: convert handle_hmp_command() to AIO_WAIT_WHILE_UNLOCKED()
  monitor: convert monitor_cleanup() to AIO_WAIT_WHILE_UNLOCKED()

 block/block-backend.c | 8 +-------
 block/export/export.c | 2 +-
 block/graph-lock.c    | 2 +-
 block/io.c            | 2 +-
 monitor/hmp.c         | 2 +-
 monitor/monitor.c     | 4 ++--
 6 files changed, 7 insertions(+), 13 deletions(-)

-- 
2.39.2

[PATCH v2 1/6] block: don't acquire AioContext lock in bdrv_drain_all()

[Stefan Hajnoczi]

There is no need for the AioContext lock in bdrv_drain_all() because
nothing in AIO_WAIT_WHILE() needs the lock and the condition is atomic.

AIO_WAIT_WHILE_UNLOCKED() has no use for the AioContext parameter other
than performing a check that is nowadays already done by the
GLOBAL_STATE_CODE()/IO_CODE() macros. Set the ctx argument to NULL here
to help us keep track of all converted callers. Eventually all callers
will have been converted and then the argument can be dropped entirely.

Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/block-backend.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index 278b04ce69..d2b6b3652d 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1835,14 +1835,8 @@ void blk_drain_all(void)
     bdrv_drain_all_begin();
 
     while ((blk = blk_all_next(blk)) != NULL) {
-        AioContext *ctx = blk_get_aio_context(blk);
-
-        aio_context_acquire(ctx);
-
         /* We may have -ENOMEDIUM completions in flight */
-        AIO_WAIT_WHILE(ctx, qatomic_mb_read(&blk->in_flight) > 0);
-
-        aio_context_release(ctx);
+        AIO_WAIT_WHILE_UNLOCKED(NULL, qatomic_mb_read(&blk->in_flight) > 0);
     }
 
     bdrv_drain_all_end();
-- 
2.39.2

Re: [PATCH v2 1/6] block: don't acquire AioContext lock in bdrv_drain_all()

[Wilfred Mallawa]

On Thu, 2023-03-09 at 14:08 -0500, Stefan Hajnoczi wrote:
> There is no need for the AioContext lock in bdrv_drain_all() because
> nothing in AIO_WAIT_WHILE() needs the lock and the condition is
> atomic.
> 
> AIO_WAIT_WHILE_UNLOCKED() has no use for the AioContext parameter
> other
> than performing a check that is nowadays already done by the
> GLOBAL_STATE_CODE()/IO_CODE() macros. Set the ctx argument to NULL
> here
> to help us keep track of all converted callers. Eventually all
> callers
> will have been converted and then the argument can be dropped
> entirely.
> 
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/block-backend.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
Reviewed-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>
> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 278b04ce69..d2b6b3652d 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -1835,14 +1835,8 @@ void blk_drain_all(void)
>      bdrv_drain_all_begin();
>  
>      while ((blk = blk_all_next(blk)) != NULL) {
> -        AioContext *ctx = blk_get_aio_context(blk);
> -
> -        aio_context_acquire(ctx);
> -
>          /* We may have -ENOMEDIUM completions in flight */
> -        AIO_WAIT_WHILE(ctx, qatomic_mb_read(&blk->in_flight) > 0);
> -
> -        aio_context_release(ctx);
> +        AIO_WAIT_WHILE_UNLOCKED(NULL, qatomic_mb_read(&blk-
> >in_flight) > 0);
>      }
>  
>      bdrv_drain_all_end();

[PATCH v2 2/6] block: convert blk_exp_close_all_type() to AIO_WAIT_WHILE_UNLOCKED()

[Stefan Hajnoczi]

There is no change in behavior. Switch to AIO_WAIT_WHILE_UNLOCKED()
instead of AIO_WAIT_WHILE() to document that this code has already been
audited and converted. The AioContext argument is already NULL so
aio_context_release() is never called anyway.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/export/export.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/export/export.c b/block/export/export.c
index 28a91c9c42..e3fee60611 100644
--- a/block/export/export.c
+++ b/block/export/export.c
@@ -306,7 +306,7 @@ void blk_exp_close_all_type(BlockExportType type)
         blk_exp_request_shutdown(exp);
     }
 
-    AIO_WAIT_WHILE(NULL, blk_exp_has_type(type));
+    AIO_WAIT_WHILE_UNLOCKED(NULL, blk_exp_has_type(type));
 }
 
 void blk_exp_close_all(void)
-- 
2.39.2

Re: [PATCH v2 2/6] block: convert blk_exp_close_all_type() to AIO_WAIT_WHILE_UNLOCKED()

[Wilfred Mallawa]

On Thu, 2023-03-09 at 14:08 -0500, Stefan Hajnoczi wrote:
> There is no change in behavior. Switch to AIO_WAIT_WHILE_UNLOCKED()
> instead of AIO_WAIT_WHILE() to document that this code has already
> been
> audited and converted. The AioContext argument is already NULL so
> aio_context_release() is never called anyway.
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/export/export.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>
> 
> diff --git a/block/export/export.c b/block/export/export.c
> index 28a91c9c42..e3fee60611 100644
> --- a/block/export/export.c
> +++ b/block/export/export.c
> @@ -306,7 +306,7 @@ void blk_exp_close_all_type(BlockExportType type)
>          blk_exp_request_shutdown(exp);
>      }
>  
> -    AIO_WAIT_WHILE(NULL, blk_exp_has_type(type));
> +    AIO_WAIT_WHILE_UNLOCKED(NULL, blk_exp_has_type(type));
>  }
>  
>  void blk_exp_close_all(void)

[PATCH v2 3/6] block: convert bdrv_graph_wrlock() to AIO_WAIT_WHILE_UNLOCKED()

[Stefan Hajnoczi]

The following conversion is safe and does not change behavior:

     GLOBAL_STATE_CODE();
     ...
  -  AIO_WAIT_WHILE(qemu_get_aio_context(), ...);
  +  AIO_WAIT_WHILE_UNLOCKED(NULL, ...);

Since we're in GLOBAL_STATE_CODE(), qemu_get_aio_context() is our home
thread's AioContext. Thus AIO_WAIT_WHILE() does not unlock the
AioContext:

  if (ctx_ && in_aio_context_home_thread(ctx_)) {                \
      while ((cond)) {                                           \
          aio_poll(ctx_, true);                                  \
          waited_ = true;                                        \
      }                                                          \

And that means AIO_WAIT_WHILE_UNLOCKED(NULL, ...) can be substituted.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/graph-lock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/graph-lock.c b/block/graph-lock.c
index 454c31e691..639526608f 100644
--- a/block/graph-lock.c
+++ b/block/graph-lock.c
@@ -127,7 +127,7 @@ void bdrv_graph_wrlock(void)
          * reader lock.
          */
         qatomic_set(&has_writer, 0);
-        AIO_WAIT_WHILE(qemu_get_aio_context(), reader_count() >= 1);
+        AIO_WAIT_WHILE_UNLOCKED(NULL, reader_count() >= 1);
         qatomic_set(&has_writer, 1);
 
         /*
-- 
2.39.2

Re: [PATCH v2 3/6] block: convert bdrv_graph_wrlock() to AIO_WAIT_WHILE_UNLOCKED()

[Wilfred Mallawa]

On Thu, 2023-03-09 at 14:08 -0500, Stefan Hajnoczi wrote:
> The following conversion is safe and does not change behavior:
> 
>      GLOBAL_STATE_CODE();
>      ...
>   -  AIO_WAIT_WHILE(qemu_get_aio_context(), ...);
>   +  AIO_WAIT_WHILE_UNLOCKED(NULL, ...);
> 
> Since we're in GLOBAL_STATE_CODE(), qemu_get_aio_context() is our
> home
> thread's AioContext. Thus AIO_WAIT_WHILE() does not unlock the
> AioContext:
> 
>   if (ctx_ && in_aio_context_home_thread(ctx_)) {                \
>       while ((cond)) {                                           \
>           aio_poll(ctx_, true);                                  \
>           waited_ = true;                                        \
>       }                                                          \
> 
> And that means AIO_WAIT_WHILE_UNLOCKED(NULL, ...) can be substituted.
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/graph-lock.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>
> 
> diff --git a/block/graph-lock.c b/block/graph-lock.c
> index 454c31e691..639526608f 100644
> --- a/block/graph-lock.c
> +++ b/block/graph-lock.c
> @@ -127,7 +127,7 @@ void bdrv_graph_wrlock(void)
>           * reader lock.
>           */
>          qatomic_set(&has_writer, 0);
> -        AIO_WAIT_WHILE(qemu_get_aio_context(), reader_count() >= 1);
> +        AIO_WAIT_WHILE_UNLOCKED(NULL, reader_count() >= 1);
>          qatomic_set(&has_writer, 1);
>  
>          /*

[PATCH v2 4/6] block: convert bdrv_drain_all_begin() to AIO_WAIT_WHILE_UNLOCKED()

[Stefan Hajnoczi]

Since the AioContext argument was already NULL, AIO_WAIT_WHILE() was
never going to unlock the AioContext. Therefore it is possible to
replace AIO_WAIT_WHILE() with AIO_WAIT_WHILE_UNLOCKED().

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/io.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/io.c b/block/io.c
index 8974d46941..db438c7657 100644
--- a/block/io.c
+++ b/block/io.c
@@ -520,7 +520,7 @@ void bdrv_drain_all_begin(void)
     bdrv_drain_all_begin_nopoll();
 
     /* Now poll the in-flight requests */
-    AIO_WAIT_WHILE(NULL, bdrv_drain_all_poll());
+    AIO_WAIT_WHILE_UNLOCKED(NULL, bdrv_drain_all_poll());
 
     while ((bs = bdrv_next_all_states(bs))) {
         bdrv_drain_assert_idle(bs);
-- 
2.39.2

Re: [PATCH v2 4/6] block: convert bdrv_drain_all_begin() to AIO_WAIT_WHILE_UNLOCKED()

[Wilfred Mallawa]

On Thu, 2023-03-09 at 14:08 -0500, Stefan Hajnoczi wrote:
> Since the AioContext argument was already NULL, AIO_WAIT_WHILE() was
> never going to unlock the AioContext. Therefore it is possible to
> replace AIO_WAIT_WHILE() with AIO_WAIT_WHILE_UNLOCKED().
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/io.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>
> 
> diff --git a/block/io.c b/block/io.c
> index 8974d46941..db438c7657 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -520,7 +520,7 @@ void bdrv_drain_all_begin(void)
>      bdrv_drain_all_begin_nopoll();
>  
>      /* Now poll the in-flight requests */
> -    AIO_WAIT_WHILE(NULL, bdrv_drain_all_poll());
> +    AIO_WAIT_WHILE_UNLOCKED(NULL, bdrv_drain_all_poll());
>  
>      while ((bs = bdrv_next_all_states(bs))) {
>          bdrv_drain_assert_idle(bs);

[PATCH v2 5/6] hmp: convert handle_hmp_command() to AIO_WAIT_WHILE_UNLOCKED()

[Stefan Hajnoczi]

The HMP monitor runs in the main loop thread. Calling
AIO_WAIT_WHILE(qemu_get_aio_context(), ...) from the main loop thread is
equivalent to AIO_WAIT_WHILE_UNLOCKED(NULL, ...) because neither unlocks
the AioContext and the latter's assertion that we're in the main loop
succeeds.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 monitor/hmp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/monitor/hmp.c b/monitor/hmp.c
index fee410362f..5cab56d355 100644
--- a/monitor/hmp.c
+++ b/monitor/hmp.c
@@ -1167,7 +1167,7 @@ void handle_hmp_command(MonitorHMP *mon, const char *cmdline)
         Coroutine *co = qemu_coroutine_create(handle_hmp_command_co, &data);
         monitor_set_cur(co, &mon->common);
         aio_co_enter(qemu_get_aio_context(), co);
-        AIO_WAIT_WHILE(qemu_get_aio_context(), !data.done);
+        AIO_WAIT_WHILE_UNLOCKED(NULL, !data.done);
     }
 
     qobject_unref(qdict);
-- 
2.39.2

Re: [PATCH v2 5/6] hmp: convert handle_hmp_command() to AIO_WAIT_WHILE_UNLOCKED()

[Wilfred Mallawa]

On Thu, 2023-03-09 at 14:08 -0500, Stefan Hajnoczi wrote:
> The HMP monitor runs in the main loop thread. Calling
> AIO_WAIT_WHILE(qemu_get_aio_context(), ...) from the main loop thread
> is
> equivalent to AIO_WAIT_WHILE_UNLOCKED(NULL, ...) because neither
> unlocks
> the AioContext and the latter's assertion that we're in the main loop
> succeeds.
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  monitor/hmp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>
> 
> diff --git a/monitor/hmp.c b/monitor/hmp.c
> index fee410362f..5cab56d355 100644
> --- a/monitor/hmp.c
> +++ b/monitor/hmp.c
> @@ -1167,7 +1167,7 @@ void handle_hmp_command(MonitorHMP *mon, const
> char *cmdline)
>          Coroutine *co = qemu_coroutine_create(handle_hmp_command_co,
> &data);
>          monitor_set_cur(co, &mon->common);
>          aio_co_enter(qemu_get_aio_context(), co);
> -        AIO_WAIT_WHILE(qemu_get_aio_context(), !data.done);
> +        AIO_WAIT_WHILE_UNLOCKED(NULL, !data.done);
>      }
>  
>      qobject_unref(qdict);

[PATCH v2 6/6] monitor: convert monitor_cleanup() to AIO_WAIT_WHILE_UNLOCKED()

[Stefan Hajnoczi]

monitor_cleanup() is called from the main loop thread. Calling
AIO_WAIT_WHILE(qemu_get_aio_context(), ...) from the main loop thread is
equivalent to AIO_WAIT_WHILE_UNLOCKED(NULL, ...) because neither unlocks
the AioContext and the latter's assertion that we're in the main loop
succeeds.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 monitor/monitor.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/monitor/monitor.c b/monitor/monitor.c
index 8dc96f6af9..602535696c 100644
--- a/monitor/monitor.c
+++ b/monitor/monitor.c
@@ -666,7 +666,7 @@ void monitor_cleanup(void)
      * We need to poll both qemu_aio_context and iohandler_ctx to make
      * sure that the dispatcher coroutine keeps making progress and
      * eventually terminates.  qemu_aio_context is automatically
-     * polled by calling AIO_WAIT_WHILE on it, but we must poll
+     * polled by calling AIO_WAIT_WHILE_UNLOCKED on it, but we must poll
      * iohandler_ctx manually.
      *
      * Letting the iothread continue while shutting down the dispatcher
@@ -679,7 +679,7 @@ void monitor_cleanup(void)
         aio_co_wake(qmp_dispatcher_co);
     }
 
-    AIO_WAIT_WHILE(qemu_get_aio_context(),
+    AIO_WAIT_WHILE_UNLOCKED(NULL,
                    (aio_poll(iohandler_get_aio_context(), false),
                     qatomic_mb_read(&qmp_dispatcher_co_busy)));
 
-- 
2.39.2

Re: [PATCH v2 6/6] monitor: convert monitor_cleanup() to AIO_WAIT_WHILE_UNLOCKED()

[Wilfred Mallawa]

On Thu, 2023-03-09 at 14:08 -0500, Stefan Hajnoczi wrote:
> monitor_cleanup() is called from the main loop thread. Calling
> AIO_WAIT_WHILE(qemu_get_aio_context(), ...) from the main loop thread
> is
> equivalent to AIO_WAIT_WHILE_UNLOCKED(NULL, ...) because neither
> unlocks
> the AioContext and the latter's assertion that we're in the main loop
> succeeds.
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  monitor/monitor.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>
> 
> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index 8dc96f6af9..602535696c 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -666,7 +666,7 @@ void monitor_cleanup(void)
>       * We need to poll both qemu_aio_context and iohandler_ctx to
> make
>       * sure that the dispatcher coroutine keeps making progress and
>       * eventually terminates.  qemu_aio_context is automatically
> -     * polled by calling AIO_WAIT_WHILE on it, but we must poll
> +     * polled by calling AIO_WAIT_WHILE_UNLOCKED on it, but we must
> poll
>       * iohandler_ctx manually.
>       *
>       * Letting the iothread continue while shutting down the
> dispatcher
> @@ -679,7 +679,7 @@ void monitor_cleanup(void)
>          aio_co_wake(qmp_dispatcher_co);
>      }
>  
> -    AIO_WAIT_WHILE(qemu_get_aio_context(),
> +    AIO_WAIT_WHILE_UNLOCKED(NULL,
>                     (aio_poll(iohandler_get_aio_context(), false),
>                      qatomic_mb_read(&qmp_dispatcher_co_busy)));
>  

Re: [PATCH v2 0/6] block: switch to AIO_WAIT_WHILE_UNLOCKED() where possible

[Kevin Wolf]

Am 09.03.2023 um 20:08 hat Stefan Hajnoczi geschrieben:
> v2:
> - Clarify NULL ctx argument in Patch 1 commit description [Kevin]
> 
> AIO_WAIT_WHILE_UNLOCKED() is the future replacement for AIO_WAIT_WHILE(). Most
> callers haven't been converted yet because they rely on the AioContext lock. I
> looked through the code and found the easy cases that can be converted today.

Thanks, applied to block-next.

Kevin