[PATCH] storage: fix file allocation behavior in file cloning

[Oskari Saarenmaa <os@xxxxxxx>]

Fixed the safezero call for allocating the rest of the file after cloning
an existing volume; it used to always use a zero offset, causing it to
only allocate the beginning of the file.

Also modified file creation to try to use fallocate(2) to pre-allocate
disk space before copying any data to make sure it fails early on if disk
is full and makes sure we can skip zero blocks when copying file contents.

If fallocate isn't available we will zero out the rest of the file after
cloning and only use sparse cloning if client requested a lower allocation
than the input volume's capacity.

Signed-off-by: Oskari Saarenmaa <os@xxxxxxx>
---
 the safezero mmap issue fixed in my previous patch never showed up because
 all safezero calls previously had 0 offset (or maybe everyone's using
 posix_fallocate)

 configure.ac                  |  8 ++++----
 src/storage/storage_backend.c | 35 ++++++++++++++++++++++++++++++-----
 2 files changed, 34 insertions(+), 9 deletions(-)

diff --git a/configure.ac b/configure.ac
index 553015a..220d95b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -253,10 +253,10 @@ AC_CHECK_SIZEOF([long])
 
 dnl Availability of various common functions (non-fatal if missing),
 dnl and various less common threadsafe functions
-AC_CHECK_FUNCS_ONCE([cfmakeraw geteuid getgid getgrnam_r getmntent_r \
-  getpwuid_r getuid kill mmap newlocale posix_fallocate posix_memalign \
-  prlimit regexec sched_getaffinity setgroups setns setrlimit symlink \
-  sysctlbyname])
+AC_CHECK_FUNCS_ONCE([cfmakeraw fallocate geteuid getgid getgrnam_r \
+  getmntent_r getpwuid_r getuid kill mmap newlocale posix_fallocate \
+  posix_memalign prlimit regexec sched_getaffinity setgroups setns \
+  setrlimit symlink sysctlbyname])
 
 dnl Availability of pthread functions (if missing, win32 threading is
 dnl assumed).  Because of $LIB_PTHREAD, we cannot use AC_CHECK_FUNCS_ONCE.
diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c
index b7edf85..5f1bc66 100644
--- a/src/storage/storage_backend.c
+++ b/src/storage/storage_backend.c
@@ -129,7 +129,7 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol,
                           virStorageVolDefPtr inputvol,
                           int fd,
                           unsigned long long *total,
-                          int is_dest_file)
+                          int want_sparse)
 {
     int inputfd = -1;
     int amtread = -1;
@@ -191,7 +191,7 @@ virStorageBackendCopyToFD(virStorageVolDefPtr vol,
             interval = ((wbytes > amtleft) ? amtleft : wbytes);
             int offset = amtread - amtleft;
 
-            if (is_dest_file && memcmp(buf+offset, zerobuf, interval) == 0) {
+            if (want_sparse && memcmp(buf+offset, zerobuf, interval) == 0) {
                 if (lseek(fd, interval, SEEK_CUR) < 0) {
                     ret = -errno;
                     virReportSystemError(errno,
@@ -315,6 +315,7 @@ static int
 createRawFile(int fd, virStorageVolDefPtr vol,
               virStorageVolDefPtr inputvol)
 {
+    int need_alloc = 1;
     int ret = 0;
     unsigned long long remain;
 
@@ -328,17 +329,41 @@ createRawFile(int fd, virStorageVolDefPtr vol,
         goto cleanup;
     }
 
+#ifdef HAVE_FALLOCATE
+    /* Try to preallocate all requested disk space, but fall back to
+     * other methods if this fails with ENOSYS or EOPNOTSUPP.
+     * NOTE: do not use posix_fallocate; posix_fallocate falls back
+     * to writing zeroes block by block in case fallocate isn't
+     * available, and since we're going to copy data from another
+     * file it doesn't make sense to write the file twice. */
+    if (fallocate(fd, 0, 0, vol->allocation) == 0) {
+        need_alloc = 0;
+    } else if (errno != ENOSYS && errno != EOPNOTSUPP) {
+        ret = -errno;
+        virReportSystemError(errno,
+                             _("cannot allocate %llu bytes in file '%s'"),
+                             vol->allocation, vol->target.path);
+        goto cleanup;
+    }
+#endif
+
     remain = vol->allocation;
 
     if (inputvol) {
-        ret = virStorageBackendCopyToFD(vol, inputvol, fd, &remain, 1);
+        /* allow zero blocks to be skipped if we've requested sparse
+         * allocation (allocation < capacity) or we have already
+         * been able to allocate the required space. */
+        int want_sparse = (need_alloc == 0) ||
+                          (vol->allocation < inputvol->capacity);
+
+        ret = virStorageBackendCopyToFD(vol, inputvol, fd, &remain, want_sparse);
         if (ret < 0) {
             goto cleanup;
         }
     }
 
-    if (remain) {
-        if (safezero(fd, 0, remain) < 0) {
+    if (remain && need_alloc) {
+        if (safezero(fd, vol->allocation - remain, remain) < 0) {
             ret = -errno;
             virReportSystemError(errno, _("cannot fill file '%s'"),
                                  vol->target.path);
-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list