Re: [Qemu-devel] [PATCH 06/11] snapshot: create bdrv_all_find_snapshot helper

[Fam Zheng <famz@redhat.com>]

On Tue, 11/17 12:08, Denis V. Lunev wrote:
> to check that snapshot is available for all loaded block drivers.
> The check bs != bs1 in hmp_info_snapshots is an optimization. The check
> for availability of this snapshot will return always true as the list
> of snapshots was collected from that image.
> 
> The patch also ensures proper locking.
> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Juan Quintela <quintela@redhat.com>
> CC: Stefan Hajnoczi <stefanha@redhat.com>
> CC: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/snapshot.c         | 20 ++++++++++++++++++++
>  include/block/snapshot.h |  1 +
>  migration/savevm.c       | 42 +++++++++---------------------------------
>  3 files changed, 30 insertions(+), 33 deletions(-)
> 
> diff --git a/block/snapshot.c b/block/snapshot.c
> index 9f07a63..eae4730 100644
> --- a/block/snapshot.c
> +++ b/block/snapshot.c
> @@ -423,3 +423,23 @@ int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bad_bs)
>      *first_bad_bs = bs;
>      return err;
>  }
> +
> +int bdrv_all_find_snapshot(const char *name, BlockDriverState **first_bad_bs)
> +{
> +    QEMUSnapshotInfo sn;
> +    int err = 0;
> +    BlockDriverState *bs = NULL;
> +
> +    while (err == 0 && (bs = bdrv_next(bs))) {
> +        AioContext *ctx = bdrv_get_aio_context(bs);
> +
> +        aio_context_acquire(ctx);
> +        if (bdrv_can_snapshot(bs)) {
> +            err = bdrv_snapshot_find(bs, &sn, name);
> +        }
> +        aio_context_release(ctx);
> +    }
> +
> +    *first_bad_bs = bs;
> +    return err;
> +}
> diff --git a/include/block/snapshot.h b/include/block/snapshot.h
> index 0a176c7..10ee582 100644
> --- a/include/block/snapshot.h
> +++ b/include/block/snapshot.h
> @@ -85,5 +85,6 @@ bool bdrv_all_can_snapshot(BlockDriverState **first_bad_bs);
>  int bdrv_all_delete_snapshot(const char *name, BlockDriverState **first_bsd_bs,
>                               Error **err);
>  int bdrv_all_goto_snapshot(const char *name, BlockDriverState **first_bsd_bs);
> +int bdrv_all_find_snapshot(const char *name, BlockDriverState **first_bad_bs);
>  
>  #endif
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 2ecc1b3..4e6d578 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -2056,6 +2056,12 @@ int load_vmstate(const char *name)
>                       bdrv_get_device_name(bs));
>          return -ENOTSUP;
>      }
> +    ret = bdrv_all_find_snapshot(name, &bs);
> +    if (ret < 0) {
> +        error_report("Device '%s' does not have the requested snapshot '%s'",
> +                     bdrv_get_device_name(bs), name);
> +        return ret;
> +    }
>  
>      bs_vm_state = find_vmstate_bs();
>      if (!bs_vm_state) {
> @@ -2073,22 +2079,6 @@ int load_vmstate(const char *name)
>          return -EINVAL;
>      }
>  
> -    /* Verify if there is any device that doesn't support snapshots and is
> -    writable and check if the requested snapshot is available too. */
> -    bs = NULL;
> -    while ((bs = bdrv_next(bs))) {
> -        if (!bdrv_can_snapshot(bs)) {
> -            continue;
> -        }
> -
> -        ret = bdrv_snapshot_find(bs, &sn, name);
> -        if (ret < 0) {
> -            error_report("Device '%s' does not have the requested snapshot '%s'",
> -                           bdrv_get_device_name(bs), name);
> -            return ret;
> -        }
> -    }
> -
>      /* Flush all IO requests so they don't interfere with the new state.  */
>      bdrv_drain_all();
>  
> @@ -2142,8 +2132,8 @@ void hmp_delvm(Monitor *mon, const QDict *qdict)
>  void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
>  {
>      BlockDriverState *bs, *bs1;
> -    QEMUSnapshotInfo *sn_tab, *sn, s, *sn_info = &s;
> -    int nb_sns, i, ret, available;
> +    QEMUSnapshotInfo *sn_tab, *sn;
> +    int nb_sns, i;
>      int total;
>      int *available_snapshots;
>  
> @@ -2167,21 +2157,7 @@ void hmp_info_snapshots(Monitor *mon, const QDict *qdict)
>      available_snapshots = g_new0(int, nb_sns);
>      total = 0;
>      for (i = 0; i < nb_sns; i++) {
> -        sn = &sn_tab[i];
> -        available = 1;
> -        bs1 = NULL;
> -
> -        while ((bs1 = bdrv_next(bs1))) {
> -            if (bdrv_can_snapshot(bs1) && bs1 != bs) {
> -                ret = bdrv_snapshot_find(bs1, sn_info, sn->id_str);
> -                if (ret < 0) {
> -                    available = 0;
> -                    break;
> -                }
> -            }
> -        }
> -
> -        if (available) {
> +        if (bdrv_all_find_snapshot(sn_tab[i].id_str, &bs1) == 0) {
>              available_snapshots[total] = i;
>              total++;
>          }
> -- 
> 2.5.0
> 
> 

Reviewed-by: Fam Zheng <famz@redhat.com>