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.
