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

[Kevin Wolf <kwolf@redhat.com>]

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