[Qemu-devel] [RFC 00/24] inplace image conversion

[Qemu-devel] [RFC 00/24] inplace image conversion

[Devin Nakamura]

This series sets out the foundations of an api for in place image conversion, and implements it 
in the QED and QCOW2 drivers.

The basic flow of conversion is as follows:
The source image is openened as normal.

The source file is the queried for conversion options via bdrv_get_conversion_options. The conversion
options contain things like the size of the clusters, the type of encryption etc.

The target image is then opened with bdrv_open_conversion_target, passing the conversion optionons
among other things allong with it. This is basically a hybrid of bdrv_open and bdrv_create.

Mappings are then fetched and set using bdrv_get_mapping and bdrv_map respectively. They both deal 
with data in the format guest_offset, host_offset, and contiguous_bytes. For example if 
guest_offset = 31, host_offset = 41, and contiguous_bytes = 59 that would mean that offset 31 in the
virtual disk would be located at offset 41 in the image file and 32 at 42 and so on up to 89 at 99.

At the end of the mapping process the image is "finalized" with bdrv_copy_header, where the target
driver copies out the old header to a temporary file, and then writes out its own header into the 
image.

I've integerated inplace conversion into qemu-img in the form: 
qemu-img convert-inplace <imagefile> <target_format>

I've also integrated bdrv_get_mapping and bdrv_map into qemu-io (with the former replacing qemu-io's
internal mapping when then block driver supports it) 

Devin Nakamura (24):
  block: add block conversion api
  block: add bdrv_get_conversion_options()
  block: add bdrv_open_conversion_target()
  block: add bdrv_get_mapping()
  block: add bdrv_map()
  block: add bdrv_copy_header()
  qed: make qed_alloc_clusters round up offset to nearest cluster
  qed: add qed_find_cluster_sync()
  qed: add qed_bdrv_get_mapping()
  qed: add qed_bdrv_map()
  qed: add open_conversion_target()
  qed: add bdrv_qed_copy_header()
  qed: add bdrv_qed_get_conversion_options()
  qcow2: fix typo in documentation for qcow2_get_cluster_offset()
  qcow2: split up the creation of new refcount table from the act of
    checking it
  qcow2: add qcow2_drop_leaked_clusters()
  qcow2: add qcow2_get_mapping
  qcow2: add qcow2_map
  qcow2: add qcow2_copy_header()
  qcow2: add get_conversion_options()
  qcow2: add qcow2_open_conversion_target()
  qemu-io: make map command use new block mapping function
  qemu-io: add setmap command
  qemu-img: add inplace conversion to qemu-img

 Makefile               |    2 +
 block.c                |  100 +++++++++++++++++++++
 block.h                |   14 +++
 block/qcow2-cluster.c  |   53 +++++++++++-
 block/qcow2-refcount.c |   71 +++++++++++++--
 block/qcow2.c          |  233 ++++++++++++++++++++++++++++++++++++++++++++++++
 block/qcow2.h          |    5 +
 block/qed-cluster.c    |   35 +++++++
 block/qed.c            |  193 ++++++++++++++++++++++++++++++++++++---
 block/qed.h            |    4 +
 block_int.h            |   88 ++++++++++++++++++
 qemu-img-cmds.hx       |    6 ++
 qemu-img.c             |   64 +++++++++++++
 qemu-io.c              |   47 ++++++++++-
 14 files changed, 888 insertions(+), 27 deletions(-)

-- 
1.7.6.rc1

[Qemu-devel] [RFC 01/24] block: add block conversion api

[Devin Nakamura]

add functions to block driver interface to support inplace image conversion

Signed-off-by: Devin Nakamura <devin122@gmail.com>
---
 block.h     |    2 +
 block_int.h |   88 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 90 insertions(+), 0 deletions(-)

diff --git a/block.h b/block.h
index 59cc410..a1c4cc8 100644
--- a/block.h
+++ b/block.h
@@ -251,6 +251,8 @@ int64_t bdrv_get_dirty_count(BlockDriverState *bs);
 void bdrv_set_in_use(BlockDriverState *bs, int in_use);
 int bdrv_in_use(BlockDriverState *bs);
 
+typedef struct BlockConversionOptions BlockConversionOptions;
+
 typedef enum {
     BLKDBG_L1_UPDATE,
 
diff --git a/block_int.h b/block_int.h
index efb6803..84bf89e 100644
--- a/block_int.h
+++ b/block_int.h
@@ -41,6 +41,9 @@
 #define BLOCK_OPT_PREALLOC      "preallocation"
 #define BLOCK_OPT_SUBFMT        "subformat"
 
+#define BLOCK_CRYPT_NONE    0
+#define BLOCK_CRYPT_AES     1
+
 typedef struct AIOPool {
     void (*cancel)(BlockDriverAIOCB *acb);
     int aiocb_size;
@@ -139,6 +142,85 @@ struct BlockDriver {
      */
     int (*bdrv_has_zero_init)(BlockDriverState *bs);
 
+    /* In-place image conversion */
+
+    /**
+     * Opens an image conversion target.
+     *
+     * @param bs          Basic Initialization done by
+     *                    bdrv_open_conversion_target() Still need to set format
+     *                    specific data.
+     * @param usr_options Creation options.
+     * @param drv_options Conversion Options
+     * @return            Returns non-zero on failure.
+     */
+    int (*bdrv_open_conversion_target)(BlockDriverState *bs,
+        BlockConversionOptions *drv_options, QEMUOptionParameter *usr_options);
+
+    /**
+     * Gets a mapping in the image file.
+     *
+     * The function starts searching for a mapping at
+     * starting_guest_offset = guest_offset + contiguous_bytes
+     *
+     * @param bs[in]                   The image in which to find mapping.
+     * @param guest_offset[in,out]     On function entry used to calculate
+     *                                 starting search address.
+     *                                 On function exit contains the starting
+     *                                 guest offset of the mapping.
+     * @param host_offset[out]         The starting image file offset for the
+     *                                 mapping.
+     * @param contiguous_bytes[in,out] On function entry used to calculate
+     *                                 starting search address.
+     *                                 On function exit contains the number of
+     *                                 bytes for which this mapping is valid.
+     *                                 A value of 0 means there are no more
+     *                                 mappings in the image.
+     * @return                         Returns non-zero on error.
+     */
+    int (*bdrv_get_mapping)(BlockDriverState *bs, uint64_t *guest_offset,
+        uint64_t *host_offset, uint64_t *contiguous_bytes);
+
+    /**
+     * Sets a mapping in the image file.
+     *
+     * @param bs               Usualy opened with bdrv_open_conversion_target
+     * @param guest_offset     The starting guest offset of the mapping
+     *                         (in bytes). Function fails and returns -EINVAL if
+     *                         not cluster aligned.
+     * @param host_offset      The starting image offset of the mapping
+     *                         (in bytes). Function fails and returns -EINVAL if
+     *                         not cluster aligned.
+     * @param contiguous_bytes The number of bytes for which this mapping exists
+     *                         Function fails and returns -EINVAL if not cluster
+     *                         aligned
+     * @return                 Returns non-zero on error
+     */
+    int (*bdrv_map)(BlockDriverState *bs, uint64_t guest_offset,
+        uint64_t host_offset, uint64_t contiguous_bytes);
+
+    /**
+     * Copies out the header of a conversion target
+     *
+     * Saves the current header for the image in a temporary file and overwrites
+     * it with the header for the new format (at the moment the header is
+     * assumed to be 1 sector)
+     *
+     * @param bs  Usualy opened with bdrv_open_conversion_target().
+     * @return    Returns non-zero on failure
+     */
+    int (*bdrv_copy_header) (BlockDriverState *bs);
+
+    /**
+     * Asks the block driver what options should be used to create a conversion
+     * target.
+     *
+     * @param options[out] Block conversion options
+     */
+    int (*bdrv_get_conversion_options)(BlockDriverState *bs,
+         BlockConversionOptions *options);
+
+
     QLIST_ENTRY(BlockDriver) list;
 };
 
@@ -263,4 +345,10 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf)
     DEFINE_PROP_UINT32("discard_granularity", _state, \
                        _conf.discard_granularity, 0)
 
+struct BlockConversionOptions {
+    int encryption_type;
+    uint64_t image_size;
+    uint64_t cluster_size;
+    uint64_t allocation_size;
+};
 #endif /* BLOCK_INT_H */
-- 
1.7.6.rc1

[Qemu-devel] [RFC 02/24] block: add bdrv_get_conversion_options()

[Devin Nakamura]

Signed-off-by: Devin Nakamura <devin122@gmail.com>
---
 block.c |   13 +++++++++++++
 block.h |    2 ++
 2 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/block.c b/block.c
index 9549b9e..4503b7b 100644
--- a/block.c
+++ b/block.c
@@ -3037,3 +3037,16 @@ out:
 
     return ret;
 }
+
+int bdrv_get_conversion_options(BlockDriverState *bs,
+                                BlockConversionOptions *options)
+{
+    if (!bs->drv) {
+        return -ENOENT;
+    }
+
+    if (!bs->drv->bdrv_get_conversion_options) {
+        return -ENOTSUP;
+    }
+    return bs->drv->bdrv_get_conversion_options(bs, options);
+}
diff --git a/block.h b/block.h
index a1c4cc8..ad7e5ea 100644
--- a/block.h
+++ b/block.h
@@ -253,6 +253,8 @@ int bdrv_in_use(BlockDriverState *bs);
 
 typedef struct BlockConversionOptions BlockConversionOptions;
 
+int bdrv_get_conversion_options(BlockDriverState *bs,
+                                BlockConversionOptions *options);
 typedef enum {
     BLKDBG_L1_UPDATE,
 
-- 
1.7.6.rc1

[Qemu-devel] [RFC 03/24] block: add bdrv_open_conversion_target()

[Devin Nakamura]

Conflicts:

	block.h

Signed-off-by: Devin Nakamura <devin122@gmail.com>
---
 block.c |   32 ++++++++++++++++++++++++++++++++
 block.h |    4 ++++
 2 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/block.c b/block.c
index 4503b7b..9530577 100644
--- a/block.c
+++ b/block.c
@@ -3038,6 +3038,38 @@ out:
     return ret;
 }
 
+int bdrv_open_conversion_target(BlockDriverState **bs, BlockDriverState *file,
+                                BlockConversionOptions *drv_options,
+                                QEMUOptionParameter *usr_options,
+                                const char *target_fmt)
+{
+    BlockDriver *drv;
+    BlockDriverState *bss;
+
+    drv = bdrv_find_format(target_fmt);
+    if (!drv) {
+        return -ENOENT;
+    }
+
+    if (!drv->bdrv_open_conversion_target) {
+        return -ENOTSUP;
+    }
+
+    *bs = bdrv_new("");
+    bss = *bs;
+    bss->file = file;
+    bss->total_sectors = drv_options->image_size >> BDRV_SECTOR_BITS;
+    bss->encrypted = 0;
+    bss->valid_key = 0;
+    bss->open_flags = 0;
+    /* buffer_alignment defaulted to 512, drivers can change this value */
+    bss->buffer_alignment = 512;
+    bss->opaque = qemu_mallocz(drv->instance_size);
+    bss->drv = drv;
+    return drv->bdrv_open_conversion_target(bss, drv_options, usr_options);
+
+}
+
 int bdrv_get_conversion_options(BlockDriverState *bs,
                                 BlockConversionOptions *options)
 {
diff --git a/block.h b/block.h
index ad7e5ea..662bae7 100644
--- a/block.h
+++ b/block.h
@@ -255,6 +255,10 @@ typedef struct BlockConversionOptions BlockConversionOptions;
 
 int bdrv_get_conversion_options(BlockDriverState *bs,
                                 BlockConversionOptions *options);
+int bdrv_open_conversion_target(BlockDriverState **bs, BlockDriverState *file,
+                                BlockConversionOptions *drv_options,
+                                QEMUOptionParameter *usr_options,
+                                const char *target_fmt);
 typedef enum {
     BLKDBG_L1_UPDATE,
 
-- 
1.7.6.rc1

[Qemu-devel] [RFC 04/24] block: add bdrv_get_mapping()

[Devin Nakamura]

Conflicts:

	block.h

Signed-off-by: Devin Nakamura <devin122@gmail.com>
---
 block.c |   29 +++++++++++++++++++++++++++++
 block.h |    2 ++
 2 files changed, 31 insertions(+), 0 deletions(-)

diff --git a/block.c b/block.c
index 9530577..d0019c4 100644
--- a/block.c
+++ b/block.c
@@ -3082,3 +3082,32 @@ int bdrv_get_conversion_options(BlockDriverState *bs,
     }
     return bs->drv->bdrv_get_conversion_options(bs, options);
 }
+
+/**
+ * Gets a mapping from an offset in the image to an offset within a file
+ *
+ * All offsets are in bytes. Functions starts its search at offset host_offset
+ * + count (offset within the image, not the file offset)
+ *
+ * @param guest_offset          used to calculate starting search location on
+ *                              function call. On return it is the starting
+ *                              offset of the mapping in the image.
+ * @param host_offset[out]      The starting file offset of the mapping.
+ * @param contiguous_bytes[out] The number of bytes for which this mapping is
+ *                              contiguous. If 0, there are no more mapppings in
+ *                              the image
+ */
+
+int bdrv_get_mapping(BlockDriverState *bs, uint64_t *guest_offset,
+                     uint64_t *host_offset, uint64_t *contiguous_bytes)
+{
+    BlockDriver *drv = bs->drv;
+    if (!drv) {
+        return -ENOMEDIUM;
+    }
+    if (!drv->bdrv_get_mapping) {
+        return -ENOTSUP;
+    }
+    return drv->bdrv_get_mapping(bs, guest_offset, host_offset,
+        contiguous_bytes);
+}
diff --git a/block.h b/block.h
index 662bae7..3459900 100644
--- a/block.h
+++ b/block.h
@@ -259,6 +259,8 @@ int bdrv_open_conversion_target(BlockDriverState **bs, BlockDriverState *file,
                                 BlockConversionOptions *drv_options,
                                 QEMUOptionParameter *usr_options,
                                 const char *target_fmt);
+int bdrv_get_mapping(BlockDriverState *bs, uint64_t *guest_offset,
+                     uint64_t *host_offset, uint64_t *contiguous_bytes);
 typedef enum {
     BLKDBG_L1_UPDATE,
 
-- 
1.7.6.rc1

[Qemu-devel] [RFC 05/24] block: add bdrv_map()

[Devin Nakamura]

Signed-off-by: Devin Nakamura <devin122@gmail.com>
---
 block.c |   14 ++++++++++++++
 block.h |    2 ++
 2 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/block.c b/block.c
index d0019c4..98a9491 100644
--- a/block.c
+++ b/block.c
@@ -3111,3 +3111,17 @@ int bdrv_get_mapping(BlockDriverState *bs, uint64_t *guest_offset,
     return drv->bdrv_get_mapping(bs, guest_offset, host_offset,
         contiguous_bytes);
 }
+
+int bdrv_map(BlockDriverState *bs, uint64_t *guest_offset,
+             uint64_t *host_offset, uint64_t *contiguous_bytes)
+{
+    BlockDriver *drv = bs->drv;
+    if (!drv) {
+        return -ENOMEDIUM;
+    }
+    if (!drv->bdrv_map) {
+        return -ENOTSUP;
+    }
+    return drv->bdrv_map(bs, guest_offset, host_offset,
+        contiguous_bytes);
+}
diff --git a/block.h b/block.h
index 3459900..e021394 100644
--- a/block.h
+++ b/block.h
@@ -261,6 +261,8 @@ int bdrv_open_conversion_target(BlockDriverState **bs, BlockDriverState *file,
                                 const char *target_fmt);
 int bdrv_get_mapping(BlockDriverState *bs, uint64_t *guest_offset,
                      uint64_t *host_offset, uint64_t *contiguous_bytes);
+int bdrv_map(BlockDriverState *bs, uint64_t *guest_offset,
+             uint64_t *host_offset, uint64_t *contiguous_bytes);
 typedef enum {
     BLKDBG_L1_UPDATE,
 
-- 
1.7.6.rc1

[Qemu-devel] [RFC 06/24] block: add bdrv_copy_header()

[Devin Nakamura]

Signed-off-by: Devin Nakamura <devin122@gmail.com>
---
 block.c |   12 ++++++++++++
 block.h |    2 ++
 2 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/block.c b/block.c
index 98a9491..28b4418 100644
--- a/block.c
+++ b/block.c
@@ -3125,3 +3125,15 @@ int bdrv_map(BlockDriverState *bs, uint64_t *guest_offset,
     return drv->bdrv_map(bs, guest_offset, host_offset,
         contiguous_bytes);
 }
+
+int bdrv_copy_header(BlockDriverState *bs)
+{
+    BlockDriver *drv = bs->drv;
+    if (!drv) {
+        return -ENOMEDIUM;
+    }
+    if (!drv->bdrv_copy_header) {
+        return -ENOTSUP;
+    }
+    return drv->bdrv_copy_header(bs);
+}
diff --git a/block.h b/block.h
index e021394..2426b62 100644
--- a/block.h
+++ b/block.h
@@ -263,6 +263,8 @@ int bdrv_get_mapping(BlockDriverState *bs, uint64_t *guest_offset,
                      uint64_t *host_offset, uint64_t *contiguous_bytes);
 int bdrv_map(BlockDriverState *bs, uint64_t *guest_offset,
              uint64_t *host_offset, uint64_t *contiguous_bytes);
+int bdrv_copy_header(BlockDriverState *bs);
+
 typedef enum {
     BLKDBG_L1_UPDATE,
 
-- 
1.7.6.rc1

[Qemu-devel] [RFC 07/24] qed: make qed_alloc_clusters round up offset to nearest cluster

[Devin Nakamura]

Signed-off-by: Devin Nakamura <devin122@gmail.com>
---
 block/qed.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/block/qed.c b/block/qed.c
index 3970379..00cf895 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -263,6 +263,9 @@ static int qed_read_string(BlockDriverState *file, uint64_t offset, size_t n,
  */
 static uint64_t qed_alloc_clusters(BDRVQEDState *s, unsigned int n)
 {
+    s->file_size = (s->file_size + s->header.cluster_size -1)
+        / s->header.cluster_size;
+    s->file_size *= s->header.cluster_size;
     uint64_t offset = s->file_size;
     s->file_size += n * s->header.cluster_size;
     return offset;
-- 
1.7.6.rc1

[Qemu-devel] [RFC 08/24] qed: add qed_find_cluster_sync()

[Devin Nakamura]

Signed-off-by: Devin Nakamura <devin122@gmail.com>
---
 block/qed-cluster.c |   35 +++++++++++++++++++++++++++++++++++
 block/qed.h         |    4 ++++
 2 files changed, 39 insertions(+), 0 deletions(-)

diff --git a/block/qed-cluster.c b/block/qed-cluster.c
index 3e19ad1..063b965 100644
--- a/block/qed-cluster.c
+++ b/block/qed-cluster.c
@@ -163,3 +163,38 @@ void qed_find_cluster(BDRVQEDState *s, QEDRequest *request, uint64_t pos,
     qed_read_l2_table(s, request, l2_offset,
                       qed_find_cluster_cb, find_cluster_cb);
 }
+
+typedef struct {
+    int ret;
+    uint64_t *offset;
+    size_t *len;
+} QEDFindClusterSyncCB;
+
+static void qed_find_cluster_sync_cb(void *opaque, int ret, uint64_t offset,
+                              size_t len)
+{
+    QEDFindClusterSyncCB *find_cluster_sync_cb = opaque;
+    *find_cluster_sync_cb->offset = offset;
+    *find_cluster_sync_cb->len = len;
+    find_cluster_sync_cb->ret = ret;
+}
+
+int qed_find_cluster_sync(BDRVQEDState *s, QEDRequest *request, uint64_t pos,
+                          size_t len, uint64_t *offset,
+                          size_t *contiguous_bytes)
+{
+    QEDFindClusterSyncCB find_cluster_cb;
+    find_cluster_cb.ret = -EINPROGRESS;
+    find_cluster_cb.offset = offset;
+    find_cluster_cb.len = contiguous_bytes;
+
+    async_context_push();
+    qed_find_cluster(s, request, pos, len, &qed_find_cluster_sync_cb,
+                     &find_cluster_cb);
+    while (find_cluster_cb.ret == -EINPROGRESS) {
+        qemu_aio_wait();
+    }
+    async_context_pop();
+
+    return find_cluster_cb.ret;
+}
diff --git a/block/qed.h b/block/qed.h
index 388fdb3..c899c15 100644
--- a/block/qed.h
+++ b/block/qed.h
@@ -239,6 +239,10 @@ int qed_write_l2_table_sync(BDRVQEDState *s, QEDRequest *request,
 void qed_find_cluster(BDRVQEDState *s, QEDRequest *request, uint64_t pos,
                       size_t len, QEDFindClusterFunc *cb, void *opaque);
 
+int qed_find_cluster_sync(BDRVQEDState *s, QEDRequest *request, uint64_t pos,
+                          size_t len, uint64_t *offset,
+                          size_t *contiguous_bytes);
+
 /**
  * Consistency check
  */
-- 
1.7.6.rc1

[Qemu-devel] [RFC 09/24] qed: add qed_bdrv_get_mapping()

[Devin Nakamura]

Fuction to get drive mapping from qed images

Signed-off-by: Devin Nakamura <devin122@gmail.com>
---
 block/qed.c |   32 ++++++++++++++++++++++++++++++++
 1 files changed, 32 insertions(+), 0 deletions(-)

diff --git a/block/qed.c b/block/qed.c
index 00cf895..dadb7f8 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -1451,6 +1451,37 @@ static int bdrv_qed_check(BlockDriverState *bs, BdrvCheckResult *result)
     return qed_check(s, result, false);
 }
 
+static int bdrv_qed_get_mapping(BlockDriverState *bs, uint64_t *guest_offset,
+                                uint64_t *host_offset,
+                                uint64_t *contiguous_bytes)
+{
+    BDRVQEDState *s = bs->opaque;
+    size_t l2_size = s->header.cluster_size * s->table_nelems;
+    uint64_t pos = *guest_offset + *contiguous_bytes;
+    uint64_t offset = pos;
+    size_t len = 0;
+    QEDRequest req = {.l2_table = NULL};
+    int ret;
+
+    if (pos >= s->header.image_size) {
+        *contiguous_bytes = 0;
+        return 0;
+    }
+
+    do {
+        pos += len;
+        ret = qed_find_cluster_sync(s, &req, pos, l2_size, &offset, &len);
+    } while (ret != QED_CLUSTER_FOUND && pos < s->header.image_size);
+    *guest_offset = pos;
+    *host_offset = offset;
+    if (pos >= s->header.image_size) {
+        *contiguous_bytes = 0;
+    } else {
+        *contiguous_bytes = len;
+    }
+    return 0;
+}
+
 static QEMUOptionParameter qed_create_options[] = {
     {
         .name = BLOCK_OPT_SIZE,
@@ -1497,6 +1528,7 @@ static BlockDriver bdrv_qed = {
     .bdrv_get_info            = bdrv_qed_get_info,
     .bdrv_change_backing_file = bdrv_qed_change_backing_file,
     .bdrv_check               = bdrv_qed_check,
+    .bdrv_get_mapping         = bdrv_qed_get_mapping,
 };
 
 static void bdrv_qed_init(void)
-- 
1.7.6.rc1

[Qemu-devel] [RFC 10/24] qed: add qed_bdrv_map()

[Devin Nakamura]

Conflicts:

	block_int.h

Signed-off-by: Devin Nakamura <devin122@gmail.com>
---
 block.c     |    4 ++--
 block.h     |    4 ++--
 block/qed.c |   47 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 51 insertions(+), 4 deletions(-)

diff --git a/block.c b/block.c
index 28b4418..dca3687 100644
--- a/block.c
+++ b/block.c
@@ -3112,8 +3112,8 @@ int bdrv_get_mapping(BlockDriverState *bs, uint64_t *guest_offset,
         contiguous_bytes);
 }
 
-int bdrv_map(BlockDriverState *bs, uint64_t *guest_offset,
-             uint64_t *host_offset, uint64_t *contiguous_bytes)
+int bdrv_map(BlockDriverState *bs, uint64_t guest_offset,
+             uint64_t host_offset, uint64_t contiguous_bytes)
 {
     BlockDriver *drv = bs->drv;
     if (!drv) {
diff --git a/block.h b/block.h
index 2426b62..e0580f0 100644
--- a/block.h
+++ b/block.h
@@ -261,8 +261,8 @@ int bdrv_open_conversion_target(BlockDriverState **bs, BlockDriverState *file,
                                 const char *target_fmt);
 int bdrv_get_mapping(BlockDriverState *bs, uint64_t *guest_offset,
                      uint64_t *host_offset, uint64_t *contiguous_bytes);
-int bdrv_map(BlockDriverState *bs, uint64_t *guest_offset,
-             uint64_t *host_offset, uint64_t *contiguous_bytes);
+int bdrv_map(BlockDriverState *bs, uint64_t guest_offset,
+             uint64_t host_offset, uint64_t contiguous_bytes);
 int bdrv_copy_header(BlockDriverState *bs);
 
 typedef enum {
diff --git a/block/qed.c b/block/qed.c
index dadb7f8..daf82fd 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -1482,6 +1482,52 @@ static int bdrv_qed_get_mapping(BlockDriverState *bs, uint64_t *guest_offset,
     return 0;
 }
 
+static int bdrv_qed_map(BlockDriverState *bs, uint64_t guest_offset,
+                        uint64_t host_offset, uint64_t contiguous_bytes)
+{
+    BDRVQEDState* s = bs->opaque;
+    if(guest_offset != qed_start_of_cluster(s, guest_offset)){
+        return -EINVAL;
+    }
+
+    if(contiguous_bytes % s->header.cluster_size != 0){
+        return -EINVAL;
+    }
+
+    while(contiguous_bytes > 0){
+        uint64_t l1_index, l2_index, first_l2;
+        int ret;
+        QEDRequest req = {.l2_table = NULL};
+
+
+        l1_index = qed_l1_index(s, guest_offset);
+        l2_index = qed_l2_index(s, guest_offset);
+        first_l2 = l2_index;
+        if(!s->l1_table->offsets[l1_index]){
+            CachedL2Table * table = qed_new_l2_table(s);
+            s->l1_table->offsets[l1_index] = table->offset;
+            qed_write_l1_table_sync(s, l1_index, 1);
+            qed_commit_l2_cache_entry(&s->l2_cache, table);
+        }
+
+        ret = qed_read_l2_table_sync(s, &req, s->l1_table->offsets[l1_index]);
+        if(ret){
+            return ret;
+        }
+
+        for(;l2_index < s->table_nelems && contiguous_bytes > 0; l2_index++){
+            req.l2_table->table->offsets[l2_index] = host_offset;
+            host_offset += s->header.cluster_size;
+            contiguous_bytes -= s->header.cluster_size;
+        }
+
+        ret = qed_write_l2_table_sync(s, &req, 0, s->table_nelems, true);
+        qed_unref_l2_cache_entry(req.l2_table);
+
+    }
+    return 0;
+}
+
 static QEMUOptionParameter qed_create_options[] = {
     {
         .name = BLOCK_OPT_SIZE,
@@ -1529,6 +1575,7 @@ static BlockDriver bdrv_qed = {
     .bdrv_change_backing_file = bdrv_qed_change_backing_file,
     .bdrv_check               = bdrv_qed_check,
     .bdrv_get_mapping         = bdrv_qed_get_mapping,
+    .bdrv_map                 = bdrv_qed_map,
 };
 
 static void bdrv_qed_init(void)
-- 
1.7.6.rc1

[Qemu-devel] [RFC 11/24] qed: add open_conversion_target()

[Devin Nakamura]

Signed-off-by: Devin Nakamura <devin122@gmail.com>
---
 block/qed.c |   86 +++++++++++++++++++++++++++++++++++++++++++++++-----------
 1 files changed, 69 insertions(+), 17 deletions(-)

diff --git a/block/qed.c b/block/qed.c
index daf82fd..b05224a 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -1451,6 +1451,57 @@ static int bdrv_qed_check(BlockDriverState *bs, BdrvCheckResult *result)
     return qed_check(s, result, false);
 }
 
+static int bdrv_qed_open_conversion_target(BlockDriverState *bs,
+                                           BlockConversionOptions *drv_options,
+                                           QEMUOptionParameter *usr_options)
+{
+    BDRVQEDState *s = bs->opaque;
+    s->bs = bs;
+    if (drv_options->encryption_type != BLOCK_CRYPT_NONE) {
+        return -1; //TODO: FIXME
+    }
+    if (drv_options->cluster_size != drv_options->allocation_size) {
+        return -1; //TODO: FIXME
+    }
+    s->header.magic = QED_MAGIC;
+    s->header.table_size = QED_DEFAULT_TABLE_SIZE;
+    if(qed_is_cluster_size_valid(drv_options->cluster_size)) {
+        s->header.cluster_size = drv_options->cluster_size;
+    } else {
+        return -EINVAL;
+    }
+    if(qed_is_image_size_valid(drv_options->image_size, s->header.cluster_size,
+                               s->header.table_size)) {
+        /*TODO: check if qed_is_image_size_valid
+        * checks image_size % cluster_size =0 */
+        s->header.image_size = drv_options->image_size;
+    } else {
+        return -EINVAL;
+    }
+    s->file_size = bs->file->total_sectors << BDRV_SECTOR_BITS;
+    s->l1_table = qed_alloc_table(s);
+    s->header.l1_table_offset = qed_alloc_clusters(s, s->header.table_size);
+    QSIMPLEQ_INIT(&s->allocating_write_reqs);
+
+
+    if (!qed_check_table_offset(s, s->header.l1_table_offset)) {
+        return -EINVAL;
+    }
+
+    s->table_nelems = (s->header.cluster_size * s->header.table_size) /
+                      sizeof(uint64_t);
+    s->l2_shift = ffs(s->header.cluster_size) - 1;
+    s->l2_mask = s->table_nelems - 1;
+    s->l1_shift = s->l2_shift + ffs(s->table_nelems) - 1;
+
+    qed_init_l2_cache(&s->l2_cache);
+
+    s->need_check_timer = qemu_new_timer_ns(vm_clock,
+                                            qed_need_check_timer_cb, s);
+    qed_write_l1_table_sync(s, 0, s->table_nelems);
+    return 0;
+}
+
 static int bdrv_qed_get_mapping(BlockDriverState *bs, uint64_t *guest_offset,
                                 uint64_t *host_offset,
                                 uint64_t *contiguous_bytes)
@@ -1559,23 +1610,24 @@ static BlockDriver bdrv_qed = {
     .instance_size            = sizeof(BDRVQEDState),
     .create_options           = qed_create_options,
 
-    .bdrv_probe               = bdrv_qed_probe,
-    .bdrv_open                = bdrv_qed_open,
-    .bdrv_close               = bdrv_qed_close,
-    .bdrv_create              = bdrv_qed_create,
-    .bdrv_flush               = bdrv_qed_flush,
-    .bdrv_is_allocated        = bdrv_qed_is_allocated,
-    .bdrv_make_empty          = bdrv_qed_make_empty,
-    .bdrv_aio_readv           = bdrv_qed_aio_readv,
-    .bdrv_aio_writev          = bdrv_qed_aio_writev,
-    .bdrv_aio_flush           = bdrv_qed_aio_flush,
-    .bdrv_truncate            = bdrv_qed_truncate,
-    .bdrv_getlength           = bdrv_qed_getlength,
-    .bdrv_get_info            = bdrv_qed_get_info,
-    .bdrv_change_backing_file = bdrv_qed_change_backing_file,
-    .bdrv_check               = bdrv_qed_check,
-    .bdrv_get_mapping         = bdrv_qed_get_mapping,
-    .bdrv_map                 = bdrv_qed_map,
+    .bdrv_probe                  = bdrv_qed_probe,
+    .bdrv_open                   = bdrv_qed_open,
+    .bdrv_close                  = bdrv_qed_close,
+    .bdrv_create                 = bdrv_qed_create,
+    .bdrv_flush                  = bdrv_qed_flush,
+    .bdrv_is_allocated           = bdrv_qed_is_allocated,
+    .bdrv_make_empty             = bdrv_qed_make_empty,
+    .bdrv_aio_readv              = bdrv_qed_aio_readv,
+    .bdrv_aio_writev             = bdrv_qed_aio_writev,
+    .bdrv_aio_flush              = bdrv_qed_aio_flush,
+    .bdrv_truncate               = bdrv_qed_truncate,
+    .bdrv_getlength              = bdrv_qed_getlength,
+    .bdrv_get_info               = bdrv_qed_get_info,
+    .bdrv_change_backing_file    = bdrv_qed_change_backing_file,
+    .bdrv_check                  = bdrv_qed_check,
+    .bdrv_open_conversion_target = bdrv_qed_open_conversion_target,
+    .bdrv_get_mapping            = bdrv_qed_get_mapping,
+    .bdrv_map                    = bdrv_qed_map,
 };
 
 static void bdrv_qed_init(void)
-- 
1.7.6.rc1

[Qemu-devel] [RFC 12/24] qed: add bdrv_qed_copy_header()

[Devin Nakamura]

Signed-off-by: Devin Nakamura <devin122@gmail.com>
---
 block/qed.c |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/block/qed.c b/block/qed.c
index b05224a..556512b 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -1579,6 +1579,21 @@ static int bdrv_qed_map(BlockDriverState *bs, uint64_t guest_offset,
     return 0;
 }
 
+static int bdrv_qed_copy_header(BlockDriverState *bs)
+{
+    BlockDriverState *backup;
+    uint8_t buffer[512];
+    /*TODO: change filename*/
+    bdrv_create_file("headerbackup.temp", NULL);
+    bdrv_file_open(&backup, "headerbackup.temp", BDRV_O_RDWR);
+    bdrv_read(bs->file, 0, buffer, 1); /*TODO: check return code*/
+    bdrv_write(backup, 0, buffer, 1); /*TODO: check return code*/
+    bdrv_close(backup);
+
+    qed_write_header_sync(bs->opaque);
+    return 0;
+}
+
 static QEMUOptionParameter qed_create_options[] = {
     {
         .name = BLOCK_OPT_SIZE,
@@ -1628,6 +1643,7 @@ static BlockDriver bdrv_qed = {
     .bdrv_open_conversion_target = bdrv_qed_open_conversion_target,
     .bdrv_get_mapping            = bdrv_qed_get_mapping,
     .bdrv_map                    = bdrv_qed_map,
+    .bdrv_copy_header            = bdrv_qed_copy_header,
 };
 
 static void bdrv_qed_init(void)
-- 
1.7.6.rc1

[Qemu-devel] [RFC 13/24] qed: add bdrv_qed_get_conversion_options()

[Devin Nakamura]

Signed-off-by: Devin Nakamura <devin122@gmail.com>
---
 block/qed.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/block/qed.c b/block/qed.c
index 556512b..26e43e2 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -1594,6 +1594,18 @@ static int bdrv_qed_copy_header(BlockDriverState *bs)
     return 0;
 }
 
+static int bdrv_qed_get_conversion_options(BlockDriverState *bs,
+                                           BlockConversionOptions *options)
+{
+    BDRVQEDState* s = bs->opaque;
+
+    options->encryption_type = 0;
+    options->cluster_size = s->header.cluster_size;
+    options->allocation_size = options->cluster_size;
+    options->image_size = s->header.image_size;
+    return 0;
+}
+
 static QEMUOptionParameter qed_create_options[] = {
     {
         .name = BLOCK_OPT_SIZE,
@@ -1644,6 +1656,7 @@ static BlockDriver bdrv_qed = {
     .bdrv_get_mapping            = bdrv_qed_get_mapping,
     .bdrv_map                    = bdrv_qed_map,
     .bdrv_copy_header            = bdrv_qed_copy_header,
+    .bdrv_get_conversion_options = bdrv_qed_get_conversion_options,
 };
 
 static void bdrv_qed_init(void)
-- 
1.7.6.rc1

[Qemu-devel] [RFC 14/24] qcow2: fix typo in documentation for qcow2_get_cluster_offset()

[Devin Nakamura]

Documentation states the num is measured in clusters, but its
actually measured in sectors

Signed-off-by: Devin Nakamura <devin122@gmail.com>
---
 block/qcow2-cluster.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 882f50a..ca56918 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -381,10 +381,10 @@ static int copy_sectors(BlockDriverState *bs, uint64_t start_sect,
  * For a given offset of the disk image, find the cluster offset in
  * qcow2 file. The offset is stored in *cluster_offset.
  *
- * on entry, *num is the number of contiguous clusters we'd like to
+ * on entry, *num is the number of contiguous sectors we'd like to
  * access following offset.
  *
- * on exit, *num is the number of contiguous clusters we can read.
+ * on exit, *num is the number of contiguous sectors we can read.
  *
  * Return 0, if the offset is found
  * Return -errno, otherwise.
-- 
1.7.6.rc1

[Qemu-devel] [RFC 15/24] qcow2: split up the creation of new refcount table from the act of checking it

[Devin Nakamura]

Signed-off-by: Devin Nakamura <devin122@gmail.com>
---
 block/qcow2-refcount.c |   39 +++++++++++++++++++++++++++++----------
 1 files changed, 29 insertions(+), 10 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 14b2f67..75f1f88 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1075,17 +1075,10 @@ fail:
     return -EIO;
 }
 
-/*
- * Checks an image for refcount consistency.
- *
- * Returns 0 if no errors are found, the number of errors in case the image is
- * detected as corrupted, and -errno when an internal error occurred.
- */
-int qcow2_check_refcounts(BlockDriverState *bs, BdrvCheckResult *res)
-{
+static int inc_refcount_table(BlockDriverState *bs, BdrvCheckResult *res, uint16_t **table) {
     BDRVQcowState *s = bs->opaque;
     int64_t size;
-    int nb_clusters, refcount1, refcount2, i;
+    int nb_clusters, i;
     QCowSnapshot *sn;
     uint16_t *refcount_table;
     int ret;
@@ -1151,6 +1144,33 @@ int qcow2_check_refcounts(BlockDriverState *bs, BdrvCheckResult *res)
             }
         }
     }
+    *table = refcount_table;
+    ret = 0;
+
+fail:
+    return ret;
+}
+
+/*
+ * Checks an image for refcount consistency.
+ *
+ * Returns 0 if no errors are found, the number of errors in case the image is
+ * detected as corrupted, and -errno when an internal error occurred.
+ */
+int qcow2_check_refcounts(BlockDriverState *bs, BdrvCheckResult *res)
+{
+    BDRVQcowState *s = bs->opaque;
+    int64_t size;
+    int nb_clusters, refcount1, refcount2, i;
+    uint16_t *refcount_table;
+    int ret;
+
+    size = bdrv_getlength(bs->file);
+    nb_clusters = size_to_clusters(s, size);
+    ret = inc_refcount_table(bs, res, &refcount_table);
+    if (ret) {
+        goto fail;
+    }
 
     /* compare ref counts */
     for(i = 0; i < nb_clusters; i++) {
@@ -1182,4 +1202,3 @@ fail:
 
     return ret;
 }
-
-- 
1.7.6.rc1

[Qemu-devel] [RFC 16/24] qcow2: add qcow2_drop_leaked_clusters()

[Devin Nakamura]

Signed-off-by: Devin Nakamura <devin122@gmail.com>
---
 block/qcow2-refcount.c |   34 ++++++++++++++++++++++++++++++++++
 block/qcow2.h          |    2 ++
 2 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 75f1f88..2f78a71 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -1202,3 +1202,37 @@ fail:
 
     return ret;
 }
+
+int qcow2_drop_leaked_clusters(BlockDriverState *bs) {
+    BDRVQcowState *s = bs->opaque;
+    int64_t size;
+    int nb_clusters, refcount, i;
+    uint16_t *refcount_table;
+    BdrvCheckResult res;
+    int ret;
+
+    ret = inc_refcount_table(bs, &res, &refcount_table);
+    if (ret) {
+        goto fail;
+    }
+    size = bdrv_getlength(bs->file);
+    nb_clusters = size_to_clusters(s, size);
+
+    for(i = 0; i < nb_clusters; i++) {
+        refcount = get_refcount(bs, i);
+        refcount = refcount_table[i] - refcount;
+        if (refcount < 0) { //we only want to change leaked clusters
+            ret = update_cluster_refcount(bs, i , refcount);
+            if (ret) {
+                goto fail;
+            }
+        }
+    }
+
+    ret = 0;
+
+fail:
+    qemu_free(refcount_table);
+
+    return ret;
+}
diff --git a/block/qcow2.h b/block/qcow2.h
index 6a0a21b..e9efb74 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -193,6 +193,8 @@ int qcow2_update_snapshot_refcount(BlockDriverState *bs,
 
 int qcow2_check_refcounts(BlockDriverState *bs, BdrvCheckResult *res);
 
+int qcow2_drop_leaked_clusters(BlockDriverState *bs) ;
+
 /* qcow2-cluster.c functions */
 int qcow2_grow_l1_table(BlockDriverState *bs, int min_size, bool exact_size);
 void qcow2_l2_cache_reset(BlockDriverState *bs);
-- 
1.7.6.rc1

[Qemu-devel] [RFC 17/24] qcow2: add qcow2_get_mapping

[Devin Nakamura]

Signed-off-by: Devin Nakamura <devin122@gmail.com>
---
 block/qcow2.c |   36 ++++++++++++++++++++++++++++++++++++
 1 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 48e1b95..05ea40c 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1340,6 +1340,40 @@ static int qcow2_load_vmstate(BlockDriverState *bs, uint8_t *buf,
     return ret;
 }
 
+static int qcow2_get_mapping(BlockDriverState *bs, uint64_t *guest_offset,
+        uint64_t *host_offset, uint64_t *contiguous_bytes)
+{
+    uint64_t cluster_offset, pos;
+    //BDRVQcowState *s = bs->opaque;
+    int ret, count;
+    pos = *guest_offset + *contiguous_bytes;
+
+    if (pos >= bs->total_sectors << BDRV_SECTOR_BITS) {
+        *contiguous_bytes = 0;
+        return 0;
+    }
+    count = 0;
+    do {
+        pos += count << BDRV_SECTOR_BITS;
+        count = INT_MAX;
+        ret = qcow2_get_cluster_offset(bs, pos, &count, &cluster_offset);
+        if (ret) {
+            *contiguous_bytes = 0;
+            return ret;
+        }
+    } while (!cluster_offset && pos < bs->total_sectors * BDRV_SECTOR_SIZE);
+
+    if (pos >= bs->total_sectors << BDRV_SECTOR_BITS || !cluster_offset) {
+        *contiguous_bytes = 0;
+        return 0;
+    }
+
+    *contiguous_bytes = count << BDRV_SECTOR_BITS;
+    *guest_offset = pos;
+    *host_offset = cluster_offset;
+    return 0;
+}
+
 static QEMUOptionParameter qcow2_create_options[] = {
     {
         .name = BLOCK_OPT_SIZE,
@@ -1409,6 +1443,8 @@ static BlockDriver bdrv_qcow2 = {
 
     .create_options = qcow2_create_options,
     .bdrv_check = qcow2_check,
+
+    .bdrv_get_mapping = qcow2_get_mapping,
 };
 
 static void bdrv_qcow2_init(void)
-- 
1.7.6.rc1

[Qemu-devel] [RFC 18/24] qcow2: add qcow2_map

[Devin Nakamura]

Signed-off-by: Devin Nakamura <devin122@gmail.com>
---
 block/qcow2-cluster.c |   49 +++++++++++++++++++++++++++++++++++++++++++++++++
 block/qcow2.c         |    1 +
 block/qcow2.h         |    3 +++
 3 files changed, 53 insertions(+), 0 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index ca56918..848f2ee 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -977,3 +977,52 @@ int qcow2_discard_clusters(BlockDriverState *bs, uint64_t offset,
 
     return 0;
 }
+
+int qcow2_map(BlockDriverState *bs, uint64_t guest_offset,
+    uint64_t host_offset, uint64_t contiguous_bytes)
+{
+    BDRVQcowState *s = bs->opaque;
+    unsigned int nelms;
+
+
+    if ((s->cluster_size - 1) & guest_offset) {
+        return -EINVAL;
+    }
+
+    if (contiguous_bytes % s->cluster_size) {
+        return -EINVAL;
+    }
+
+    nelms = s->l2_size / sizeof(uint64_t);
+
+    while (contiguous_bytes > 0) {
+        unsigned int l1_index, l2_index;
+        uint64_t *l2_table;
+        int ret;
+        l1_index = guest_offset >> (s->l2_bits + s->cluster_bits);
+        l2_index = (guest_offset  >> s->cluster_bits) & (s->l2_size - 1);
+
+        if (!s->l1_table[l1_index]) {
+            ret = l2_allocate(bs, l1_index, &l2_table);
+            if (ret) {
+                return ret;
+            }
+        } else {
+            ret = l2_load(bs, s->l1_table[l1_index] & ~QCOW_OFLAG_COPIED, &l2_table);
+            if (ret) {
+                return ret;
+            }
+        }
+        qcow2_cache_entry_mark_dirty(s->l2_table_cache, l2_table);
+
+        for (; l2_index < nelms && contiguous_bytes > 0;  l2_index++) {
+            l2_table[l2_index] = cpu_to_be64(host_offset | QCOW_OFLAG_COPIED);
+            host_offset += s->cluster_size;
+            contiguous_bytes -= s->cluster_size;
+        }
+
+        qcow2_cache_put(bs, s->l2_table_cache, (void **) &l2_table);
+
+    }
+    return 0;
+}
diff --git a/block/qcow2.c b/block/qcow2.c
index 05ea40c..b75364d 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1445,6 +1445,7 @@ static BlockDriver bdrv_qcow2 = {
     .bdrv_check = qcow2_check,
 
     .bdrv_get_mapping = qcow2_get_mapping,
+    .bdrv_map         = qcow2_map,
 };
 
 static void bdrv_qcow2_init(void)
diff --git a/block/qcow2.h b/block/qcow2.h
index e9efb74..feea866 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -245,4 +245,7 @@ int qcow2_cache_get_empty(BlockDriverState *bs, Qcow2Cache *c, uint64_t offset,
     void **table);
 int qcow2_cache_put(BlockDriverState *bs, Qcow2Cache *c, void **table);
 
+int qcow2_map(BlockDriverState *bs, uint64_t guest_offset, uint64_t host_ofset,
+              uint64_t contiguous_bytes);
+
 #endif
-- 
1.7.6.rc1

[Qemu-devel] [RFC 19/24] qcow2: add qcow2_copy_header()

[Devin Nakamura]

Signed-off-by: Devin Nakamura <devin122@gmail.com>
---
 block/qcow2.c |   54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 54 insertions(+), 0 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index b75364d..3bb28d2 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1374,6 +1374,59 @@ static int qcow2_get_mapping(BlockDriverState *bs, uint64_t *guest_offset,
     return 0;
 }
 
+static int qcow2_copy_header(BlockDriverState *bs)
+{
+    BlockDriverState *backup;
+    uint8_t buffer[512];
+    int ret;
+    QCowHeader header;
+    BDRVQcowState *s = bs->opaque;
+    ret = qcow2_cache_flush(bs, s->l2_table_cache);
+    if (ret) {
+        return ret;
+    }
+
+    /*TODO: change filename*/
+    bdrv_create_file("headerbackup.temp", NULL);
+    bdrv_file_open(&backup, "headerbackup.temp", BDRV_O_RDWR);
+    bdrv_read(bs->file, 0, buffer, 1); /*TODO: check return code*/
+    bdrv_write(backup, 0, buffer, 1); /*TODO: check return code*/
+    bdrv_close(backup);
+
+    header.magic = QCOW_MAGIC;
+    header.version = 2;
+    header.backing_file_offset = 0;
+    header.backing_file_size = 0;
+    header.size = bs->total_sectors << BDRV_SECTOR_BITS;
+    header.cluster_bits = s->cluster_bits;
+    header.crypt_method = QCOW_CRYPT_NONE;
+    header.l1_table_offset = s->l1_table_offset;
+    header.l1_size = s->l1_size;
+    header.refcount_table_offset = s->refcount_table_offset;
+    header.refcount_table_clusters = s->refcount_table_size; /*TODO: FIXME*/
+    header.snapshots_offset = s->snapshots_offset;
+    header.nb_snapshots = s->nb_snapshots;
+
+    cpu_to_be64s(&header.backing_file_offset);
+    cpu_to_be32s(&header.backing_file_size);
+    cpu_to_be32s(&header.cluster_bits);
+    cpu_to_be32s(&header.crypt_method);
+    cpu_to_be32s(&header.l1_size);
+    cpu_to_be64s(&header.l1_table_offset);
+    cpu_to_be32s(&header.magic);
+    cpu_to_be32s(&header.nb_snapshots);
+    cpu_to_be32s(&header.refcount_table_clusters);
+    cpu_to_be64s(&header.refcount_table_offset);
+    cpu_to_be64s(&header.size);
+    cpu_to_be64s(&header.snapshots_offset);
+    cpu_to_be32s(&header.version);
+    ret = bdrv_pwrite_sync(bs->file, 0, &header, sizeof(QCowHeader));
+    if (ret) {
+        return ret;
+    }
+    return qcow2_drop_leaked_clusters(bs);
+}
+
 static QEMUOptionParameter qcow2_create_options[] = {
     {
         .name = BLOCK_OPT_SIZE,
@@ -1446,6 +1499,7 @@ static BlockDriver bdrv_qcow2 = {
 
     .bdrv_get_mapping = qcow2_get_mapping,
     .bdrv_map         = qcow2_map,
+    .bdrv_copy_header = qcow2_copy_header,
 };
 
 static void bdrv_qcow2_init(void)
-- 
1.7.6.rc1

[Qemu-devel] [RFC 20/24] qcow2: add get_conversion_options()

[Devin Nakamura]

Signed-off-by: Devin Nakamura <devin122@gmail.com>
---
 block/qcow2.c |   24 +++++++++++++++++++++---
 1 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 3bb28d2..86df65d 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1427,6 +1427,23 @@ static int qcow2_copy_header(BlockDriverState *bs)
     return qcow2_drop_leaked_clusters(bs);
 }
 
+static int qcow2_get_conversion_options(BlockDriverState *bs,
+                                        BlockConversionOptions *options)
+{
+    BDRVQcowState *s = bs->opaque;
+
+    options->image_size = bs->total_sectors * BDRV_SECTOR_SIZE;
+    options->cluster_size = s->cluster_sectors * BDRV_SECTOR_SIZE;
+    options->allocation_size =  s->cluster_sectors * BDRV_SECTOR_SIZE;
+    if (s->crypt_method == QCOW_CRYPT_NONE) {
+        options->encryption_type = BLOCK_CRYPT_NONE;
+    } else if (s->crypt_method == QCOW_CRYPT_AES) {
+        options->encryption_type = BLOCK_CRYPT_AES;
+    } else {
+        return -1; //TODO: FIXME
+    }
+    return 0;
+}
 static QEMUOptionParameter qcow2_create_options[] = {
     {
         .name = BLOCK_OPT_SIZE,
@@ -1497,9 +1514,10 @@ static BlockDriver bdrv_qcow2 = {
     .create_options = qcow2_create_options,
     .bdrv_check = qcow2_check,
 
-    .bdrv_get_mapping = qcow2_get_mapping,
-    .bdrv_map         = qcow2_map,
-    .bdrv_copy_header = qcow2_copy_header,
+    .bdrv_get_mapping            = qcow2_get_mapping,
+    .bdrv_map                    = qcow2_map,
+    .bdrv_copy_header            = qcow2_copy_header,
+    .bdrv_get_conversion_options = qcow2_get_conversion_options,
 };
 
 static void bdrv_qcow2_init(void)
-- 
1.7.6.rc1

[Qemu-devel] [RFC 21/24] qcow2: add qcow2_open_conversion_target()

[Devin Nakamura]

Still in very raw form.  Not likely to work yet

Signed-off-by: Devin Nakamura <devin122@gmail.com>
---
 block/qcow2.c |  124 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 124 insertions(+), 0 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 86df65d..f1e1e12 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1444,6 +1444,129 @@ static int qcow2_get_conversion_options(BlockDriverState *bs,
     }
     return 0;
 }
+
+
+static int qcow2_open_conversion_target(BlockDriverState *bs,
+                                        BlockConversionOptions *drv_options,
+                                        QEMUOptionParameter *usr_options)
+{
+    uint64_t imgsize, sectorsize, clusterbits;
+    uint64_t num_refcount_blocks;
+    uint16_t *refcount_block;
+    uint64_t table_index, table_limit, i;
+
+    sectorsize = drv_options->cluster_size;
+    clusterbits = 0;
+    while (~sectorsize & 1) {
+        sectorsize >>=1;
+        clusterbits++;
+    }
+    if (sectorsize != 1) {
+        return -EINVAL;
+    }
+
+
+    imgsize = drv_options->image_size;
+
+    BDRVQcowState *s = bs->opaque;
+    s->crypt_method_header = QCOW_CRYPT_NONE;
+    s->cluster_bits = clusterbits;
+    s->cluster_size = 1 << s->cluster_bits;
+    s->cluster_sectors = 1 << (s->cluster_bits - 9);
+    s->l2_bits = s->cluster_bits - 3; /* L2 is always one cluster */
+    s->l2_size = 1 << s->l2_bits;
+    bs->total_sectors = imgsize / 512;
+    s->csize_shift = (62 - (s->cluster_bits - 8));
+    s->csize_mask = (1 << (s->cluster_bits - 8)) - 1;
+    s->cluster_offset_mask = (1LL << s->csize_shift) - 1;
+    //s->refcount_table_offset = 0;
+    //s->refcount_table_size = 0;
+    s->snapshots_offset = 0;
+    s->nb_snapshots = 0;
+    /* alloc L2 table/refcount block cache */
+    s->l2_table_cache = qcow2_cache_create(bs, L2_CACHE_SIZE, true); //todo: double check writethrough
+    s->refcount_block_cache = qcow2_cache_create(bs, REFCOUNT_CACHE_SIZE,
+        true);
+
+    s->cluster_cache = qemu_malloc(s->cluster_size);
+    /* one more sector for decompressed data alignment */
+    s->cluster_data = qemu_malloc(QCOW_MAX_CRYPT_CLUSTERS * s->cluster_size
+                                  + 512);
+    s->cluster_cache_offset = -1;
+
+    //Make up a refcount table
+    const uint64_t num_clusters = bs->file->total_sectors >>
+        (clusterbits - BDRV_SECTOR_BITS);
+    const uint64_t cluster_size = 1 << s->cluster_bits;
+    const uint64_t uint64_per_cluster = cluster_size / sizeof(uint64_t);
+    const uint64_t uint16_per_cluster = cluster_size / sizeof(uint16_t);
+    const uint64_t num_l2_tables = (num_clusters + uint64_per_cluster - 1) / 
+                                    uint64_per_cluster;
+    const uint64_t num_l1_clusters = (num_l2_tables + uint64_per_cluster - 1)
+                                     / uint64_per_cluster);
+    s->l1_size = num_l2_tables;
+    s->l1_table = qemu_mallocz(s->l1_size * s->cluster_size);
+    num_refcount_blocks = num_clusters + num_l2_tables + num_l1_clusters;
+    num_refcount_blocks = (num_refcount_blocks + uint16_per_cluster - 1) /
+                          uint16_per_cluster;
+
+    /* Account for refcount blocks used to track refcount blocks */
+    num_refcount_blocks *= uint16_per_cluster;
+    num_refcount_blocks /= uint16_per_cluster -1;
+    num_refcount_blocks += 1; //todo: fix rounding
+
+    /* Account for refcount blocks used to track refcount table */
+    num_refcount_blocks *= uint64_per_cluster;
+    num_refcount_blocks /= uint64_per_cluster -1;
+    num_refcount_blocks += 1; // todo: fix rounding
+
+    s->refcount_table_size = (num_refcount_blocks / uint64_per_cluster) +1;
+    s->refcount_table = qemu_mallocz(s->refcount_table_size * s->cluster_size);
+    refcount_block = qemu_mallocz(s->cluster_size);
+
+//    s->refcount_table_size = num_refcount_blocks;
+    s->free_cluster_index = num_clusters;
+    s->l1_table_offset = s->free_cluster_index << s->cluster_bits;
+    s->free_cluster_index += num_l1_clusters;
+    s->refcount_table_offset = s->free_cluster_index << s->cluster_bits;
+    s->free_cluster_index++;
+
+    /* assign offsets for all refcount blocks */
+    for (table_index = 0; table_index < num_refcount_blocks; table_index++) {
+        s->refcount_table[table_index] = s->free_cluster_index++ << s->cluster_bits;
+    }
+
+    /* set all refcounts in the block to 1 */
+    for (i=0; i < uint16_per_cluster; i++) {
+        refcount_block[i] = cpu_to_be16(1);
+    }
+
+    //TODO: double check this, it seems a bit fishy
+    table_limit = s->free_cluster_index / uint16_per_cluster;
+    for(table_index = 0; table_index < table_limit; table_index++) {
+        bdrv_write_sync(bs->file, s->refcount_table[table_index] >> BDRV_SECTOR_BITS,
+            (uint8_t*) refcount_block, s->cluster_sectors);
+    }
+
+    table_limit = s->free_cluster_index % uint16_per_cluster;
+    for(i=table_limit; i < uint64_per_cluster; i++) {
+        refcount_block[i] = 0;
+    }
+    bdrv_write_sync(bs->file, s->refcount_table[table_index] >> BDRV_SECTOR_BITS,
+            (uint8_t*) refcount_block, s->cluster_sectors);
+    uint64_t* be_refcount_table = qemu_mallocz(s->refcount_table_size * s->cluster_size);
+    for(i=0; i < s->refcount_table_size; i++) {
+        be_refcount_table[i] = cpu_to_be64(s->refcount_table[i]);
+    }
+    bdrv_write_sync(bs->file, s->refcount_table_offset >> BDRV_SECTOR_BITS,
+                    (uint8_t*)be_refcount_table,s->refcount_table_size * s->cluster_size);
+    QLIST_INIT(&s->cluster_allocs);
+
+    qemu_free(refcount_block);
+    return 0;
+}
+
+
 static QEMUOptionParameter qcow2_create_options[] = {
     {
         .name = BLOCK_OPT_SIZE,
@@ -1518,6 +1641,7 @@ static BlockDriver bdrv_qcow2 = {
     .bdrv_map                    = qcow2_map,
     .bdrv_copy_header            = qcow2_copy_header,
     .bdrv_get_conversion_options = qcow2_get_conversion_options,
+    .bdrv_open_conversion_target = qcow2_open_conversion_target,
 };
 
 static void bdrv_qcow2_init(void)
-- 
1.7.6.rc1

[Qemu-devel] [RFC 22/24] qemu-io: make map command use new block mapping function

[Devin Nakamura]

bdrv_get_mapping will be used when it is defined,
otherwise default to old behaviour.

Signed-off-by: Devin Nakamura <devin122@gmail.com>
---
 qemu-io.c |   23 ++++++++++++++++++++++-
 1 files changed, 22 insertions(+), 1 deletions(-)

diff --git a/qemu-io.c b/qemu-io.c
index a553d0c..caf51fe 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -1543,7 +1543,7 @@ static const cmdinfo_t alloc_cmd = {
     .oneline    = "checks if a sector is present in the file",
 };
 
-static int map_f(int argc, char **argv)
+static int map_f_old(int argc, char **argv)
 {
     int64_t offset;
     int64_t nb_sectors;
@@ -1570,6 +1570,27 @@ static int map_f(int argc, char **argv)
     return 0;
 }
 
+static int map_f(int argc, char **argv)
+{
+    uint64_t host_offset, guest_offset, count;
+    int ret;
+
+    if (!bs->drv->bdrv_get_mapping) {
+        return map_f_old(argc, argv);
+    }
+
+    guest_offset = count = 0;
+    printf("  Guest Offset   -    Host Offset   -      Count\n");
+    do {
+        ret = bdrv_get_mapping(bs, &guest_offset, &host_offset, &count);
+        if (count) {
+            printf("%016lx - %016lx - %016lx\n", guest_offset, host_offset,
+                   count);
+        }
+    } while (!ret && count > 0);
+    return ret;
+}
+
 static const cmdinfo_t map_cmd = {
        .name           = "map",
        .argmin         = 0,
-- 
1.7.6.rc1

[Qemu-devel] [RFC 23/24] qemu-io: add setmap command

[Devin Nakamura]

Signed-off-by: Devin Nakamura <devin122@gmail.com>
---
 qemu-io.c |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/qemu-io.c b/qemu-io.c
index caf51fe..a49f62a 100644
--- a/qemu-io.c
+++ b/qemu-io.c
@@ -1601,6 +1601,29 @@ static const cmdinfo_t map_cmd = {
 };
 
 
+static int setmap_f(int argc, char **argv)
+{
+    uint64_t guest_offset, host_offset, count;
+    guest_offset = cvtnum(argv[1]);
+    host_offset = cvtnum(argv[2]);
+    if (argc == 4) {
+        count =  cvtnum(argv[3]);
+    } else {
+        /*count = bs->s*/
+        count = 0; /*TODO: fix*/
+    }
+    return bdrv_map(bs, guest_offset, host_offset, count);
+}
+
+static const cmdinfo_t setmap_cmd = {
+    .name = "setmap",
+    .argmin = 2,
+    .argmax = 3,
+    .cfunc = setmap_f,
+    .args = "",
+    .oneline = "Sets mapping in image file",
+};
+
 static int close_f(int argc, char **argv)
 {
     bdrv_close(bs);
@@ -1835,6 +1858,7 @@ int main(int argc, char **argv)
     add_command(&discard_cmd);
     add_command(&alloc_cmd);
     add_command(&map_cmd);
+    add_command(&setmap_cmd);
 
     add_args_command(init_args_command);
     add_check_command(init_check_command);
-- 
1.7.6.rc1

[Qemu-devel] [RFC 24/24] qemu-img: add inplace conversion to qemu-img

[Devin Nakamura]

Signed-off-by: Devin Nakamura <devin122@gmail.com>
---
 Makefile         |    2 +
 qemu-img-cmds.hx |    6 +++++
 qemu-img.c       |   64 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 72 insertions(+), 0 deletions(-)

diff --git a/Makefile b/Makefile
index daa3aa0..243c818 100644
--- a/Makefile
+++ b/Makefile
@@ -148,6 +148,8 @@ qemu-nbd$(EXESUF): qemu-nbd.o qemu-tool.o qemu-error.o $(oslib-obj-y) $(trace-ob
 
 qemu-io$(EXESUF): qemu-io.o cmd.o qemu-tool.o qemu-error.o $(oslib-obj-y) $(trace-obj-y) $(block-obj-y) $(qobject-obj-y) $(version-obj-y) qemu-timer-common.o
 
+qemu-convert$(EXESUF): qemu-convert.o cmd.o qemu-tool.o qemu-error.o $(oslib-obj-y) $(trace-obj-y) $(block-obj-y) $(qobject-obj-y) $(version-obj-y) qemu-timer-common.o
+
 qemu-img-cmds.h: $(SRC_PATH)/qemu-img-cmds.hx
 	$(call quiet-command,sh $(SRC_PATH)/scripts/hxtool -h < $< > $@,"  GEN   $@")
 
diff --git a/qemu-img-cmds.hx b/qemu-img-cmds.hx
index 1299e83..ea97f83 100644
--- a/qemu-img-cmds.hx
+++ b/qemu-img-cmds.hx
@@ -33,6 +33,12 @@ STEXI
 @item convert [-c] [-p] [-f @var{fmt}] [-O @var{output_fmt}] [-o @var{options}] [-s @var{snapshot_name}] @var{filename} [@var{filename2} [...]] @var{output_filename}
 ETEXI
 
+DEF("convert-inplace", img_convert_inplace,
+    "convert-inplace filename target_format")
+STEXI
+@item convert-inplace @var{filename} @var{target_format}
+ETEXI
+
 DEF("info", img_info,
     "info [-f fmt] filename")
 STEXI
diff --git a/qemu-img.c b/qemu-img.c
index b205e98..c22066e 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1024,6 +1024,70 @@ out:
     return 0;
 }
 
+static int img_convert_inplace(int argc, char **argv)
+{
+    int ret;
+    BlockDriverState *bs_src, *bs_tgt;
+    QEMUOptionParameter *options = NULL;
+    BlockConversionOptions drv_options;
+    uint64_t guest_offset, host_offset, count;
+    guest_offset = host_offset = count = 0;
+    bs_src = bdrv_new("");
+    ret = bdrv_open(bs_src, argv[1], BDRV_O_RDWR, NULL);
+    if (ret) {
+        printf("Failed to open source image: %s\n", argv[1]);
+        return ret;
+    }
+
+    ret = bdrv_get_conversion_options(bs_src, &drv_options);
+    if (ret) {
+        printf("Failed getting conversion options\n");
+        goto end_before_tgt;
+    }
+
+    ret = bdrv_open_conversion_target(&bs_tgt, bs_src->file, &drv_options, options, argv[2]);
+    if (ret) {
+        printf("Failed to open conversion target, format: %s\n", argv[2]);
+        switch(ret) {
+            case -ENOENT:
+                printf("No block driver found\n"); break;
+            case -ENOTSUP:
+                printf("Not supported by block driver\n"); break;
+            default:
+                printf("Unknown error (%i)\n", ret);
+        }
+        goto end_before_tgt;
+    }
+
+    do {
+        ret = bdrv_get_mapping(bs_src, &guest_offset, &host_offset, &count);
+        if (!ret) {
+            ret = bdrv_map(bs_tgt, guest_offset, host_offset, count);
+        }
+    } while (count != 0);
+
+    if (ret) {
+        printf("An error occured during mapping process\n");
+        goto end;
+    }
+    ret = bdrv_copy_header(bs_tgt);
+    if (ret) {
+        printf("An error occured during header copy\n");
+        goto end;
+    }
+
+    ret = 0;
+
+end:
+    bdrv_close(bs_tgt);
+end_before_tgt:
+    bdrv_close(bs_src);
+    return ret;
+
+
+
+}
+
 
 static void dump_snapshots(BlockDriverState *bs)
 {
-- 
1.7.6.rc1

Re: [Qemu-devel] [RFC 01/24] block: add block conversion api

[Kevin Wolf]

Am 29.07.2011 06:49, schrieb Devin Nakamura:
> add functions to block driver interface to support inplace image conversion
> 
> Signed-off-by: Devin Nakamura <devin122@gmail.com>
> ---
>  block.h     |    2 +
>  block_int.h |   88 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 90 insertions(+), 0 deletions(-)
> 
> diff --git a/block.h b/block.h
> index 59cc410..a1c4cc8 100644
> --- a/block.h
> +++ b/block.h
> @@ -251,6 +251,8 @@ int64_t bdrv_get_dirty_count(BlockDriverState *bs);
>  void bdrv_set_in_use(BlockDriverState *bs, int in_use);
>  int bdrv_in_use(BlockDriverState *bs);
>  
> +typedef struct BlockConversionOptions BlockConversionOptions;
> +
>  typedef enum {
>      BLKDBG_L1_UPDATE,
>  
> diff --git a/block_int.h b/block_int.h
> index efb6803..84bf89e 100644
> --- a/block_int.h
> +++ b/block_int.h
> @@ -41,6 +41,9 @@
>  #define BLOCK_OPT_PREALLOC      "preallocation"
>  #define BLOCK_OPT_SUBFMT        "subformat"
>  
> +#define BLOCK_CRYPT_NONE    0
> +#define BLOCK_CRYPT_AES     1
> +
>  typedef struct AIOPool {
>      void (*cancel)(BlockDriverAIOCB *acb);
>      int aiocb_size;
> @@ -139,6 +142,85 @@ struct BlockDriver {
>       */
>      int (*bdrv_has_zero_init)(BlockDriverState *bs);
>  
> +    /* In-place image conversion */
> +
> +    /**
> +     * Opens an image conversion target.
> +     *
> +     * @param bs          Basic Initialization done by
> +     *                    bdrv_open_conversion_target() Still need to set format
> +     *                    specific data.
> +     * @param usr_options Creation options.
> +     * @param drv_options Conversion Options
> +     * @return            Returns non-zero on failure.
> +     */
> +    int (*bdrv_open_conversion_target)(BlockDriverState *bs,
> +        BlockConversionOptions *drv_options, QEMUOptionParameter *usr_options);
> +
> +    /**
> +     * Gets a mapping in the image file.
> +     *
> +     * The function starts searching for a mapping at
> +     * starting_guest_offset = guest_offset + contiguous_bytes
> +     *
> +     * @param bs[in]                   The image in which to find mapping.
> +     * @param guest_offset[in,out]     On function entry used to calculate
> +     *                                 starting search address.
> +     *                                 On function exit contains the starting
> +     *                                 guest offset of the mapping.
> +     * @param host_offset[out]         The starting image file offset for the
> +     *                                 mapping.
> +     * @param contiguous_bytes[in,out] On function entry used to calculate
> +     *                                 starting search address.
> +     *                                 On function exit contains the number of
> +     *                                 bytes for which this mapping is valid.
> +     *                                 A value of 0 means there are no more
> +     *                                 mappings in the image.
> +     * @return                         Returns non-zero on error.
> +     */
> +    int (*bdrv_get_mapping)(BlockDriverState *bs, uint64_t *guest_offset,
> +        uint64_t *host_offset, uint64_t *contiguous_bytes);

Last time I suggested to change the semantics of this function as follows:

> I think it would be less surprising if the function worked like
> bdrv_is_allocated(). That is, it doesn't search for mapped clusters, but
> always returns information for exactly the given offset. It would return
> if the range starting at the given offset is used or free.
> 
> If the caller wants to find the next existing mapping, he can just take
> contiguous_bytes of the free region and add it to his current offset.

Do you believe that this change would be a bad idea or did you just
forget it?

> +    /**
> +     * Sets a mapping in the image file.
> +     *
> +     * @param bs               Usualy opened with bdrv_open_conversion_target

Typo: "Usually"

> +     * @param guest_offset     The starting guest offset of the mapping
> +     *                         (in bytes). Function fails and returns -EINVAL if
> +     *                         not cluster aligned.
> +     * @param host_offset      The starting image offset of the mapping
> +     *                         (in bytes). Function fails and returns -EINVAL if
> +     *                         not cluster aligned.
> +     * @param contiguous_bytes The number of bytes for which this mapping exists
> +     *                         Function fails and returns -EINVAL if not cluster
> +     *                         aligned
> +     * @return                 Returns non-zero on error
> +     */
> +    int (*bdrv_map)(BlockDriverState *bs, uint64_t guest_offset,
> +        uint64_t host_offset, uint64_t contiguous_bytes);

Usually you would describe the -EINVAL part as part of @return. Not that
it's unclear this way, just gets a bit longer than necessary.

> +
> +    /**
> +     * Copies out the header of a conversion target
> +     *
> +     * Saves the current header for the image in a temporary file and overwrites
> +     * it with the header for the new format (at the moment the header is
> +     * assumed to be 1 sector)
> +     *
> +     * @param bs  Usualy opened with bdrv_open_conversion_target().
> +     * @return    Returns non-zero on failure
> +     */
> +    int (*bdrv_copy_header) (BlockDriverState *bs);

Is it true with the current implementation that the old header is saved?
I think it's supposed to be used if updating the header goes wrong. Who
will read the temporary file in this case? Does the user need to know
where it is or even have control over it?

> +
> +    /**
> +     * Asks the block driver what options should be used to create a conversion
> +     * target.
> +     *
> +     * @param options[out] Block conversion options
> +     */
> +    int (*bdrv_get_conversion_options)(BlockDriverState *bs,
> +         BlockConversionOptions *options);
> +
> +
>      QLIST_ENTRY(BlockDriver) list;
>  };
>  
> @@ -263,4 +345,10 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf)
>      DEFINE_PROP_UINT32("discard_granularity", _state, \
>                         _conf.discard_granularity, 0)
>  
> +struct BlockConversionOptions {
> +    int encryption_type;

This could be an enum.

> +    uint64_t image_size;
> +    uint64_t cluster_size;
> +    uint64_t allocation_size;
> +};

Comments explaining the difference between cluster_size and allocation_size?

Kevin

Re: [Qemu-devel] [RFC 03/24] block: add bdrv_open_conversion_target()

[Kevin Wolf]

Am 29.07.2011 06:49, schrieb Devin Nakamura:
> Conflicts:
> 
> 	block.h

You can probably remove this note. ;-)

> 
> Signed-off-by: Devin Nakamura <devin122@gmail.com>
> ---
>  block.c |   32 ++++++++++++++++++++++++++++++++
>  block.h |    4 ++++
>  2 files changed, 36 insertions(+), 0 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 4503b7b..9530577 100644
> --- a/block.c
> +++ b/block.c
> @@ -3038,6 +3038,38 @@ out:
>      return ret;
>  }
>  
> +int bdrv_open_conversion_target(BlockDriverState **bs, BlockDriverState *file,
> +                                BlockConversionOptions *drv_options,
> +                                QEMUOptionParameter *usr_options,
> +                                const char *target_fmt)
> +{
> +    BlockDriver *drv;
> +    BlockDriverState *bss;
> +
> +    drv = bdrv_find_format(target_fmt);
> +    if (!drv) {
> +        return -ENOENT;
> +    }
> +
> +    if (!drv->bdrv_open_conversion_target) {
> +        return -ENOTSUP;
> +    }
> +
> +    *bs = bdrv_new("");
> +    bss = *bs;
> +    bss->file = file;
> +    bss->total_sectors = drv_options->image_size >> BDRV_SECTOR_BITS;
> +    bss->encrypted = 0;
> +    bss->valid_key = 0;
> +    bss->open_flags = 0;
> +    /* buffer_alignment defaulted to 512, drivers can change this value */
> +    bss->buffer_alignment = 512;
> +    bss->opaque = qemu_mallocz(drv->instance_size);
> +    bss->drv = drv;
> +    return drv->bdrv_open_conversion_target(bss, drv_options, usr_options);
> +
> +}

How big are the differences really to bdrv_open_common? Have you checked
if you could reuse it? It looks to me as if you're just handling fewer
cases and could achieve the same by passing the right flags to
bdrv_open_common. The only real difference is drv->bdrv_open vs.
drv->bdrv_open_conversion_target, which could probably be handled by
another flag.

The problem with keeping it separate is that it makes it easy to change
bdrv_open without changing bdrv_open_conversion_target. Most people
touching the code won't use in-place conversion very often, so they
won't notice any breakage in their testing.

Kevin

Re: [Qemu-devel] [RFC 07/24] qed: make qed_alloc_clusters round up offset to nearest cluster

[Kevin Wolf]

Am 29.07.2011 06:49, schrieb Devin Nakamura:
> Signed-off-by: Devin Nakamura <devin122@gmail.com>
> ---
>  block/qed.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/block/qed.c b/block/qed.c
> index 3970379..00cf895 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> @@ -263,6 +263,9 @@ static int qed_read_string(BlockDriverState *file, uint64_t offset, size_t n,
>   */
>  static uint64_t qed_alloc_clusters(BDRVQEDState *s, unsigned int n)
>  {
> +    s->file_size = (s->file_size + s->header.cluster_size -1)
> +        / s->header.cluster_size;
> +    s->file_size *= s->header.cluster_size;

This looks a bit simpler:

s->file_size = qed_start_of_cluster(s->file_size +
s->header.cluster_size - 1);

Kevin

Re: [Qemu-devel] [RFC 16/24] qcow2: add qcow2_drop_leaked_clusters()

[Kevin Wolf]

Am 29.07.2011 06:49, schrieb Devin Nakamura:
> Signed-off-by: Devin Nakamura <devin122@gmail.com>
> ---
>  block/qcow2-refcount.c |   34 ++++++++++++++++++++++++++++++++++
>  block/qcow2.h          |    2 ++
>  2 files changed, 36 insertions(+), 0 deletions(-)

Just for the record, discussed on IRC: Maybe it would be better to leave
this functionality inside the normal qcow2_check_refcounts()
implementation which would get a new flag that says "fix leaked clusters".

This would allow a trivial implementation of a qemu-img check --fix,
which is something that we've been wanting for a while.

Kevin

Re: [Qemu-devel] [RFC 18/24] qcow2: add qcow2_map

[Kevin Wolf]

Am 29.07.2011 06:49, schrieb Devin Nakamura:
> Signed-off-by: Devin Nakamura <devin122@gmail.com>
> ---
>  block/qcow2-cluster.c |   49 +++++++++++++++++++++++++++++++++++++++++++++++++
>  block/qcow2.c         |    1 +
>  block/qcow2.h         |    3 +++
>  3 files changed, 53 insertions(+), 0 deletions(-)
> 
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index ca56918..848f2ee 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -977,3 +977,52 @@ int qcow2_discard_clusters(BlockDriverState *bs, uint64_t offset,
>  
>      return 0;
>  }
> +
> +int qcow2_map(BlockDriverState *bs, uint64_t guest_offset,
> +    uint64_t host_offset, uint64_t contiguous_bytes)
> +{
> +    BDRVQcowState *s = bs->opaque;
> +    unsigned int nelms;
> +
> +
> +    if ((s->cluster_size - 1) & guest_offset) {

Usually you would have guest_offset first.

> +        return -EINVAL;
> +    }
> +
> +    if (contiguous_bytes % s->cluster_size) {
> +        return -EINVAL;
> +    }

Any reason why the two checks are different? I don't really care if they
use & or %, but they should use the same thing.

host_offset isn't checked at all?

> +
> +    nelms = s->l2_size / sizeof(uint64_t);

s->l2_size is already in elements, not in bytes.

> +
> +    while (contiguous_bytes > 0) {
> +        unsigned int l1_index, l2_index;
> +        uint64_t *l2_table;
> +        int ret;
> +        l1_index = guest_offset >> (s->l2_bits + s->cluster_bits);
> +        l2_index = (guest_offset  >> s->cluster_bits) & (s->l2_size - 1);
> +
> +        if (!s->l1_table[l1_index]) {
> +            ret = l2_allocate(bs, l1_index, &l2_table);
> +            if (ret) {
> +                return ret;
> +            }
> +        } else {
> +            ret = l2_load(bs, s->l1_table[l1_index] & ~QCOW_OFLAG_COPIED, &l2_table);
> +            if (ret) {
> +                return ret;
> +            }
> +        }

Can't you use get_cluster_table() for this part?

> +        qcow2_cache_entry_mark_dirty(s->l2_table_cache, l2_table);
> +
> +        for (; l2_index < nelms && contiguous_bytes > 0;  l2_index++) {
> +            l2_table[l2_index] = cpu_to_be64(host_offset | QCOW_OFLAG_COPIED);

You should increase the refcount for host_offset and set
QCOW_OFLAG_COPIED only if it becomes 1. Or maybe we should just fail
bdrv_map if the old refcount is != 0.

> +            host_offset += s->cluster_size;
> +            contiguous_bytes -= s->cluster_size;
> +        }
> +
> +        qcow2_cache_put(bs, s->l2_table_cache, (void **) &l2_table);
> +
> +    }
> +    return 0;
> +}

Kevin

Re: [Qemu-devel] [RFC 19/24] qcow2: add qcow2_copy_header()

[Kevin Wolf]

Am 29.07.2011 06:49, schrieb Devin Nakamura:
> Signed-off-by: Devin Nakamura <devin122@gmail.com>
> ---
>  block/qcow2.c |   54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 54 insertions(+), 0 deletions(-)

Do we really need to build a completely new header? We should already
have one at a different offset that we only need to copy, no?

Kevin

Re: [Qemu-devel] [RFC 21/24] qcow2: add qcow2_open_conversion_target()

[Kevin Wolf]

Am 29.07.2011 06:49, schrieb Devin Nakamura:
> Still in very raw form.  Not likely to work yet
> 
> Signed-off-by: Devin Nakamura <devin122@gmail.com>

I don't think it's quite as easy.

The problem is that qcow2 will access the image header in some places,
for example when growing the L1 or refcount table. You need to make sure
that it doesn't destroy the source format header, but writes to the
target format header at a different offset.

I think much of qcow2_open and qcow2_open_conversion_target could be the
same. That is, both could call a common function that takes a parameter
for the header offset.

The other part of qcow2_open_conversion_target (creating a new header
and a basic L1/refcount table) could possibly be shared with
qcow2_create2, though I'm not quite as sure about this one.

> ---
>  block/qcow2.c |  124 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 124 insertions(+), 0 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 86df65d..f1e1e12 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1444,6 +1444,129 @@ static int qcow2_get_conversion_options(BlockDriverState *bs,
>      }
>      return 0;
>  }
> +
> +
> +static int qcow2_open_conversion_target(BlockDriverState *bs,
> +                                        BlockConversionOptions *drv_options,
> +                                        QEMUOptionParameter *usr_options)
> +{
> +    uint64_t imgsize, sectorsize, clusterbits;
> +    uint64_t num_refcount_blocks;
> +    uint16_t *refcount_block;
> +    uint64_t table_index, table_limit, i;
> +
> +    sectorsize = drv_options->cluster_size;

Sectors aren't clusters.

> +    clusterbits = 0;
> +    while (~sectorsize & 1) {
> +        sectorsize >>=1;
> +        clusterbits++;
> +    }
> +    if (sectorsize != 1) {
> +        return -EINVAL;
> +    }

Have a look at qcow2_create2, it uses ffs() to do this.

> +
> +
> +    imgsize = drv_options->image_size;
> +
> +    BDRVQcowState *s = bs->opaque;
> +    s->crypt_method_header = QCOW_CRYPT_NONE;

Shouldn't this be taken from drv_options?

> +    s->cluster_bits = clusterbits;
> +    s->cluster_size = 1 << s->cluster_bits;
> +    s->cluster_sectors = 1 << (s->cluster_bits - 9);
> +    s->l2_bits = s->cluster_bits - 3; /* L2 is always one cluster */
> +    s->l2_size = 1 << s->l2_bits;
> +    bs->total_sectors = imgsize / 512;
> +    s->csize_shift = (62 - (s->cluster_bits - 8));
> +    s->csize_mask = (1 << (s->cluster_bits - 8)) - 1;
> +    s->cluster_offset_mask = (1LL << s->csize_shift) - 1;
> +    //s->refcount_table_offset = 0;
> +    //s->refcount_table_size = 0;
> +    s->snapshots_offset = 0;
> +    s->nb_snapshots = 0;
> +    /* alloc L2 table/refcount block cache */
> +    s->l2_table_cache = qcow2_cache_create(bs, L2_CACHE_SIZE, true); //todo: double check writethrough

writethrough mode will hurt performance and isn't required here.

> +    s->refcount_block_cache = qcow2_cache_create(bs, REFCOUNT_CACHE_SIZE,
> +        true);
> +
> +    s->cluster_cache = qemu_malloc(s->cluster_size);
> +    /* one more sector for decompressed data alignment */
> +    s->cluster_data = qemu_malloc(QCOW_MAX_CRYPT_CLUSTERS * s->cluster_size
> +                                  + 512);
> +    s->cluster_cache_offset = -1;
> +
> +    //Make up a refcount table
> +    const uint64_t num_clusters = bs->file->total_sectors >>
> +        (clusterbits - BDRV_SECTOR_BITS);

This is the number of cluster in the image file.

> +    const uint64_t cluster_size = 1 << s->cluster_bits;
> +    const uint64_t uint64_per_cluster = cluster_size / sizeof(uint64_t);
> +    const uint64_t uint16_per_cluster = cluster_size / sizeof(uint16_t);
> +    const uint64_t num_l2_tables = (num_clusters + uint64_per_cluster - 1) / 
> +                                    uint64_per_cluster;
> +    const uint64_t num_l1_clusters = (num_l2_tables + uint64_per_cluster - 1)
> +                                     / uint64_per_cluster);

L1/L2 tables are for clusters on the virtual disk. This can be something
completely different, and with some bad luck even larger than what you
calculate here (consider an image where in each L2 table only one
cluster is allocated - this will make the image files very small, but
requires still many L2 tables).

> +    s->l1_size = num_l2_tables;
> +    s->l1_table = qemu_mallocz(s->l1_size * s->cluster_size);
> +    num_refcount_blocks = num_clusters + num_l2_tables + num_l1_clusters;
> +    num_refcount_blocks = (num_refcount_blocks + uint16_per_cluster - 1) /
> +                          uint16_per_cluster;
> +
> +    /* Account for refcount blocks used to track refcount blocks */
> +    num_refcount_blocks *= uint16_per_cluster;
> +    num_refcount_blocks /= uint16_per_cluster -1;
> +    num_refcount_blocks += 1; //todo: fix rounding

Not sure what you're trying to do here, but align_offset() from qcow2.h
could help.

> +
> +    /* Account for refcount blocks used to track refcount table */
> +    num_refcount_blocks *= uint64_per_cluster;
> +    num_refcount_blocks /= uint64_per_cluster -1;
> +    num_refcount_blocks += 1; // todo: fix rounding
> +
> +    s->refcount_table_size = (num_refcount_blocks / uint64_per_cluster) +1;

I think the algorithm to calculate the required refcount table size
should be shared with qcow2_alloc_refcount_block.

Doesn't your algorithm forget to count the clusters used by refcount
tables/block themselves?

> +    s->refcount_table = qemu_mallocz(s->refcount_table_size * s->cluster_size);
> +    refcount_block = qemu_mallocz(s->cluster_size);
> +
> +//    s->refcount_table_size = num_refcount_blocks;
> +    s->free_cluster_index = num_clusters;
> +    s->l1_table_offset = s->free_cluster_index << s->cluster_bits;
> +    s->free_cluster_index += num_l1_clusters;
> +    s->refcount_table_offset = s->free_cluster_index << s->cluster_bits;
> +    s->free_cluster_index++;
> +
> +    /* assign offsets for all refcount blocks */
> +    for (table_index = 0; table_index < num_refcount_blocks; table_index++) {
> +        s->refcount_table[table_index] = s->free_cluster_index++ << s->cluster_bits;
> +    }
> +
> +    /* set all refcounts in the block to 1 */
> +    for (i=0; i < uint16_per_cluster; i++) {
> +        refcount_block[i] = cpu_to_be16(1);
> +    }
> +
> +    //TODO: double check this, it seems a bit fishy

That's my impression of the whole operation, too. :-)

This is why I thought it would be nice to share this code with normal
image creation. Then we have this fishy code at least only once.

Kevin

Re: [Qemu-devel] [RFC 22/24] qemu-io: make map command use new block mapping function

[Kevin Wolf]

Am 29.07.2011 06:49, schrieb Devin Nakamura:
> bdrv_get_mapping will be used when it is defined,
> otherwise default to old behaviour.
> 
> Signed-off-by: Devin Nakamura <devin122@gmail.com>

Hm, I think I would use a new command for this, like 'get_mapping'. The
old 'map' command can still be useful even for formats supporting
bdrv_get_mapping. You would use it whenever you are only interested
which offsets are allocated, but don't care about the offsets in the
image file to which they are mapped. This makes the output much shorter.

Kevin

Re: [Qemu-devel] [RFC 22/24] qemu-io: make map command use new block mapping function

[Devin Nakamura]

On Mon, Aug 1, 2011 at 11:38 AM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 29.07.2011 06:49, schrieb Devin Nakamura:
>> bdrv_get_mapping will be used when it is defined,
>> otherwise default to old behaviour.
>>
>> Signed-off-by: Devin Nakamura <devin122@gmail.com>
>
> Hm, I think I would use a new command for this, like 'get_mapping'. The
> old 'map' command can still be useful even for formats supporting
> bdrv_get_mapping. You would use it whenever you are only interested
> which offsets are allocated, but don't care about the offsets in the
> image file to which they are mapped. This makes the output much shorter.
>
> Kevin
>

Ok, no problem. Its on the todo list.

Re: [Qemu-devel] [RFC 21/24] qcow2: add qcow2_open_conversion_target()

[Devin Nakamura]

On Mon, Aug 1, 2011 at 11:26 AM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 29.07.2011 06:49, schrieb Devin Nakamura:
>> Still in very raw form.  Not likely to work yet
>>
>> Signed-off-by: Devin Nakamura <devin122@gmail.com>
>
> I don't think it's quite as easy.
>
> The problem is that qcow2 will access the image header in some places,
> for example when growing the L1 or refcount table. You need to make sure
> that it doesn't destroy the source format header, but writes to the
> target format header at a different offset.
This is why I've made sure to size the L1 and refcount tables so they
wont need to be resized. But I suppose that's really a balancing act
that's likely to break if someone makes changes to the current code,
or when snapshots are implemented

> I think much of qcow2_open and qcow2_open_conversion_target could be the
> same. That is, both could call a common function that takes a parameter
> for the header offset.
I'm almost certain I could do that (considering I lifted a lot of the
code for qcow2_open_conversion_target from qcow2_open)
> The other part of qcow2_open_conversion_target (creating a new header
> and a basic L1/refcount table) could possibly be shared with
> qcow2_create2, though I'm not quite as sure about this one.


>> +    //TODO: double check this, it seems a bit fishy
>
> That's my impression of the whole operation, too. :-)
>
> This is why I thought it would be nice to share this code with normal
> image creation. Then we have this fishy code at least only once.
>
> Kevin
>

Re: [Qemu-devel] [RFC 01/24] block: add block conversion api

[Devin Nakamura]

On Mon, Aug 1, 2011 at 9:34 AM, Kevin Wolf <kwolf@redhat.com> wrote:
>> +    /**
>> +     * Gets a mapping in the image file.
>> +     *
>> +     * The function starts searching for a mapping at
>> +     * starting_guest_offset = guest_offset + contiguous_bytes
>> +     *
>> +     * @param bs[in]                   The image in which to find mapping.
>> +     * @param guest_offset[in,out]     On function entry used to calculate
>> +     *                                 starting search address.
>> +     *                                 On function exit contains the starting
>> +     *                                 guest offset of the mapping.
>> +     * @param host_offset[out]         The starting image file offset for the
>> +     *                                 mapping.
>> +     * @param contiguous_bytes[in,out] On function entry used to calculate
>> +     *                                 starting search address.
>> +     *                                 On function exit contains the number of
>> +     *                                 bytes for which this mapping is valid.
>> +     *                                 A value of 0 means there are no more
>> +     *                                 mappings in the image.
>> +     * @return                         Returns non-zero on error.
>> +     */
>> +    int (*bdrv_get_mapping)(BlockDriverState *bs, uint64_t *guest_offset,
>> +        uint64_t *host_offset, uint64_t *contiguous_bytes);
>
> Last time I suggested to change the semantics of this function as follows:
>
>> I think it would be less surprising if the function worked like
>> bdrv_is_allocated(). That is, it doesn't search for mapped clusters, but
>> always returns information for exactly the given offset. It would return
>> if the range starting at the given offset is used or free.
>>
>> If the caller wants to find the next existing mapping, he can just take
>> contiguous_bytes of the free region and add it to his current offset.
>
> Do you believe that this change would be a bad idea or did you just
> forget it?
I was just kept putting it off because for some reason I had the idea
that it would be a lot of work to change it.  I've done it now and it
turns out I only had to change about 3 line of code.

>> +
>> +    /**
>> +     * Copies out the header of a conversion target
>> +     *
>> +     * Saves the current header for the image in a temporary file and overwrites
>> +     * it with the header for the new format (at the moment the header is
>> +     * assumed to be 1 sector)
>> +     *
>> +     * @param bs  Usualy opened with bdrv_open_conversion_target().
>> +     * @return    Returns non-zero on failure
>> +     */
>> +    int (*bdrv_copy_header) (BlockDriverState *bs);
>
> Is it true with the current implementation that the old header is saved?
> I think it's supposed to be used if updating the header goes wrong. Who
> will read the temporary file in this case? Does the user need to know
> where it is or even have control over it?
Yes, the header is in fact save in the current implementation. Its
always saved to headerbackup.temp, or something to that effect. Soon I
plan on replacing that with a randomly generated temp file and
informing the user of the name.  Also, I plan on implementing the
ability to override it with a command line option

>> +    uint64_t image_size;
>> +    uint64_t cluster_size;
>> +    uint64_t allocation_size;
>> +};
>
> Comments explaining the difference between cluster_size and allocation_size?
>
At the moment there is none, and I don't think allocation_size has a
future since the allocation size of the source image isn't really
important.

Re: [Qemu-devel] [RFC 18/24] qcow2: add qcow2_map

[Kevin Wolf]

[ Adding back qemu-devel to CC ]

Am 02.08.2011 06:27, schrieb Devin Nakamura:
> On Mon, Aug 1, 2011 at 10:32 AM, Kevin Wolf <kwolf@redhat.com> wrote:
>> Am 29.07.2011 06:49, schrieb Devin Nakamura:
>>> Signed-off-by: Devin Nakamura <devin122@gmail.com>
>>> ---
>>>  block/qcow2-cluster.c |   49 +++++++++++++++++++++++++++++++++++++++++++++++++
>>>  block/qcow2.c         |    1 +
>>>  block/qcow2.h         |    3 +++
>>>  3 files changed, 53 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
>>> index ca56918..848f2ee 100644
>>> --- a/block/qcow2-cluster.c
>>> +++ b/block/qcow2-cluster.c
>>> @@ -977,3 +977,52 @@ int qcow2_discard_clusters(BlockDriverState *bs, uint64_t offset,
>>>
>>>      return 0;
>>>  }
>>> +
>>> +int qcow2_map(BlockDriverState *bs, uint64_t guest_offset,
>>> +    uint64_t host_offset, uint64_t contiguous_bytes)
>>> +{
>>> +    BDRVQcowState *s = bs->opaque;
>>> +    unsigned int nelms;
>>> +
>>> +
>>> +    if ((s->cluster_size - 1) & guest_offset) {
>>
>> Usually you would have guest_offset first.
>>
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    if (contiguous_bytes % s->cluster_size) {
>>> +        return -EINVAL;
>>> +    }
>>
>> Any reason why the two checks are different? I don't really care if they
>> use & or %, but they should use the same thing.
> Hmm, I have no idea how that got there. Its especially weird, since I
> cant see myself writing the & check. The % one is much more my style.

The & version is a bit more efficient because it doesn't have to do a
division. But compared to I/O it doesn't make a big difference.

>>> +        qcow2_cache_entry_mark_dirty(s->l2_table_cache, l2_table);
>>> +
>>> +        for (; l2_index < nelms && contiguous_bytes > 0;  l2_index++) {
>>> +            l2_table[l2_index] = cpu_to_be64(host_offset | QCOW_OFLAG_COPIED);
>>
>> You should increase the refcount for host_offset and set
>> QCOW_OFLAG_COPIED only if it becomes 1. Or maybe we should just fail
>> bdrv_map if the old refcount is != 0.
> The problem with that is the refcounts are all 1. During
> open_conversion_target() the refcounts for all the clusters that are
> less than or equal to the file size are set to 1. This is to avoid
> allocating a cluster and overwriting original image data.

Hm, good point.

Then just check that the refcount is 1? If at some point we wanted to
support snapshots, how would we do that?

In any case, I think we need to update the comment of bdrv_map to say
something about the refcount of the clusters that are mapped (currently
something like "doesn't change the refcount, be sure to increase it
beforehand").

Kevin

Re: [Qemu-devel] [RFC 01/24] block: add block conversion api

[Stefan Hajnoczi]

On Fri, Jul 29, 2011 at 12:49:31AM -0400, Devin Nakamura wrote:
> +    /**
> +     * Gets a mapping in the image file.
> +     *
> +     * The function starts searching for a mapping at
> +     * starting_guest_offset = guest_offset + contiguous_bytes
> +     *
> +     * @param bs[in]                   The image in which to find mapping.
> +     * @param guest_offset[in,out]     On function entry used to calculate
> +     *                                 starting search address.
> +     *                                 On function exit contains the starting
> +     *                                 guest offset of the mapping.
> +     * @param host_offset[out]         The starting image file offset for the
> +     *                                 mapping.
> +     * @param contiguous_bytes[in,out] On function entry used to calculate
> +     *                                 starting search address.
> +     *                                 On function exit contains the number of
> +     *                                 bytes for which this mapping is valid.
> +     *                                 A value of 0 means there are no more
> +     *                                 mappings in the image.
> +     * @return                         Returns non-zero on error.
> +     */
> +    int (*bdrv_get_mapping)(BlockDriverState *bs, uint64_t *guest_offset,
> +        uint64_t *host_offset, uint64_t *contiguous_bytes);

Will the output value of guest_offset ever be smaller than the input
value?

It seems like this function is designed to be used as an iterator (hence
the starting address calculation).  Explicitly stating that it can be
used as an iterator with contiguous_bytes starting at 0 would be
helpful.

> +     * @param guest_offset     The starting guest offset of the mapping
> +     *                         (in bytes). Function fails and returns -EINVAL if
> +     *                         not cluster aligned.
> +     * @param host_offset      The starting image offset of the mapping
> +     *                         (in bytes). Function fails and returns -EINVAL if
> +     *                         not cluster aligned.
> +     * @param contiguous_bytes The number of bytes for which this mapping exists
> +     *                         Function fails and returns -EINVAL if not cluster
> +     *                         aligned
> +     * @return                 Returns non-zero on error
> +     */
> +    int (*bdrv_map)(BlockDriverState *bs, uint64_t guest_offset,
> +        uint64_t host_offset, uint64_t contiguous_bytes);

What flush semantics does this function have?  Do I need to call
bdrv_flush() to ensure the metadata updates are on disk?

> +
> +    /**
> +     * Copies out the header of a conversion target
> +     *
> +     * Saves the current header for the image in a temporary file and overwrites
> +     * it with the header for the new format (at the moment the header is
> +     * assumed to be 1 sector)
> +     *
> +     * @param bs  Usualy opened with bdrv_open_conversion_target().
> +     * @return    Returns non-zero on failure
> +     */
> +    int (*bdrv_copy_header) (BlockDriverState *bs);

What is the purpose of the temporary file, what filename does it need to
have, etc?  (I know some of the answers, but please document them.)

> +
> +    /**
> +     * Asks the block driver what options should be used to create a conversion
> +     * target.
> +     *
> +     * @param options[out] Block conversion options
> +     */
> +    int (*bdrv_get_conversion_options)(BlockDriverState *bs,
> +         BlockConversionOptions *options);
> +
> +
>      QLIST_ENTRY(BlockDriver) list;
>  };
>  
> @@ -263,4 +345,10 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf)
>      DEFINE_PROP_UINT32("discard_granularity", _state, \
>                         _conf.discard_granularity, 0)
>  
> +struct BlockConversionOptions {
> +    int encryption_type;
> +    uint64_t image_size;
> +    uint64_t cluster_size;

These two fields can be extracted using existing block.h APIs.  Does it
make sense to add a bdrv_get_encryption_type() instead?  That way
qemu-img info can also show display the encryption type and you can drop
this struct.

> +    uint64_t allocation_size;

What is this?

Stefan

Re: [Qemu-devel] [RFC 03/24] block: add bdrv_open_conversion_target()

[Stefan Hajnoczi]

On Fri, Jul 29, 2011 at 12:49:33AM -0400, Devin Nakamura wrote:
> +int bdrv_open_conversion_target(BlockDriverState **bs, BlockDriverState *file,
> +                                BlockConversionOptions *drv_options,
> +                                QEMUOptionParameter *usr_options,
> +                                const char *target_fmt)
> +{
> +    BlockDriver *drv;
> +    BlockDriverState *bss;
> +
> +    drv = bdrv_find_format(target_fmt);
> +    if (!drv) {
> +        return -ENOENT;
> +    }
> +
> +    if (!drv->bdrv_open_conversion_target) {
> +        return -ENOTSUP;
> +    }
> +
> +    *bs = bdrv_new("");
> +    bss = *bs;
> +    bss->file = file;
> +    bss->total_sectors = drv_options->image_size >> BDRV_SECTOR_BITS;
> +    bss->encrypted = 0;
> +    bss->valid_key = 0;
> +    bss->open_flags = 0;
> +    /* buffer_alignment defaulted to 512, drivers can change this value */
> +    bss->buffer_alignment = 512;
> +    bss->opaque = qemu_mallocz(drv->instance_size);
> +    bss->drv = drv;
> +    return drv->bdrv_open_conversion_target(bss, drv_options, usr_options);

Normally a function that fails does not leave anything allocated.
Please delete bss and *bs = NULL before returning an error.

Stefan

Re: [Qemu-devel] [RFC 04/24] block: add bdrv_get_mapping()

[Stefan Hajnoczi]

On Fri, Jul 29, 2011 at 12:49:34AM -0400, Devin Nakamura wrote:
> +/**
> + * Gets a mapping from an offset in the image to an offset within a file
> + *
> + * All offsets are in bytes. Functions starts its search at offset host_offset
> + * + count (offset within the image, not the file offset)

This function starts its search at *guest_offset + *contiguous_bytes.

> + *
> + * @param guest_offset          used to calculate starting search location on

[in, out]

Stefan

Re: [Qemu-devel] [RFC 09/24] qed: add qed_bdrv_get_mapping()

[Stefan Hajnoczi]

On Fri, Jul 29, 2011 at 12:49:39AM -0400, Devin Nakamura wrote:
> +static int bdrv_qed_get_mapping(BlockDriverState *bs, uint64_t *guest_offset,
> +                                uint64_t *host_offset,
> +                                uint64_t *contiguous_bytes)
> +{
> +    BDRVQEDState *s = bs->opaque;
> +    size_t l2_size = s->header.cluster_size * s->table_nelems;
> +    uint64_t pos = *guest_offset + *contiguous_bytes;
> +    uint64_t offset = pos;
> +    size_t len = 0;
> +    QEDRequest req = {.l2_table = NULL};
> +    int ret;
> +
> +    if (pos >= s->header.image_size) {
> +        *contiguous_bytes = 0;
> +        return 0;
> +    }
> +
> +    do {
> +        pos += len;
> +        ret = qed_find_cluster_sync(s, &req, pos, l2_size, &offset, &len);
> +    } while (ret != QED_CLUSTER_FOUND && pos < s->header.image_size);

qed_unref_l2_cache_entry(request.l2_table);

Stefan

Re: [Qemu-devel] [RFC 01/24] block: add block conversion api

[Kevin Wolf]

Am 02.08.2011 10:56, schrieb Stefan Hajnoczi:
>> @@ -263,4 +345,10 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf)
>>      DEFINE_PROP_UINT32("discard_granularity", _state, \
>>                         _conf.discard_granularity, 0)
>>  
>> +struct BlockConversionOptions {
>> +    int encryption_type;
>> +    uint64_t image_size;
>> +    uint64_t cluster_size;
> 
> These two fields can be extracted using existing block.h APIs.  Does it
> make sense to add a bdrv_get_encryption_type() instead?  That way
> qemu-img info can also show display the encryption type and you can drop
> this struct.

Hm... We already have BlockDriverInfo, which is used by qemu-img. Would
it make sense to add the fields there? In any case I would prefer
something that fills a whole struct at once instead of calling ten
separate functions and building the struct in the caller.

Kevin