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

[Fiona Ebner <f.ebner@proxmox.com>]

Am 15.05.25 um 14:28 schrieb Kevin Wolf:
> 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.

AFAICT, all accesses are either in a function with GLOBAL_STATE_CODE()
directly or in a function with GRAPH_WRLOCK (in the cases of
bdrv_replace_child_tran() and bdrv_replace_child_noperm()).

This does not change for the new accesses added by the series here.

Back then, there was no GLOBAL_STATE_CODE() macro yet, so I suppose this
wasn't easy to verify.

>> 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.

I'd add a patch dropping the atomic accesses and document that
quiesce_counter can only be accessed in the main thread.

Best Regards,
Fiona