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

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

Paolo