[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH v2 3/3] virstoragefile: Refactor virStorageFileResize




On 08/21/2014 11:53 AM, Ján Tomko wrote:
> On 08/11/2014 10:30 PM, John Ferlan wrote:
>> Currently virStorageFileResize() function uses build conditionals to
>> choose either the posix_fallocate() or mmap() with no fallback in order
>> to preallocate the space in the newly resized file.
>>
>> This patch will modify the logic in order to allow fallbacks in the event
>> that posix_fallocate() or the sys_fallocate syscall() doesn't work properly.
>> The fallback will be to use the slow safewrite of zero filled buffers to
>> the file.
>>
>> Use the virFileFdPosixFallocate() rather than a local function.
>>
>> Signed-off-by: John Ferlan <jferlan redhat com>
>> ---
>>  src/util/virstoragefile.c | 52 +++++++++++++++++++++++++++++++----------------
>>  1 file changed, 34 insertions(+), 18 deletions(-)
>>
>> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
>> index 5b6b2f5..4d37de1 100644
>> --- a/src/util/virstoragefile.c
>> +++ b/src/util/virstoragefile.c
>> @@ -1086,6 +1086,39 @@ virStorageFileChainGetBroken(virStorageSourcePtr chain,
>>      return 0;
>>  }
>>  
>> +static int
>> +resizeSysFallocate(const char *path,
>> +                   int fd,
>> +                   off_t offset,
>> +                   off_t len)
>> +{
>> +    int rc = -1;
>> +#if HAVE_SYS_SYSCALL_H && defined(SYS_fallocate)
>> +    if ((rc = syscall(SYS_fallocate, fd, 0, offset, len)) != 0) {
>> +        virReportSystemError(errno,
>> +                             _("Failed to pre-allocate space for "
>> +                             "file '%s'"), path);
> 
> I think this should not log an error, since we have a fallback.
> VIR_DEBUG maybe?
> 

Yep - right.  Although to match the other path, VIR_WARN...

>> +    }
>> +#endif
>> +    return rc;
>> +}
>> +
>> +static int
>> +doResize(const char *path,
>> +         int fd,
>> +         off_t offset,
>> +         off_t len)
>> +{
>> +    if (virFileFdPosixFallocate(fd, offset, len) == 0 ||
>> +        resizeSysFallocate(path, fd, offset, len) == 0 ||
>> +        safezero(fd, offset, len) == 0)
>> +        return 0;
>> +
>> +    virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
>> +                   _("preallocate is not supported on this platform"));
>> +    return -1;
> 
> safezero is always supported. And this function should do:
> return safezero(fd, offset, len);
> 
> 

Hmm.. Perhaps a better way to do this would be to modify safezero() to
add a 4th boolean parameter "resize" and make the "safezero_mmap()" be
the false side and the check/call to SYS_fallocate() be the true side.
That way all the logic resides in virfile.c

Thus doResize() goes away and resizeSysFallocate() moves to virfile.c.
All virStorageFileResize() does for the "if (preAllocate)" condition "if
(safezero(...,true) < 0)"

See the attached patch file that "should" be able to be "git am"'d on
top of the existing patches (although I did rebase to top to tree this
morning).

Does that seem better?

John



>> +}
>> +
>>  
>>  /**
>>   * virStorageFileResize:
>> @@ -1113,25 +1146,8 @@ 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 (doResize(path, fd, offset, len) < 0)
>>              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,
>>
> 
> 
>From 94fce5c3b3ba6575998d6be7203b8e3a657c2bdf Mon Sep 17 00:00:00 2001
From: John Ferlan <jferlan redhat com>
Date: Thu, 21 Aug 2014 12:43:52 -0400
Subject: [PATCH] Rework virStorageFileResize

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                | 29 ++++++++++++++++++++++++++---
 src/util/virfile.h                |  2 +-
 src/util/virstoragefile.c         | 39 +--------------------------------------
 5 files changed, 31 insertions(+), 45 deletions(-)

diff --git a/src/locking/lock_driver_sanlock.c b/src/locking/lock_driver_sanlock.c
index ea43051..b368f17 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 9c775c9..a6be9d6 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -401,7 +401,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 6139810..a642e5c 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
@@ -1104,6 +1107,21 @@ safezero_mmap(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)
+    if ((rc = syscall(SYS_fallocate, fd, 0, offset, len)) != 0) {
+        VIR_WARN("Failed to SYS_fallocate '%lu' bytes with errno=%d",
+                 len, errno);
+    }
+#endif
+    return rc;
+}
+
+static int
 safezero_slow(int fd, off_t offset, off_t len)
 {
     int r;
@@ -1141,12 +1159,17 @@ safezero_slow(int fd, off_t offset, off_t len)
 }
 
 int
-safezero(int fd, off_t offset, off_t len)
+safezero(int fd, off_t offset, off_t len, bool resize)
 {
     if (virFileFdPosixFallocate(fd, offset, len) == 0)
         return 0;
-    if (safezero_mmap(fd, offset, len) == 0)
-        return 0;
+    if (resize) {
+        if (safezero_sys_fallocate(fd, offset, len) == 0)
+            return 0;
+    } else {
+        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 29a1776..546f674 100644
--- a/src/util/virfile.h
+++ b/src/util/virfile.h
@@ -43,7 +43,7 @@ ssize_t safewrite(int fd, const void *buf, size_t count)
     ATTRIBUTE_RETURN_CHECK;
 int virFileFdPosixFallocate(int fd, off_t offset, off_t len)
     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 4d37de1..77a39ac 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
 
@@ -1086,40 +1083,6 @@ virStorageFileChainGetBroken(virStorageSourcePtr chain,
     return 0;
 }
 
-static int
-resizeSysFallocate(const char *path,
-                   int fd,
-                   off_t offset,
-                   off_t len)
-{
-    int rc = -1;
-#if HAVE_SYS_SYSCALL_H && defined(SYS_fallocate)
-    if ((rc = syscall(SYS_fallocate, fd, 0, offset, len)) != 0) {
-        virReportSystemError(errno,
-                             _("Failed to pre-allocate space for "
-                             "file '%s'"), path);
-    }
-#endif
-    return rc;
-}
-
-static int
-doResize(const char *path,
-         int fd,
-         off_t offset,
-         off_t len)
-{
-    if (virFileFdPosixFallocate(fd, offset, len) == 0 ||
-        resizeSysFallocate(path, fd, offset, len) == 0 ||
-        safezero(fd, offset, len) == 0)
-        return 0;
-
-    virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
-                   _("preallocate is not supported on this platform"));
-    return -1;
-}
-
-
 /**
  * virStorageFileResize:
  *
@@ -1146,7 +1109,7 @@ virStorageFileResize(const char *path,
     }
 
     if (pre_allocate) {
-        if (doResize(path, fd, offset, len) < 0)
+        if (safezero(fd, offset, len, true) < 0)
             goto cleanup;
     } else {
         if (ftruncate(fd, capacity) < 0) {
-- 
1.9.3


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]