Re: [Qemu-devel] [PATCH] Use snapshots from backing disks

[Kevin Wolf]

Am 09.03.2010 02:13, schrieb Rob Earhart:
> Modify the snapshot load path to find and load snapshots contained in backing
> disks, useful when the current disk is a differencing disk.
> 
> Add the source of a snapshot when listing snapshots.
> 
> This should only break backwards compatibility for scenarios depending on not
> being able to load snapshots from backing disks (which doesn't seem like a
> problem), and for code which parses the snapshot list output (if any).
> 
> Signed-off-by: Rob Earhart <address@hidden>

I think I have considered this kind of thing some time ago, and I seem
to remember that there was something dangerous about it. However, I can
remember right away what it was. Give me a day or two to think about it
once again.

> ---
>  block.c                |   10 +++++
>  block.h                |    2 +
>  block/qcow2-snapshot.c |  105 
> ++++++++++++++++++++++++++++++++++++------------
>  qemu-img.c             |   25 +++++++-----
>  savevm.c               |   92 +++++++++++++++++++++++++++++-------------
>  5 files changed, 170 insertions(+), 64 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 31d1ba4..3588700 100644
> --- a/block.c
> +++ b/block.c
> @@ -1481,6 +1481,16 @@ void bdrv_get_backing_filename(BlockDriverState *bs,
>      }
>  }
> 
> +const char *bdrv_get_filename(BlockDriverState *bs)
> +{
> +    return bs->filename;
> +}
> +
> +BlockDriverState* bdrv_get_backing_bdrv(BlockDriverState *bs)
> +{
> +    return bs->backing_hd;
> +}
> +

This kind of simple getters is not the way the qemu block drivers work.
Just access bs->filename/backing_hd directly.

>  int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
>                            const uint8_t *buf, int nb_sectors)
>  {
> diff --git a/block.h b/block.h
> index edf5704..1d04322 100644
> --- a/block.h
> +++ b/block.h
> @@ -179,6 +179,8 @@ int bdrv_get_info(BlockDriverState *bs,
> BlockDriverInfo *bdi);
>  const char *bdrv_get_encrypted_filename(BlockDriverState *bs);
>  void bdrv_get_backing_filename(BlockDriverState *bs,
>                                 char *filename, int filename_size);
> +const char *bdrv_get_filename(BlockDriverState *bs);
> +BlockDriverState* bdrv_get_backing_bdrv(BlockDriverState *bs);
>  int bdrv_snapshot_create(BlockDriverState *bs,
>                           QEMUSnapshotInfo *sn_info);
>  int bdrv_snapshot_goto(BlockDriverState *bs,
> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
> index 8ddaea2..ca06fcf 100644
> --- a/block/qcow2-snapshot.c
> +++ b/block/qcow2-snapshot.c
> @@ -191,16 +191,23 @@ static int qcow_write_snapshots(BlockDriverState *bs)
>  static void find_new_snapshot_id(BlockDriverState *bs,
>                                   char *id_str, int id_str_size)
>  {
> -    BDRVQcowState *s = bs->opaque;
> -    QCowSnapshot *sn;
> -    int i, id, id_max = 0;
> +    int id_max = 0;
> +
> +    assert(bs);
> +
> +    do {
> +        BDRVQcowState *s = bs->opaque;
> +        QCowSnapshot *sn;
> +        int i, id;
> +
> +        for(i = 0; i < s->nb_snapshots; i++) {
> +            sn = s->snapshots + i;
> +            id = strtoul(sn->id_str, NULL, 10);
> +            if (id > id_max)
> +                id_max = id;
> +        }
> +    } while ((bs = bdrv_get_backing_bdrv(bs)));

You assume that the backing file is a qcow2 file, too. This isn't
correct generally, the backing file can be any format. In that case your
code will fail horribly.

> 
> -    for(i = 0; i < s->nb_snapshots; i++) {
> -        sn = s->snapshots + i;
> -        id = strtoul(sn->id_str, NULL, 10);
> -        if (id > id_max)
> -            id_max = id;
> -    }
>      snprintf(id_str, id_str_size, "%d", id_max + 1);
>  }
> 
> @@ -316,41 +323,88 @@ int qcow2_snapshot_goto(BlockDriverState *bs,
> const char *snapshot_id)
>  {
>      BDRVQcowState *s = bs->opaque;
>      QCowSnapshot *sn;
> -    int i, snapshot_index, l1_size2;
> +    int i, snapshot_index, l1_size2, ret;
> 
>      snapshot_index = find_snapshot_by_id_or_name(bs, snapshot_id);
> -    if (snapshot_index < 0)
> -        return -ENOENT;
> +
> +    if (snapshot_index < 0) {
> +        /* if there is a backing file, use it. In this case, the
> +         * current image needs to be reset to be consistent
> +         */
> +        if (bs->backing_hd) {
> +            ret = bdrv_snapshot_goto(bs->backing_hd, snapshot_id);
> +            if (ret < 0) {
> +                return ret;
> +            }

This is fundamentally broken. You can't change the backing file, and I
think bdrv_snapshot_goto does change it. It's considered read-only. This
will break all other images basing on the same backing file.

> +            /* reset the sectors of the current image (Note: there can
> +             * still be snapshots, so the refcounts must be kept
> +             * consistent)
> +             */
> +            if (qcow2_update_snapshot_refcount(bs, s->l1_table_offset,
> +                                               s->l1_size, -1) < 0) {
> +                return -EIO;
> +            }

If qemu crashes here (or the following bdrv_pwrite returns an error), we
get a corrupted image with refcounts of 0 for clusters that are still used.

> +            l1_size2 = s->l1_size * sizeof(uint64_t);
> +            memset(s->l1_table, 0, l1_size2);
> +            if (bdrv_pwrite(s->hd, s->l1_table_offset,
> +                            s->l1_table, l1_size2) != l1_size2) {
> +                return -EIO;
> +            }

Isn't this leaking the L2 tables? And are there no in-memory data
structures that need to be invalidated, like L1/L2 caches?

If you do the thing right, I think it's an implementation for
qcow_make_empty, which if completely ifdef'd out currently. Which
probably means that doing it right is not so easy.

> +            return 0;
> +        } else {
> +            return -ENOENT;
> +        }
> +    }
> +
> +    assert(0 <= snapshot_index);
> +
>      sn = &s->snapshots[snapshot_index];
> 
> -    if (qcow2_update_snapshot_refcount(bs, s->l1_table_offset,
> s->l1_size, -1) < 0)
> -        goto fail;
> +    /* if the file is read-only, all can be done by updating the
> +     * in-memory L1 table. Otherwise, we'll need to do some writes.
> +     */
> 
> -    if (qcow2_grow_l1_table(bs, sn->l1_size) < 0)
> -        goto fail;
> +    if (!bs->read_only) {
> +        if (qcow2_update_snapshot_refcount(bs, s->l1_table_offset,
> +                                           s->l1_size, -1)
> +            < 0) {
> +            return -EIO;
> +        }
> +    }
> +
> +    if (qcow2_grow_l1_table(bs, sn->l1_size) < 0) {
> +        return -EIO;
> +    }
> 
>      s->l1_size = sn->l1_size;
>      l1_size2 = s->l1_size * sizeof(uint64_t);
>      /* copy the snapshot l1 table to the current l1 table */
>      if (bdrv_pread(s->hd, sn->l1_table_offset,
> -                   s->l1_table, l1_size2) != l1_size2)
> -        goto fail;
> -    if (bdrv_pwrite(s->hd, s->l1_table_offset,
> -                    s->l1_table, l1_size2) != l1_size2)
> -        goto fail;
> +                   s->l1_table, l1_size2) != l1_size2) {
> +        return -EIO;
> +    }
> +    if (!bs->read_only) {
> +        if (bdrv_pwrite(s->hd, s->l1_table_offset,
> +                        s->l1_table, l1_size2) != l1_size2) {
> +            return -EIO;
> +        }
> +    }
>      for(i = 0;i < s->l1_size; i++) {
>          be64_to_cpus(&s->l1_table[i]);
>      }
> 
> -    if (qcow2_update_snapshot_refcount(bs, s->l1_table_offset,
> s->l1_size, 1) < 0)
> -        goto fail;
> +    if (!bs->read_only) {
> +        if (qcow2_update_snapshot_refcount(bs, s->l1_table_offset,
> +                                           s->l1_size, 1)
> +            < 0) {
> +            return -EIO;
> +        }
> +    }
> 
>  #ifdef DEBUG_ALLOC
>      qcow2_check_refcounts(bs);
>  #endif
>      return 0;
> - fail:
> -    return -EIO;
>  }
> 
>  int qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id)
> @@ -416,4 +470,3 @@ int qcow2_snapshot_list(BlockDriverState *bs,
> QEMUSnapshotInfo **psn_tab)
>      *psn_tab = sn_tab;
>      return s->nb_snapshots;
>  }
> -
> diff --git a/qemu-img.c b/qemu-img.c
> index 0c9f2d4..345a768 100644
> --- a/qemu-img.c
> +++ b/qemu-img.c
> @@ -873,16 +873,21 @@ static void dump_snapshots(BlockDriverState *bs)
>      int nb_sns, i;
>      char buf[256];
> 
> -    nb_sns = bdrv_snapshot_list(bs, &sn_tab);
> -    if (nb_sns <= 0)
> -        return;
> -    printf("Snapshot list:\n");
> -    printf("%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
> -    for(i = 0; i < nb_sns; i++) {
> -        sn = &sn_tab[i];
> -        printf("%s\n", bdrv_snapshot_dump(buf, sizeof(buf), sn));
> -    }
> -    qemu_free(sn_tab);
> +    assert(bs);
> +
> +    do {
> +        nb_sns = bdrv_snapshot_list(bs, &sn_tab);
> +        if (nb_sns <= 0)
> +            continue;
> +        printf("Snapshot list (from %s):\n",
> +               bdrv_get_filename(bs));
> +        printf("%s\n", bdrv_snapshot_dump(buf, sizeof(buf), NULL));
> +        for(i = 0; i < nb_sns; i++) {
> +            sn = &sn_tab[i];
> +            printf("%s\n", bdrv_snapshot_dump(buf, sizeof(buf), sn));
> +        }
> +        qemu_free(sn_tab);
> +    } while ((bs = bdrv_get_backing_bdrv(bs)));

IMHO, bdrv_snapshot_list should implement this and return a list of all
available snapshots.

Kevin