Re: [Qemu-devel] [PATCH 3/5] qcow2: Employ metadata overlap checks

[Kevin Wolf]

Am 26.08.2013 um 15:04 hat Max Reitz geschrieben:
> The pre-write overlap check function is now called before most of the
> qcow2 writes (aborting it on collision or other error).
> 
> Signed-off-by: Max Reitz <address@hidden>
> ---
>  block/qcow2-cache.c    | 17 +++++++++++++++++
>  block/qcow2-cluster.c  | 23 +++++++++++++++++++++++
>  block/qcow2-snapshot.c | 24 ++++++++++++++++++++++++
>  block/qcow2.c          | 38 +++++++++++++++++++++++++++++++++++++-
>  4 files changed, 101 insertions(+), 1 deletion(-)

> @@ -368,6 +384,13 @@ static int coroutine_fn copy_sectors(BlockDriverState 
> *bs,
>                          &s->aes_encrypt_key);
>      }
>  
> +    ret = qcow2_pre_write_overlap_check(bs, QCOW2_OL_DEFAULT,
> +            ((cluster_offset >> 9) + n_start) << 9, n * BDRV_SECTOR_SIZE);

Looks a bit overcomplicated, I'd like something like this better:

cluster_offset + n_start * BDRV_SECTOR_SIZE

> +    if (ret) {
> +        ret = (ret < 0) ? ret : -EIO;

I wonder whether the -EIO logic should be moved into
qcow2_pre_write_overlap_check(). Currently each single caller seems to
have this check.

> +        goto out;
> +    }
> +
>      BLKDBG_EVENT(bs->file, BLKDBG_COW_WRITE);
>      ret = bdrv_co_writev(bs->file, (cluster_offset >> 9) + n_start, n, 
> &qiov);
>      if (ret < 0) {
> diff --git a/block/qcow2-snapshot.c b/block/qcow2-snapshot.c
> index 0caac90..6f69ecc 100644
> --- a/block/qcow2-snapshot.c
> +++ b/block/qcow2-snapshot.c
> @@ -189,6 +189,15 @@ static int qcow2_write_snapshots(BlockDriverState *bs)
>          return ret;
>      }
>  
> +    /* The snapshot list position has not yet been updated, so these clusters
> +     * must indeed be completely free */
> +    ret = qcow2_pre_write_overlap_check(bs, QCOW2_OL_DEFAULT,
> +                                        offset, s->nb_snapshots * sizeof(h));
> +    if (ret) {
> +        return (ret < 0) ? ret : -EIO;
> +    }

This doesn't check the full size. snapshots_size should have the right
value.

> +
> +
>      /* Write all snapshots to the new list */
>      for(i = 0; i < s->nb_snapshots; i++) {
>          sn = s->snapshots + i;
> @@ -363,6 +372,13 @@ int qcow2_snapshot_create(BlockDriverState *bs, 
> QEMUSnapshotInfo *sn_info)
>          l1_table[i] = cpu_to_be64(s->l1_table[i]);
>      }
>  
> +    ret = qcow2_pre_write_overlap_check(bs, QCOW2_OL_DEFAULT,
> +            sn->l1_table_offset, s->l1_size * sizeof(uint64_t));
> +    if (ret) {
> +        ret = (ret < 0) ? ret : -EIO;
> +        goto fail;
> +    }
> +
>      ret = bdrv_pwrite(bs->file, sn->l1_table_offset, l1_table,
>                        s->l1_size * sizeof(uint64_t));
>      if (ret < 0) {
> @@ -475,6 +491,14 @@ int qcow2_snapshot_goto(BlockDriverState *bs, const char 
> *snapshot_id)
>          goto fail;
>      }
>  
> +    ret = qcow2_pre_write_overlap_check(bs,
> +            QCOW2_OL_DEFAULT & ~QCOW2_OL_ACTIVE_L1,
> +            s->l1_table_offset, cur_l1_bytes);
> +    if (ret) {
> +        ret = (ret < 0) ? ret : -EIO;
> +        goto fail;
> +    }
> +
>      ret = bdrv_pwrite_sync(bs->file, s->l1_table_offset, sn_l1_table,
>                             cur_l1_bytes);
>      if (ret < 0) {
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 1d0d7ca..95497c6 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -621,6 +621,8 @@ static int qcow2_open(BlockDriverState *bs, QDict 
> *options, int flags)
>      qcow2_free_snapshots(bs);
>      qcow2_refcount_close(bs);
>      g_free(s->l1_table);
> +    /* else pre-write overlap checks in cache_destroy may crash */
> +    s->l1_table = NULL;
>      if (s->l2_table_cache) {
>          qcow2_cache_destroy(bs, s->l2_table_cache);
>      }
> @@ -920,6 +922,14 @@ static coroutine_fn int qcow2_co_writev(BlockDriverState 
> *bs,
>                  cur_nr_sectors * 512);
>          }
>  
> +        ret = qcow2_pre_write_overlap_check(bs, QCOW2_OL_DEFAULT,
> +                ((cluster_offset >> 9) + index_in_cluster) << 9,

Same thing as above.

> +                cur_nr_sectors << 9);
> +        if (ret) {
> +            ret = (ret < 0) ? ret : -EIO;
> +            goto fail;
> +        }
> +
>          qemu_co_mutex_unlock(&s->lock);
>          BLKDBG_EVENT(bs->file, BLKDBG_WRITE_AIO);
>          trace_qcow2_writev_data(qemu_coroutine_self(),

Kevin