Re: [PATCH 1/3] block: make BlockBackend->quiesce_counter atomic

[Hanna Czenczek]

On 27.02.23 21:57, Stefan Hajnoczi wrote:
> The main loop thread increments/decrements BlockBackend->quiesce_counter
> when drained sections begin/end. The counter is read in the I/O code
> path. Therefore this field is used to communicate between threads
> without a lock.
>
> Use qatomic_set()/qatomic_read() to make it clear that this field is
> accessed by multiple threads.
>
> Acquire/release are not necessary because the BlockBackend->in_flight
> counter already uses sequentially consistent accesses and running I/O
> requests hold that counter when blk_wait_while_drained() is called.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>   block/block-backend.c | 18 +++++++++++-------
>   1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/block/block-backend.c b/block/block-backend.c
> index 278b04ce69..f00bf2ab35 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c

[...]

> @@ -2568,7 +2568,9 @@ static void blk_root_drained_begin(BdrvChild *child)
>       BlockBackend *blk = child->opaque;
>       ThrottleGroupMember *tgm = &blk->public.throttle_group_member;
>   
> -    if (++blk->quiesce_counter == 1) {
> +    int new_counter = qatomic_read(&blk->quiesce_counter) + 1;
> +    qatomic_set(&blk->quiesce_counter, new_counter);
> +    if (new_counter == 1) {
>           if (blk->dev_ops && blk->dev_ops->drained_begin) {
>               blk->dev_ops->drained_begin(blk->dev_opaque);
>           }

[...]

> @@ -2597,12 +2599,14 @@ static bool blk_root_drained_poll(BdrvChild *child)

[...]

>       assert(blk->public.throttle_group_member.io_limits_disabled);
>       qatomic_dec(&blk->public.throttle_group_member.io_limits_disabled);
>   
> -    if (--blk->quiesce_counter == 0) {
> +    int new_counter = qatomic_read(&blk->quiesce_counter) - 1;
> +    qatomic_set(&blk->quiesce_counter, new_counter);

I don’t quite understand why you decided not to use simple atomic 
increments/decrements with just SeqCst in these places.  Maybe it is 
fine this way, but it isn’t trivial to see.  As far as I understand, 
these aren’t hot paths, so I don’t think we’d lose performance by using 
fully atomic operations here.

Hanna