| Message ID | 8ff0f6493e8ad300948cb1e2c8fd45f82e265ac4.1388112645.git.hutao@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Dec 27, 2013 at 11:05:53AM +0800, Hu Tao wrote: > 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 | 46 ++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 46 insertions(+) > > diff --git a/block/raw-posix.c b/block/raw-posix.c > index 6f6b8c1..a722d27 100644 > --- a/block/raw-posix.c > +++ b/block/raw-posix.c > @@ -1160,17 +1160,52 @@ 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) Why is this function called raw_preallocate2()? Please choose a descriptive name. > +{ > + int ret = -1; > + > + ret = fallocate(fd, 0, offset, length); > + if (ret < 0) { > + ret = -errno; > + } > + > + /* fallback to posix_fallocate() if fallocate() is not supported */ > + if (ret == -ENOSYS || ret == -EOPNOTSUPP) { > + ret = posix_fallocate(fd, offset, length); From the posix_fallocate(3) man page: "posix_fallocate() returns zero on success, or an error number on failure. Note that errno is not set." You need to negate the error number: if (ret > 0) { ret = -ret; /* posix_fallocate(3) returns a positive errno */ } > + } > + > + return ret; > +} > +#else > +static int raw_preallocate2(int fd, int64_t offset, int64_t length) > +{ > + return posix_fallocate(fd, offset, length); Same error number problem as above. > @@ -1185,6 +1220,12 @@ static int raw_create(const char *filename, QEMUOptionParameter *options, > result = -errno; > error_setg_errno(errp, -result, "Could not resize file"); > } > + if (prealloc == PREALLOC_MODE_FULL) { > + result = raw_preallocate2(fd, 0, total_size); What if ftruncate() failed? We should not try to fallocate. Please add a proper error return path. > + if (result != 0) > + error_setg_errno(errp, -result, > + "Could not preallocate data for the new file"); WARNING: braces {} are necessary for all arms of this statement Please run scripts/checkpatch.pl before submitting patches, it checks QEMU coding style.
On Fri, Dec 27, 2013 at 11:05:53AM +0800, Hu Tao wrote: > diff --git a/block/raw-posix.c b/block/raw-posix.c > index 6f6b8c1..a722d27 100644 > --- a/block/raw-posix.c > +++ b/block/raw-posix.c > @@ -1160,17 +1160,52 @@ 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); > + if (ret < 0) { > + ret = -errno; > + } > + > + /* fallback to posix_fallocate() if fallocate() is not supported */ > + if (ret == -ENOSYS || ret == -EOPNOTSUPP) { > + ret = posix_fallocate(fd, offset, length); > + } > + > + return ret; > +} Why is this Linux-specific #ifdef necessary? glibc will try to use the fallocate(2) system call, if possible. Stefan
On Fri, Jan 17, 2014 at 04:25:14PM +0800, Stefan Hajnoczi wrote: > On Fri, Dec 27, 2013 at 11:05:53AM +0800, Hu Tao wrote: > > 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 | 46 ++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 46 insertions(+) > > > > diff --git a/block/raw-posix.c b/block/raw-posix.c > > index 6f6b8c1..a722d27 100644 > > --- a/block/raw-posix.c > > +++ b/block/raw-posix.c > > @@ -1160,17 +1160,52 @@ 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) > > Why is this function called raw_preallocate2()? Please choose a > descriptive name. Sure. > > > +{ > > + int ret = -1; > > + > > + ret = fallocate(fd, 0, offset, length); > > + if (ret < 0) { > > + ret = -errno; > > + } > > + > > + /* fallback to posix_fallocate() if fallocate() is not supported */ > > + if (ret == -ENOSYS || ret == -EOPNOTSUPP) { > > + ret = posix_fallocate(fd, offset, length); > > From the posix_fallocate(3) man page: > "posix_fallocate() returns zero on success, or an error number on > failure. Note that errno is not set." > > You need to negate the error number: > if (ret > 0) { > ret = -ret; /* posix_fallocate(3) returns a positive errno */ > } Sure. > > > + } > > + > > + return ret; > > +} > > +#else > > +static int raw_preallocate2(int fd, int64_t offset, int64_t length) > > +{ > > + return posix_fallocate(fd, offset, length); > > Same error number problem as above. > > > @@ -1185,6 +1220,12 @@ static int raw_create(const char *filename, QEMUOptionParameter *options, > > result = -errno; > > error_setg_errno(errp, -result, "Could not resize file"); > > } > > + if (prealloc == PREALLOC_MODE_FULL) { > > + result = raw_preallocate2(fd, 0, total_size); > > What if ftruncate() failed? We should not try to fallocate. Please add > a proper error return path. Sure. > > > + if (result != 0) > > + error_setg_errno(errp, -result, > > + "Could not preallocate data for the new file"); > > WARNING: braces {} are necessary for all arms of this statement > > Please run scripts/checkpatch.pl before submitting patches, it checks > QEMU coding style. Thanks!
On Fri, Jan 17, 2014 at 04:56:42PM +0800, Stefan Hajnoczi wrote: > On Fri, Dec 27, 2013 at 11:05:53AM +0800, Hu Tao wrote: > > diff --git a/block/raw-posix.c b/block/raw-posix.c > > index 6f6b8c1..a722d27 100644 > > --- a/block/raw-posix.c > > +++ b/block/raw-posix.c > > @@ -1160,17 +1160,52 @@ 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); > > + if (ret < 0) { > > + ret = -errno; > > + } > > + > > + /* fallback to posix_fallocate() if fallocate() is not supported */ > > + if (ret == -ENOSYS || ret == -EOPNOTSUPP) { > > + ret = posix_fallocate(fd, offset, length); > > + } > > + > > + return ret; > > +} > > Why is this Linux-specific #ifdef necessary? glibc will try to use the > fallocate(2) system call, if possible. Fine. I'll drop this.
diff --git a/block/raw-posix.c b/block/raw-posix.c index 6f6b8c1..a722d27 100644 --- a/block/raw-posix.c +++ b/block/raw-posix.c @@ -1160,17 +1160,52 @@ 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); + if (ret < 0) { + ret = -errno; + } + + /* fallback to posix_fallocate() if fallocate() is not supported */ + if (ret == -ENOSYS || ret == -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_create(const char *filename, QEMUOptionParameter *options, Error **errp) { int fd; int result = 0; int64_t total_size = 0; + PreallocMode prealloc = PREALLOC_MODE_OFF; /* Read out 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++; } @@ -1185,6 +1220,12 @@ static int raw_create(const char *filename, QEMUOptionParameter *options, result = -errno; error_setg_errno(errp, -result, "Could not resize file"); } + if (prealloc == PREALLOC_MODE_FULL) { + result = raw_preallocate2(fd, 0, total_size); + if (result != 0) + 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"); @@ -1340,6 +1381,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 } };
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 | 46 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+)