Re: [Qemu-devel] [PATCH v7 for 2.0 3/4] raw-posix: Add full image preallocation option

[address@hidden]

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 <address@hidden>
> > ---
> >  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