[libvirt] [PATCH 0/2] Refactor safezero and virStorageFileResize
Originally included as part of a series to fix a problem with file creation and resize within NFS storage pools that ended up being an issue somewhere lower in the stack of posix_fallocate or the setting of the block size in the meta data. The following two patches extract out the refactoring of safezero and reworking virStorageFileResize to utilize safezero instead. Original series and review comments for comparison: http://www.redhat.com/archives/libvir-list/2014-August/msg00491.html I figured rather than lose any of the code and there is some value in utilizing the same code in order to handle a similar function that I'd repost as a separate series. John Ferlan (2): virfile: Refactor safezero virstoragefile: Have virStorageFileResize use safezero src/locking/lock_driver_sanlock.c | 4 +-- src/storage/storage_backend.c | 2 +- src/util/virfile.c | 63 +++++++++++++++++++++++++++++++++------ src/util/virfile.h | 2 +- src/util/virstoragefile.c | 29 ++++++------------ 5 files changed, 67 insertions(+), 33 deletions(-) -- 1.9.3
Currently build conditionals decide which of two safezero() functions should be built - either the posix_fallocate() or mmap() with a fallback to a slower safewrite() algorithm in order to preallocate space in a raw file. This patch will refactor safezero to utilize static functions for either posix_fallocate or mmap/safewrite. The build conditional still exist, but are only for shorter sections of code. The posix_fallocate path will make use of the ret/errno setting to contain the logic for safezero to decide whether it needs to fallback to other algorithms. A return of -1 with errno not changed will indicate the conditional is not present; otherwise, a return of -1 with errno change indicates the call was made and it failed (no functional difference to current algorithm). The mmap/safewrite option changes only slightly to handle the ftruncate failure for mmap. That is, previously if the ftruncate failed, there was no fallback to the slow safewrite option. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/util/virfile.c | 47 ++++++++++++++++++++++++++++++++++++----------- 1 file changed, 36 insertions(+), 11 deletions(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index b4d762f..5e8c306 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -1034,26 +1034,24 @@ safewrite(int fd, const void *buf, size_t count) return nwritten; } -#ifdef HAVE_POSIX_FALLOCATE -int -safezero(int fd, off_t offset, off_t len) +static int +safezero_posix_fallocate(int fd, off_t offset, off_t len) { +#ifdef HAVE_POSIX_FALLOCATE int ret = posix_fallocate(fd, offset, len); if (ret == 0) return 0; errno = ret; +#endif return -1; } -#else - -int -safezero(int fd, off_t offset, off_t len) +static int +safezero_mmap(int fd, off_t offset, off_t len) { +#ifdef HAVE_MMAP int r; char *buf; - unsigned long long remain, bytes; -# ifdef HAVE_MMAP static long pagemask; off_t map_skip; @@ -1080,7 +1078,16 @@ safezero(int fd, off_t offset, off_t len) /* fall back to writing zeroes using safewrite if mmap fails (for * example because of virtual memory limits) */ -# endif /* HAVE_MMAP */ +#endif /* HAVE_MMAP */ + return -1; +} + +static int +safezero_slow(int fd, off_t offset, off_t len) +{ + int r; + char *buf; + unsigned long long remain, bytes; if (lseek(fd, offset, SEEK_SET) < 0) return -1; @@ -1111,8 +1118,26 @@ safezero(int fd, off_t offset, off_t len) VIR_FREE(buf); return 0; } -#endif /* HAVE_POSIX_FALLOCATE */ +int safezero(int fd, off_t offset, off_t len) +{ + int ret; + + /* posix_fallocate returns 0 on success or error number on failure, + * but errno is not set so use that to our advantage since we set + * errno to the returned value if we make the call. If we don't make + * the call because it doesn't exist, then errno won't change and + * we can try other methods. + */ + errno = 0; + ret = safezero_posix_fallocate(fd, offset, len); + if (ret == 0 || errno != 0) + return ret; + + if (safezero_mmap(fd, offset, len) == 0) + return 0; + return safezero_slow(fd, offset, len); +} #if defined HAVE_MNTENT_H && defined HAVE_GETMNTENT_R /* search /proc/mounts for mount point of *type; return pointer to -- 1.9.3
Currently virStorageFileResize() function uses build conditionals to choose either the posix_fallocate() or syscall(SYS_fallocate) with no fallback in order to preallocate the space in the newly resized file. Since the safezero code has a similar set of conditionals modify the resize and safezero code in order to allow the resize logic to make use of safezero to unify the look/feel of the code paths. Add a new boolean (resize) to safezero() to make the optional decision whether to try syscall(SYS_fallocate) if the posix_fallocate fails because HAVE_POSIX_FALLOCATE is not defined (eg, return -1 and errno == 0). Create a local safezero_sys_fallocate in order to handle the resize code paths that support that. If not present, the set errno = ENOSYS in order to allow the caller to handle the failure scenarios. Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/locking/lock_driver_sanlock.c | 4 ++-- src/storage/storage_backend.c | 2 +- src/util/virfile.c | 22 +++++++++++++++++++++- src/util/virfile.h | 2 +- src/util/virstoragefile.c | 29 +++++++++-------------------- 5 files changed, 34 insertions(+), 25 deletions(-) diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c index 60f305c..9fc97db 100644 --- a/src/locking/lock_driver_sanlock.c +++ b/src/locking/lock_driver_sanlock.c @@ -281,7 +281,7 @@ static int virLockManagerSanlockSetupLockspace(void) /* * Pre allocate enough data for 1 block of leases at preferred alignment */ - if (safezero(fd, 0, rv) < 0) { + if (safezero(fd, 0, rv, false) < 0) { virReportSystemError(errno, _("Unable to allocate lockspace %s"), path); @@ -690,7 +690,7 @@ static int virLockManagerSanlockCreateLease(struct sanlk_resource *res) /* * Pre allocate enough data for 1 block of leases at preferred alignment */ - if (safezero(fd, 0, rv) < 0) { + if (safezero(fd, 0, rv, false) < 0) { virReportSystemError(errno, _("Unable to allocate lease %s"), res->disks[0].path); diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index b990a82..472cec6 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -399,7 +399,7 @@ createRawFile(int fd, virStorageVolDefPtr vol, } if (remain && need_alloc) { - if (safezero(fd, vol->target.allocation - remain, remain) < 0) { + if (safezero(fd, vol->target.allocation - remain, remain, false) < 0) { ret = -errno; virReportSystemError(errno, _("cannot fill file '%s'"), vol->target.path); diff --git a/src/util/virfile.c b/src/util/virfile.c index 5e8c306..4483cce 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -42,6 +42,9 @@ #if HAVE_MMAP # include <sys/mman.h> #endif +#if HAVE_SYS_SYSCALL_H +# include <sys/syscall.h> +#endif #ifdef __linux__ # if HAVE_LINUX_MAGIC_H @@ -1047,6 +1050,20 @@ safezero_posix_fallocate(int fd, off_t offset, off_t len) } static int +safezero_sys_fallocate(int fd, + off_t offset, + off_t len) +{ + int rc = -1; +#if HAVE_SYS_SYSCALL_H && defined(SYS_fallocate) + rc = syscall(SYS_fallocate, fd, 0, offset, len); +#else + errno = ENOSYS; +#endif + return rc; +} + +static int safezero_mmap(int fd, off_t offset, off_t len) { #ifdef HAVE_MMAP @@ -1119,7 +1136,7 @@ safezero_slow(int fd, off_t offset, off_t len) return 0; } -int safezero(int fd, off_t offset, off_t len) +int safezero(int fd, off_t offset, off_t len, bool resize) { int ret; @@ -1134,6 +1151,9 @@ int safezero(int fd, off_t offset, off_t len) if (ret == 0 || errno != 0) return ret; + if (resize) + return safezero_sys_fallocate(fd, offset, len); + if (safezero_mmap(fd, offset, len) == 0) return 0; return safezero_slow(fd, offset, len); diff --git a/src/util/virfile.h b/src/util/virfile.h index 403d0ba..b8e30c3 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -41,7 +41,7 @@ typedef enum { ssize_t saferead(int fd, void *buf, size_t count) ATTRIBUTE_RETURN_CHECK; ssize_t safewrite(int fd, const void *buf, size_t count) ATTRIBUTE_RETURN_CHECK; -int safezero(int fd, off_t offset, off_t len) +int safezero(int fd, off_t offset, off_t len, bool resize) ATTRIBUTE_RETURN_CHECK; /* Don't call these directly - use the macros below */ diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index c424251..9efe748 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -43,9 +43,6 @@ #include "viruri.h" #include "dirname.h" #include "virbuffer.h" -#if HAVE_SYS_SYSCALL_H -# include <sys/syscall.h> -#endif #define VIR_FROM_THIS VIR_FROM_STORAGE @@ -1120,25 +1117,17 @@ virStorageFileResize(const char *path, } if (pre_allocate) { -#if HAVE_POSIX_FALLOCATE - if ((rc = posix_fallocate(fd, offset, len)) != 0) { - virReportSystemError(rc, - _("Failed to pre-allocate space for " - "file '%s'"), path); - goto cleanup; - } -#elif HAVE_SYS_SYSCALL_H && defined(SYS_fallocate) - if (syscall(SYS_fallocate, fd, 0, offset, len) != 0) { - virReportSystemError(errno, - _("Failed to pre-allocate space for " - "file '%s'"), path); + if (safezero(fd, offset, len, true) != 0) { + if (errno == ENOSYS) + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("preallocate is not supported on this " + "platform")); + else + virReportSystemError(errno, + _("Failed to pre-allocate space for " + "file '%s'"), path); goto cleanup; } -#else - virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", - _("preallocate is not supported on this platform")); - goto cleanup; -#endif } else { if (ftruncate(fd, capacity) < 0) { virReportSystemError(errno, -- 1.9.3
On 15.12.2014 22:13, John Ferlan wrote:
Originally included as part of a series to fix a problem with file creation and resize within NFS storage pools that ended up being an issue somewhere lower in the stack of posix_fallocate or the setting of the block size in the meta data. The following two patches extract out the refactoring of safezero and reworking virStorageFileResize to utilize safezero instead.
Original series and review comments for comparison:
http://www.redhat.com/archives/libvir-list/2014-August/msg00491.html
I figured rather than lose any of the code and there is some value in utilizing the same code in order to handle a similar function that I'd repost as a separate series.
John Ferlan (2): virfile: Refactor safezero virstoragefile: Have virStorageFileResize use safezero
src/locking/lock_driver_sanlock.c | 4 +-- src/storage/storage_backend.c | 2 +- src/util/virfile.c | 63 +++++++++++++++++++++++++++++++++------ src/util/virfile.h | 2 +- src/util/virstoragefile.c | 29 ++++++------------ 5 files changed, 67 insertions(+), 33 deletions(-)
ACK to both Michal
participants (2)
-
John Ferlan -
Michal Privoznik