From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([208.118.235.92]:49353)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1S4xk7-0002B5-Ke
	for qemu-devel@nongnu.org; Tue, 06 Mar 2012 12:00:32 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1S4xjz-0005Ji-TW
	for qemu-devel@nongnu.org; Tue, 06 Mar 2012 12:00:07 -0500
Received: from mail-lpp01m010-f45.google.com ([209.85.215.45]:55947)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1S4xjz-0005IP-An
	for qemu-devel@nongnu.org; Tue, 06 Mar 2012 11:59:59 -0500
Received: by lahe6 with SMTP id e6so6926985lah.4
	for <qemu-devel@nongnu.org>; Tue, 06 Mar 2012 08:59:56 -0800 (PST)
MIME-Version: 1.0
In-Reply-To: <1330570610-8523-1-git-send-email-wdongxu@linux.vnet.ibm.com>
References: <1330570610-8523-1-git-send-email-wdongxu@linux.vnet.ibm.com>
Date: Tue, 6 Mar 2012 16:59:56 +0000
Message-ID: <CAJSP0QXoNic1gyj2TTDP1urJVEHFay5V5u-vGG2B8xADiRfj3w@mail.gmail.com>
From: Stefan Hajnoczi <stefanha@gmail.com>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable
Subject: Re: [Qemu-devel] [PATCH 1/2 v7] block: add-cow file format
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
Cc: Kevin Wolf <kwolf@redhat.com>, Marcelo Tosatti <mtosatti@redhat.com>, qemu-devel@nongnu.org, Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>

On Thu, Mar 1, 2012 at 2:56 AM, Dong Xu Wang <wdongxu@linux.vnet.ibm.com> w=
rote:
> diff --git a/block/add-cow-cache.c b/block/add-cow-cache.c
> new file mode 100644
> index 0000000..6be02ff
> --- /dev/null
> +++ b/block/add-cow-cache.c
> @@ -0,0 +1,171 @@
> +/*
> + * Cache For QEMU ADD-COW Disk Format
> + *
> + * Copyright IBM, Corp. 2012
> + *
> + * Authors:
> + * =A0Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
> + * This work is licensed under the terms of the GNU LGPL, version 2 or l=
ater.
> + * See the COPYING.LIB file in the top-level directory.
> + *
> + */
> +
> +#include "block_int.h"
> +#include "qemu-common.h"
> +#include "add-cow.h"

This patch is missing add-cow.h.

> diff --git a/block/add-cow.c b/block/add-cow.c
> new file mode 100644
> index 0000000..6897a52
> --- /dev/null
> +++ b/block/add-cow.c
> @@ -0,0 +1,402 @@
> +/*
> + * QEMU ADD-COW Disk Format
> + *
> + * Copyright IBM, Corp. 2012
> + *
> + * Authors:
> + * =A0Dong Xu Wang <wdongxu@linux.vnet.ibm.com>
> + * This work is licensed under the terms of the GNU LGPL, version 2 or l=
ater.
> + * 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 int add_cow_probe(const uint8_t *buf, int buf_size, const char *f=
ilename)
> +{
> + =A0 =A0const AddCowHeader *header =3D (const void *)buf;

Minor coding style comment: please cast to const AddCowHeader* instead of v=
oid.

> +
> + =A0 =A0if (be64_to_cpu(header->magic) =3D=3D ADD_COW_MAGIC &&
> + =A0 =A0 =A0 =A0be32_to_cpu(header->version) =3D=3D ADD_COW_VERSION) {
> + =A0 =A0 =A0 =A0return 100;
> + =A0 =A0} else {
> + =A0 =A0 =A0 =A0return 0;
> + =A0 =A0}
> +}
> +
> +static int add_cow_open(BlockDriverState *bs, int flags)
> +{
> + =A0 =A0AddCowHeader =A0 =A0 =A0 =A0header;
> + =A0 =A0char =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0image_filename[ADD_COW_FILE_=
LEN];
> + =A0 =A0BlockDriver =A0 =A0 =A0 =A0 *image_drv =3D NULL;
> + =A0 =A0int =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ret;
> + =A0 =A0BDRVAddCowState =A0 =A0 *s =3D bs->opaque;
> +
> + =A0 =A0ret =3D bdrv_pread(bs->file, 0, &header, sizeof(header));
> + =A0 =A0if (ret !=3D sizeof(header)) {
> + =A0 =A0 =A0 =A0goto fail;
> + =A0 =A0}
> +
> + =A0 =A0if (be64_to_cpu(header.magic) !=3D ADD_COW_MAGIC) {
> + =A0 =A0 =A0 =A0ret =3D -EINVAL;
> + =A0 =A0 =A0 =A0goto fail;
> + =A0 =A0}
> + =A0 =A0if (be32_to_cpu(header.version) !=3D ADD_COW_VERSION) {
> + =A0 =A0 =A0 =A0char version[64];
> + =A0 =A0 =A0 =A0snprintf(version, sizeof(version), "ADD-COW version %d",=
 header.version);

be32_to_cpu(header.version)

> + =A0 =A0 =A0 =A0qerror_report(QERR_UNKNOWN_BLOCK_FORMAT_FEATURE,
> + =A0 =A0 =A0 =A0 =A0 =A0bs->device_name, "add-cow", version);
> + =A0 =A0 =A0 =A0ret =3D -ENOTSUP;
> + =A0 =A0 =A0 =A0goto fail;
> + =A0 =A0}
> +
> + =A0 =A0QEMU_BUILD_BUG_ON(sizeof(bs->backing_file) !=3D sizeof(header.ba=
cking_file));
> + =A0 =A0strncpy(bs->backing_file, header.backing_file,
> + =A0 =A0 =A0 =A0 =A0 =A0sizeof(bs->backing_file));

Please use pstrcpy() - it always NUL-terminates the string.  strncpy()
only stores a '\0' when the source string is *shorter than* max
length, so a max length string results in bs->backing_file missing the
'\0'.

> +
> + =A0 =A0if (header.image_file[0] =3D=3D '\0') {
> + =A0 =A0 =A0 =A0ret =3D -ENOENT;
> + =A0 =A0 =A0 =A0goto fail;
> + =A0 =A0}
> + =A0 =A0s->image_hd =3D bdrv_new("");
> + =A0 =A0if (path_has_protocol(header.image_file)) {

Please safely copy in header.image_file before treating it as a
NUL-terminated string.

> + =A0 =A0 =A0 =A0strncpy(image_filename, header.image_file, sizeof(image_=
filename));
> + =A0 =A0} else {
> + =A0 =A0 =A0 =A0path_combine(image_filename, sizeof(image_filename),
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 bs->filename, header.image_file=
);
> + =A0 =A0}
> +
> + =A0 =A0image_drv =3D bdrv_find_format("raw");
> + =A0 =A0ret =3D bdrv_open(s->image_hd, image_filename, flags, image_drv)=
;
> + =A0 =A0if (ret < 0) {
> + =A0 =A0 =A0 =A0bdrv_delete(s->image_hd);
> + =A0 =A0 =A0 =A0goto fail;
> + =A0 =A0}
> + =A0 =A0bs->total_sectors =3D s->image_hd->total_sectors;
> + =A0 =A0s->cluster_size =3D ADD_COW_CLUSTER_SIZE;
> + =A0 =A0s->bitmap_cache =3D add_cow_cache_create(bs, ADD_COW_CACHE_SIZE)=
;
> + =A0 =A0qemu_co_mutex_init(&s->lock);
> + =A0 =A0return 0;
> + fail:
> + =A0 =A0return ret;
> +}
> +
> +static inline bool is_bit_set(BlockDriverState *bs, int64_t bitnum)
> +{
> + =A0 =A0BDRVAddCowState *s =3D bs->opaque;
> + =A0 =A0uint64_t offset =3D bitnum >> 3;
> + =A0 =A0uint8_t *bitmap;
> + =A0 =A0int ret =3D add_cow_cache_get(bs, s->bitmap_cache,
> + =A0 =A0 =A0 =A0offset & ~(ADD_COW_CLUSTER_SIZE - 1), (void **)&bitmap);

(void **) should not be necessary.

> + =A0 =A0if (ret < 0) {
> + =A0 =A0 =A0 =A0abort();
> + =A0 =A0}
> +
> + =A0 =A0return *(bitmap + (offset & (ADD_COW_CLUSTER_SIZE - 1))) & (1 <<=
 (bitnum % 8));

The cluster concept confuses me.  Normally the point of a cluster is
to reduce metadata size, so you would not index using bitnum % 8.

> +}
> +
> +static coroutine_fn int add_cow_is_allocated(BlockDriverState *bs,
> + =A0 =A0 =A0 =A0int64_t sector_num, int nb_sectors, int *num_same)
> +{
> + =A0 =A0int changed;
> +
> + =A0 =A0if (nb_sectors =3D=3D 0) {
> + =A0 =A0 =A0 =A0*num_same =3D nb_sectors;
> + =A0 =A0 =A0 =A0return 0;
> + =A0 =A0}
> +
> + =A0 =A0changed =3D is_bit_set(bs, sector_num);
> + =A0 =A0for (*num_same =3D 1; *num_same < nb_sectors; (*num_same)++) {
> + =A0 =A0 =A0 =A0if (is_bit_set(bs, sector_num + *num_same) !=3D changed)=
 {
> + =A0 =A0 =A0 =A0 =A0 =A0break;
> + =A0 =A0 =A0 =A0}
> + =A0 =A0}
> +
> + =A0 =A0return changed;
> +}
> +
> +static int add_cow_update_bitmap(BlockDriverState *bs, int64_t sector_nu=
m,
> + =A0 =A0 =A0 =A0int nb_sectors)
> +{
> + =A0 =A0BDRVAddCowState *s =3D bs->opaque;
> + =A0 =A0uint8_t *bitmap;
> +
> + =A0 =A0int i, ret =3D 0;
> + =A0 =A0for (i =3D 0; i < nb_sectors; i++) {
> + =A0 =A0 =A0 =A0int ret =3D add_cow_cache_get(bs, s->bitmap_cache,
> + =A0 =A0 =A0 =A0 =A0 =A0(sector_num + i) / 8 & ~(ADD_COW_CLUSTER_SIZE - =
1), (void **)&bitmap);
> + =A0 =A0 =A0 =A0if (ret < 0) {
> + =A0 =A0 =A0 =A0 =A0 =A0abort();
> + =A0 =A0 =A0 =A0}
> + =A0 =A0 =A0 =A0*(bitmap + ((sector_num + i) / 8 & (ADD_COW_CLUSTER_SIZE=
 - 1))) |=3D
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0(1 << ((sector_num + i) % 8));
> + =A0 =A0 =A0 =A0add_cow_cache_entry_mark_dirty(s->bitmap_cache, bitmap);
> +
> + =A0 =A0}
> + =A0 =A0ret =3D add_cow_cache_flush(bs, s->bitmap_cache);

Data must be flushed to the cow file before writing metadata updates,
otherwise a crash in between could result in zero sectors instead of
backing file sectors.

> +static int add_cow_backing_read(BlockDriverState *bs, QEMUIOVector *qiov=
,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0int64_t sector_num, int nb_sectors)

This function name is confusing because we don't actually perform a read he=
re.

I'm also not sure why we need to handle the case where a request spans
the end of the device.  bdrv_check_byte_request, called from
bdrv_co_do_readv() should prevent such requests?

> +{
> + =A0 =A0int n1;
> + =A0 =A0if ((sector_num + nb_sectors) <=3D bs->total_sectors) {
> + =A0 =A0 =A0 =A0return nb_sectors;
> + =A0 =A0}
> + =A0 =A0if (sector_num >=3D bs->total_sectors) {
> + =A0 =A0 =A0 =A0n1 =3D 0;
> + =A0 =A0} else {
> + =A0 =A0 =A0 =A0n1 =3D bs->total_sectors - sector_num;
> + =A0 =A0}
> +
> + =A0 =A0qemu_iovec_memset_skip(qiov, 0, BDRV_SECTOR_SIZE * (nb_sectors -=
 n1),
> + =A0 =A0 =A0 =A0 =A0 =A0BDRV_SECTOR_SIZE * n1);
> + =A0 =A0return n1;
> +}
> +
> +static coroutine_fn int add_cow_co_readv(BlockDriverState *bs, int64_t s=
ector_num,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 int remaining_sectors, =
QEMUIOVector *qiov)
> +{
> + =A0 =A0BDRVAddCowState *s =3D bs->opaque;
> + =A0 =A0int cur_nr_sectors;
> + =A0 =A0uint64_t bytes_done =3D 0;
> + =A0 =A0QEMUIOVector hd_qiov;
> + =A0 =A0int n, n1, ret =3D 0;
> +
> + =A0 =A0qemu_iovec_init(&hd_qiov, qiov->niov);
> + =A0 =A0qemu_co_mutex_lock(&s->lock);
> + =A0 =A0while (remaining_sectors !=3D 0) {
> + =A0 =A0 =A0 =A0cur_nr_sectors =3D remaining_sectors;
> + =A0 =A0 =A0 =A0if (add_cow_is_allocated(bs, sector_num, cur_nr_sectors,=
 &n)) {
> + =A0 =A0 =A0 =A0 =A0 =A0cur_nr_sectors =3D n;
> + =A0 =A0 =A0 =A0 =A0 =A0qemu_iovec_reset(&hd_qiov);
> + =A0 =A0 =A0 =A0 =A0 =A0qemu_iovec_copy(&hd_qiov, qiov, bytes_done,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0cur_nr_sectors *=
 BDRV_SECTOR_SIZE);
> + =A0 =A0 =A0 =A0 =A0 =A0ret =3D bdrv_co_readv(s->image_hd, sector_num, n=
, &hd_qiov);

During the read it seems safe to unlock s->lock.  We need to acquire
it again immediately afterwards, but it allows other requests to
access the cow cache in the meantime.

> + =A0 =A0 =A0 =A0 =A0 =A0if (ret < 0) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0goto fail;
> + =A0 =A0 =A0 =A0 =A0 =A0}
> + =A0 =A0 =A0 =A0} else {
> + =A0 =A0 =A0 =A0 =A0 =A0cur_nr_sectors =3D n;
> + =A0 =A0 =A0 =A0 =A0 =A0if (bs->backing_hd) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0n1 =3D add_cow_backing_read(bs->backing_=
hd, &hd_qiov,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0sector_num, cur_nr_sectors);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (n1 > 0) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0qemu_iovec_reset(&hd_qiov);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0qemu_iovec_copy(&hd_qiov, qiov, =
bytes_done,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0cur_nr_s=
ectors * BDRV_SECTOR_SIZE);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ret =3D bdrv_co_readv(bs->backin=
g_hd, sector_num,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 =A0n, &hd_qiov);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (ret < 0) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0goto fail;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0}
> + =A0 =A0 =A0 =A0 =A0 =A0} else {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0qemu_iovec_reset(&hd_qiov);

We need to zero bytes, otherwise qiov will contain uninitialized bytes
instead of zeroes read when there is no backing file.

> + =A0 =A0 =A0 =A0 =A0 =A0}
> + =A0 =A0 =A0 =A0}
> + =A0 =A0 =A0 =A0remaining_sectors -=3D cur_nr_sectors;
> + =A0 =A0 =A0 =A0sector_num +=3D cur_nr_sectors;
> + =A0 =A0 =A0 =A0bytes_done +=3D cur_nr_sectors * BDRV_SECTOR_SIZE;
> + =A0 =A0}
> +fail:
> + =A0 =A0qemu_co_mutex_unlock(&s->lock);
> + =A0 =A0qemu_iovec_destroy(&hd_qiov);
> + =A0 =A0return ret;
> +}
> +
> +static coroutine_fn int add_cow_co_writev(BlockDriverState *bs, int64_t =
sector_num,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0int remaining_sector=
s, QEMUIOVector *qiov)
> +{
> + =A0 =A0BDRVAddCowState *s =3D bs->opaque;
> + =A0 =A0int ret =3D 0;
> + =A0 =A0QEMUIOVector hd_qiov;
> + =A0 =A0qemu_iovec_init(&hd_qiov, qiov->niov);
> + =A0 =A0qemu_co_mutex_lock(&s->lock);
> + =A0 =A0qemu_iovec_reset(&hd_qiov);
> + =A0 =A0qemu_iovec_copy(&hd_qiov, qiov, 0, remaining_sectors * BDRV_SECT=
OR_SIZE);

Why use hd_qiov?  It should be possible to use qiov directly, I think.

> + =A0 =A0ret =3D bdrv_co_writev(s->image_hd,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 sector_num,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 remaining_sectors, &hd_qiov);
> + =A0 =A0if (ret < 0) {
> + =A0 =A0 =A0 =A0goto fail;
> + =A0 =A0}
> +
> + =A0 =A0ret =3D add_cow_update_bitmap(bs, sector_num, remaining_sectors)=
;
> + =A0 =A0if (ret < 0) {
> + =A0 =A0 =A0 =A0goto fail;
> + =A0 =A0}
> +fail:
> + =A0 =A0qemu_co_mutex_unlock(&s->lock);
> + =A0 =A0qemu_iovec_destroy(&hd_qiov);
> + =A0 =A0return ret;
> +}
> +
> +static int bdrv_add_cow_truncate(BlockDriverState *bs, int64_t offset)
> +{
> + =A0 =A0int ret =3D 0;
> + =A0 =A0int64_t image_sectors =3D offset / BDRV_SECTOR_SIZE;
> + =A0 =A0BDRVAddCowState *s =3D bs->opaque;
> + =A0 =A0int64_t old_image_sector =3D s->image_hd->total_sectors;
> +
> + =A0 =A0ret =3D bdrv_truncate(bs->file, sizeof(AddCowHeader) + ((image_s=
ectors + 7) >> 3));
> + =A0 =A0if (ret < 0) {
> + =A0 =A0 =A0 =A0bdrv_truncate(s->image_hd, old_image_sector * BDRV_SECTO=
R_SIZE);
> + =A0 =A0 =A0 =A0return ret;
> + =A0 =A0}
> + =A0 =A0return ret;
> +}

I'm surprised that we truncate the image file...but only in the error case.

Also, do we need to resize the cow cache?

> +
> +static coroutine_fn int add_cow_co_flush(BlockDriverState *bs)
> +{
> + =A0 =A0BDRVAddCowState *s =3D bs->opaque;
> + =A0 =A0int ret =3D bdrv_co_flush(s->image_hd);
> + =A0 =A0if (ret < 0) {
> + =A0 =A0 =A0 =A0return ret;
> + =A0 =A0}
> +
> + =A0 =A0qemu_co_mutex_lock(&s->lock);
> + =A0 =A0ret =3D add_cow_cache_flush(bs, s->bitmap_cache);

I suggest moving qemu_co_mutex_unlock(&s->lock) here so you don't need
to duplicate it in the success and error cases.

> + =A0 =A0if (ret < 0) {
> + =A0 =A0 =A0 =A0qemu_co_mutex_unlock(&s->lock);
> + =A0 =A0 =A0 =A0return ret;
> + =A0 =A0}
> + =A0 =A0qemu_co_mutex_unlock(&s->lock);
> + =A0 =A0return bdrv_co_flush(bs->file);
> +}
> +
> +static QEMUOptionParameter add_cow_create_options[] =3D {
> + =A0 =A0{
> + =A0 =A0 =A0 =A0.name =3D BLOCK_OPT_SIZE,
> + =A0 =A0 =A0 =A0.type =3D OPT_SIZE,
> + =A0 =A0 =A0 =A0.help =3D "Virtual disk size"
> + =A0 =A0},
> + =A0 =A0{
> + =A0 =A0 =A0 =A0.name =3D BLOCK_OPT_BACKING_FILE,
> + =A0 =A0 =A0 =A0.type =3D OPT_STRING,
> + =A0 =A0 =A0 =A0.help =3D "File name of a base image"
> + =A0 =A0},
> + =A0 =A0{
> + =A0 =A0 =A0 =A0.name =3D BLOCK_OPT_IMAGE_FILE,
> + =A0 =A0 =A0 =A0.type =3D OPT_STRING,
> + =A0 =A0 =A0 =A0.help =3D "File name of a image file"
> + =A0 =A0},
> + =A0 =A0{
> + =A0 =A0 =A0 =A0.name =3D BLOCK_OPT_BACKING_FMT,
> + =A0 =A0 =A0 =A0.type =3D OPT_STRING,
> + =A0 =A0 =A0 =A0.help =3D "Image format of the base image"
> + =A0 =A0},
> + =A0 =A0{ NULL }
> +};
> +
> +static BlockDriver bdrv_add_cow =3D {
> + =A0 =A0.format_name =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=3D "add-cow",
> + =A0 =A0.instance_size =A0 =A0 =A0 =A0 =A0 =A0 =A0=3D sizeof(BDRVAddCowS=
tate),
> + =A0 =A0.bdrv_probe =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =3D add_cow_probe,
> + =A0 =A0.bdrv_open =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=3D add_cow_open,
> + =A0 =A0.bdrv_close =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =3D add_cow_close,
> + =A0 =A0.bdrv_create =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=3D add_cow_create,
> + =A0 =A0.bdrv_co_is_allocated =A0 =A0 =A0 =3D add_cow_is_allocated,
> +
> + =A0 =A0.bdrv_co_readv =A0 =A0 =A0 =A0 =A0 =A0 =A0=3D add_cow_co_readv,
> + =A0 =A0.bdrv_co_writev =A0 =A0 =A0 =A0 =A0 =A0 =3D add_cow_co_writev,
> + =A0 =A0.bdrv_truncate =A0 =A0 =A0 =A0 =A0 =A0 =A0=3D bdrv_add_cow_trunc=
ate,
> +
> + =A0 =A0.create_options =A0 =A0 =A0 =A0 =A0 =A0 =3D add_cow_create_optio=
ns,
> + =A0 =A0.bdrv_co_flush_to_disk =A0 =A0 =A0=3D add_cow_co_flush,
> +};
> +
> +static void bdrv_add_cow_init(void)
> +{
> + =A0 =A0bdrv_register(&bdrv_add_cow);
> +}
> +
> +block_init(bdrv_add_cow_init);
> diff --git a/block_int.h b/block_int.h
> index b460c36..8126f27 100644
> --- a/block_int.h
> +++ b/block_int.h
> @@ -50,6 +50,7 @@
> =A0#define BLOCK_OPT_TABLE_SIZE =A0 =A0"table_size"
> =A0#define BLOCK_OPT_PREALLOC =A0 =A0 =A0"preallocation"
> =A0#define BLOCK_OPT_SUBFMT =A0 =A0 =A0 =A0"subformat"
> +#define BLOCK_OPT_IMAGE_FILE =A0 =A0"image_file"
>
> =A0typedef struct BdrvTrackedRequest BdrvTrackedRequest;
>
> diff --git a/docs/specs/add-cow.txt b/docs/specs/add-cow.txt
> new file mode 100644
> index 0000000..db992a4
> --- /dev/null
> +++ b/docs/specs/add-cow.txt
> @@ -0,0 +1,68 @@
> +=3D=3D General =3D=3D
> +
> +Raw file format does not support backing_file and copy on write feature.
> +The add-cow image format makes it possible to use backing files with raw
> +image by keeping a separate .add-cow metadata file. =A0Once all sectors
> +have been written to in the raw image it is safe to discard the .add-cow
> +and backing files and instead use the raw image directly.
> +
> +When using add-cow, procedures may like this:
> +(ubuntu.img is a disk image which has been installed OS.)
> + =A0 =A01) =A0Create a raw image with the same size of ubuntu.img
> + =A0 =A0 =A0 =A0 =A0 =A0qemu-img create -f raw test.raw 8G
> + =A0 =A02) =A0Create a add-cow image which will store dirty bitmap
> + =A0 =A0 =A0 =A0 =A0 =A0qemu-img create -f add-cow test.add-cow -o backi=
ng_file=3Dubuntu.img,image_file=3Dtest.raw
> + =A0 =A03) =A0Run qemu with add-cow image
> + =A0 =A0 =A0 =A0 =A0 =A0qemu -drive if=3Dvirtio,file=3Dtest.add-cow
> +
> +=3DSpecification=3D
> +
> +The file format looks like this:
> +
> + +---------------+--------------------------+
> + | =A0 =A0 Header =A0 =A0| =A0 =A0 =A0 =A0 =A0 Data =A0 =A0 =A0 =A0 =A0 =
|
> + +---------------+--------------------------+
> +
> +All numbers in add-cow are stored in Big Endian byte order.
> +
> +=3D=3D Header =3D=3D
> +
> +The Header is included in the first bytes:
> +
> + =A0 =A0Byte =A00 - =A07: =A0 =A0 =A0 magic
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0add-cow magic string ("A=
DD_COW\xff")
> +
> + =A0 =A0 =A0 =A0 =A08 - =A011: =A0 =A0 =A0version
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0Version number (only val=
id value is 1 now)
> +
> + =A0 =A0 =A0 =A0 =A012 - 1035: =A0 =A0backing_file
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0backing_file file name r=
elated to add-cow file. All
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0unused bytes are padded =
with zeros. Must not be longer
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0than 1023 bytes.
> +
> + =A0 =A0 =A0 =A0 1036 - 2059: =A0 image_file
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0image_file is a raw file=
. All unused bytes are padded
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0with zeros. Must not be =
longer than 1023 bytes.
> +
> + =A0 =A0 =A0 =A0 2060 =A0- 2559: =A0 The Reserved field is used to make =
sure Data field starts
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0at the multiple of 512, =
not used currently. All bytes are
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0filled with 0.
> +
> +=3D=3D Data =3D=3D
> +
> +The Data field starts at the 2560th byte, stores a bitmap related to bac=
king_file
> +and image_file. The bitmap will track whether the sector in backing_file=
 is dirty
> +or not.

Cluster size is not mentioned and not documented in the add-cow header
structure.

I didn't look closely at how the cache and clusters are supposed to
work but I think they don't really work.  What I would expect is:

1. The bitmap operates at ADD_COW_CLUSTER_SIZE granularity.  This
means the bits in the add-cow file actually represent
ADD_COW_CLUSTER_SIZE sectors of data allocation, not just 1 sector.

2. Code that accesses the bitmap first divides by ADD_COW_CLUSTER_SIZE
to operate on an entire cluster.  Right now the lookup code seems to
do it *during* the bitmap lookup instead of before.

Sorry, this isn't a good explanation but I've run out of time to
really understand how this works in your patch.  I suggest looking
carefully at the is_allocated lookup and the cache to see if you're
really maintaining the bitmap at cluster granularity.

Stefan