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

[Stefan Hajnoczi]

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)" 
> > Cc: Paolo Bonzini 
> > Signed-off-by: Stefan Hajnoczi 
> 
> 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


signature.asc
Description: PGP signature

Re: [Qemu-block] [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)" 
> Cc: Paolo Bonzini 
> Signed-off-by: Stefan Hajnoczi 

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

Kevin

Re: [Qemu-block] [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)" 
> Cc: Paolo Bonzini 
> Signed-off-by: Stefan Hajnoczi 
> ---
> 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(_->wakeup, true); \
> +/* Increment bs->wakeup before evaluating cond. */ \
> +atomic_inc(_->wakeup);  \
>  while (busy_) {\
>  if ((cond)) {  \
>  waited_ = busy_ = true;\
> @@ -399,7 +398,7 @@ void bdrv_drain_all(void);
>  waited_ |= busy_;  \
>  }  \
>  }  \
> -atomic_set(_->wakeup, false);   \
> +atomic_dec(_->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 

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

Paolo

[Qemu-block] [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)" 
Cc: Paolo Bonzini 
Signed-off-by: Stefan Hajnoczi 
---
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(_->wakeup, true); \
+/* Increment bs->wakeup before evaluating cond. */ \
+atomic_inc(_->wakeup);  \
 while (busy_) {\
 if ((cond)) {  \
 waited_ = busy_ = true;\
@@ -399,7 +398,7 @@ void bdrv_drain_all(void);
 waited_ |= busy_;  \
 }  \
 }  \
-atomic_set(_->wakeup, false);   \
+atomic_dec(_->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