| Message ID | 20170313214045.26857-5-mreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Mar 13, 2017 at 10:40:36PM +0100, Max Reitz wrote: > Currently, raw_regular_truncate() is intended for setting the size of a > newly created file. However, we also want to use it for truncating an > existing file in which case only the newly added space (when growing) > should be preallocated. > > This also means that if resizing failed, we should try to restore the > original file size. This is important when using preallocation. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block/file-posix.c | 73 ++++++++++++++++++++++++++++++++++++++++++++---------- > 1 file changed, 60 insertions(+), 13 deletions(-) > > diff --git a/block/file-posix.c b/block/file-posix.c > index 35a9e43f3e..cd229324ba 100644 > --- a/block/file-posix.c > +++ b/block/file-posix.c > @@ -1384,11 +1384,39 @@ static void raw_close(BlockDriverState *bs) > } > } > > -static int raw_regular_truncate(int fd, int64_t offset, PreallocMode prealloc, > +/** > + * Truncates the given regular file @fd to @offset and, when growing, fills the > + * new space according to @prealloc. > + * > + * @create must be true iff the file is new. In that case, @bs is ignored. If > + * @create is false, @bs must be valid and correspond to the same file as @fd. Returns: 0 on success, -errno on failure. > + */ > +static int raw_regular_truncate(int fd, BlockDriverState *bs, int64_t offset, The presence of both a file descriptor and a BlockDriverState (actually it must be a BDRVRawState) arguments is unusual. It seems bs is needed for bdrv_getlength(). How about using fstat(fd, &st) and then dropping bs and create? > + PreallocMode prealloc, bool create, > Error **errp) > { > int result = 0; > - char *buf; > + int64_t current_length = 0; > + char *buf = NULL; > + > + assert(create || bs); > + if (!create) { > + BDRVRawState *s = bs->opaque; > + assert(s->fd == fd); > + } > + > + if (!create) { > + current_length = bdrv_getlength(bs); > + if (current_length < 0) { > + error_setg_errno(errp, -current_length, > + "Could not inquire current length"); > + return -current_length; > + } > + > + if (current_length > offset && prealloc != PREALLOC_MODE_OFF) { > + return -ENOTSUP; > + } Missing error_setg().
On 20.03.2017 12:00, Stefan Hajnoczi wrote: > On Mon, Mar 13, 2017 at 10:40:36PM +0100, Max Reitz wrote: >> Currently, raw_regular_truncate() is intended for setting the size of a >> newly created file. However, we also want to use it for truncating an >> existing file in which case only the newly added space (when growing) >> should be preallocated. >> >> This also means that if resizing failed, we should try to restore the >> original file size. This is important when using preallocation. >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> block/file-posix.c | 73 ++++++++++++++++++++++++++++++++++++++++++++---------- >> 1 file changed, 60 insertions(+), 13 deletions(-) >> >> diff --git a/block/file-posix.c b/block/file-posix.c >> index 35a9e43f3e..cd229324ba 100644 >> --- a/block/file-posix.c >> +++ b/block/file-posix.c >> @@ -1384,11 +1384,39 @@ static void raw_close(BlockDriverState *bs) >> } >> } >> >> -static int raw_regular_truncate(int fd, int64_t offset, PreallocMode prealloc, >> +/** >> + * Truncates the given regular file @fd to @offset and, when growing, fills the >> + * new space according to @prealloc. >> + * >> + * @create must be true iff the file is new. In that case, @bs is ignored. If >> + * @create is false, @bs must be valid and correspond to the same file as @fd. > > Returns: 0 on success, -errno on failure. Yep, will add. >> + */ >> +static int raw_regular_truncate(int fd, BlockDriverState *bs, int64_t offset, > > The presence of both a file descriptor and a BlockDriverState (actually > it must be a BDRVRawState) arguments is unusual. It seems bs is needed > for bdrv_getlength(). > > How about using fstat(fd, &st) and then dropping bs and create? Well, I could do that, but bdrv_getlength() is a bit simpler and I don't see much of an issue with specifying both an fd and a bs. >> + PreallocMode prealloc, bool create, >> Error **errp) >> { >> int result = 0; >> - char *buf; >> + int64_t current_length = 0; >> + char *buf = NULL; >> + >> + assert(create || bs); >> + if (!create) { >> + BDRVRawState *s = bs->opaque; >> + assert(s->fd == fd); >> + } >> + >> + if (!create) { >> + current_length = bdrv_getlength(bs); >> + if (current_length < 0) { >> + error_setg_errno(errp, -current_length, >> + "Could not inquire current length"); >> + return -current_length; >> + } >> + >> + if (current_length > offset && prealloc != PREALLOC_MODE_OFF) { >> + return -ENOTSUP; >> + } > > Missing error_setg(). Indeed! Will fix. Max
On Mon, Mar 20, 2017 at 04:11:24PM +0100, Max Reitz wrote: > On 20.03.2017 12:00, Stefan Hajnoczi wrote: > > On Mon, Mar 13, 2017 at 10:40:36PM +0100, Max Reitz wrote: > >> +static int raw_regular_truncate(int fd, BlockDriverState *bs, int64_t offset, > > > > The presence of both a file descriptor and a BlockDriverState (actually > > it must be a BDRVRawState) arguments is unusual. It seems bs is needed > > for bdrv_getlength(). > > > > How about using fstat(fd, &st) and then dropping bs and create? > > Well, I could do that, but bdrv_getlength() is a bit simpler and I don't > see much of an issue with specifying both an fd and a bs. Arguments that provide overlapping information make the function harder to understand and use correctly (there are combinations of these arguments that are invalid or don't make sense). Saving a few lines of code in the function implementation is a poor trade-off IMO. Stefan
On 22.03.2017 17:44, Stefan Hajnoczi wrote: > On Mon, Mar 20, 2017 at 04:11:24PM +0100, Max Reitz wrote: >> On 20.03.2017 12:00, Stefan Hajnoczi wrote: >>> On Mon, Mar 13, 2017 at 10:40:36PM +0100, Max Reitz wrote: >>>> +static int raw_regular_truncate(int fd, BlockDriverState *bs, int64_t offset, >>> >>> The presence of both a file descriptor and a BlockDriverState (actually >>> it must be a BDRVRawState) arguments is unusual. It seems bs is needed >>> for bdrv_getlength(). >>> >>> How about using fstat(fd, &st) and then dropping bs and create? >> >> Well, I could do that, but bdrv_getlength() is a bit simpler and I don't >> see much of an issue with specifying both an fd and a bs. > > Arguments that provide overlapping information make the function harder > to understand and use correctly (there are combinations of these > arguments that are invalid or don't make sense). Saving a few lines of > code in the function implementation is a poor trade-off IMO. My brain likes to agree but for some reason my heart really prefers block layer functions (where I know what can go wrong) over POSIX functions (where I always have the feeling of maybe not having though of everything). I guess I'll have to silence my heart for a bit, then. Max
On Wed, Mar 22, 2017 at 05:53:00PM +0100, Max Reitz wrote: > On 22.03.2017 17:44, Stefan Hajnoczi wrote: > > On Mon, Mar 20, 2017 at 04:11:24PM +0100, Max Reitz wrote: > >> On 20.03.2017 12:00, Stefan Hajnoczi wrote: > >>> On Mon, Mar 13, 2017 at 10:40:36PM +0100, Max Reitz wrote: > >>>> +static int raw_regular_truncate(int fd, BlockDriverState *bs, int64_t offset, > >>> > >>> The presence of both a file descriptor and a BlockDriverState (actually > >>> it must be a BDRVRawState) arguments is unusual. It seems bs is needed > >>> for bdrv_getlength(). > >>> > >>> How about using fstat(fd, &st) and then dropping bs and create? > >> > >> Well, I could do that, but bdrv_getlength() is a bit simpler and I don't > >> see much of an issue with specifying both an fd and a bs. > > > > Arguments that provide overlapping information make the function harder > > to understand and use correctly (there are combinations of these > > arguments that are invalid or don't make sense). Saving a few lines of > > code in the function implementation is a poor trade-off IMO. > > My brain likes to agree but for some reason my heart really prefers > block layer functions (where I know what can go wrong) over POSIX > functions (where I always have the feeling of maybe not having though of > everything). > > I guess I'll have to silence my heart for a bit, then. In matters of the heart I cannot give advice on this mailing list, sorry. Stefan
diff --git a/block/file-posix.c b/block/file-posix.c index 35a9e43f3e..cd229324ba 100644 --- a/block/file-posix.c +++ b/block/file-posix.c @@ -1384,11 +1384,39 @@ static void raw_close(BlockDriverState *bs) } } -static int raw_regular_truncate(int fd, int64_t offset, PreallocMode prealloc, +/** + * Truncates the given regular file @fd to @offset and, when growing, fills the + * new space according to @prealloc. + * + * @create must be true iff the file is new. In that case, @bs is ignored. If + * @create is false, @bs must be valid and correspond to the same file as @fd. + */ +static int raw_regular_truncate(int fd, BlockDriverState *bs, int64_t offset, + PreallocMode prealloc, bool create, Error **errp) { int result = 0; - char *buf; + int64_t current_length = 0; + char *buf = NULL; + + assert(create || bs); + if (!create) { + BDRVRawState *s = bs->opaque; + assert(s->fd == fd); + } + + if (!create) { + current_length = bdrv_getlength(bs); + if (current_length < 0) { + error_setg_errno(errp, -current_length, + "Could not inquire current length"); + return -current_length; + } + + if (current_length > offset && prealloc != PREALLOC_MODE_OFF) { + return -ENOTSUP; + } + } switch (prealloc) { #ifdef CONFIG_POSIX_FALLOCATE @@ -1398,17 +1426,17 @@ static int raw_regular_truncate(int fd, int64_t offset, PreallocMode prealloc, * file systems that do not support fallocate(), trying to check if a * block is allocated before allocating it, so don't do that here. */ - result = -posix_fallocate(fd, 0, offset); + result = -posix_fallocate(fd, current_length, offset - current_length); if (result != 0) { /* posix_fallocate() doesn't set errno. */ error_setg_errno(errp, -result, - "Could not preallocate data for the new file"); + "Could not preallocate new data"); } - return result; + goto out; #endif case PREALLOC_MODE_FULL: { - int64_t num = 0, left = offset; + int64_t num = 0, left = offset - current_length; /* * Knowing the final size from the beginning could allow the file @@ -1418,19 +1446,27 @@ static int raw_regular_truncate(int fd, int64_t offset, PreallocMode prealloc, if (ftruncate(fd, offset) != 0) { result = -errno; error_setg_errno(errp, -result, "Could not resize file"); - return result; + goto out; } buf = g_malloc0(65536); + result = lseek(fd, current_length, SEEK_SET); + if (result < 0) { + result = -errno; + error_setg_errno(errp, -result, + "Failed to seek to the old end of file"); + goto out; + } + while (left > 0) { num = MIN(left, 65536); result = write(fd, buf, num); if (result < 0) { result = -errno; error_setg_errno(errp, -result, - "Could not write to the new file"); - break; + "Could not write zeros for preallocation"); + goto out; } left -= result; } @@ -1439,11 +1475,11 @@ static int raw_regular_truncate(int fd, int64_t offset, PreallocMode prealloc, if (result < 0) { result = -errno; error_setg_errno(errp, -result, - "Could not flush new file to disk"); + "Could not flush file to disk"); + goto out; } } - g_free(buf); - return result; + goto out; } case PREALLOC_MODE_OFF: if (ftruncate(fd, offset) != 0) { @@ -1457,6 +1493,17 @@ static int raw_regular_truncate(int fd, int64_t offset, PreallocMode prealloc, PreallocMode_lookup[prealloc]); return result; } + +out: + if (result < 0 && !create) { + if (ftruncate(fd, current_length) < 0) { + error_report("Failed to restore old file length: %s", + strerror(errno)); + } + } + + g_free(buf); + return result; } static int raw_truncate(BlockDriverState *bs, int64_t offset, @@ -1727,7 +1774,7 @@ static int raw_create(const char *filename, QemuOpts *opts, Error **errp) #endif } - result = raw_regular_truncate(fd, total_size, prealloc, errp); + result = raw_regular_truncate(fd, NULL, total_size, prealloc, true, errp); if (result < 0) { goto out_close; }
Currently, raw_regular_truncate() is intended for setting the size of a newly created file. However, we also want to use it for truncating an existing file in which case only the newly added space (when growing) should be preallocated. This also means that if resizing failed, we should try to restore the original file size. This is important when using preallocation. Signed-off-by: Max Reitz <mreitz@redhat.com> --- block/file-posix.c | 73 ++++++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 60 insertions(+), 13 deletions(-)