[Qemu-devel] [PATCH] block: make BDRV_POLL_WHILE() re-entrancy safe

[Qemu-devel] [PATCH] block: make BDRV_POLL_WHILE() re-entrancy safe

[Stefan Hajnoczi]

Nested BDRV_POLL_WHILE() calls can occur.  Currently
assert(!bs_->wakeup) will fail when this happens.

This patch converts bs->wakeup from bool to a counter.

Nesting works correctly because outer BDRV_POLL_WHILE() callers evaluate
the condition again after the inner caller completes (invoking the inner
caller counts as aio_poll() progress).

Reported-by: "fuweiwei (C)" <fuweiwei2@huawei.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
Fu Weiwei: Please retest and let us know if this fixes the assertion
failure you hit.  Thanks!
---
 include/block/block.h     | 7 +++----
 include/block/block_int.h | 2 +-
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index fac401ba3e..990b97f0ad 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -385,9 +385,8 @@ void bdrv_drain_all(void);
          * other I/O threads' AioContexts (see for example \
          * block_job_defer_to_main_loop for how to do it). \
          */                                                \
-        assert(!bs_->wakeup);                              \
-        /* Set bs->wakeup before evaluating cond.  */      \
-        atomic_mb_set(&bs_->wakeup, true);                 \
+        /* Increment bs->wakeup before evaluating cond. */ \
+        atomic_inc(&bs_->wakeup);                          \
         while (busy_) {                                    \
             if ((cond)) {                                  \
                 waited_ = busy_ = true;                    \
@@ -399,7 +398,7 @@ void bdrv_drain_all(void);
                 waited_ |= busy_;                          \
             }                                              \
         }                                                  \
-        atomic_set(&bs_->wakeup, false);                   \
+        atomic_dec(&bs_->wakeup);                          \
     }                                                      \
     waited_; })
 
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 5ea63f8fa8..0f360c0ed5 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -712,7 +712,7 @@ struct BlockDriverState {
     /* Internal to BDRV_POLL_WHILE and bdrv_wakeup.  Accessed with atomic
      * ops.
      */
-    bool wakeup;
+    unsigned wakeup;
 
     /* counter for nested bdrv_io_plug.
      * Accessed with atomic ops.
-- 
2.14.3

Re: [Qemu-devel] [PATCH] block: make BDRV_POLL_WHILE() re-entrancy safe

[Paolo Bonzini]

On 06/03/2018 11:53, Stefan Hajnoczi wrote:
> Nested BDRV_POLL_WHILE() calls can occur.  Currently
> assert(!bs_->wakeup) will fail when this happens.
> 
> This patch converts bs->wakeup from bool to a counter.
> 
> Nesting works correctly because outer BDRV_POLL_WHILE() callers evaluate
> the condition again after the inner caller completes (invoking the inner
> caller counts as aio_poll() progress).
> 
> Reported-by: "fuweiwei (C)" <fuweiwei2@huawei.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> Fu Weiwei: Please retest and let us know if this fixes the assertion
> failure you hit.  Thanks!
> ---
>  include/block/block.h     | 7 +++----
>  include/block/block_int.h | 2 +-
>  2 files changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/include/block/block.h b/include/block/block.h
> index fac401ba3e..990b97f0ad 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -385,9 +385,8 @@ void bdrv_drain_all(void);
>           * other I/O threads' AioContexts (see for example \
>           * block_job_defer_to_main_loop for how to do it). \
>           */                                                \
> -        assert(!bs_->wakeup);                              \
> -        /* Set bs->wakeup before evaluating cond.  */      \
> -        atomic_mb_set(&bs_->wakeup, true);                 \
> +        /* Increment bs->wakeup before evaluating cond. */ \
> +        atomic_inc(&bs_->wakeup);                          \
>          while (busy_) {                                    \
>              if ((cond)) {                                  \
>                  waited_ = busy_ = true;                    \
> @@ -399,7 +398,7 @@ void bdrv_drain_all(void);
>                  waited_ |= busy_;                          \
>              }                                              \
>          }                                                  \
> -        atomic_set(&bs_->wakeup, false);                   \
> +        atomic_dec(&bs_->wakeup);                          \
>      }                                                      \
>      waited_; })
>  
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 5ea63f8fa8..0f360c0ed5 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -712,7 +712,7 @@ struct BlockDriverState {
>      /* Internal to BDRV_POLL_WHILE and bdrv_wakeup.  Accessed with atomic
>       * ops.
>       */
> -    bool wakeup;
> +    unsigned wakeup;
>  
>      /* counter for nested bdrv_io_plug.
>       * Accessed with atomic ops.
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

At least, the assertion made it fail fast. :)

Paolo

Re: [Qemu-devel] [PATCH] block: make BDRV_POLL_WHILE() re-entrancy safe

[Kevin Wolf]

Am 06.03.2018 um 11:53 hat Stefan Hajnoczi geschrieben:
> Nested BDRV_POLL_WHILE() calls can occur.  Currently
> assert(!bs_->wakeup) will fail when this happens.
> 
> This patch converts bs->wakeup from bool to a counter.
> 
> Nesting works correctly because outer BDRV_POLL_WHILE() callers evaluate
> the condition again after the inner caller completes (invoking the inner
> caller counts as aio_poll() progress).
> 
> Reported-by: "fuweiwei (C)" <fuweiwei2@huawei.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Doesn't this conflict with your own AIO_WAIT_WHILE() patch?

Kevin

Re: [Qemu-devel] [PATCH] block: make BDRV_POLL_WHILE() re-entrancy safe

[Stefan Hajnoczi]

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

On Tue, Mar 06, 2018 at 12:08:44PM +0100, Kevin Wolf wrote:
> Am 06.03.2018 um 11:53 hat Stefan Hajnoczi geschrieben:
> > Nested BDRV_POLL_WHILE() calls can occur.  Currently
> > assert(!bs_->wakeup) will fail when this happens.
> > 
> > This patch converts bs->wakeup from bool to a counter.
> > 
> > Nesting works correctly because outer BDRV_POLL_WHILE() callers evaluate
> > the condition again after the inner caller completes (invoking the inner
> > caller counts as aio_poll() progress).
> > 
> > Reported-by: "fuweiwei (C)" <fuweiwei2@huawei.com>
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> 
> Doesn't this conflict with your own AIO_WAIT_WHILE() patch?

Yes, I wanted this patch to be easy for Weiwei to test without
dependencies.

AIO_WAIT_WHILE() has just hit qemu.git/master, so I'll rebase and send a
v2.

Stefan

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