3:13 p.m.
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
3:13 p.m.
New subject: [libvirt] [PATCH 1/2] virfile: Refactor safezero
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(a)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
3:13 p.m.
New subject: [libvirt] [PATCH 2/2] virstoragefile: Have virStorageFileResize use safezero
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(a)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
8:36 a.m.
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
3631
days inactive
3632
days old
3 comments
2 participants
participants (2)
-
John Ferlan -
Michal Privoznik