Re: [Qemu-devel] [PATCH V18 5/6] add-cow file format core code.

[Stefan Hajnoczi <stefanha@gmail.com>]

On Wed, Apr 10, 2013 at 04:11:52PM +0800, Dong Xu Wang wrote:
> +    header.cluster_bits = ffs(cluster_size) - 1;
> +    if (header.cluster_bits < MIN_CLUSTER_BITS ||
> +        header.cluster_bits > MAX_CLUSTER_BITS ||
> +        (1 << header.cluster_bits) != cluster_size) {
> +        error_report(
> +            "Cluster size must be a power of two between %d and %dk",
> +            1 << MIN_CLUSTER_BITS, 1 << (MAX_CLUSTER_BITS - 10));
> +        return -EINVAL;
> +    }
> +
> +   header.header_size = MAX(cluster_size, DEFAULT_HEADER_SIZE);

Indentation.

> +    if (backing_filename) {
> +        header.backing_offset = sizeof(header);
> +        header.backing_size = strlen(backing_filename);
> +
> +        if (!backing_fmt) {
> +            backing_bs = bdrv_new("image");
> +            ret = bdrv_open(backing_bs, backing_filename, NULL,
> +                            BDRV_O_RDWR | BDRV_O_CACHE_WB, NULL);
> +            if (ret < 0) {
> +                return ret;

backing_bs is leaked.

> +    ret = bdrv_file_open(&bs, filename, NULL, BDRV_O_RDWR);
> +    if (ret < 0) {
> +        return ret;
> +    }
> +    snprintf(header.backing_fmt, sizeof(header.backing_fmt), "%s",
> +             backing_fmt ? backing_fmt : "");
> +    snprintf(header.image_fmt, sizeof(header.image_fmt), "%s",
> +             image_format ? image_format : "raw");

snprintf() doesn't have the semantics in the add-cow specification:

" 44 - 59:    backing file format
              Format of backing file. It will be filled with
              0 if backing file name offset is 0. If backing
              file name offset is non-empty, it must be
              non-empty. It is coded in free-form ASCII, and
              is not NUL-terminated. Zero padded on the right.

  60 - 75:    image file format
              Format of image file. It must be non-empty. It
              is coded in free-form ASCII, and is not
              NUL-terminated. Zero padded on the right."

strncpy() does the zero padding and doesn't NUL-terminate if the max buffer
size is used.

> +    if ((s->header.compat_features & ACOW_F_ALL_ALLOCATED) == 0) {
> +        snprintf(bs->backing_format, sizeof(bs->backing_format),
> +                 "%s", s->header.backing_fmt);

s->header.backing_fmt is not NUL-terminated so using snprintf() is
inappropriate (could it read beyond the end of .backing_fmt?).

> +    }
> +
> +    if (s->header.cluster_bits < MIN_CLUSTER_BITS ||
> +        s->header.cluster_bits > MAX_CLUSTER_BITS) {
> +        ret = -EINVAL;
> +        goto fail;
> +    }
> +
> +    s->cluster_size = 1 << s->header.cluster_bits;
> +    if (s->header.header_size != MAX(s->cluster_size, DEFAULT_HEADER_SIZE)) {
> +        char buf[64];
> +        snprintf(buf, sizeof(buf), "Header size: %d",

%u or PRIu32 since header_size is uint32_t.  This avoids compiler or
code scanner warnings.

> +    s->image_hd = bdrv_new("");
> +    ret = bdrv_open(s->image_hd, image_filename, NULL, flags,
> +                    bdrv_find_format(s->header.image_fmt));

Cannot use image_fmt as a string since it is not NUL-terminated.

> +static coroutine_fn int add_cow_co_writev(BlockDriverState *bs,
> +                                          int64_t sector_num,
> +                                          int remaining_sectors,
> +                                          QEMUIOVector *qiov)
> +{
> +    BDRVAddCowState *s = bs->opaque;
> +    int ret = 0, i;
> +    QEMUIOVector hd_qiov;
> +    uint8_t *table;
> +    uint64_t offset;
> +    int mask = s->cluster_sectors - 1;
> +    int cluster_mask = s->cluster_size - 1;
> +
> +    qemu_co_mutex_lock(&s->lock);
> +    qemu_iovec_init(&hd_qiov, qiov->niov);
> +    ret = bdrv_co_writev(s->image_hd, sector_num,
> +                         remaining_sectors, qiov);

All writes are serialized.  This means write performance will be very
poor for multi-threaded workloads.

qcow2 tracks allocating writes and allows them to execute at the same
time if they do not overlap clusters.

> +
> +    if (ret < 0) {
> +        goto fail;
> +    }
> +    if ((s->header.compat_features & ACOW_F_ALL_ALLOCATED) == 0) {
> +        /* Copy content of unmodified sectors */
> +        if (!is_cluster_head(sector_num, s->cluster_sectors)
> +            && !is_allocated(bs, sector_num)) {
> +            ret = copy_sectors(bs, sector_num & ~mask, sector_num);
> +            if (ret < 0) {
> +                goto fail;
> +            }
> +        }
> +
> +        if (!is_cluster_tail(sector_num + remaining_sectors - 1,
> +                             s->cluster_sectors)
> +            && !is_allocated(bs, sector_num + remaining_sectors - 1)) {
> +            ret = copy_sectors(bs, sector_num + remaining_sectors,
> +                               ((sector_num + remaining_sectors) | mask) + 1);
> +            if (ret < 0) {
> +                goto fail;
> +            }
> +        }

This trashes the cluster when remaining_sectors = 0, sector_num =
cluster_sectors, and sector cluster_sectors - 1 is unallocated.

Probably best to return early when remaining_sectors == 0.

> +
> +        for (i = sector_num / s->cluster_sectors;
> +            i <= (sector_num + remaining_sectors - 1) / s->cluster_sectors;
> +            i++) {
> +            offset = s->header.header_size
> +                + (offset_in_bitmap(i * s->cluster_sectors,
> +                s->cluster_sectors) & (~cluster_mask));
> +            ret = block_cache_get(bs, s->bitmap_cache, offset, (void **)&table);
> +            if (ret < 0) {
> +                goto fail;
> +            }
> +            if ((table[i / 8] & (1 << (i % 8))) == 0) {
> +                table[i / 8] |= (1 << (i % 8));

i is based on sector_num while table[] starts at offset, not sector 0.
The index expression i / 8 leads to out-of-bounds accesses.

I think you forgot to & (s->cluster_sectors - 1).

> +static coroutine_fn int add_cow_co_flush(BlockDriverState *bs)
> +{
> +    BDRVAddCowState *s = bs->opaque;
> +    int ret;
> +
> +    qemu_co_mutex_lock(&s->lock);
> +    if (s->bitmap_cache) {
> +        ret = block_cache_flush(bs, s->bitmap_cache);
> +        if (ret < 0) {
> +            return ret;
> +        }
> +    }
> +    ret = bdrv_flush(s->image_hd);

This is the wrong way around.  We must flush image_hd first so that
valid data is on disk.  Then we can flush bitmap_cache to mark the
clusters allocated.

Beyond explicit flushes you also need to make sure that image_hd is
flushed *before* bitmap_cache tables are written out (e.g. cache
eviction when the cache becomes full).  It seems this code is missing.

Also please use bdrv_co_flush() instead of bdrv_flush() in
add_cow_co_flush() since this is a coroutine function.

> diff --git a/block/block-cache.c b/block/block-cache.c
> index 3544691..4824632 100644
> --- a/block/block-cache.c
> +++ b/block/block-cache.c
> @@ -130,7 +130,7 @@ static int block_cache_entry_flush(BlockDriverState *bs, BlockCache *c, int i)
>      } else if (c->table_type == BLOCK_TABLE_L2) {
>          BLKDBG_EVENT(bs->file, BLKDBG_L2_UPDATE);
>      } else if (c->table_type == BLOCK_TABLE_BITMAP) {
> -        BLKDBG_EVENT(bs->file, BLKDBG_COW_WRITE);
> +        BLKDBG_EVENT(bs->file, BLKDBG_L2_LOAD);
>      }
>  
>      ret = bdrv_pwrite(bs->file, c->entries[i].offset,
> @@ -265,7 +265,7 @@ static int block_cache_do_get(BlockDriverState *bs, BlockCache *c,
>          if (c->table_type == BLOCK_TABLE_L2) {
>              BLKDBG_EVENT(bs->file, BLKDBG_L2_LOAD);
>          } else if (c->table_type == BLOCK_TABLE_BITMAP) {
> -            BLKDBG_EVENT(bs->file, BLKDBG_COW_READ);
> +            BLKDBG_EVENT(bs->file, BLKDBG_L2_LOAD);
>          }
>  
>          ret = bdrv_pread(bs->file, offset, c->entries[i].table,

I commented on this in the previous patch.  Please squash this fix into
the previous patch.

> diff --git a/block/qcow2.h b/block/qcow2.h
> index e7f6aec..a4e514b 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -98,6 +98,9 @@ typedef struct QCowSnapshot {
>      uint64_t vm_clock_nsec;
>  } QCowSnapshot;
>  
> +struct BlockCache;
> +typedef struct BlockCache BlockCache;
> +
>  typedef struct Qcow2UnknownHeaderExtension {
>      uint32_t magic;
>      uint32_t len;
> @@ -389,7 +392,4 @@ int qcow2_snapshot_load_tmp(BlockDriverState *bs, const char *snapshot_name);
>  void qcow2_free_snapshots(BlockDriverState *bs);
>  int qcow2_read_snapshots(BlockDriverState *bs);
>  
> -/* qcow2-cache.c functions */
> -
> -

More qcow2-cache.c move cleanups?  Please squash into the previous
patch.