Re: [PATCH v2 2/3] block: make BlockBackend->disable_request_queuing atomic

[Paolo Bonzini]

On 3/7/23 22:04, Stefan Hajnoczi wrote:
> This field is accessed by multiple threads without a lock. Use explicit
> qatomic_read()/qatomic_set() calls. There is no need for acquire/release
> because blk_set_disable_request_queuing() doesn't provide any
> guarantees (it helps that it's used at BlockBackend creation time and
> not when there is I/O in flight).

This in turn means itdoes not need to be atomic - atomics are only 
needed if there are concurrent writes.  No big deal; I am now 
resurrecting the series from the time I had noticed the queued_requests 
thread-safety problem, so this will be simplified in 8.1.  For now your 
version is okay, thanks for fixing it!

Paolo

> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> Reviewed-by: Hanna Czenczek <hreitz@redhat.com>
> ---
>   block/block-backend.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 68807be32b..0cba4add20 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -82,7 +82,7 @@ struct BlockBackend {
>   
>       int quiesce_counter; /* atomic: written under BQL, read by other threads */
>       CoQueue queued_requests;
> -    bool disable_request_queuing;
> +    bool disable_request_queuing; /* atomic */
>   
>       VMChangeStateEntry *vmsh;
>       bool force_allow_inactivate;
> @@ -1232,7 +1232,7 @@ void blk_set_allow_aio_context_change(BlockBackend *blk, 
> bool allow)
>   void blk_set_disable_request_queuing(BlockBackend *blk, bool disable)
>   {
>       IO_CODE();
> -    blk->disable_request_queuing = disable;
> +    qatomic_set(&blk->disable_request_queuing, disable);
>   }
>   
>   static int coroutine_fn GRAPH_RDLOCK
> @@ -1271,7 +1271,8 @@ static void coroutine_fn 
> blk_wait_while_drained(BlockBackend *blk)
>   {
>       assert(blk->in_flight > 0);
>   
> -    if (qatomic_read(&blk->quiesce_counter) && !blk->disable_request_queuing) {
> +    if (qatomic_read(&blk->quiesce_counter) &&
> +        !qatomic_read(&blk->disable_request_queuing)) {
>           blk_dec_in_flight(blk);
>           qemu_co_queue_wait(&blk->queued_requests, NULL);
>           blk_inc_in_flight(blk);