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?
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?
>> @@ -4522,6 +4516,14 @@ BlockReopenQueue *bdrv_reopen_queue(BlockReopenQueue
>> *bs_queue,
>> QDict *options, bool keep_old_opts)
>> {
>> GLOBAL_STATE_CODE();
>> +
>> + if (bs_queue == NULL) {
>> + /*
>> + * paired with bdrv_drain_all_end() in bdrv_reopen_queue_free().
>> + */
>> + bdrv_drain_all_begin();
>
> Having this comment is a good idea. It fits on a single line, though.
Ack.
>
>> + }
>> +
>> GRAPH_RDLOCK_GUARD_MAINLOOP();
>>
>> return bdrv_reopen_queue_child(bs_queue, bs, options, NULL, 0, false,
>> @@ -4534,12 +4536,16 @@ void bdrv_reopen_queue_free(BlockReopenQueue
>> *bs_queue)
>> if (bs_queue) {
>> BlockReopenQueueEntry *bs_entry, *next;
>> QTAILQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
>> - bdrv_drained_end(bs_entry->state.bs);
>> qobject_unref(bs_entry->state.explicit_options);
>> qobject_unref(bs_entry->state.options);
>> g_free(bs_entry);
>> }
>> g_free(bs_queue);
>> +
>> + /*
>> + * paired with bdrv_drain_all_begin() in bdrv_reopen_queue().
>> + */
>> + bdrv_drain_all_end();
>
> This could be a single line comment, too.
Ack.
Best Regards,
Fiona
