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

[Stefan Hajnoczi <stefanha@redhat.com>]

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

On Fri, Mar 03, 2023 at 04:29:54PM +0100, Hanna Czenczek wrote:
> 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.

Good idea. It would be much easier to read.

Stefan

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