Re: [PATCH 02/11] block: move drain outside of read-locked bdrv_reopen_queue_child()

[Kevin Wolf <kwolf@redhat.com>]

Am 15.05.2025 um 13:48 hat Fiona Ebner geschrieben:
> Am 14.05.25 um 18:36 schrieb Kevin Wolf:
> > Am 08.05.2025 um 16:09 hat Fiona Ebner geschrieben:
> >> @@ -4368,6 +4368,7 @@ bdrv_reopen_queue_child(BlockReopenQueue *bs_queue, BlockDriverState *bs,
> >>                          bool keep_old_opts)
> >>  {
> >>      assert(bs != NULL);
> >> +    assert(qatomic_read(&bs->quiesce_counter) > 0);
> > 
> > BlockDriverState.quiesce_counter isn't accessed with atomics elsewhere.
> > Did you confuse it with BlockBackend.quiesce_counter?
> 
> No, but I saw that it is modified via qatomic_fetch_inc/dec(). And those
> modifications are in bdrv_do_drained_begin/end() which are
> IO_OR_GS_CODE(). So isn't it more correct to read via atomics here?

Aha, I missed these two places. Looks like Paolo's commit 414c2ec wasn't
very thorough with converting.

The commit message is also empty, so I don't know why we made this
change. Both places are GLOBAL_STATE_CODE(), so I don't think we
actually need atomics to synchronise these two places. Maybe there are
other accesses in iothreads, but then those should have been using
atomics, too.

> The documentation in include/block/block_int-common.h for struct
> BlockDriverState also states:
> >     /* Accessed with atomic ops.  */
> >     int quiesce_counter;
> 
> Should I rather add a patch to have the other readers use atomics too?

Either all accesses should use atomics or none of them. I'm not
completely sure which way is the right one. Using atomics everywhere is
the safe option, but I'm not sure if we ever access quiesce_counter
outside of the main thread.

Kevin