On 2013/4/18 18:03, Stefan Hajnoczi wrote:
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.
okay.
+ 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.
Okay. will fix.
+ 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.
Okay.
+ 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?).
Okay.
+ } + + 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.
Okay.
+ 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.
Okay.
+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.
Okay, will refer qcow2 related code.
+ + 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.
Okay.
+ + 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).
Okay.
+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.
Okay.
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.
Okay.
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.
Okay.
