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 <stefa...@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
signature.asc
Description: PGP signature
