Re: [PATCH v2 03/24] block/snapshot: move drain outside of read-locked bdrv_snapshot_delete()

[Kevin Wolf <kwolf@redhat.com>]

Am 20.05.2025 um 12:29 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.
> 
> The return value of bdrv_all_delete_snapshot() changes from -1 to
> -errno propagated from failed sub-calls. This is fine for the existing
> callers of bdrv_all_delete_snapshot().
> 
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> 
> Changes in v2:
> * Use 'must be drained' instead of 'needs to be drained'.
> * Use goto for cleanup/error handling in bdrv_all_delete_snapshot().
> * Don't use atomics to access bs->quiesce_counter.
> 
>  block/snapshot.c | 26 +++++++++++++++-----------
>  blockdev.c       | 25 +++++++++++++++++--------
>  qemu-img.c       |  2 ++
>  3 files changed, 34 insertions(+), 19 deletions(-)

> @@ -571,19 +569,22 @@ int bdrv_all_delete_snapshot(const char *name,
>      ERRP_GUARD();
>      g_autoptr(GList) bdrvs = NULL;
>      GList *iterbdrvs;
> +    int ret = 0;

The initialisation here is technically not needed...

>      GLOBAL_STATE_CODE();
> -    GRAPH_RDLOCK_GUARD_MAINLOOP();
>  
> -    if (bdrv_all_get_snapshot_devices(has_devices, devices, &bdrvs, errp) < 0) {
> -        return -1;
> +    bdrv_drain_all_begin();
> +    bdrv_graph_rdlock_main_loop();
> +
> +    ret = bdrv_all_get_snapshot_devices(has_devices, devices, &bdrvs, errp);
> +    if (ret < 0) {
> +        goto out;
>      }

...because ret is assigned here before anyone reads it. But it doesn't
hurt either, so:

Reviewed-by: Kevin Wolf <kwolf@redhat.com>