[Qemu-devel] [RFC PATCH v2 0/6] qemu-img: add preallocation=full

[Qemu-devel] [RFC PATCH v2 0/6] qemu-img: add preallocation=full

[Hu Tao]

This series implements full image preallocation to create a non-sparse image
file at creation time, both for raw and qcow2 format. The purpose is to avoid
performance deterioration of the guest cause by sparse image.

This series implements full preallocation by using fallocate()/posix_fallocate(),
which have the advantage that it is fast when creating large image file.
Zero-filling is not implemented, as writing zeros to image could be slow for large
file. (fallocate() ensures zero-filling, but posix_fallocate()) If users want it,
we can maybe add a option to let users have a choice. Suggestions?

Base on Kevin's patch at:
http://lists.gnu.org/archive/html/qemu-devel/2011-01/msg03017.html

Hu Tao (6):
  block: introduce prealloc_mode
  block: add BlockDriver.bdrv_preallocate.
  block/raw-posix: implement bdrv_preallocate
  raw-posix: Add full image preallocation option
  qcow2: implement bdrv_preallocate
  qcow2: Add full image preallocation option

 block.c                   | 13 +++++++++++
 block/qcow2.c             | 32 +++++++++++++++++++++------
 block/raw-posix.c         | 56 +++++++++++++++++++++++++++++++++++++++++++++++
 include/block/block.h     |  7 ++++++
 include/block/block_int.h |  3 +++
 5 files changed, 104 insertions(+), 7 deletions(-)

-- 
1.8.3.1

[Qemu-devel] [RFC PATCH v2 1/6] block: introduce prealloc_mode

[Hu Tao]

This patch prepares for the subsequent patches.

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 block/qcow2.c         | 6 +++---
 include/block/block.h | 6 ++++++
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 6e5d98d..b054a01 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1608,7 +1608,7 @@ static int qcow2_create(const char *filename, QEMUOptionParameter *options,
     uint64_t sectors = 0;
     int flags = 0;
     size_t cluster_size = DEFAULT_CLUSTER_SIZE;
-    int prealloc = 0;
+    int prealloc = PREALLOC_OFF;
     int version = 3;
     Error *local_err = NULL;
     int ret;
@@ -1629,9 +1629,9 @@ static int qcow2_create(const char *filename, QEMUOptionParameter *options,
             }
         } else if (!strcmp(options->name, BLOCK_OPT_PREALLOC)) {
             if (!options->value.s || !strcmp(options->value.s, "off")) {
-                prealloc = 0;
+                prealloc = PREALLOC_OFF;
             } else if (!strcmp(options->value.s, "metadata")) {
-                prealloc = 1;
+                prealloc = PREALLOC_METADATA;
             } else {
                 error_setg(errp, "Invalid preallocation mode: '%s'",
                            options->value.s);
diff --git a/include/block/block.h b/include/block/block.h
index 3560deb..461eaa4 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -487,4 +487,10 @@ int bdrv_debug_breakpoint(BlockDriverState *bs, const char *event,
 int bdrv_debug_resume(BlockDriverState *bs, const char *tag);
 bool bdrv_debug_is_suspended(BlockDriverState *bs, const char *tag);
 
+enum prealloc_mode {
+    PREALLOC_OFF = 0,
+    PREALLOC_METADATA,
+    PREALLOC_FULL,
+};
+
 #endif
-- 
1.8.3.1

[Qemu-devel] [RFC PATCH v2 2/6] block: add BlockDriver.bdrv_preallocate.

[Hu Tao]

This field is used to preallocate disk space for block
device.

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 block.c                   | 13 +++++++++++++
 include/block/block.h     |  1 +
 include/block/block_int.h |  3 +++
 3 files changed, 17 insertions(+)

diff --git a/block.c b/block.c
index 382ea71..3985133 100644
--- a/block.c
+++ b/block.c
@@ -3102,6 +3102,19 @@ int bdrv_has_zero_init(BlockDriverState *bs)
     return 0;
 }
 
+int bdrv_preallocate(BlockDriverState *bs, int64_t offset, int64_t length)
+{
+    if (bs->backing_hd) {
+        return -1;
+    }
+
+    if (bs->drv->bdrv_preallocate) {
+        return bs->drv->bdrv_preallocate(bs, offset, length);
+    }
+
+    return -1;
+}
+
 typedef struct BdrvCoGetBlockStatusData {
     BlockDriverState *bs;
     BlockDriverState *base;
diff --git a/include/block/block.h b/include/block/block.h
index 461eaa4..8d48c8a 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -315,6 +315,7 @@ void bdrv_drain_all(void);
 int bdrv_discard(BlockDriverState *bs, int64_t sector_num, int nb_sectors);
 int bdrv_co_discard(BlockDriverState *bs, int64_t sector_num, int nb_sectors);
 int bdrv_has_zero_init_1(BlockDriverState *bs);
+int bdrv_preallocate(BlockDriverState *bs, int64_t offset, int64_t length);
 int bdrv_has_zero_init(BlockDriverState *bs);
 int64_t bdrv_get_block_status(BlockDriverState *bs, int64_t sector_num,
                               int nb_sectors, int *pnum);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 1666066..6ac3735 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -227,6 +227,9 @@ struct BlockDriver {
      */
     int (*bdrv_has_zero_init)(BlockDriverState *bs);
 
+    int (*bdrv_preallocate)(BlockDriverState *bs, int64_t offset,
+                            int64_t length);
+
     QLIST_ENTRY(BlockDriver) list;
 };
 
-- 
1.8.3.1

[Qemu-devel] [RFC PATCH v2 3/6] block/raw-posix: implement bdrv_preallocate

[Hu Tao]

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 block/raw-posix.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index f836c8e..c563073 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -1050,6 +1050,39 @@ static int64_t raw_get_allocated_file_size(BlockDriverState *bs)
     return (int64_t)st.st_blocks * 512;
 }
 
+#ifdef __linux__
+static int raw_preallocate2(int fd, int64_t offset, int64_t length)
+{
+    int ret = -1;
+
+    ret = fallocate(fd, 0, offset, length);
+
+    /* fallback to posix_fallocate() if fallocate() is not supported */
+    if (ret < 0 && (errno == ENOSYS || errno == EOPNOTSUPP)) {
+        ret = posix_fallocate(fd, offset, length);
+    }
+
+    return ret;
+}
+#else
+static int raw_preallocate2(int fd, int64_t offset, int64_t length)
+{
+    return posix_fallocate(fd, offset, length);
+}
+#endif
+
+static int raw_preallocate(BlockDriverState *bs, int64_t offset, int64_t length)
+{
+    BDRVRawState *s = bs->opaque;
+    int64_t len = bdrv_getlength(bs);
+
+    if (offset + length < 0 || offset + length > len) {
+        return -1;
+    }
+
+    return raw_preallocate2(s->fd, offset, length);
+}
+
 static int raw_create(const char *filename, QEMUOptionParameter *options,
                       Error **errp)
 {
@@ -1221,6 +1254,7 @@ static BlockDriver bdrv_file = {
     .bdrv_close = raw_close,
     .bdrv_create = raw_create,
     .bdrv_has_zero_init = bdrv_has_zero_init_1,
+    .bdrv_preallocate = raw_preallocate,
     .bdrv_co_get_block_status = raw_co_get_block_status,
 
     .bdrv_aio_readv = raw_aio_readv,
-- 
1.8.3.1

[Qemu-devel] [RFC PATCH v2 4/6] raw-posix: Add full image preallocation option

[Hu Tao]

This patch adds a new option preallocation for raw format, and implements
full preallocation.

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 block/raw-posix.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/block/raw-posix.c b/block/raw-posix.c
index c563073..200886d 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -1089,11 +1089,22 @@ static int raw_create(const char *filename, QEMUOptionParameter *options,
     int fd;
     int result = 0;
     int64_t total_size = 0;
+    int prealloc = PREALLOC_OFF;
 
     /* Read out options */
     while (options && options->name) {
         if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
             total_size = options->value.n / BDRV_SECTOR_SIZE;
+        } else if (!strcmp(options->name, BLOCK_OPT_PREALLOC)) {
+            if (!options->value.s || !strcmp(options->value.s, "off")) {
+                prealloc = PREALLOC_OFF;
+            } else if (!strcmp(options->value.s, "full")) {
+                prealloc = PREALLOC_FULL;
+            } else {
+                error_setg(errp, "Invalid preallocation mode: '%s'",
+                           options->value.s);
+                return -EINVAL;
+            }
         }
         options++;
     }
@@ -1108,6 +1119,12 @@ static int raw_create(const char *filename, QEMUOptionParameter *options,
             result = -errno;
             error_setg_errno(errp, -result, "Could not resize file");
         }
+        if (prealloc == PREALLOC_FULL &&
+            raw_preallocate2(fd, 0, total_size * BDRV_SECTOR_SIZE) != 0) {
+            result = -errno;
+            error_setg_errno(errp, -result,
+                             "Could not preallocate data for the new file");
+        }
         if (qemu_close(fd) != 0) {
             result = -errno;
             error_setg_errno(errp, -result, "Could not close the new file");
@@ -1238,6 +1255,11 @@ static QEMUOptionParameter raw_create_options[] = {
         .type = OPT_SIZE,
         .help = "Virtual disk size"
     },
+    {
+        .name = BLOCK_OPT_PREALLOC,
+        .type = OPT_STRING,
+        .help = "Preallocation mode (allowed values: off, full)"
+    },
     { NULL }
 };
 
-- 
1.8.3.1

[Qemu-devel] [RFC PATCH v2 5/6] qcow2: implement bdrv_preallocate

[Hu Tao]

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 block/qcow2.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/block/qcow2.c b/block/qcow2.c
index b054a01..a23fade 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2180,6 +2180,12 @@ static int qcow2_amend_options(BlockDriverState *bs,
     return 0;
 }
 
+static int qcow2_preallocate(BlockDriverState *bs, int64_t offset,
+                             int64_t length)
+{
+    return bdrv_preallocate(bs->file, offset, length);
+}
+
 static QEMUOptionParameter qcow2_create_options[] = {
     {
         .name = BLOCK_OPT_SIZE,
@@ -2234,6 +2240,7 @@ static BlockDriver bdrv_qcow2 = {
     .bdrv_reopen_prepare  = qcow2_reopen_prepare,
     .bdrv_create        = qcow2_create,
     .bdrv_has_zero_init = bdrv_has_zero_init_1,
+    .bdrv_preallocate   = qcow2_preallocate,
     .bdrv_co_get_block_status = qcow2_co_get_block_status,
     .bdrv_set_key       = qcow2_set_key,
     .bdrv_make_empty    = qcow2_make_empty,
-- 
1.8.3.1

[Qemu-devel] [RFC PATCH v2 6/6] qcow2: Add full image preallocation option

[Hu Tao]

This adds a preallocation=full mode to qcow2 image creation, which
creates a non-sparse image file.

Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
---
 block/qcow2.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index a23fade..492c5e6 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1385,7 +1385,7 @@ static int qcow2_change_backing_file(BlockDriverState *bs,
     return qcow2_update_header(bs);
 }
 
-static int preallocate(BlockDriverState *bs)
+static int preallocate(BlockDriverState *bs, enum prealloc_mode mode)
 {
     uint64_t nb_sectors;
     uint64_t offset;
@@ -1394,6 +1394,8 @@ static int preallocate(BlockDriverState *bs)
     int ret;
     QCowL2Meta *meta;
 
+    assert(mode != PREALLOC_OFF);
+
     nb_sectors = bdrv_getlength(bs) >> 9;
     offset = 0;
 
@@ -1424,6 +1426,13 @@ static int preallocate(BlockDriverState *bs)
         offset += num << 9;
     }
 
+    if (mode == PREALLOC_FULL) {
+        ret = bdrv_preallocate(bs->file, 0, bdrv_getlength(bs));
+        if (ret < 0) {
+            return ret;
+        }
+    }
+
     /*
      * It is expected that the image file is large enough to actually contain
      * all of the allocated clusters (otherwise we get failing reads after
@@ -1572,11 +1581,11 @@ static int qcow2_create2(const char *filename, int64_t total_size,
         }
     }
 
-    /* And if we're supposed to preallocate metadata, do that now */
+    /* And if we're supposed to preallocate data, do that now */
     if (prealloc) {
         BDRVQcowState *s = bs->opaque;
         qemu_co_mutex_lock(&s->lock);
-        ret = preallocate(bs);
+        ret = preallocate(bs, prealloc);
         qemu_co_mutex_unlock(&s->lock);
         if (ret < 0) {
             error_setg_errno(errp, -ret, "Could not preallocate metadata");
@@ -1632,6 +1641,8 @@ static int qcow2_create(const char *filename, QEMUOptionParameter *options,
                 prealloc = PREALLOC_OFF;
             } else if (!strcmp(options->value.s, "metadata")) {
                 prealloc = PREALLOC_METADATA;
+            } else if (!strcmp(options->value.s, "full")) {
+                prealloc = PREALLOC_FULL;
             } else {
                 error_setg(errp, "Invalid preallocation mode: '%s'",
                            options->value.s);
@@ -2221,7 +2232,7 @@ static QEMUOptionParameter qcow2_create_options[] = {
     {
         .name = BLOCK_OPT_PREALLOC,
         .type = OPT_STRING,
-        .help = "Preallocation mode (allowed values: off, metadata)"
+        .help = "Preallocation mode (allowed values: off, metadata, full)"
     },
     {
         .name = BLOCK_OPT_LAZY_REFCOUNTS,
-- 
1.8.3.1

Re: [Qemu-devel] [RFC PATCH v2 2/6] block: add BlockDriver.bdrv_preallocate.

[Fam Zheng]

On 2013年11月27日 10:15, Hu Tao wrote:
> This field is used to preallocate disk space for block
> device.
>
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>   block.c                   | 13 +++++++++++++
>   include/block/block.h     |  1 +
>   include/block/block_int.h |  3 +++
>   3 files changed, 17 insertions(+)
>
> diff --git a/block.c b/block.c
> index 382ea71..3985133 100644
> --- a/block.c
> +++ b/block.c
> @@ -3102,6 +3102,19 @@ int bdrv_has_zero_init(BlockDriverState *bs)
>       return 0;
>   }
>
> +int bdrv_preallocate(BlockDriverState *bs, int64_t offset, int64_t length)
> +{
> +    if (bs->backing_hd) {
> +        return -1;

Better to use -ENOTSUP.

> +    }
> +
> +    if (bs->drv->bdrv_preallocate) {
> +        return bs->drv->bdrv_preallocate(bs, offset, length);
> +    }
> +
> +    return -1;

-ENOTSUP

Fam

> +}
> +
>   typedef struct BdrvCoGetBlockStatusData {
>       BlockDriverState *bs;
>       BlockDriverState *base;
> diff --git a/include/block/block.h b/include/block/block.h
> index 461eaa4..8d48c8a 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -315,6 +315,7 @@ void bdrv_drain_all(void);
>   int bdrv_discard(BlockDriverState *bs, int64_t sector_num, int nb_sectors);
>   int bdrv_co_discard(BlockDriverState *bs, int64_t sector_num, int nb_sectors);
>   int bdrv_has_zero_init_1(BlockDriverState *bs);
> +int bdrv_preallocate(BlockDriverState *bs, int64_t offset, int64_t length);
>   int bdrv_has_zero_init(BlockDriverState *bs);
>   int64_t bdrv_get_block_status(BlockDriverState *bs, int64_t sector_num,
>                                 int nb_sectors, int *pnum);
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 1666066..6ac3735 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -227,6 +227,9 @@ struct BlockDriver {
>        */
>       int (*bdrv_has_zero_init)(BlockDriverState *bs);
>
> +    int (*bdrv_preallocate)(BlockDriverState *bs, int64_t offset,
> +                            int64_t length);
> +
>       QLIST_ENTRY(BlockDriver) list;
>   };
>
>

Re: [Qemu-devel] [RFC PATCH v2 3/6] block/raw-posix: implement bdrv_preallocate

[Fam Zheng]

On 2013年11月27日 10:15, Hu Tao wrote:
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>   block/raw-posix.c | 34 ++++++++++++++++++++++++++++++++++
>   1 file changed, 34 insertions(+)
>
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index f836c8e..c563073 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -1050,6 +1050,39 @@ static int64_t raw_get_allocated_file_size(BlockDriverState *bs)
>       return (int64_t)st.st_blocks * 512;
>   }
>
> +#ifdef __linux__
> +static int raw_preallocate2(int fd, int64_t offset, int64_t length)
> +{
> +    int ret = -1;
> +
> +    ret = fallocate(fd, 0, offset, length);
> +
> +    /* fallback to posix_fallocate() if fallocate() is not supported */
> +    if (ret < 0 && (errno == ENOSYS || errno == EOPNOTSUPP)) {
> +        ret = posix_fallocate(fd, offset, length);
> +    }
> +
> +    return ret;
> +}
> +#else
> +static int raw_preallocate2(int fd, int64_t offset, int64_t length)
> +{
> +    return posix_fallocate(fd, offset, length);
> +}
> +#endif
> +
> +static int raw_preallocate(BlockDriverState *bs, int64_t offset, int64_t length)
> +{
> +    BDRVRawState *s = bs->opaque;
> +    int64_t len = bdrv_getlength(bs);
> +
> +    if (offset + length < 0 || offset + length > len) {
> +        return -1;

-EINVAL

Fam

> +    }
> +
> +    return raw_preallocate2(s->fd, offset, length);
> +}
> +
>   static int raw_create(const char *filename, QEMUOptionParameter *options,
>                         Error **errp)
>   {
> @@ -1221,6 +1254,7 @@ static BlockDriver bdrv_file = {
>       .bdrv_close = raw_close,
>       .bdrv_create = raw_create,
>       .bdrv_has_zero_init = bdrv_has_zero_init_1,
> +    .bdrv_preallocate = raw_preallocate,
>       .bdrv_co_get_block_status = raw_co_get_block_status,
>
>       .bdrv_aio_readv = raw_aio_readv,
>

Re: [Qemu-devel] [RFC PATCH v2 5/6] qcow2: implement bdrv_preallocate

[Fam Zheng]

On 2013年11月27日 10:15, Hu Tao wrote:
> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> ---
>   block/qcow2.c | 7 +++++++
>   1 file changed, 7 insertions(+)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index b054a01..a23fade 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2180,6 +2180,12 @@ static int qcow2_amend_options(BlockDriverState *bs,
>       return 0;
>   }
>
> +static int qcow2_preallocate(BlockDriverState *bs, int64_t offset,
> +                             int64_t length)
> +{
> +    return bdrv_preallocate(bs->file, offset, length);
> +}
> +

What's the semantics of .bdrv_preallocate? I think you should map 
[offset, offset + length) to clusters in image file, and then forward to 
bs->file, rather than this direct wrapper.

E.g. bdrv_preallocate(qcow2_bs, 0, cluster_size) should call 
bdrv_preallocate(qcow2_bs->file, offset_off_first_cluster, cluster_size).

Fam

>   static QEMUOptionParameter qcow2_create_options[] = {
>       {
>           .name = BLOCK_OPT_SIZE,
> @@ -2234,6 +2240,7 @@ static BlockDriver bdrv_qcow2 = {
>       .bdrv_reopen_prepare  = qcow2_reopen_prepare,
>       .bdrv_create        = qcow2_create,
>       .bdrv_has_zero_init = bdrv_has_zero_init_1,
> +    .bdrv_preallocate   = qcow2_preallocate,
>       .bdrv_co_get_block_status = qcow2_co_get_block_status,
>       .bdrv_set_key       = qcow2_set_key,
>       .bdrv_make_empty    = qcow2_make_empty,
>

Re: [Qemu-devel] [RFC PATCH v2 0/6] qemu-img: add preallocation=full

[Fam Zheng]

On 2013年11月27日 10:15, Hu Tao wrote:
> This series implements full image preallocation to create a non-sparse image
> file at creation time, both for raw and qcow2 format. The purpose is to avoid
> performance deterioration of the guest cause by sparse image.
>
> This series implements full preallocation by using fallocate()/posix_fallocate(),
> which have the advantage that it is fast when creating large image file.
> Zero-filling is not implemented, as writing zeros to image could be slow for large
> file. (fallocate() ensures zero-filling, but posix_fallocate()) If users want it,
> we can maybe add a option to let users have a choice. Suggestions?
>

We have zero init in raw-posix:

static BlockDriver bdrv_file = {
     ...
     .bdrv_has_zero_init = bdrv_has_zero_init_1,
}

so I think we have to zero-fill it if posix_fallocate doesn't ensure.

Fam

> Base on Kevin's patch at:
> http://lists.gnu.org/archive/html/qemu-devel/2011-01/msg03017.html
>
> Hu Tao (6):
>    block: introduce prealloc_mode
>    block: add BlockDriver.bdrv_preallocate.
>    block/raw-posix: implement bdrv_preallocate
>    raw-posix: Add full image preallocation option
>    qcow2: implement bdrv_preallocate
>    qcow2: Add full image preallocation option
>
>   block.c                   | 13 +++++++++++
>   block/qcow2.c             | 32 +++++++++++++++++++++------
>   block/raw-posix.c         | 56 +++++++++++++++++++++++++++++++++++++++++++++++
>   include/block/block.h     |  7 ++++++
>   include/block/block_int.h |  3 +++
>   5 files changed, 104 insertions(+), 7 deletions(-)
>

Re: [Qemu-devel] [RFC PATCH v2 5/6] qcow2: implement bdrv_preallocate

[Hu Tao]

On Wed, Nov 27, 2013 at 11:01:23AM +0800, Fam Zheng wrote:
> On 2013年11月27日 10:15, Hu Tao wrote:
> >Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> >---
> >  block/qcow2.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> >diff --git a/block/qcow2.c b/block/qcow2.c
> >index b054a01..a23fade 100644
> >--- a/block/qcow2.c
> >+++ b/block/qcow2.c
> >@@ -2180,6 +2180,12 @@ static int qcow2_amend_options(BlockDriverState *bs,
> >      return 0;
> >  }
> >
> >+static int qcow2_preallocate(BlockDriverState *bs, int64_t offset,
> >+                             int64_t length)
> >+{
> >+    return bdrv_preallocate(bs->file, offset, length);
> >+}
> >+
> 
> What's the semantics of .bdrv_preallocate? I think you should map
> [offset, offset + length) to clusters in image file, and then
> forward to bs->file, rather than this direct wrapper.
> 
> E.g. bdrv_preallocate(qcow2_bs, 0, cluster_size) should call
> bdrv_preallocate(qcow2_bs->file, offset_off_first_cluster,
> cluster_size).

You mean data clusters here, right? Is there a single function to get
the offset of the first data cluster?

Re: [Qemu-devel] [RFC PATCH v2 5/6] qcow2: implement bdrv_preallocate

[Fam Zheng]

On 2013年11月27日 14:01, Hu Tao wrote:
> On Wed, Nov 27, 2013 at 11:01:23AM +0800, Fam Zheng wrote:
>> On 2013年11月27日 10:15, Hu Tao wrote:
>>> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
>>> ---
>>>   block/qcow2.c | 7 +++++++
>>>   1 file changed, 7 insertions(+)
>>>
>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>> index b054a01..a23fade 100644
>>> --- a/block/qcow2.c
>>> +++ b/block/qcow2.c
>>> @@ -2180,6 +2180,12 @@ static int qcow2_amend_options(BlockDriverState *bs,
>>>       return 0;
>>>   }
>>>
>>> +static int qcow2_preallocate(BlockDriverState *bs, int64_t offset,
>>> +                             int64_t length)
>>> +{
>>> +    return bdrv_preallocate(bs->file, offset, length);
>>> +}
>>> +
>>
>> What's the semantics of .bdrv_preallocate? I think you should map
>> [offset, offset + length) to clusters in image file, and then
>> forward to bs->file, rather than this direct wrapper.
>>
>> E.g. bdrv_preallocate(qcow2_bs, 0, cluster_size) should call
>> bdrv_preallocate(qcow2_bs->file, offset_off_first_cluster,
>> cluster_size).
>
> You mean data clusters here, right? Is there a single function to get
> the offset of the first data cluster?
>

There is a function, qcow2_get_cluster_offset.

Fam

Re: [Qemu-devel] [RFC PATCH v2 5/6] qcow2: implement bdrv_preallocate

[Peter Lieven]

Am 27.11.2013 07:40, schrieb Fam Zheng:
> On 2013年11月27日 14:01, Hu Tao wrote:
>> On Wed, Nov 27, 2013 at 11:01:23AM +0800, Fam Zheng wrote:
>>> On 2013年11月27日 10:15, Hu Tao wrote:
>>>> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
>>>> ---
>>>>   block/qcow2.c | 7 +++++++
>>>>   1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>>> index b054a01..a23fade 100644
>>>> --- a/block/qcow2.c
>>>> +++ b/block/qcow2.c
>>>> @@ -2180,6 +2180,12 @@ static int qcow2_amend_options(BlockDriverState *bs,
>>>>       return 0;
>>>>   }
>>>>
>>>> +static int qcow2_preallocate(BlockDriverState *bs, int64_t offset,
>>>> +                             int64_t length)
>>>> +{
>>>> +    return bdrv_preallocate(bs->file, offset, length);
>>>> +}
>>>> +
>>>
>>> What's the semantics of .bdrv_preallocate? I think you should map
>>> [offset, offset + length) to clusters in image file, and then
>>> forward to bs->file, rather than this direct wrapper.
>>>
>>> E.g. bdrv_preallocate(qcow2_bs, 0, cluster_size) should call
>>> bdrv_preallocate(qcow2_bs->file, offset_off_first_cluster,
>>> cluster_size).
>>
>> You mean data clusters here, right? Is there a single function to get
>> the offset of the first data cluster?
>>
>
> There is a function, qcow2_get_cluster_offset.
This should return no valid offset as long as the cluster is not allocated.

I think you actually have to "write" all clusters of a qcow2 one by one.
Eventually this write could be an fallocate call instead of a zero write.

Peter

Re: [Qemu-devel] [RFC PATCH v2 5/6] qcow2: implement bdrv_preallocate

[Fam Zheng]

On 2013年11月27日 18:03, Peter Lieven wrote:
> Am 27.11.2013 07:40, schrieb Fam Zheng:
>> On 2013年11月27日 14:01, Hu Tao wrote:
>>> On Wed, Nov 27, 2013 at 11:01:23AM +0800, Fam Zheng wrote:
>>>> On 2013年11月27日 10:15, Hu Tao wrote:
>>>>> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
>>>>> ---
>>>>>    block/qcow2.c | 7 +++++++
>>>>>    1 file changed, 7 insertions(+)
>>>>>
>>>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>>>> index b054a01..a23fade 100644
>>>>> --- a/block/qcow2.c
>>>>> +++ b/block/qcow2.c
>>>>> @@ -2180,6 +2180,12 @@ static int qcow2_amend_options(BlockDriverState *bs,
>>>>>        return 0;
>>>>>    }
>>>>>
>>>>> +static int qcow2_preallocate(BlockDriverState *bs, int64_t offset,
>>>>> +                             int64_t length)
>>>>> +{
>>>>> +    return bdrv_preallocate(bs->file, offset, length);
>>>>> +}
>>>>> +
>>>>
>>>> What's the semantics of .bdrv_preallocate? I think you should map
>>>> [offset, offset + length) to clusters in image file, and then
>>>> forward to bs->file, rather than this direct wrapper.
>>>>
>>>> E.g. bdrv_preallocate(qcow2_bs, 0, cluster_size) should call
>>>> bdrv_preallocate(qcow2_bs->file, offset_off_first_cluster,
>>>> cluster_size).
>>>
>>> You mean data clusters here, right? Is there a single function to get
>>> the offset of the first data cluster?
>>>
>>
>> There is a function, qcow2_get_cluster_offset.
> This should return no valid offset as long as the cluster is not allocated.
>
> I think you actually have to "write" all clusters of a qcow2 one by one.
> Eventually this write could be an fallocate call instead of a zero write.
>

Yes, I was wrong about qcow2_get_cluster_offset. The logic here is more 
like cluster allocation in qcow2_alloc_cluster_offset. Maybe we can 
reuse that.

Fam

Re: [Qemu-devel] [RFC PATCH v2 5/6] qcow2: implement bdrv_preallocate

[Peter Lieven]

Am 27.11.2013 11:07, schrieb Fam Zheng:
> On 2013年11月27日 18:03, Peter Lieven wrote:
>> Am 27.11.2013 07:40, schrieb Fam Zheng:
>>> On 2013年11月27日 14:01, Hu Tao wrote:
>>>> On Wed, Nov 27, 2013 at 11:01:23AM +0800, Fam Zheng wrote:
>>>>> On 2013年11月27日 10:15, Hu Tao wrote:
>>>>>> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
>>>>>> ---
>>>>>>    block/qcow2.c | 7 +++++++
>>>>>>    1 file changed, 7 insertions(+)
>>>>>>
>>>>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>>>>> index b054a01..a23fade 100644
>>>>>> --- a/block/qcow2.c
>>>>>> +++ b/block/qcow2.c
>>>>>> @@ -2180,6 +2180,12 @@ static int qcow2_amend_options(BlockDriverState *bs,
>>>>>>        return 0;
>>>>>>    }
>>>>>>
>>>>>> +static int qcow2_preallocate(BlockDriverState *bs, int64_t offset,
>>>>>> +                             int64_t length)
>>>>>> +{
>>>>>> +    return bdrv_preallocate(bs->file, offset, length);
>>>>>> +}
>>>>>> +
>>>>>
>>>>> What's the semantics of .bdrv_preallocate? I think you should map
>>>>> [offset, offset + length) to clusters in image file, and then
>>>>> forward to bs->file, rather than this direct wrapper.
>>>>>
>>>>> E.g. bdrv_preallocate(qcow2_bs, 0, cluster_size) should call
>>>>> bdrv_preallocate(qcow2_bs->file, offset_off_first_cluster,
>>>>> cluster_size).
>>>>
>>>> You mean data clusters here, right? Is there a single function to get
>>>> the offset of the first data cluster?
>>>>
>>>
>>> There is a function, qcow2_get_cluster_offset.
>> This should return no valid offset as long as the cluster is not allocated.
>>
>> I think you actually have to "write" all clusters of a qcow2 one by one.
>> Eventually this write could be an fallocate call instead of a zero write.
>>
>
> Yes, I was wrong about qcow2_get_cluster_offset. The logic here is more like cluster allocation in qcow2_alloc_cluster_offset. Maybe we can reuse that.
What I don't like about the preallocation is that we would loose the information that a cluster contains no valid data and would read it e.g. during
conversion.

I think what we want is a preallocated image with all clusters sequentally mapped into the qcow2 file. Preallocate all the cluster extends, but still
have the information in the table that the cluster in fact has no valid data. So we would need a valid cluster offset while still haveing the
flag that the cluster is unallocated. I think this would require thoughtfully checking all the cluster functions if they can easily cope with this.

The quetion is Hu, what do you want to achieve? Do you want that the space on the filesystem is preallocated so you can't overcommit or
do you also want a sequential mapping of all the clusters into the file?

Peter

Re: [Qemu-devel] [RFC PATCH v2 5/6] qcow2: implement bdrv_preallocate

[Hu Tao]

On Wed, Nov 27, 2013 at 11:13:40AM +0100, Peter Lieven wrote:
> Am 27.11.2013 11:07, schrieb Fam Zheng:
> > On 2013年11月27日 18:03, Peter Lieven wrote:
> >> Am 27.11.2013 07:40, schrieb Fam Zheng:
> >>> On 2013年11月27日 14:01, Hu Tao wrote:
> >>>> On Wed, Nov 27, 2013 at 11:01:23AM +0800, Fam Zheng wrote:
> >>>>> On 2013年11月27日 10:15, Hu Tao wrote:
> >>>>>> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> >>>>>> ---
> >>>>>>    block/qcow2.c | 7 +++++++
> >>>>>>    1 file changed, 7 insertions(+)
> >>>>>>
> >>>>>> diff --git a/block/qcow2.c b/block/qcow2.c
> >>>>>> index b054a01..a23fade 100644
> >>>>>> --- a/block/qcow2.c
> >>>>>> +++ b/block/qcow2.c
> >>>>>> @@ -2180,6 +2180,12 @@ static int qcow2_amend_options(BlockDriverState *bs,
> >>>>>>        return 0;
> >>>>>>    }
> >>>>>>
> >>>>>> +static int qcow2_preallocate(BlockDriverState *bs, int64_t offset,
> >>>>>> +                             int64_t length)
> >>>>>> +{
> >>>>>> +    return bdrv_preallocate(bs->file, offset, length);
> >>>>>> +}
> >>>>>> +
> >>>>>
> >>>>> What's the semantics of .bdrv_preallocate? I think you should map
> >>>>> [offset, offset + length) to clusters in image file, and then
> >>>>> forward to bs->file, rather than this direct wrapper.
> >>>>>
> >>>>> E.g. bdrv_preallocate(qcow2_bs, 0, cluster_size) should call
> >>>>> bdrv_preallocate(qcow2_bs->file, offset_off_first_cluster,
> >>>>> cluster_size).
> >>>>
> >>>> You mean data clusters here, right? Is there a single function to get
> >>>> the offset of the first data cluster?
> >>>>
> >>>
> >>> There is a function, qcow2_get_cluster_offset.
> >> This should return no valid offset as long as the cluster is not allocated.
> >>
> >> I think you actually have to "write" all clusters of a qcow2 one by one.
> >> Eventually this write could be an fallocate call instead of a zero write.
> >>
> >
> > Yes, I was wrong about qcow2_get_cluster_offset. The logic here is more like cluster allocation in qcow2_alloc_cluster_offset. Maybe we can reuse that.
> What I don't like about the preallocation is that we would loose the information that a cluster contains no valid data and would read it e.g. during
> conversion.

So the information is stored in table and you mean we shouldn't clear
table when do preallocation? I'm not sure how the information could be
useful on a newly-created image, but it seems ideal to keep informations
in table.

> 
> I think what we want is a preallocated image with all clusters sequentally mapped into the qcow2 file. Preallocate all the cluster extends, but still
> have the information in the table that the cluster in fact has no valid data. So we would need a valid cluster offset while still haveing the
> flag that the cluster is unallocated. I think this would require thoughtfully checking all the cluster functions if they can easily cope with this.
> 
> The quetion is Hu, what do you want to achieve? Do you want that the space on the filesystem is preallocated so you can't overcommit or
> do you also want a sequential mapping of all the clusters into the file?

The goal is to avoid sparse file as it can cause performance problem. So
the first one. I'm not sure about the second but IIUC, one fallocate()
is enough for all clusters if they are sequentially mapped.

Re: [Qemu-devel] [RFC PATCH v2 5/6] qcow2: implement bdrv_preallocate

[Peter Lieven]

On 28.11.2013 09:48, Hu Tao wrote:
> On Wed, Nov 27, 2013 at 11:13:40AM +0100, Peter Lieven wrote:
>> Am 27.11.2013 11:07, schrieb Fam Zheng:
>>> On 2013年11月27日 18:03, Peter Lieven wrote:
>>>> Am 27.11.2013 07:40, schrieb Fam Zheng:
>>>>> On 2013年11月27日 14:01, Hu Tao wrote:
>>>>>> On Wed, Nov 27, 2013 at 11:01:23AM +0800, Fam Zheng wrote:
>>>>>>> On 2013年11月27日 10:15, Hu Tao wrote:
>>>>>>>> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
>>>>>>>> ---
>>>>>>>>     block/qcow2.c | 7 +++++++
>>>>>>>>     1 file changed, 7 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>>>>>>> index b054a01..a23fade 100644
>>>>>>>> --- a/block/qcow2.c
>>>>>>>> +++ b/block/qcow2.c
>>>>>>>> @@ -2180,6 +2180,12 @@ static int qcow2_amend_options(BlockDriverState *bs,
>>>>>>>>         return 0;
>>>>>>>>     }
>>>>>>>>
>>>>>>>> +static int qcow2_preallocate(BlockDriverState *bs, int64_t offset,
>>>>>>>> +                             int64_t length)
>>>>>>>> +{
>>>>>>>> +    return bdrv_preallocate(bs->file, offset, length);
>>>>>>>> +}
>>>>>>>> +
>>>>>>> What's the semantics of .bdrv_preallocate? I think you should map
>>>>>>> [offset, offset + length) to clusters in image file, and then
>>>>>>> forward to bs->file, rather than this direct wrapper.
>>>>>>>
>>>>>>> E.g. bdrv_preallocate(qcow2_bs, 0, cluster_size) should call
>>>>>>> bdrv_preallocate(qcow2_bs->file, offset_off_first_cluster,
>>>>>>> cluster_size).
>>>>>> You mean data clusters here, right? Is there a single function to get
>>>>>> the offset of the first data cluster?
>>>>>>
>>>>> There is a function, qcow2_get_cluster_offset.
>>>> This should return no valid offset as long as the cluster is not allocated.
>>>>
>>>> I think you actually have to "write" all clusters of a qcow2 one by one.
>>>> Eventually this write could be an fallocate call instead of a zero write.
>>>>
>>> Yes, I was wrong about qcow2_get_cluster_offset. The logic here is more like cluster allocation in qcow2_alloc_cluster_offset. Maybe we can reuse that.
>> What I don't like about the preallocation is that we would loose the information that a cluster contains no valid data and would read it e.g. during
>> conversion.
> So the information is stored in table and you mean we shouldn't clear
> table when do preallocation? I'm not sure how the information could be
> useful on a newly-created image, but it seems ideal to keep informations
> in table.
When you want to e.g. convert this qcow2 later the performance is lower than needed because
you read all those preallocated sectors altough you could now they are empty.
>
>> I think what we want is a preallocated image with all clusters sequentally mapped into the qcow2 file. Preallocate all the cluster extends, but still
>> have the information in the table that the cluster in fact has no valid data. So we would need a valid cluster offset while still haveing the
>> flag that the cluster is unallocated. I think this would require thoughtfully checking all the cluster functions if they can easily cope with this.
>>
>> The quetion is Hu, what do you want to achieve? Do you want that the space on the filesystem is preallocated so you can't overcommit or
>> do you also want a sequential mapping of all the clusters into the file?
> The goal is to avoid sparse file as it can cause performance problem. So
> the first one. I'm not sure about the second but IIUC, one fallocate()
> is enough for all clusters if they are sequentially mapped.
If you do not premap them they are allocated in the order they are written.
So if you are going to preallocate the whole file anyway, you should sequentally map all clusters into the file
AND still keep the information that they are in fact not yet written.

Peter

Re: [Qemu-devel] [RFC PATCH v2 5/6] qcow2: implement bdrv_preallocate

[Hu Tao]

On Thu, Nov 28, 2013 at 11:03:04AM +0100, Peter Lieven wrote:
> On 28.11.2013 09:48, Hu Tao wrote:
> >On Wed, Nov 27, 2013 at 11:13:40AM +0100, Peter Lieven wrote:
> >>Am 27.11.2013 11:07, schrieb Fam Zheng:
> >>>On 2013年11月27日 18:03, Peter Lieven wrote:
> >>>>Am 27.11.2013 07:40, schrieb Fam Zheng:
> >>>>>On 2013年11月27日 14:01, Hu Tao wrote:
> >>>>>>On Wed, Nov 27, 2013 at 11:01:23AM +0800, Fam Zheng wrote:
> >>>>>>>On 2013年11月27日 10:15, Hu Tao wrote:
> >>>>>>>>Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> >>>>>>>>---
> >>>>>>>>    block/qcow2.c | 7 +++++++
> >>>>>>>>    1 file changed, 7 insertions(+)
> >>>>>>>>
> >>>>>>>>diff --git a/block/qcow2.c b/block/qcow2.c
> >>>>>>>>index b054a01..a23fade 100644
> >>>>>>>>--- a/block/qcow2.c
> >>>>>>>>+++ b/block/qcow2.c
> >>>>>>>>@@ -2180,6 +2180,12 @@ static int qcow2_amend_options(BlockDriverState *bs,
> >>>>>>>>        return 0;
> >>>>>>>>    }
> >>>>>>>>
> >>>>>>>>+static int qcow2_preallocate(BlockDriverState *bs, int64_t offset,
> >>>>>>>>+                             int64_t length)
> >>>>>>>>+{
> >>>>>>>>+    return bdrv_preallocate(bs->file, offset, length);
> >>>>>>>>+}
> >>>>>>>>+
> >>>>>>>What's the semantics of .bdrv_preallocate? I think you should map
> >>>>>>>[offset, offset + length) to clusters in image file, and then
> >>>>>>>forward to bs->file, rather than this direct wrapper.
> >>>>>>>
> >>>>>>>E.g. bdrv_preallocate(qcow2_bs, 0, cluster_size) should call
> >>>>>>>bdrv_preallocate(qcow2_bs->file, offset_off_first_cluster,
> >>>>>>>cluster_size).
> >>>>>>You mean data clusters here, right? Is there a single function to get
> >>>>>>the offset of the first data cluster?
> >>>>>>
> >>>>>There is a function, qcow2_get_cluster_offset.
> >>>>This should return no valid offset as long as the cluster is not allocated.
> >>>>
> >>>>I think you actually have to "write" all clusters of a qcow2 one by one.
> >>>>Eventually this write could be an fallocate call instead of a zero write.
> >>>>
> >>>Yes, I was wrong about qcow2_get_cluster_offset. The logic here is more like cluster allocation in qcow2_alloc_cluster_offset. Maybe we can reuse that.
> >>What I don't like about the preallocation is that we would loose the information that a cluster contains no valid data and would read it e.g. during
> >>conversion.
> >So the information is stored in table and you mean we shouldn't clear
> >table when do preallocation? I'm not sure how the information could be
> >useful on a newly-created image, but it seems ideal to keep informations
> >in table.
> When you want to e.g. convert this qcow2 later the performance is lower than needed because
> you read all those preallocated sectors altough you could now they are empty.
> >
> >>I think what we want is a preallocated image with all clusters sequentally mapped into the qcow2 file. Preallocate all the cluster extends, but still
> >>have the information in the table that the cluster in fact has no valid data. So we would need a valid cluster offset while still haveing the
> >>flag that the cluster is unallocated. I think this would require thoughtfully checking all the cluster functions if they can easily cope with this.
> >>
> >>The quetion is Hu, what do you want to achieve? Do you want that the space on the filesystem is preallocated so you can't overcommit or
> >>do you also want a sequential mapping of all the clusters into the file?
> >The goal is to avoid sparse file as it can cause performance problem. So
> >the first one. I'm not sure about the second but IIUC, one fallocate()
> >is enough for all clusters if they are sequentially mapped.
> If you do not premap them they are allocated in the order they are written.
> So if you are going to preallocate the whole file anyway, you should sequentally map all clusters into the file
> AND still keep the information that they are in fact not yet written.

Can this be achieved by first fallocate() the disk file, then allocate
metadata? This way all metadata clusters are allocated before any data
clusters, leaving all data clusters at the end of file.

-- 
Regards,
Hu Tao

Re: [Qemu-devel] [RFC PATCH v2 5/6] qcow2: implement bdrv_preallocate

[Hu Tao]

On Wed, Dec 11, 2013 at 03:33:40PM +0800, Hu Tao wrote:
> On Thu, Nov 28, 2013 at 11:03:04AM +0100, Peter Lieven wrote:
> > On 28.11.2013 09:48, Hu Tao wrote:
> > >On Wed, Nov 27, 2013 at 11:13:40AM +0100, Peter Lieven wrote:
> > >>Am 27.11.2013 11:07, schrieb Fam Zheng:
> > >>>On 2013年11月27日 18:03, Peter Lieven wrote:
> > >>>>Am 27.11.2013 07:40, schrieb Fam Zheng:
> > >>>>>On 2013年11月27日 14:01, Hu Tao wrote:
> > >>>>>>On Wed, Nov 27, 2013 at 11:01:23AM +0800, Fam Zheng wrote:
> > >>>>>>>On 2013年11月27日 10:15, Hu Tao wrote:
> > >>>>>>>>Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> > >>>>>>>>---
> > >>>>>>>>    block/qcow2.c | 7 +++++++
> > >>>>>>>>    1 file changed, 7 insertions(+)
> > >>>>>>>>
> > >>>>>>>>diff --git a/block/qcow2.c b/block/qcow2.c
> > >>>>>>>>index b054a01..a23fade 100644
> > >>>>>>>>--- a/block/qcow2.c
> > >>>>>>>>+++ b/block/qcow2.c
> > >>>>>>>>@@ -2180,6 +2180,12 @@ static int qcow2_amend_options(BlockDriverState *bs,
> > >>>>>>>>        return 0;
> > >>>>>>>>    }
> > >>>>>>>>
> > >>>>>>>>+static int qcow2_preallocate(BlockDriverState *bs, int64_t offset,
> > >>>>>>>>+                             int64_t length)
> > >>>>>>>>+{
> > >>>>>>>>+    return bdrv_preallocate(bs->file, offset, length);
> > >>>>>>>>+}
> > >>>>>>>>+
> > >>>>>>>What's the semantics of .bdrv_preallocate? I think you should map
> > >>>>>>>[offset, offset + length) to clusters in image file, and then
> > >>>>>>>forward to bs->file, rather than this direct wrapper.
> > >>>>>>>
> > >>>>>>>E.g. bdrv_preallocate(qcow2_bs, 0, cluster_size) should call
> > >>>>>>>bdrv_preallocate(qcow2_bs->file, offset_off_first_cluster,
> > >>>>>>>cluster_size).
> > >>>>>>You mean data clusters here, right? Is there a single function to get
> > >>>>>>the offset of the first data cluster?
> > >>>>>>
> > >>>>>There is a function, qcow2_get_cluster_offset.
> > >>>>This should return no valid offset as long as the cluster is not allocated.
> > >>>>
> > >>>>I think you actually have to "write" all clusters of a qcow2 one by one.
> > >>>>Eventually this write could be an fallocate call instead of a zero write.
> > >>>>
> > >>>Yes, I was wrong about qcow2_get_cluster_offset. The logic here is more like cluster allocation in qcow2_alloc_cluster_offset. Maybe we can reuse that.
> > >>What I don't like about the preallocation is that we would loose the information that a cluster contains no valid data and would read it e.g. during
> > >>conversion.
> > >So the information is stored in table and you mean we shouldn't clear
> > >table when do preallocation? I'm not sure how the information could be
> > >useful on a newly-created image, but it seems ideal to keep informations
> > >in table.
> > When you want to e.g. convert this qcow2 later the performance is lower than needed because
> > you read all those preallocated sectors altough you could now they are empty.
> > >
> > >>I think what we want is a preallocated image with all clusters sequentally mapped into the qcow2 file. Preallocate all the cluster extends, but still
> > >>have the information in the table that the cluster in fact has no valid data. So we would need a valid cluster offset while still haveing the
> > >>flag that the cluster is unallocated. I think this would require thoughtfully checking all the cluster functions if they can easily cope with this.
> > >>
> > >>The quetion is Hu, what do you want to achieve? Do you want that the space on the filesystem is preallocated so you can't overcommit or
> > >>do you also want a sequential mapping of all the clusters into the file?
> > >The goal is to avoid sparse file as it can cause performance problem. So
> > >the first one. I'm not sure about the second but IIUC, one fallocate()
> > >is enough for all clusters if they are sequentially mapped.
> > If you do not premap them they are allocated in the order they are written.
> > So if you are going to preallocate the whole file anyway, you should sequentally map all clusters into the file
> > AND still keep the information that they are in fact not yet written.
> 
> Can this be achieved by first fallocate() the disk file, then allocate
> metadata? This way all metadata clusters are allocated before any data
> clusters, leaving all data clusters at the end of file.

Any comments?

Re: [Qemu-devel] [RFC PATCH v2 5/6] qcow2: implement bdrv_preallocate

[Fam Zheng]

On 2013年12月11日 15:33, Hu Tao wrote:
> On Thu, Nov 28, 2013 at 11:03:04AM +0100, Peter Lieven wrote:
>> On 28.11.2013 09:48, Hu Tao wrote:
>>> On Wed, Nov 27, 2013 at 11:13:40AM +0100, Peter Lieven wrote:
>>>> Am 27.11.2013 11:07, schrieb Fam Zheng:
>>>>> On 2013年11月27日 18:03, Peter Lieven wrote:
>>>>>> Am 27.11.2013 07:40, schrieb Fam Zheng:
>>>>>>> On 2013年11月27日 14:01, Hu Tao wrote:
>>>>>>>> On Wed, Nov 27, 2013 at 11:01:23AM +0800, Fam Zheng wrote:
>>>>>>>>> On 2013年11月27日 10:15, Hu Tao wrote:
>>>>>>>>>> Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
>>>>>>>>>> ---
>>>>>>>>>>     block/qcow2.c | 7 +++++++
>>>>>>>>>>     1 file changed, 7 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>>>>>>>>> index b054a01..a23fade 100644
>>>>>>>>>> --- a/block/qcow2.c
>>>>>>>>>> +++ b/block/qcow2.c
>>>>>>>>>> @@ -2180,6 +2180,12 @@ static int qcow2_amend_options(BlockDriverState *bs,
>>>>>>>>>>         return 0;
>>>>>>>>>>     }
>>>>>>>>>>
>>>>>>>>>> +static int qcow2_preallocate(BlockDriverState *bs, int64_t offset,
>>>>>>>>>> +                             int64_t length)
>>>>>>>>>> +{
>>>>>>>>>> +    return bdrv_preallocate(bs->file, offset, length);
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>> What's the semantics of .bdrv_preallocate? I think you should map
>>>>>>>>> [offset, offset + length) to clusters in image file, and then
>>>>>>>>> forward to bs->file, rather than this direct wrapper.
>>>>>>>>>
>>>>>>>>> E.g. bdrv_preallocate(qcow2_bs, 0, cluster_size) should call
>>>>>>>>> bdrv_preallocate(qcow2_bs->file, offset_off_first_cluster,
>>>>>>>>> cluster_size).
>>>>>>>> You mean data clusters here, right? Is there a single function to get
>>>>>>>> the offset of the first data cluster?
>>>>>>>>
>>>>>>> There is a function, qcow2_get_cluster_offset.
>>>>>> This should return no valid offset as long as the cluster is not allocated.
>>>>>>
>>>>>> I think you actually have to "write" all clusters of a qcow2 one by one.
>>>>>> Eventually this write could be an fallocate call instead of a zero write.
>>>>>>
>>>>> Yes, I was wrong about qcow2_get_cluster_offset. The logic here is more like cluster allocation in qcow2_alloc_cluster_offset. Maybe we can reuse that.
>>>> What I don't like about the preallocation is that we would loose the information that a cluster contains no valid data and would read it e.g. during
>>>> conversion.
>>> So the information is stored in table and you mean we shouldn't clear
>>> table when do preallocation? I'm not sure how the information could be
>>> useful on a newly-created image, but it seems ideal to keep informations
>>> in table.
>> When you want to e.g. convert this qcow2 later the performance is lower than needed because
>> you read all those preallocated sectors altough you could now they are empty.
>>>
>>>> I think what we want is a preallocated image with all clusters sequentally mapped into the qcow2 file. Preallocate all the cluster extends, but still
>>>> have the information in the table that the cluster in fact has no valid data. So we would need a valid cluster offset while still haveing the
>>>> flag that the cluster is unallocated. I think this would require thoughtfully checking all the cluster functions if they can easily cope with this.
>>>>
>>>> The quetion is Hu, what do you want to achieve? Do you want that the space on the filesystem is preallocated so you can't overcommit or
>>>> do you also want a sequential mapping of all the clusters into the file?
>>> The goal is to avoid sparse file as it can cause performance problem. So
>>> the first one. I'm not sure about the second but IIUC, one fallocate()
>>> is enough for all clusters if they are sequentially mapped.
>> If you do not premap them they are allocated in the order they are written.
>> So if you are going to preallocate the whole file anyway, you should sequentally map all clusters into the file
>> AND still keep the information that they are in fact not yet written.
>
> Can this be achieved by first fallocate() the disk file, then allocate
> metadata? This way all metadata clusters are allocated before any data
> clusters, leaving all data clusters at the end of file.
>

I think Peter means your need to sequentially map clusters into the 
file, so that sequential IO in guest is translated to sequential IO on 
the image file.

fallocate() or posix_fallocate() should work. You need to set zero flag 
on the allocated cluster when mapping it in L2, instead of actually 
writing zeros.

Fam

Re: [Qemu-devel] [RFC PATCH v2 5/6] qcow2: implement bdrv_preallocate

[Hu Tao]

On Mon, Dec 16, 2013 at 05:21:23PM +0800, Fam Zheng wrote:
> On 2013年12月11日 15:33, Hu Tao wrote:
> >On Thu, Nov 28, 2013 at 11:03:04AM +0100, Peter Lieven wrote:
> >>On 28.11.2013 09:48, Hu Tao wrote:
> >>>On Wed, Nov 27, 2013 at 11:13:40AM +0100, Peter Lieven wrote:
> >>>>Am 27.11.2013 11:07, schrieb Fam Zheng:
> >>>>>On 2013年11月27日 18:03, Peter Lieven wrote:
> >>>>>>Am 27.11.2013 07:40, schrieb Fam Zheng:
> >>>>>>>On 2013年11月27日 14:01, Hu Tao wrote:
> >>>>>>>>On Wed, Nov 27, 2013 at 11:01:23AM +0800, Fam Zheng wrote:
> >>>>>>>>>On 2013年11月27日 10:15, Hu Tao wrote:
> >>>>>>>>>>Signed-off-by: Hu Tao <hutao@cn.fujitsu.com>
> >>>>>>>>>>---
> >>>>>>>>>>    block/qcow2.c | 7 +++++++
> >>>>>>>>>>    1 file changed, 7 insertions(+)
> >>>>>>>>>>
> >>>>>>>>>>diff --git a/block/qcow2.c b/block/qcow2.c
> >>>>>>>>>>index b054a01..a23fade 100644
> >>>>>>>>>>--- a/block/qcow2.c
> >>>>>>>>>>+++ b/block/qcow2.c
> >>>>>>>>>>@@ -2180,6 +2180,12 @@ static int qcow2_amend_options(BlockDriverState *bs,
> >>>>>>>>>>        return 0;
> >>>>>>>>>>    }
> >>>>>>>>>>
> >>>>>>>>>>+static int qcow2_preallocate(BlockDriverState *bs, int64_t offset,
> >>>>>>>>>>+                             int64_t length)
> >>>>>>>>>>+{
> >>>>>>>>>>+    return bdrv_preallocate(bs->file, offset, length);
> >>>>>>>>>>+}
> >>>>>>>>>>+
> >>>>>>>>>What's the semantics of .bdrv_preallocate? I think you should map
> >>>>>>>>>[offset, offset + length) to clusters in image file, and then
> >>>>>>>>>forward to bs->file, rather than this direct wrapper.
> >>>>>>>>>
> >>>>>>>>>E.g. bdrv_preallocate(qcow2_bs, 0, cluster_size) should call
> >>>>>>>>>bdrv_preallocate(qcow2_bs->file, offset_off_first_cluster,
> >>>>>>>>>cluster_size).
> >>>>>>>>You mean data clusters here, right? Is there a single function to get
> >>>>>>>>the offset of the first data cluster?
> >>>>>>>>
> >>>>>>>There is a function, qcow2_get_cluster_offset.
> >>>>>>This should return no valid offset as long as the cluster is not allocated.
> >>>>>>
> >>>>>>I think you actually have to "write" all clusters of a qcow2 one by one.
> >>>>>>Eventually this write could be an fallocate call instead of a zero write.
> >>>>>>
> >>>>>Yes, I was wrong about qcow2_get_cluster_offset. The logic here is more like cluster allocation in qcow2_alloc_cluster_offset. Maybe we can reuse that.
> >>>>What I don't like about the preallocation is that we would loose the information that a cluster contains no valid data and would read it e.g. during
> >>>>conversion.
> >>>So the information is stored in table and you mean we shouldn't clear
> >>>table when do preallocation? I'm not sure how the information could be
> >>>useful on a newly-created image, but it seems ideal to keep informations
> >>>in table.
> >>When you want to e.g. convert this qcow2 later the performance is lower than needed because
> >>you read all those preallocated sectors altough you could now they are empty.
> >>>
> >>>>I think what we want is a preallocated image with all clusters sequentally mapped into the qcow2 file. Preallocate all the cluster extends, but still
> >>>>have the information in the table that the cluster in fact has no valid data. So we would need a valid cluster offset while still haveing the
> >>>>flag that the cluster is unallocated. I think this would require thoughtfully checking all the cluster functions if they can easily cope with this.
> >>>>
> >>>>The quetion is Hu, what do you want to achieve? Do you want that the space on the filesystem is preallocated so you can't overcommit or
> >>>>do you also want a sequential mapping of all the clusters into the file?
> >>>The goal is to avoid sparse file as it can cause performance problem. So
> >>>the first one. I'm not sure about the second but IIUC, one fallocate()
> >>>is enough for all clusters if they are sequentially mapped.
> >>If you do not premap them they are allocated in the order they are written.
> >>So if you are going to preallocate the whole file anyway, you should sequentally map all clusters into the file
> >>AND still keep the information that they are in fact not yet written.
> >
> >Can this be achieved by first fallocate() the disk file, then allocate
> >metadata? This way all metadata clusters are allocated before any data
> >clusters, leaving all data clusters at the end of file.
> >
> 
> I think Peter means your need to sequentially map clusters into the
> file, so that sequential IO in guest is translated to sequential IO
> on the image file.

The question is how to map data clusters sequentially. If I read code
correctly, qemu allocates clusters (containing data and metadata) on
demand.  So if we allocates all metadata clusters right after the
creation of image file, we can leave all left space allocated for data
laterly. Fix me.

> 
> fallocate() or posix_fallocate() should work. You need to set zero
> flag on the allocated cluster when mapping it in L2, instead of
> actually writing zeros.

You mean bit 0 of L2 entry by zero flag? But the doc says it is always 0
with version 2.