[Qemu-devel] [PATCH] qcow2: Allow different cluster sizes

[Qemu-devel] [PATCH] qcow2: Allow different cluster sizes

[Kevin Wolf]

Add an option to specify the cluster size of a newly created qcow2 image.
Default is 4k which is the same value that was hard-coded before.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2.c |   53 ++++++++++++++++++++++++++++++++++++++++++++++++-----
 block_int.h   |    1 +
 2 files changed, 49 insertions(+), 5 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index b6171d2..c1821d9 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -62,6 +62,9 @@
 
 #define REFCOUNT_SHIFT 1 /* refcount size is 2 bytes */
 
+#define MIN_CLUSTER_BITS 9
+#define MAX_CLUSTER_BITS 16
+
 typedef struct QCowHeader {
     uint32_t magic;
     uint32_t version;
@@ -300,8 +303,8 @@ static int qcow_open(BlockDriverState *bs, const char *filename, int flags)
     if (header.magic != QCOW_MAGIC || header.version != QCOW_VERSION)
         goto fail;
     if (header.size <= 1 ||
-        header.cluster_bits < 9 ||
-        header.cluster_bits > 16)
+        header.cluster_bits < MIN_CLUSTER_BITS ||
+        header.cluster_bits > MAX_CLUSTER_BITS)
         goto fail;
     if (header.crypt_method > QCOW_CRYPT_AES)
         goto fail;
@@ -1583,9 +1586,30 @@ static void create_refcount_update(QCowCreateState *s,
     }
 }
 
+static int get_bits_from_size(size_t size)
+{
+    int res = 0;
+
+    if (size == 0) {
+        return -1;
+    }
+
+    while (size != 1) {
+        /* Not a power of two */
+        if (size & 1) {
+            return -1;
+        }
+
+        size >>= 1;
+        res++;
+    }
+
+    return res;
+}
+
 static int qcow_create2(const char *filename, int64_t total_size,
                         const char *backing_file, const char *backing_format,
-                        int flags)
+                        int flags, size_t cluster_size)
 {
 
     int fd, header_size, backing_filename_len, l1_size, i, shift, l2_bits;
@@ -1619,8 +1643,20 @@ static int qcow_create2(const char *filename, int64_t total_size,
         header.backing_file_size = cpu_to_be32(backing_filename_len);
         header_size += backing_filename_len;
     }
-    s->cluster_bits = 12;  /* 4 KB clusters */
+
+    /* Cluster size */
+    s->cluster_bits = get_bits_from_size(cluster_size);
+    if (s->cluster_bits < MIN_CLUSTER_BITS ||
+        s->cluster_bits > MAX_CLUSTER_BITS)
+    {
+        fprintf(stderr, "Cluster size must be a power of two between "
+            "%d and %dk\n",
+            1 << MIN_CLUSTER_BITS,
+            1 << (MAX_CLUSTER_BITS - 10));
+        return -EINVAL;
+    }
     s->cluster_size = 1 << s->cluster_bits;
+
     header.cluster_bits = cpu_to_be32(s->cluster_bits);
     header_size = (header_size + 7) & ~7;
     if (flags & BLOCK_FLAG_ENCRYPT) {
@@ -1702,6 +1738,7 @@ static int qcow_create(const char *filename, QEMUOptionParameter *options)
     const char *backing_fmt = NULL;
     uint64_t sectors = 0;
     int flags = 0;
+    size_t cluster_size = 4096;
 
     /* Read out options */
     while (options && options->name) {
@@ -1713,11 +1750,16 @@ static int qcow_create(const char *filename, QEMUOptionParameter *options)
             backing_fmt = options->value.s;
         } else if (!strcmp(options->name, BLOCK_OPT_ENCRYPT)) {
             flags |= options->value.n ? BLOCK_FLAG_ENCRYPT : 0;
+        } else if (!strcmp(options->name, BLOCK_OPT_CLUSTER_SIZE)) {
+            if (options->value.n) {
+                cluster_size = options->value.n;
+            }
         }
         options++;
     }
 
-    return qcow_create2(filename, sectors, backing_file, backing_fmt, flags);
+    return qcow_create2(filename, sectors, backing_file, backing_fmt, flags,
+        cluster_size);
 }
 
 static int qcow_make_empty(BlockDriverState *bs)
@@ -2915,6 +2957,7 @@ static QEMUOptionParameter qcow_create_options[] = {
     { BLOCK_OPT_BACKING_FILE,   OPT_STRING },
     { BLOCK_OPT_BACKING_FMT,    OPT_STRING },
     { BLOCK_OPT_ENCRYPT,        OPT_FLAG },
+    { BLOCK_OPT_CLUSTER_SIZE,   OPT_SIZE },
     { NULL }
 };
 
diff --git a/block_int.h b/block_int.h
index 782de6c..0820ed1 100644
--- a/block_int.h
+++ b/block_int.h
@@ -36,6 +36,7 @@
 #define BLOCK_OPT_COMPAT6       "compat6"
 #define BLOCK_OPT_BACKING_FILE  "backing_file"
 #define BLOCK_OPT_BACKING_FMT   "backing_fmt"
+#define BLOCK_OPT_CLUSTER_SIZE  "cluster_size"
 
 typedef struct AIOPool {
     void (*cancel)(BlockDriverAIOCB *acb);
-- 
1.6.0.6

[Qemu-devel] Re: [PATCH] qcow2: Allow different cluster sizes

[Kevin Wolf]

Kevin Wolf schrieb:
> Add an option to specify the cluster size of a newly created qcow2 image.
> Default is 4k which is the same value that was hard-coded before.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

This patch obviously depends on the previously sent bdrv_create series.
I just wanted to put it on the list already. It can probably be applied
at the same time as the bdrv_create series.

Kevin

Re: [Qemu-devel] [PATCH] qcow2: Allow different cluster sizes

[Avi Kivity]

Kevin Wolf wrote:
> Add an option to specify the cluster size of a newly created qcow2 image.
> Default is 4k which is the same value that was hard-coded before.
>   

I did an autotest run with 64K clusters, and everything seems to work 
fine.  I think we should change the default to 64K; this both improves 
performance and reduces memory utilization for large disks.

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [PATCH] qcow2: Allow different cluster sizes

[Anthony Liguori]

Kevin Wolf wrote:
> Add an option to specify the cluster size of a newly created qcow2 image.
> Default is 4k which is the same value that was hard-coded before.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>   

Does this depend on another patch?  It doesn't apply to master at all...

-- 
Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH] qcow2: Allow different cluster sizes

[Anthony Liguori]

Kevin Wolf wrote:
> Kevin Wolf schrieb:
>   
>> Add an option to specify the cluster size of a newly created qcow2 image.
>> Default is 4k which is the same value that was hard-coded before.
>>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>     
>
> This patch obviously depends on the previously sent bdrv_create series.
> I just wanted to put it on the list already. It can probably be applied
> at the same time as the bdrv_create series.
>   

Okay, I've gotten it all straightened out.

Regards,

Anthony Liguori

> Kevin
>