diff mbox

[v7,for,2.0,3/4] raw-posix: Add full image preallocation option

Message ID f405bdc0bb5f350ace0962bcb3f1200e038b25be.1395035846.git.hutao@cn.fujitsu.com
State New
Headers show

Commit Message

Hu Tao March 17, 2014, 6:53 a.m. UTC 
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 | 43 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 35 insertions(+), 8 deletions(-)

Comments

Kevin Wolf March 20, 2014, 1:22 p.m. UTC | #1 
Am 17.03.2014 um 07:53 hat Hu Tao geschrieben:
> 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 | 43 +++++++++++++++++++++++++++++++++++--------
>  1 file changed, 35 insertions(+), 8 deletions(-)
> 
> diff --git a/block/raw-posix.c b/block/raw-posix.c
> index a363b71..e004ff8 100644
> --- a/block/raw-posix.c
> +++ b/block/raw-posix.c
> @@ -1240,6 +1240,7 @@ static int raw_create(const char *filename, QEMUOptionParameter *options,
>      int fd;
>      int result = 0;
>      int64_t total_size = 0;
> +    PreallocMode prealloc = PREALLOC_MODE_OFF;
>  
>      strstart(filename, "file:", &filename);
>  
> @@ -1247,6 +1248,16 @@ static int raw_create(const char *filename, QEMUOptionParameter *options,
>      while (options && options->name) {
>          if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
>              total_size = options->value.n & BDRV_SECTOR_MASK;
> +        } else if (!strcmp(options->name, BLOCK_OPT_PREALLOC)) {
> +            if (!options->value.s || !strcmp(options->value.s, "off")) {
> +                prealloc = PREALLOC_MODE_OFF;
> +            } else if (!strcmp(options->value.s, "full")) {
> +                prealloc = PREALLOC_MODE_FULL;
> +            } else {
> +                error_setg(errp, "Invalid preallocation mode: '%s'",
> +                           options->value.s);
> +                return -EINVAL;
> +            }
>          }
>          options++;
>      }
> @@ -1256,16 +1267,27 @@ static int raw_create(const char *filename, QEMUOptionParameter *options,
>      if (fd < 0) {
>          result = -errno;
>          error_setg_errno(errp, -result, "Could not create file");
> -    } else {
> -        if (ftruncate(fd, total_size) != 0) {
> -            result = -errno;
> -            error_setg_errno(errp, -result, "Could not resize file");
> -        }
> -        if (qemu_close(fd) != 0) {
> -            result = -errno;
> -            error_setg_errno(errp, -result, "Could not close the new file");
> +        goto out;
> +    }
> +    if (ftruncate(fd, total_size) != 0) {
> +        result = -errno;
> +        error_setg_errno(errp, -result, "Could not resize file");
> +        goto out_close;
> +    }
> +    if (prealloc == PREALLOC_MODE_FULL) {
> +        /* posix_fallocate() doesn't set errno. */
> +        result = -posix_fallocate(fd, 0, total_size);
> +        if (result != 0) {
> +            error_setg_errno(errp, -result,
> +                             "Could not preallocate data for the new file");
>          }

Can you please make that PREALLOC_MODE_METADATA?

You're really only preallocating metadata on a filesystem level here. If
the backing block device were thin provisioned, we wouldn't make any
attempt to preallocate that (e.g. with a loop writing zeroes or
bdrv_write_zeroes() without BDRV_REQ_MAY_UNMAP), so I'd hardly call it
"full" preallocation.

Kevin
Hu Tao March 28, 2014, 2:33 a.m. UTC | #2 
On Thu, Mar 20, 2014 at 02:22:19PM +0100, Kevin Wolf wrote:
> Am 17.03.2014 um 07:53 hat Hu Tao geschrieben:

> > 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 | 43 +++++++++++++++++++++++++++++++++++--------

> >  1 file changed, 35 insertions(+), 8 deletions(-)

> > 

> > diff --git a/block/raw-posix.c b/block/raw-posix.c

> > index a363b71..e004ff8 100644

> > --- a/block/raw-posix.c

> > +++ b/block/raw-posix.c

> > @@ -1240,6 +1240,7 @@ static int raw_create(const char *filename, QEMUOptionParameter *options,

> >      int fd;

> >      int result = 0;

> >      int64_t total_size = 0;

> > +    PreallocMode prealloc = PREALLOC_MODE_OFF;

> >  

> >      strstart(filename, "file:", &filename);

> >  

> > @@ -1247,6 +1248,16 @@ static int raw_create(const char *filename, QEMUOptionParameter *options,

> >      while (options && options->name) {

> >          if (!strcmp(options->name, BLOCK_OPT_SIZE)) {

> >              total_size = options->value.n & BDRV_SECTOR_MASK;

> > +        } else if (!strcmp(options->name, BLOCK_OPT_PREALLOC)) {

> > +            if (!options->value.s || !strcmp(options->value.s, "off")) {

> > +                prealloc = PREALLOC_MODE_OFF;

> > +            } else if (!strcmp(options->value.s, "full")) {

> > +                prealloc = PREALLOC_MODE_FULL;

> > +            } else {

> > +                error_setg(errp, "Invalid preallocation mode: '%s'",

> > +                           options->value.s);

> > +                return -EINVAL;

> > +            }

> >          }

> >          options++;

> >      }

> > @@ -1256,16 +1267,27 @@ static int raw_create(const char *filename, QEMUOptionParameter *options,

> >      if (fd < 0) {

> >          result = -errno;

> >          error_setg_errno(errp, -result, "Could not create file");

> > -    } else {

> > -        if (ftruncate(fd, total_size) != 0) {

> > -            result = -errno;

> > -            error_setg_errno(errp, -result, "Could not resize file");

> > -        }

> > -        if (qemu_close(fd) != 0) {

> > -            result = -errno;

> > -            error_setg_errno(errp, -result, "Could not close the new file");

> > +        goto out;

> > +    }

> > +    if (ftruncate(fd, total_size) != 0) {

> > +        result = -errno;

> > +        error_setg_errno(errp, -result, "Could not resize file");

> > +        goto out_close;

> > +    }

> > +    if (prealloc == PREALLOC_MODE_FULL) {

> > +        /* posix_fallocate() doesn't set errno. */

> > +        result = -posix_fallocate(fd, 0, total_size);

> > +        if (result != 0) {

> > +            error_setg_errno(errp, -result,

> > +                             "Could not preallocate data for the new file");

> >          }

> 

> Can you please make that PREALLOC_MODE_METADATA?

> 

> You're really only preallocating metadata on a filesystem level here. If

> the backing block device were thin provisioned, we wouldn't make any


Thanks for reminding me! I was not aware of thin privisioning case.

> attempt to preallocate that (e.g. with a loop writing zeroes or

> bdrv_write_zeroes() without BDRV_REQ_MAY_UNMAP), so I'd hardly call it

> "full" preallocation.


Just to confirm it, we can only do full preallocation by writing zeros
to the backing device? If yes, I agree that this patch doesn't do full
preallocation.

In the other hand, this patch changes the behaviour of qcow2 metadata
preallocation, I think it is not reasonable to still call it
PREALLOC_MODE_METADATA. What's your opinion?

Regards,
Hu Tao
diff mbox

Patch

diff --git a/block/raw-posix.c b/block/raw-posix.c
index a363b71..e004ff8 100644
--- a/block/raw-posix.c
+++ b/block/raw-posix.c
@@ -1240,6 +1240,7 @@  static int raw_create(const char *filename, QEMUOptionParameter *options,
     int fd;
     int result = 0;
     int64_t total_size = 0;
+    PreallocMode prealloc = PREALLOC_MODE_OFF;
 
     strstart(filename, "file:", &filename);
 
@@ -1247,6 +1248,16 @@  static int raw_create(const char *filename, QEMUOptionParameter *options,
     while (options && options->name) {
         if (!strcmp(options->name, BLOCK_OPT_SIZE)) {
             total_size = options->value.n & BDRV_SECTOR_MASK;
+        } else if (!strcmp(options->name, BLOCK_OPT_PREALLOC)) {
+            if (!options->value.s || !strcmp(options->value.s, "off")) {
+                prealloc = PREALLOC_MODE_OFF;
+            } else if (!strcmp(options->value.s, "full")) {
+                prealloc = PREALLOC_MODE_FULL;
+            } else {
+                error_setg(errp, "Invalid preallocation mode: '%s'",
+                           options->value.s);
+                return -EINVAL;
+            }
         }
         options++;
     }
@@ -1256,16 +1267,27 @@  static int raw_create(const char *filename, QEMUOptionParameter *options,
     if (fd < 0) {
         result = -errno;
         error_setg_errno(errp, -result, "Could not create file");
-    } else {
-        if (ftruncate(fd, total_size) != 0) {
-            result = -errno;
-            error_setg_errno(errp, -result, "Could not resize file");
-        }
-        if (qemu_close(fd) != 0) {
-            result = -errno;
-            error_setg_errno(errp, -result, "Could not close the new file");
+        goto out;
+    }
+    if (ftruncate(fd, total_size) != 0) {
+        result = -errno;
+        error_setg_errno(errp, -result, "Could not resize file");
+        goto out_close;
+    }
+    if (prealloc == PREALLOC_MODE_FULL) {
+        /* posix_fallocate() doesn't set errno. */
+        result = -posix_fallocate(fd, 0, total_size);
+        if (result != 0) {
+            error_setg_errno(errp, -result,
+                             "Could not preallocate data for the new file");
         }
     }
+out_close:
+    if (qemu_close(fd) != 0) {
+        result = -errno;
+        error_setg_errno(errp, -result, "Could not close the new file");
+    }
+out:
     return result;
 }
 
@@ -1416,6 +1438,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 }
 };