On Tue, Sep 11, 2012 at 5:40 PM, Kevin Wolf <kw...@redhat.com> wrote:
> Am 10.08.2012 17:39, schrieb Dong Xu Wang:
>> add-cow file format core code. It use block-cache.c as cache code.
>>
>> Signed-off-by: Dong Xu Wang <wdon...@linux.vnet.ibm.com>
>> ---
>> block/Makefile.objs | 1 +
>> block/add-cow.c | 613
>> +++++++++++++++++++++++++++++++++++++++++++++++++++
>> block/add-cow.h | 85 +++++++
>> block_int.h | 2 +
>> 4 files changed, 701 insertions(+), 0 deletions(-)
>> create mode 100644 block/add-cow.c
>> create mode 100644 block/add-cow.h
>>
>> diff --git a/block/Makefile.objs b/block/Makefile.objs
>> index 23bdfc8..7ed5051 100644
>> --- a/block/Makefile.objs
>> +++ b/block/Makefile.objs
>> @@ -2,6 +2,7 @@ block-obj-y += raw.o cow.o qcow.o vdi.o vmdk.o cloop.o dmg.o
>> bochs.o vpc.o vvfat
>> block-obj-y += qcow2.o qcow2-refcount.o qcow2-cluster.o qcow2-snapshot.o
>> block-obj-y += qed.o qed-gencb.o qed-l2-cache.o qed-table.o qed-cluster.o
>> block-obj-y += qed-check.o
>> +block-obj-y += add-cow.o
>> block-obj-y += block-cache.o
>> block-obj-y += parallels.o nbd.o blkdebug.o sheepdog.o blkverify.o
>> block-obj-y += stream.o
>> diff --git a/block/add-cow.c b/block/add-cow.c
>> new file mode 100644
>> index 0000000..d4711d5
>> --- /dev/null
>> +++ b/block/add-cow.c
>> @@ -0,0 +1,613 @@
>> +/*
>> + * QEMU ADD-COW Disk Format
>> + *
>> + * Copyright IBM, Corp. 2012
>> + *
>> + * Authors:
>> + * Dong Xu Wang <wdon...@linux.vnet.ibm.com>
>> + *
>> + * This work is licensed under the terms of the GNU LGPL, version 2 or
>> later.
>> + * See the COPYING.LIB file in the top-level directory.
>> + *
>> + */
>> +
>> +#include "qemu-common.h"
>> +#include "block_int.h"
>> +#include "module.h"
>> +#include "add-cow.h"
>> +
>> +static void add_cow_header_le_to_cpu(const AddCowHeader *le, AddCowHeader
>> *cpu)
>> +{
>> + cpu->magic = le64_to_cpu(le->magic);
>> + cpu->version = le32_to_cpu(le->version);
>> +
>> + cpu->backing_filename_offset =
>> le32_to_cpu(le->backing_filename_offset);
>> + cpu->backing_filename_size =
>> le32_to_cpu(le->backing_filename_size);
>> +
>> + cpu->image_filename_offset =
>> le32_to_cpu(le->image_filename_offset);
>> + cpu->image_filename_size = le32_to_cpu(le->image_filename_size);
>> +
>> + cpu->features = le64_to_cpu(le->features);
>> + cpu->optional_features = le64_to_cpu(le->optional_features);
>> + cpu->header_pages_size = le32_to_cpu(le->header_pages_size);
>> +}
>> +
>> +static void add_cow_header_cpu_to_le(const AddCowHeader *cpu, AddCowHeader
>> *le)
>> +{
>> + le->magic = cpu_to_le64(cpu->magic);
>> + le->version = cpu_to_le32(cpu->version);
>> +
>> + le->backing_filename_offset =
>> cpu_to_le32(cpu->backing_filename_offset);
>> + le->backing_filename_size =
>> cpu_to_le32(cpu->backing_filename_size);
>> +
>> + le->image_filename_offset =
>> cpu_to_le32(cpu->image_filename_offset);
>> + le->image_filename_size = cpu_to_le32(cpu->image_filename_size);
>> +
>> + le->features = cpu_to_le64(cpu->features);
>> + le->optional_features = cpu_to_le64(cpu->optional_features);
>> + le->header_pages_size = cpu_to_le32(cpu->header_pages_size);
>> +}
>> +
>> +static int add_cow_probe(const uint8_t *buf, int buf_size, const char
>> *filename)
>> +{
>> + const AddCowHeader *header = (const AddCowHeader *)buf;
>> +
>> + if (le64_to_cpu(header->magic) == ADD_COW_MAGIC &&
>> + le32_to_cpu(header->version) == ADD_COW_VERSION) {
>> + return 100;
>> + } else {
>> + return 0;
>> + }
>> +}
>> +
>> +static int add_cow_create(const char *filename, QEMUOptionParameter
>> *options)
>> +{
>> + AddCowHeader header = {
>> + .magic = ADD_COW_MAGIC,
>> + .version = ADD_COW_VERSION,
>> + .features = 0,
>> + .optional_features = 0,
>> + .header_pages_size = ADD_COW_DEFAULT_PAGE_SIZE,
>> + };
>> + AddCowHeader le_header;
>> + int64_t image_len = 0;
>> + const char *backing_filename = NULL;
>> + const char *backing_fmt = NULL;
>> + const char *image_filename = NULL;
>> + const char *image_format = NULL;
>> + BlockDriverState *bs, *image_bs = NULL, *backing_bs = NULL;
>> + BlockDriver *drv = bdrv_find_format("add-cow");
>> + BDRVAddCowState s;
>> + int ret;
>> +
>> + while (options && options->name) {
>> + if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
>> + image_len = options->value.n;
>> + } else if (!strcmp(options->name, BLOCK_OPT_BACKING_FILE)) {
>> + backing_filename = options->value.s;
>> + } else if (!strcmp(options->name, BLOCK_OPT_BACKING_FMT)) {
>> + backing_fmt = options->value.s;
>> + } else if (!strcmp(options->name, BLOCK_OPT_IMAGE_FILE)) {
>> + image_filename = options->value.s;
>> + } else if (!strcmp(options->name, BLOCK_OPT_IMAGE_FORMAT)) {
>> + image_format = options->value.s;
>> + }
>> + options++;
>> + }
>> +
>> + if (backing_filename) {
>> + header.backing_filename_offset = sizeof(header)
>> + + sizeof(s.backing_file_format) + sizeof(s.image_file_format);
>> + header.backing_filename_size = strlen(backing_filename);
>> +
>> + if (!backing_fmt) {
>> + backing_bs = bdrv_new("image");
>> + ret = bdrv_open(backing_bs, backing_filename, BDRV_O_RDWR
>> + | BDRV_O_CACHE_WB, NULL);
>> + if (ret < 0) {
>> + return ret;
>> + }
>> + backing_fmt = bdrv_get_format_name(backing_bs);
>> + bdrv_delete(backing_bs);
>> + }
>> + } else {
>> + header.features |= ADD_COW_F_All_ALLOCATED;
>> + }
>> +
>> + if (image_filename) {
>> + header.image_filename_offset =
>> + sizeof(header) + sizeof(s.backing_file_format)
>> + + sizeof(s.image_file_format) +
>> header.backing_filename_size;
>> + header.image_filename_size = strlen(image_filename);
>> + } else {
>> + error_report("Error: image_file should be given.");
>> + return -EINVAL;
>> + }
>> +
>> + if (backing_filename && !strcmp(backing_filename, image_filename)) {
>> + error_report("Error: Trying to create an image with the "
>> + "same backing file name as the image file name");
>> + return -EINVAL;
>> + }
>> +
>> + if (!strcmp(filename, image_filename)) {
>> + error_report("Error: Trying to create an image with the "
>> + "same filename as the image file name");
>> + return -EINVAL;
>> + }
>> +
>> + if (header.image_filename_offset + header.image_filename_size
>> + > ADD_COW_PAGE_SIZE * ADD_COW_DEFAULT_PAGE_SIZE) {
>> + error_report("image_file name or backing_file name too long.");
>> + return -ENOSPC;
>> + }
>> +
>> + ret = bdrv_file_open(&image_bs, image_filename, BDRV_O_RDWR);
>> + if (ret < 0) {
>> + return ret;
>> + }
>> + bdrv_delete(image_bs);
>> +
>> + ret = bdrv_create_file(filename, NULL);
>> + if (ret < 0) {
>> + return ret;
>> + }
>> +
>> + ret = bdrv_file_open(&bs, filename, BDRV_O_RDWR);
>> + if (ret < 0) {
>> + return ret;
>> + }
>> + add_cow_header_cpu_to_le(&header, &le_header);
>> + ret = bdrv_pwrite(bs, 0, &le_header, sizeof(le_header));
>> + if (ret < 0) {
>> + bdrv_delete(bs);
>> + return ret;
>> + }
>> +
>> + ret = bdrv_pwrite(bs, sizeof(le_header), backing_fmt ? backing_fmt : "",
>> + backing_fmt ? strlen(backing_fmt) : 0);
>
> The spec requires zero padding, which you don't do here.
Okay.
>
>> + if (ret < 0) {
>> + bdrv_delete(bs);
>> + return ret;
>> + }
>> +
>> + ret = bdrv_pwrite(bs, sizeof(le_header) + sizeof(s.backing_file_format),
>> + image_format ? image_format : "raw",
>> + image_format ? strlen(image_format) : sizeof("raw"));
>
> And here.
Okay.
>
>> + if (ret < 0) {
>> + bdrv_delete(bs);
>> + return ret;
>> + }
>> +
>> + if (backing_filename) {
>> + ret = bdrv_pwrite(bs, header.backing_filename_offset,
>> + backing_filename, header.backing_filename_size);
>> + if (ret < 0) {
>> + bdrv_delete(bs);
>> + return ret;
>> + }
>> + }
>> +
>> + ret = bdrv_pwrite(bs, header.image_filename_offset,
>> + image_filename, header.image_filename_size);
>> + if (ret < 0) {
>> + bdrv_delete(bs);
>> + return ret;
>> + }
>> +
>> + ret = bdrv_open(bs, filename, BDRV_O_RDWR | BDRV_O_NO_FLUSH, drv);
>> + if (ret < 0) {
>> + bdrv_delete(bs);
>> + return ret;
>> + }
>> +
>> + ret = bdrv_truncate(bs, image_len);
>> + bdrv_delete(bs);
>> + return ret;
>> +}
>> +
>> +static int add_cow_open(BlockDriverState *bs, int flags)
>> +{
>> + char image_filename[ADD_COW_FILE_LEN];
>> + char tmp_name[ADD_COW_FILE_LEN];
>> + BlockDriver *image_drv = NULL;
>> + int ret;
>> + int sector_per_byte;
>> + BDRVAddCowState *s = bs->opaque;
>> + AddCowHeader le_header;
>> +
>> + ret = bdrv_pread(bs->file, 0, &le_header, sizeof(le_header));
>> + if (ret != sizeof(s->header)) {
>
> if (ret < 0) would be more consistent with the rest of the code.
>
Okay.
>> + goto fail;
>> + }
>> +
>> + add_cow_header_le_to_cpu(&le_header, &s->header);
>> +
>> + if (le64_to_cpu(s->header.magic) != ADD_COW_MAGIC) {
>
> Isn't this one endianess conversion too much? s->header is already LE.
>
> Did you test add-cow on a big endian host?
My fault, will correct it in next version.
>
>> + ret = -EINVAL;
>> + goto fail;
>> + }
>> +
>> + if (s->header.version != ADD_COW_VERSION) {
>> + char version[64];
>> + snprintf(version, sizeof(version), "ADD-COW version %d",
>> + s->header.version);
>> + qerror_report(QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
>> + bs->device_name, "add-cow", version);
>> + ret = -ENOTSUP;
>> + goto fail;
>> + }
>> +
>> + if (s->header.features & ~ADD_COW_FEATURE_MASK) {
>> + char buf[64];
>> + snprintf(buf, sizeof(buf), "%" PRIx64,
>> + s->header.features & ~ADD_COW_FEATURE_MASK);
>
> This message is a bit terse, most users will be confused with an error
> message that only consists of a hex number. Maybe better "Feature flags:
> %" PRIx64.
>
Okay.
>> + qerror_report(QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
>> + bs->device_name, "add-cow", buf);
>> + return -ENOTSUP;
>> + }
>> +
>> + if ((s->header.features & ADD_COW_F_All_ALLOCATED) == 0) {
>> + ret = bdrv_read_string(bs->file, sizeof(s->header),
>> + sizeof(s->backing_file_format) - 1, s->backing_file_format,
>> + sizeof(s->backing_file_format));
>> + if (ret < 0) {
>> + goto fail;
>> + }
>> + }
>
> Would be great if this was not only read into memory, but actually
> used... It must end up in bs->backing_format in order take effect.
>
>> +
>> + ret = bdrv_read_string(bs->file,
>> + sizeof(s->header) + sizeof(s->image_file_format),
>> + sizeof(s->image_file_format) - 1, s->image_file_format,
>> + sizeof(s->image_file_format));
>> + if (ret < 0) {
>> + goto fail;
>> + }
>
> This one is unused, too.
>
Okay.
>> +
>> + if ((s->header.features & ADD_COW_F_All_ALLOCATED) == 0) {
>> + ret = bdrv_read_string(bs->file, s->header.backing_filename_offset,
>> + s->header.backing_filename_size, bs->backing_file,
>> + sizeof(bs->backing_file));
>> + if (ret < 0) {
>> + goto fail;
>> + }
>> + }
>> +
>> + ret = bdrv_read_string(bs->file, s->header.image_filename_offset,
>> + s->header.image_filename_size, tmp_name,
>> + sizeof(tmp_name));
>> + if (ret < 0) {
>> + goto fail;
>> + }
>> +
>> + s->image_hd = bdrv_new("");
>> + if (path_has_protocol(image_filename)) {
>> + pstrcpy(image_filename, sizeof(image_filename), tmp_name);
>> + } else {
>> + path_combine(image_filename, sizeof(image_filename),
>> + bs->filename, tmp_name);
>> + }
>> +
>> + ret = bdrv_open(s->image_hd, image_filename, flags, image_drv);
>
> image_drv is always NULL.
>
>> + if (ret < 0) {
>> + bdrv_delete(s->image_hd);
>> + goto fail;
>> + }
>> +
>> + bs->total_sectors = bdrv_getlength(s->image_hd) >> 9;
>> + s->cluster_size = ADD_COW_CLUSTER_SIZE;
>> + sector_per_byte = SECTORS_PER_CLUSTER * 8;
>> + s->bitmap_size =
>> + (bs->total_sectors + sector_per_byte - 1) / sector_per_byte;
>> + s->bitmap_cache =
>> + block_cache_create(bs, ADD_COW_CACHE_SIZE,
>> ADD_COW_CACHE_ENTRY_SIZE);
>> +
>> + qemu_co_mutex_init(&s->lock);
>> + return 0;
>> +fail:
>> + if (s->bitmap_cache) {
>> + block_cache_destroy(bs, s->bitmap_cache, BLOCK_TABLE_BITMAP);
>> + }
>> + return ret;
>> +}
>> +
>> +static void add_cow_close(BlockDriverState *bs)
>> +{
>> + BDRVAddCowState *s = bs->opaque;
>> + block_cache_destroy(bs, s->bitmap_cache, BLOCK_TABLE_BITMAP);
>> + bdrv_delete(s->image_hd);
>> +}
>> +
>> +static bool is_allocated(BlockDriverState *bs, int64_t sector_num)
>> +{
>> + BDRVAddCowState *s = bs->opaque;
>> + BlockCache *c = s->bitmap_cache;
>> + int64_t cluster_num = sector_num / SECTORS_PER_CLUSTER;
>> + uint8_t *table = NULL;
>> + uint64_t offset = ADD_COW_PAGE_SIZE * s->header.header_pages_size
>> + + (offset_in_bitmap(sector_num) & (~(c->entry_size - 1)));
>> + int ret = block_cache_get(bs, s->bitmap_cache, offset,
>> + (void **)&table, BLOCK_TABLE_BITMAP, ADD_COW_CACHE_ENTRY_SIZE);
>
> No matching block_cache_put?
>
>> +
>> + if (ret < 0) {
>> + return ret;
>> + }
>> + return table[cluster_num / 8 % ADD_COW_CACHE_ENTRY_SIZE]
>> + & (1 << (cluster_num % 8));
>> +}
>> +
>> +static coroutine_fn int add_cow_is_allocated(BlockDriverState *bs,
>> + int64_t sector_num, int nb_sectors, int *num_same)
>> +{
>> + BDRVAddCowState *s = bs->opaque;
>> + int changed;
>> +
>> + if (nb_sectors == 0) {
>> + *num_same = 0;
>> + return 0;
>> + }
>> +
>> + if (s->header.features & ADD_COW_F_All_ALLOCATED) {
>> + *num_same = nb_sectors - 1;
>
> Why - 1?
>
>> + return 1;
>> + }
>> + changed = is_allocated(bs, sector_num);
>> +
>> + for (*num_same = 1; *num_same < nb_sectors; (*num_same)++) {
>> + if (is_allocated(bs, sector_num + *num_same) != changed) {
>> + break;
>> + }
>> + }
>> + return changed;
>> +}
>> +
>> +static int add_cow_backing_read(BlockDriverState *bs, QEMUIOVector *qiov,
>> + int64_t sector_num, int nb_sectors)
>> +{
>> + int n1;
>> + if ((sector_num + nb_sectors) <= bs->total_sectors) {
>> + return nb_sectors;
>> + }
>> + if (sector_num >= bs->total_sectors) {
>> + n1 = 0;
>> + } else {
>> + n1 = bs->total_sectors - sector_num;
>> + }
>> +
>> + qemu_iovec_memset(qiov, BDRV_SECTOR_SIZE * n1,
>> + 0, BDRV_SECTOR_SIZE * (nb_sectors - n1));
>> +
>> + return n1;
>> +}
>> +
>> +static coroutine_fn int add_cow_co_readv(BlockDriverState *bs,
>> + int64_t sector_num, int remaining_sectors, QEMUIOVector *qiov)
>> +{
>> + BDRVAddCowState *s = bs->opaque;
>> + int cur_nr_sectors;
>> + uint64_t bytes_done = 0;
>> + QEMUIOVector hd_qiov;
>> + int n, n1, ret = 0;
>> +
>> + qemu_iovec_init(&hd_qiov, qiov->niov);
>> + qemu_co_mutex_lock(&s->lock);
>> + while (remaining_sectors != 0) {
>> + cur_nr_sectors = remaining_sectors;
>> + if (add_cow_is_allocated(bs, sector_num, cur_nr_sectors, &n)) {
>> + cur_nr_sectors = n;
>
> One of n and cur_nr_sectors is redundant.
Okay.
>
>> + qemu_iovec_reset(&hd_qiov);
>> + qemu_iovec_concat(&hd_qiov, qiov, bytes_done,
>> + cur_nr_sectors * BDRV_SECTOR_SIZE);
>> + qemu_co_mutex_unlock(&s->lock);
>> + ret = bdrv_co_readv(s->image_hd, sector_num, n, &hd_qiov);
>> + qemu_co_mutex_lock(&s->lock);
>> + if (ret < 0) {
>> + goto fail;
>> + }
>> + } else {
>> + cur_nr_sectors = n;
>> + if (bs->backing_hd) {
>> + qemu_iovec_reset(&hd_qiov);
>> + qemu_iovec_concat(&hd_qiov, qiov, bytes_done,
>> + cur_nr_sectors * BDRV_SECTOR_SIZE);
>> + n1 = add_cow_backing_read(bs->backing_hd, &hd_qiov,
>> + sector_num, cur_nr_sectors);
>> + if (n1 > 0) {
>> + qemu_co_mutex_unlock(&s->lock);
>> + ret = bdrv_co_readv(bs->backing_hd, sector_num,
>> + n, &hd_qiov);
>> + qemu_co_mutex_lock(&s->lock);
>> + if (ret < 0) {
>> + goto fail;
>> + }
>> + }
>> + } else {
>> + qemu_iovec_memset(&hd_qiov, 0, 0,
>> + BDRV_SECTOR_SIZE * cur_nr_sectors);
>> + }
>> + }
>> + remaining_sectors -= cur_nr_sectors;
>> + sector_num += cur_nr_sectors;
>> + bytes_done += cur_nr_sectors * BDRV_SECTOR_SIZE;
>> + }
>> +fail:
>> + qemu_co_mutex_unlock(&s->lock);
>> + qemu_iovec_destroy(&hd_qiov);
>> + return ret;
>> +}
>> +
>> +static int coroutine_fn copy_sectors(BlockDriverState *bs,
>> + int n_start, int n_end)
>> +{
>> + BDRVAddCowState *s = bs->opaque;
>> + QEMUIOVector qiov;
>> + struct iovec iov;
>> + int n, ret;
>> +
>> + n = n_end - n_start;
>> + if (n <= 0) {
>> + return 0;
>> + }
>> +
>> + iov.iov_len = n * BDRV_SECTOR_SIZE;
>> + iov.iov_base = qemu_blockalign(bs, iov.iov_len);
>> +
>> + qemu_iovec_init_external(&qiov, &iov, 1);
>> +
>> + ret = bdrv_co_readv(bs->backing_hd, n_start, n, &qiov);
>> + if (ret < 0) {
>> + goto out;
>> + }
>> + ret = bdrv_co_writev(s->image_hd, n_start, n, &qiov);
>> + if (ret < 0) {
>> + goto out;
>> + }
>> +
>> + ret = 0;
>> +out:
>> + qemu_vfree(iov.iov_base);
>> + return ret;
>> +}
>> +
>> +static coroutine_fn int add_cow_co_writev(BlockDriverState *bs,
>> + int64_t sector_num, int remaining_sectors, QEMUIOVector *qiov)
>> +{
>> + BDRVAddCowState *s = bs->opaque;
>> + BlockCache *c = s->bitmap_cache;
>> + int ret = 0, i;
>> + QEMUIOVector hd_qiov;
>> + uint8_t *table;
>> + uint64_t offset;
>> +
>> + 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);
>> +
>> + if (ret < 0) {
>> + goto fail;
>> + }
>> + if ((s->header.features & ADD_COW_F_All_ALLOCATED) == 0) {
>> + /* Copy content of unmodified sectors */
>> + if (!is_cluster_head(sector_num) && !is_allocated(bs, sector_num)) {
>> + ret = copy_sectors(bs, sector_num & ~(SECTORS_PER_CLUSTER - 1),
>> + sector_num);
>> + if (ret < 0) {
>> + goto fail;
>> + }
>> + }
>> +
>> + if (!is_cluster_tail(sector_num + remaining_sectors - 1)
>> + && !is_allocated(bs, sector_num + remaining_sectors - 1)) {
>> + ret = copy_sectors(bs, sector_num + remaining_sectors,
>> + ((sector_num + remaining_sectors) | (SECTORS_PER_CLUSTER -
>> 1)) + 1);
>> + if (ret < 0) {
>> + goto fail;
>> + }
>> + }
>> +
>> + for (i = sector_num / SECTORS_PER_CLUSTER;
>> + i <= (sector_num + remaining_sectors - 1) / SECTORS_PER_CLUSTER;
>> + i++) {
>> + offset = ADD_COW_PAGE_SIZE * s->header.header_pages_size
>> + + (offset_in_bitmap(i * SECTORS_PER_CLUSTER) &
>> (~(c->entry_size - 1)));
>
> The maths in this loop looks a bit confusing, but I think it's correct.
>
>> + ret = block_cache_get(bs, s->bitmap_cache, offset,
>> + (void **)&table, BLOCK_TABLE_BITMAP,
>> ADD_COW_CACHE_ENTRY_SIZE);
>> + if (ret < 0) {
>> + goto fail;
>> + }
>> + if ((table[i / 8] & (1 << (i % 8))) == 0) {
>> + table[i / 8] |= (1 << (i % 8));
>> + block_cache_entry_mark_dirty(s->bitmap_cache, table);
>> + }
>
> Missing block_cache_put again?
>
>> + }
>> + }
>> + ret = 0;
>> +fail:
>> + qemu_co_mutex_unlock(&s->lock);
>> + qemu_iovec_destroy(&hd_qiov);
>> + return ret;
>> +}
>> +
>> +static int bdrv_add_cow_truncate(BlockDriverState *bs, int64_t size)
>> +{
>> + BDRVAddCowState *s = bs->opaque;
>> + int sector_per_byte = SECTORS_PER_CLUSTER * 8;
>> + int ret;
>> + uint32_t bitmap_pos = s->header.header_pages_size * ADD_COW_PAGE_SIZE;
>> + int64_t bitmap_size =
>> + (size / BDRV_SECTOR_SIZE + sector_per_byte - 1) / sector_per_byte;
>> + bitmap_size = (bitmap_size + ADD_COW_CACHE_ENTRY_SIZE - 1)
>> + & (~(ADD_COW_CACHE_ENTRY_SIZE - 1));
>> +
>> + ret = bdrv_truncate(bs->file, bitmap_pos + bitmap_size);
>> + if (ret < 0) {
>> + return ret;
>> + }
>> + return 0;
>> +}
>
> So you don't truncate s->image_file? Does this work?
s->image_file should be truncated? Image file can have a larger virtual size
than backing_file, my understanding is we should not truncate image file.
>
>> +
>> +static coroutine_fn int add_cow_co_flush(BlockDriverState *bs)
>> +{
>> + BDRVAddCowState *s = bs->opaque;
>> + int ret;
>> +
>> + qemu_co_mutex_lock(&s->lock);
>> + ret = block_cache_flush(bs, s->bitmap_cache, BLOCK_TABLE_BITMAP,
>> + ADD_COW_CACHE_ENTRY_SIZE);
>> + qemu_co_mutex_unlock(&s->lock);
>> + return ret;
>> +}
>
> What about flushing s->image_file?
>
>> +
>> +static QEMUOptionParameter add_cow_create_options[] = {
>> + {
>> + .name = BLOCK_OPT_SIZE,
>> + .type = OPT_SIZE,
>> + .help = "Virtual disk size"
>> + },
>> + {
>> + .name = BLOCK_OPT_BACKING_FILE,
>> + .type = OPT_STRING,
>> + .help = "File name of a base image"
>> + },
>> + {
>> + .name = BLOCK_OPT_BACKING_FMT,
>> + .type = OPT_STRING,
>> + .help = "Image format of the base image"
>> + },
>> + {
>> + .name = BLOCK_OPT_IMAGE_FILE,
>> + .type = OPT_STRING,
>> + .help = "File name of a image file"
>> + },
>> + {
>> + .name = BLOCK_OPT_IMAGE_FORMAT,
>> + .type = OPT_STRING,
>> + .help = "Image format of the image file"
>> + },
>> + { NULL }
>> +};
>> +
>> +static BlockDriver bdrv_add_cow = {
>> + .format_name = "add-cow",
>> + .instance_size = sizeof(BDRVAddCowState),
>> + .bdrv_probe = add_cow_probe,
>> + .bdrv_open = add_cow_open,
>> + .bdrv_close = add_cow_close,
>> + .bdrv_create = add_cow_create,
>> + .bdrv_co_readv = add_cow_co_readv,
>> + .bdrv_co_writev = add_cow_co_writev,
>> + .bdrv_truncate = bdrv_add_cow_truncate,
>> + .bdrv_co_is_allocated = add_cow_is_allocated,
>> +
>> + .create_options = add_cow_create_options,
>> + .bdrv_co_flush_to_os = add_cow_co_flush,
>> +};
>> +
>> +static void bdrv_add_cow_init(void)
>> +{
>> + bdrv_register(&bdrv_add_cow);
>> +}
>> +
>> +block_init(bdrv_add_cow_init);
>> diff --git a/block/add-cow.h b/block/add-cow.h
>> new file mode 100644
>> index 0000000..f058376
>> --- /dev/null
>> +++ b/block/add-cow.h
>> @@ -0,0 +1,85 @@
>> +/*
>> + * QEMU ADD-COW Disk Format
>> + *
>> + * Copyright IBM, Corp. 2012
>> + *
>> + * Authors:
>> + * Dong Xu Wang <wdon...@linux.vnet.ibm.com>
>> + *
>> + * This work is licensed under the terms of the GNU LGPL, version 2 or
>> later.
>> + * See the COPYING.LIB file in the top-level directory.
>> + *
>> + */
>> +
>> +#ifndef BLOCK_ADD_COW_H
>> +#define BLOCK_ADD_COW_H
>> +#include "block-cache.h"
>> +
>> +enum {
>> + ADD_COW_F_All_ALLOCATED = 0X01,
>> + ADD_COW_FEATURE_MASK = ADD_COW_F_All_ALLOCATED,
>> +
>> + ADD_COW_MAGIC = (((uint64_t)'A' << 56) | ((uint64_t)'D' << 48) | \
>> + ((uint64_t)'D' << 40) | ((uint64_t)'_' << 32) | \
>> + ((uint64_t)'C' << 24) | ((uint64_t)'O' << 16) | \
>> + ((uint64_t)'W' << 8) | 0xFF),
>> + ADD_COW_VERSION = 1,
>> + ADD_COW_FILE_LEN = 1024,
>> + ADD_COW_CACHE_SIZE = 16,
>> + ADD_COW_CACHE_ENTRY_SIZE = 65536,
>> + ADD_COW_CLUSTER_SIZE = 65536,
>> + SECTORS_PER_CLUSTER = (ADD_COW_CLUSTER_SIZE / BDRV_SECTOR_SIZE),
>> + ADD_COW_PAGE_SIZE = 4096,
>> + ADD_COW_DEFAULT_PAGE_SIZE = 1,
>> +};
>> +
>> +typedef struct AddCowHeader {
>> + uint64_t magic;
>> + uint32_t version;
>> +
>> + uint32_t backing_filename_offset;
>> + uint32_t backing_filename_size;
>> +
>> + uint32_t image_filename_offset;
>> + uint32_t image_filename_size;
>> +
>> + uint64_t features;
>> + uint64_t optional_features;
>> + uint32_t header_pages_size;
>> +} QEMU_PACKED AddCowHeader;
>
> Why aren't backing/image_file_format part of the header here? They are
> in the spec. It would also simplify some offset calculation code.
>
Anthony said "It's far better to shrink the size of the header and use
an offset/len
pointer to the backing file string. Limiting backing files to 1023 is
unacceptable"
http://lists.gnu.org/archive/html/qemu-devel/2012-05/msg04110.html
So I use offset and length instead of using string directly.
>> +
>> +typedef struct BDRVAddCowState {
>> + BlockDriverState *image_hd;
>> + CoMutex lock;
>> + int cluster_size;
>> + BlockCache *bitmap_cache;
>> + uint64_t bitmap_size;
>> + AddCowHeader header;
>> + char backing_file_format[16];
>> + char image_file_format[16];
>> +} BDRVAddCowState;
>> +
>> +/* Convert sector_num to offset in bitmap */
>> +static inline int64_t offset_in_bitmap(int64_t sector_num)
>> +{
>> + int64_t cluster_num = sector_num / SECTORS_PER_CLUSTER;
>> + return cluster_num / 8;
>> +}
>> +
>> +static inline bool is_cluster_head(int64_t sector_num)
>> +{
>> + return sector_num % SECTORS_PER_CLUSTER == 0;
>> +}
>> +
>> +static inline bool is_cluster_tail(int64_t sector_num)
>> +{
>> + return (sector_num + 1) % SECTORS_PER_CLUSTER == 0;
>> +}
>> +
>> +BlockCache *add_cow_cache_create(BlockDriverState *bs, int num_tables);
>> +int add_cow_cache_destroy(BlockDriverState *bs, BlockCache *c);
>> +void add_cow_cache_entry_mark_dirty(BlockCache *c, void *table);
>> +int add_cow_cache_get(BlockDriverState *bs, BlockCache *c, uint64_t offset,
>> + void **table);
>> +int add_cow_cache_flush(BlockDriverState *bs, BlockCache *c);
>
> These functions don't really exist any more, do they?
Right, sorry.
>
> Kevin
>
Thank you, Kevin.
