Am 14.05.25 um 18:52 schrieb Kevin Wolf:
> Am 08.05.2025 um 16:09 hat Fiona Ebner geschrieben:
>> This is in preparation to mark bdrv_drained_begin() as GRAPH_UNLOCKED.
>>
>> More granular draining is not trivially possible, because
>> bdrv_snapshot_delete() can recursively call itself.
>>
>> Signed-off-by: Fiona Ebner <f.eb...@proxmox.com>
>> ---
>> block/snapshot.c | 18 ++++++++++++------
>> blockdev.c | 25 +++++++++++++++++--------
>> qemu-img.c | 2 ++
>> 3 files changed, 31 insertions(+), 14 deletions(-)
>>
>> diff --git a/block/snapshot.c b/block/snapshot.c
>> index 22567f1fb9..7788e1130b 100644
>> --- a/block/snapshot.c
>> +++ b/block/snapshot.c
>> @@ -327,7 +327,7 @@ int bdrv_snapshot_goto(BlockDriverState *bs,
>>
>> /**
>> * Delete an internal snapshot by @snapshot_id and @name.
>> - * @bs: block device used in the operation
>> + * @bs: block device used in the operation, needs to be drained
>
> Forgot to add this piece of nitpicking on the previous patch: Other
> places say "must be drained", which I slightly prefer because of how
> RFC 2119 has "MUST", but not "NEEDS TO". Matter of taste, I guess, but
> if you agree, we could change it for the non-RFC series.
Sure, will do!
>> @@ -573,9 +571,13 @@ int bdrv_all_delete_snapshot(const char *name,
>> GList *iterbdrvs;
>>
>> GLOBAL_STATE_CODE();
>> - GRAPH_RDLOCK_GUARD_MAINLOOP();
>> +
>> + bdrv_drain_all_begin();
>> + bdrv_graph_rdlock_main_loop();
>>
>> if (bdrv_all_get_snapshot_devices(has_devices, devices, &bdrvs, errp) <
>> 0) {
>> + bdrv_graph_rdunlock_main_loop();
>> + bdrv_drain_all_end();
>> return -1;
>> }
>
> I think this wants to be changed into:
>
> ret = bdrv_all_get_snapshot_devices(has_devices, devices, &bdrvs, errp);
> if (ret < 0) {
> goto out;
> }
>
> (Changing the return value from -1 to -errno is fine for the callers.)
>
>> @@ -594,12 +596,16 @@ int bdrv_all_delete_snapshot(const char *name,
>> if (ret < 0) {
>> error_prepend(errp, "Could not delete snapshot '%s' on '%s': ",
>> name, bdrv_get_device_or_node_name(bs));
>> + bdrv_graph_rdunlock_main_loop();
>> + bdrv_drain_all_end();
>> return -1;
>> }
>
> Same here.
Ack.
Best Regards,
Fiona
