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.
> * @snapshot_id: unique snapshot ID, or NULL
> * @name: snapshot name, or NULL
> * @errp: location to store error
> @@ -356,6 +356,8 @@ int bdrv_snapshot_delete(BlockDriverState *bs,
> BlockDriverState *fallback_bs = bdrv_snapshot_fallback(bs);
> int ret;
>
> + assert(qatomic_read(&bs->quiesce_counter) > 0);
> +
> GLOBAL_STATE_CODE();
>
> if (!drv) {
> @@ -368,9 +370,6 @@ int bdrv_snapshot_delete(BlockDriverState *bs,
> return -EINVAL;
> }
>
> - /* drain all pending i/o before deleting snapshot */
> - bdrv_drained_begin(bs);
> -
> if (drv->bdrv_snapshot_delete) {
> ret = drv->bdrv_snapshot_delete(bs, snapshot_id, name, errp);
> } else if (fallback_bs) {
> @@ -382,7 +381,6 @@ int bdrv_snapshot_delete(BlockDriverState *bs,
> ret = -ENOTSUP;
> }
>
> - bdrv_drained_end(bs);
> return ret;
> }
>
> @@ -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.
>
> iterbdrvs = iterbdrvs->next;
> }
>
> + bdrv_graph_rdunlock_main_loop();
> + bdrv_drain_all_end();
> return 0;
> }
Kevin
