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

[Libguestfs] [PATCH libnbd v3 1/2] copy: Do not use trim for zeroing



Current code uses NBD_CMD_TRIM for writing zeroes without allocating
space (--allocate not specified). This incorrect and can lead to
corrupting the target image.

NBD manual warns about NBD_CMD_TRIM:

    After issuing this command, a client MUST NOT make any assumptions
    about the contents of the export affected by this command, until
    overwriting it again with NBD_CMD_WRITE or NBD_CMD_WRITE_ZEROES

Add allocate flag to the zero operation, to allow zeroing by punching
hole (default) or zeroing by allocating space.

nbd-ops use NBD_CMD_WRITE_ZEROES with or without NBD_FLAG_NO_HOLE
flag. file-ops use fallocate with PUNCH_HOLE or ZERO_RANGE flag.

The copy-sparse tests was modified to require zero operation with
may_trim flag.

Since trimming is not used now, remove synch_trim, asynch_trim, can_trim
and update the comments.

Signed-off-by: Nir Soffer <nsoffer redhat com>
---
 copy/copy-sparse.sh         | 66 +++++++++++++--------------
 copy/file-ops.c             | 90 ++++++++++++++++++-------------------
 copy/multi-thread-copying.c | 24 ++++------
 copy/nbd-ops.c              | 51 +++------------------
 copy/nbdcopy.h              | 20 +++------
 copy/null-ops.c             | 28 ++----------
 copy/pipe-ops.c             | 12 +++--
 copy/synch-copying.c        |  8 ++--
 8 files changed, 111 insertions(+), 188 deletions(-)

diff --git a/copy/copy-sparse.sh b/copy/copy-sparse.sh
index 846767b..c43b41a 100755
--- a/copy/copy-sparse.sh
+++ b/copy/copy-sparse.sh
@@ -33,7 +33,7 @@ cleanup_fn rm -f $out
 
 # Copy from a sparse data disk to an nbdkit-eval-plugin instance which
 # is logging everything.  This allows us to see exactly what nbdcopy
-# is writing, to ensure it is writing and trimming the target as
+# is writing, to ensure it is writing and zeroing the target as
 # expected.
 $VG nbdcopy -S 0 -- \
     [ nbdkit --exit-with-parent data data='
@@ -60,38 +60,38 @@ if [ "$(cat $out)" != "pwrite 1 4294967296
 pwrite 32768 0
 pwrite 32768 1073709056
 pwrite 32768 4294934528
-trim 134184960 32768
-trim 134184960 4160749568
-trim 134184960 939524096
-trim 134217728 1073741824
-trim 134217728 1207959552
-trim 134217728 134217728
-trim 134217728 1342177280
-trim 134217728 1476395008
-trim 134217728 1610612736
-trim 134217728 1744830464
-trim 134217728 1879048192
-trim 134217728 2013265920
-trim 134217728 2147483648
-trim 134217728 2281701376
-trim 134217728 2415919104
-trim 134217728 2550136832
-trim 134217728 268435456
-trim 134217728 2684354560
-trim 134217728 2818572288
-trim 134217728 2952790016
-trim 134217728 3087007744
-trim 134217728 3221225472
-trim 134217728 3355443200
-trim 134217728 3489660928
-trim 134217728 3623878656
-trim 134217728 3758096384
-trim 134217728 3892314112
-trim 134217728 402653184
-trim 134217728 4026531840
-trim 134217728 536870912
-trim 134217728 671088640
-trim 134217728 805306368" ]; then
+zero 134184960 32768 may_trim
+zero 134184960 4160749568 may_trim
+zero 134184960 939524096 may_trim
+zero 134217728 1073741824 may_trim
+zero 134217728 1207959552 may_trim
+zero 134217728 1342177280 may_trim
+zero 134217728 134217728 may_trim
+zero 134217728 1476395008 may_trim
+zero 134217728 1610612736 may_trim
+zero 134217728 1744830464 may_trim
+zero 134217728 1879048192 may_trim
+zero 134217728 2013265920 may_trim
+zero 134217728 2147483648 may_trim
+zero 134217728 2281701376 may_trim
+zero 134217728 2415919104 may_trim
+zero 134217728 2550136832 may_trim
+zero 134217728 2684354560 may_trim
+zero 134217728 268435456 may_trim
+zero 134217728 2818572288 may_trim
+zero 134217728 2952790016 may_trim
+zero 134217728 3087007744 may_trim
+zero 134217728 3221225472 may_trim
+zero 134217728 3355443200 may_trim
+zero 134217728 3489660928 may_trim
+zero 134217728 3623878656 may_trim
+zero 134217728 3758096384 may_trim
+zero 134217728 3892314112 may_trim
+zero 134217728 4026531840 may_trim
+zero 134217728 402653184 may_trim
+zero 134217728 536870912 may_trim
+zero 134217728 671088640 may_trim
+zero 134217728 805306368 may_trim" ]; then
     echo "$0: output does not match expected"
     exit 1
 fi
diff --git a/copy/file-ops.c b/copy/file-ops.c
index 2860764..1ae1cfc 100644
--- a/copy/file-ops.c
+++ b/copy/file-ops.c
@@ -221,11 +221,9 @@ file_synch_write (struct rw *rw,
 }
 
 static bool
-file_synch_trim (struct rw *rw, uint64_t offset, uint64_t count)
+file_punch_hole(int fd, uint64_t offset, uint64_t count)
 {
 #ifdef FALLOC_FL_PUNCH_HOLE
-  struct rw_file *rwf = (struct rw_file *)rw;
-  int fd = rwf->fd;
   int r;
 
   r = fallocate (fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
@@ -235,42 +233,58 @@ file_synch_trim (struct rw *rw, uint64_t offset, uint64_t count)
     exit (EXIT_FAILURE);
   }
   return true;
-#else /* !FALLOC_FL_PUNCH_HOLE */
-  return false;
 #endif
+  return false;
 }
 
 static bool
-file_synch_zero (struct rw *rw, uint64_t offset, uint64_t count)
+file_zero_range(int fd, uint64_t offset, uint64_t count)
 {
-  struct rw_file *rwf = (struct rw_file *)rw;
-
-  if (! rwf->is_block) {
 #ifdef FALLOC_FL_ZERO_RANGE
-    int fd = rwf->fd;
-    int r;
+  int r;
 
-    r = fallocate (fd, FALLOC_FL_ZERO_RANGE, offset, count);
-    if (r == -1) {
-      perror ("fallocate: FALLOC_FL_ZERO_RANGE");
-      exit (EXIT_FAILURE);
-    }
-    return true;
-#endif
+  r = fallocate (fd, FALLOC_FL_ZERO_RANGE, offset, count);
+  if (r == -1) {
+    perror ("fallocate: FALLOC_FL_ZERO_RANGE");
+    exit (EXIT_FAILURE);
   }
-  else if (rwf->is_block && IS_ALIGNED (offset | count, rwf->sector_size)) {
+  return true;
+#endif
+  return false;
+}
+
+static bool
+file_zeroout(int fd, uint64_t offset, uint64_t count)
+{
 #ifdef BLKZEROOUT
-    int fd = rwf->fd;
-    int r;
-    uint64_t range[2] = {offset, count};
+  int r;
+  uint64_t range[2] = {offset, count};
 
-    r = ioctl (fd, BLKZEROOUT, &range);
-    if (r == -1) {
-      perror ("ioctl: BLKZEROOUT");
-      exit (EXIT_FAILURE);
-    }
-    return true;
+  r = ioctl (fd, BLKZEROOUT, &range);
+  if (r == -1) {
+    perror ("ioctl: BLKZEROOUT");
+    exit (EXIT_FAILURE);
+  }
+  return true;
 #endif
+    return false;
+}
+
+static bool
+file_synch_zero (struct rw *rw, uint64_t offset, uint64_t count, bool allocate)
+{
+  struct rw_file *rwf = (struct rw_file *)rw;
+
+  if (!rwf->is_block) {
+    if (allocate) {
+        return file_zero_range (rwf->fd, offset, count);
+    } else {
+        return file_punch_hole (rwf->fd, offset, count);
+    }
+  }
+  else if (IS_ALIGNED (offset | count, rwf->sector_size)) {
+    /* Always allocate, discard and gurantee zeroing. */
+    return file_zeroout (rwf->fd, offset, count);
   }
 
   return false;
@@ -304,25 +318,11 @@ file_asynch_write (struct rw *rw,
   }
 }
 
-static bool
-file_asynch_trim (struct rw *rw, struct command *command,
-                  nbd_completion_callback cb)
-{
-  if (!file_synch_trim (rw, command->offset, command->slice.len))
-    return false;
-  errno = 0;
-  if (cb.callback (cb.user_data, &errno) == -1) {
-    perror (rw->name);
-    exit (EXIT_FAILURE);
-  }
-  return true;
-}
-
 static bool
 file_asynch_zero (struct rw *rw, struct command *command,
-                  nbd_completion_callback cb)
+                  nbd_completion_callback cb, bool allocate)
 {
-  if (!file_synch_zero (rw, command->offset, command->slice.len))
+  if (!file_synch_zero (rw, command->offset, command->slice.len, allocate))
     return false;
   errno = 0;
   if (cb.callback (cb.user_data, &errno) == -1) {
@@ -437,11 +437,9 @@ static struct rw_ops file_ops = {
   .flush = file_flush,
   .synch_read = file_synch_read,
   .synch_write = file_synch_write,
-  .synch_trim = file_synch_trim,
   .synch_zero = file_synch_zero,
   .asynch_read = file_asynch_read,
   .asynch_write = file_asynch_write,
-  .asynch_trim = file_asynch_trim,
   .asynch_zero = file_asynch_zero,
   .in_flight = file_in_flight,
   .get_polling_fd = get_polling_fd_not_supported,
diff --git a/copy/multi-thread-copying.c b/copy/multi-thread-copying.c
index 4f57054..c36a165 100644
--- a/copy/multi-thread-copying.c
+++ b/copy/multi-thread-copying.c
@@ -160,8 +160,8 @@ worker_thread (void *indexp)
       size_t len;
 
       if (exts.ptr[i].zero) {
-        /* The source is zero so we can proceed directly to
-         * skipping, trimming or writing zeroes at the destination.
+        /* The source is zero so we can proceed directly to skipping,
+         * fast zeroing, or writing zeroes at the destination.
          */
         command = calloc (1, sizeof *command);
         if (command == NULL) {
@@ -487,11 +487,12 @@ finished_read (void *vp, int *error)
  * --destination-is-zero:
  *                 do nothing
  *
- * --allocated:    we must write zeroes either using an efficient
+ * --allocated:    write zeroes allocating space using an efficient
  *                 zeroing command or writing a command of zeroes
  *
- * (neither flag)  try trimming if supported, else write zeroes
- *                 as above
+ * (neither flag)  write zeroes punching a hole using an efficient
+ *                 zeroing command or fallback to writing a command
+ *                 of zeroes.
  *
  * This takes over ownership of the command and frees it eventually.
  */
@@ -503,22 +504,13 @@ fill_dst_range_with_zeroes (struct command *command)
   if (destination_is_zero)
     goto free_and_return;
 
-  if (!allocated) {
-    /* Try trimming. */
-    if (dst->ops->asynch_trim (dst, command,
-                               (nbd_completion_callback) {
-                                 .callback = free_command,
-                                 .user_data = command,
-                               }))
-      return;
-  }
-
   /* Try efficient zeroing. */
   if (dst->ops->asynch_zero (dst, command,
                              (nbd_completion_callback) {
                                .callback = free_command,
                                .user_data = command,
-                             }))
+                             },
+                             allocated))
     return;
 
   /* Fall back to loop writing zeroes.  This is going to be slow
diff --git a/copy/nbd-ops.c b/copy/nbd-ops.c
index 0f52587..05b7b21 100644
--- a/copy/nbd-ops.c
+++ b/copy/nbd-ops.c
@@ -45,7 +45,7 @@ struct rw_nbd {
   direction d;
 
   handles handles;              /* One handle per connection. */
-  bool can_trim, can_zero;      /* Cached nbd_can_trim, nbd_can_zero. */
+  bool can_zero;                /* Cached nbd_can_zero. */
 };
 
 static void
@@ -91,7 +91,6 @@ open_one_nbd_handle (struct rw_nbd *rwn)
    * the same way.
    */
   if (rwn->handles.size == 0) {
-    rwn->can_trim = nbd_can_trim (nbd) > 0;
     rwn->can_zero = nbd_can_zero (nbd) > 0;
     rwn->rw.size = nbd_get_size (nbd);
     if (rwn->rw.size == -1) {
@@ -261,30 +260,16 @@ nbd_ops_synch_write (struct rw *rw,
 }
 
 static bool
-nbd_ops_synch_trim (struct rw *rw, uint64_t offset, uint64_t count)
-{
-  struct rw_nbd *rwn = (struct rw_nbd *) rw;
-
-  if (!rwn->can_trim)
-    return false;
-
-  if (nbd_trim (rwn->handles.ptr[0], count, offset, 0) == -1) {
-    fprintf (stderr, "%s: %s\n", rw->name, nbd_get_error ());
-    exit (EXIT_FAILURE);
-  }
-  return true;
-}
-
-static bool
-nbd_ops_synch_zero (struct rw *rw, uint64_t offset, uint64_t count)
+nbd_ops_synch_zero (struct rw *rw, uint64_t offset, uint64_t count,
+                    bool allocate)
 {
   struct rw_nbd *rwn = (struct rw_nbd *) rw;
 
   if (!rwn->can_zero)
     return false;
 
-  if (nbd_zero (rwn->handles.ptr[0],
-                count, offset, LIBNBD_CMD_FLAG_NO_HOLE) == -1) {
+  if (nbd_zero (rwn->handles.ptr[0], count, offset,
+                allocate ? LIBNBD_CMD_FLAG_NO_HOLE : 0) == -1) {
     fprintf (stderr, "%s: %s\n", rw->name, nbd_get_error ());
     exit (EXIT_FAILURE);
   }
@@ -323,29 +308,9 @@ nbd_ops_asynch_write (struct rw *rw,
   }
 }
 
-static bool
-nbd_ops_asynch_trim (struct rw *rw, struct command *command,
-                     nbd_completion_callback cb)
-{
-  struct rw_nbd *rwn = (struct rw_nbd *) rw;
-
-  if (!rwn->can_trim)
-    return false;
-
-  assert (command->slice.len <= UINT32_MAX);
-
-  if (nbd_aio_trim (rwn->handles.ptr[command->index],
-                    command->slice.len, command->offset,
-                    cb, 0) == -1) {
-    fprintf (stderr, "%s: %s\n", rw->name, nbd_get_error ());
-    exit (EXIT_FAILURE);
-  }
-  return true;
-}
-
 static bool
 nbd_ops_asynch_zero (struct rw *rw, struct command *command,
-                     nbd_completion_callback cb)
+                     nbd_completion_callback cb, bool allocate)
 {
   struct rw_nbd *rwn = (struct rw_nbd *) rw;
 
@@ -356,7 +321,7 @@ nbd_ops_asynch_zero (struct rw *rw, struct command *command,
 
   if (nbd_aio_zero (rwn->handles.ptr[command->index],
                     command->slice.len, command->offset,
-                    cb, LIBNBD_CMD_FLAG_NO_HOLE) == -1) {
+                    cb, allocate ? LIBNBD_CMD_FLAG_NO_HOLE : 0) == -1) {
     fprintf (stderr, "%s: %s\n", rw->name, nbd_get_error ());
     exit (EXIT_FAILURE);
   }
@@ -536,11 +501,9 @@ static struct rw_ops nbd_ops = {
   .flush = nbd_ops_flush,
   .synch_read = nbd_ops_synch_read,
   .synch_write = nbd_ops_synch_write,
-  .synch_trim = nbd_ops_synch_trim,
   .synch_zero = nbd_ops_synch_zero,
   .asynch_read = nbd_ops_asynch_read,
   .asynch_write = nbd_ops_asynch_write,
-  .asynch_trim = nbd_ops_asynch_trim,
   .asynch_zero = nbd_ops_asynch_zero,
   .in_flight = nbd_ops_in_flight,
   .get_polling_fd = nbd_ops_get_polling_fd,
diff --git a/copy/nbdcopy.h b/copy/nbdcopy.h
index 2db0383..4496722 100644
--- a/copy/nbdcopy.h
+++ b/copy/nbdcopy.h
@@ -79,11 +79,11 @@ struct slice {
 
 /* Commands for asynchronous operations in flight.
  *
- * We don't store the command type (read/write/trim/etc) because it is
+ * We don't store the command type (read/write/zero/etc) because it is
  * implicit in the function being called and because commands
- * naturally change from read -> write/trim/etc as they progress.
+ * naturally change from read -> write/zero/etc as they progress.
  *
- * slice.buffer may be NULL for commands (like trim) that have no
+ * slice.buffer may be NULL for commands (like zero) that have no
  * associated data.
  *
  * A separate set of commands, slices and buffers is maintained per
@@ -151,11 +151,9 @@ struct rw_ops {
   void (*synch_write) (struct rw *rw,
                        const void *data, size_t len, uint64_t offset);
 
-  /* Synchronously trim.  If not possible, returns false. */
-  bool (*synch_trim) (struct rw *rw, uint64_t offset, uint64_t count);
-
   /* Synchronously zero.  If not possible, returns false. */
-  bool (*synch_zero) (struct rw *rw, uint64_t offset, uint64_t count);
+  bool (*synch_zero) (struct rw *rw, uint64_t offset, uint64_t count,
+                      bool allocate);
 
   /* Asynchronous I/O operations.  These start the operation and call
    * 'cb' on completion.
@@ -173,17 +171,11 @@ struct rw_ops {
                         struct command *command,
                         nbd_completion_callback cb);
 
-  /* Asynchronously trim.  command->slice.buffer is not used.  If not
-   * possible, returns false.
-   */
-  bool (*asynch_trim) (struct rw *rw, struct command *command,
-                       nbd_completion_callback cb);
-
   /* Asynchronously zero.  command->slice.buffer is not used.  If not possible,
    * returns false.
    */
   bool (*asynch_zero) (struct rw *rw, struct command *command,
-                       nbd_completion_callback cb);
+                       nbd_completion_callback cb, bool allocate);
 
   /* Number of asynchronous commands in flight for a particular thread. */
   unsigned (*in_flight) (struct rw *rw, uintptr_t index);
diff --git a/copy/null-ops.c b/copy/null-ops.c
index b75ca1f..a38666d 100644
--- a/copy/null-ops.c
+++ b/copy/null-ops.c
@@ -26,8 +26,8 @@
 #include "nbdcopy.h"
 
 /* This sinks writes and aborts on any read-like operations.  It
- * should be faster than using /dev/null because it "supports" trim
- * and fast zeroing.
+ * should be faster than using /dev/null because it "supports" fast
+ * zeroing.
  */
 
 static struct rw_ops null_ops;
@@ -99,13 +99,7 @@ null_synch_write (struct rw *rw,
 }
 
 static bool
-null_synch_trim (struct rw *rw, uint64_t offset, uint64_t count)
-{
-  return true;
-}
-
-static bool
-null_synch_zero (struct rw *rw, uint64_t offset, uint64_t count)
+null_synch_zero (struct rw *rw, uint64_t offset, uint64_t count, bool allocate)
 {
   return true;
 }
@@ -130,21 +124,9 @@ null_asynch_write (struct rw *rw,
   }
 }
 
-static bool
-null_asynch_trim (struct rw *rw, struct command *command,
-                  nbd_completion_callback cb)
-{
-  errno = 0;
-  if (cb.callback (cb.user_data, &errno) == -1) {
-    perror (rw->name);
-    exit (EXIT_FAILURE);
-  }
-  return true;
-}
-
 static bool
 null_asynch_zero (struct rw *rw, struct command *command,
-                  nbd_completion_callback cb)
+                  nbd_completion_callback cb, bool allocate)
 {
   errno = 0;
   if (cb.callback (cb.user_data, &errno) == -1) {
@@ -178,11 +160,9 @@ static struct rw_ops null_ops = {
   .flush = null_flush,
   .synch_read = null_synch_read,
   .synch_write = null_synch_write,
-  .synch_trim = null_synch_trim,
   .synch_zero = null_synch_zero,
   .asynch_read = null_asynch_read,
   .asynch_write = null_asynch_write,
-  .asynch_trim = null_asynch_trim,
   .asynch_zero = null_asynch_zero,
   .in_flight = null_in_flight,
   .get_polling_fd = get_polling_fd_not_supported,
diff --git a/copy/pipe-ops.c b/copy/pipe-ops.c
index 08f7211..f9b8599 100644
--- a/copy/pipe-ops.c
+++ b/copy/pipe-ops.c
@@ -125,7 +125,7 @@ pipe_synch_write (struct rw *rw,
 }
 
 static bool
-pipe_synch_trim_zero (struct rw *rw, uint64_t offset, uint64_t count)
+pipe_synch_zero (struct rw *rw, uint64_t offset, uint64_t count, bool allocate)
 {
   return false; /* not supported by pipes */
 }
@@ -147,8 +147,8 @@ pipe_asynch_write (struct rw *rw,
 }
 
 static bool
-pipe_asynch_trim_zero (struct rw *rw, struct command *command,
-                       nbd_completion_callback cb)
+pipe_asynch_zero (struct rw *rw, struct command *command,
+                       nbd_completion_callback cb, bool allocate)
 {
   return false; /* not supported by pipes */
 }
@@ -173,8 +173,7 @@ static struct rw_ops pipe_ops = {
 
   .synch_read = pipe_synch_read,
   .synch_write = pipe_synch_write,
-  .synch_trim = pipe_synch_trim_zero,
-  .synch_zero = pipe_synch_trim_zero,
+  .synch_zero = pipe_synch_zero,
 
   /* Asynch pipe read/write operations are not defined.  These should
    * never be called because pipes/streams/sockets force synchronous
@@ -185,8 +184,7 @@ static struct rw_ops pipe_ops = {
   .asynch_read = pipe_asynch_read,
   .asynch_write = pipe_asynch_write,
 
-  .asynch_trim = pipe_asynch_trim_zero,
-  .asynch_zero = pipe_asynch_trim_zero,
+  .asynch_zero = pipe_asynch_zero,
   .in_flight = pipe_in_flight,
 
   .get_polling_fd = get_polling_fd_not_supported,
diff --git a/copy/synch-copying.c b/copy/synch-copying.c
index 985f005..17bda16 100644
--- a/copy/synch-copying.c
+++ b/copy/synch-copying.c
@@ -69,10 +69,10 @@ synch_copying (void)
         assert (exts.ptr[i].length <= count);
 
         if (exts.ptr[i].zero) {
-          if (!dst->ops->synch_trim (dst, offset, exts.ptr[i].length) &&
-              !dst->ops->synch_zero (dst, offset, exts.ptr[i].length)) {
-            /* If neither trimming nor efficient zeroing are possible,
-             * write zeroes the hard way.
+          if (!dst->ops->synch_zero (dst, offset, exts.ptr[i].length, false) &&
+              !dst->ops->synch_zero (dst, offset, exts.ptr[i].length, true)) {
+            /* If efficient zeroing (punching a hole or allocating
+             * space) are possible, write zeroes the hard way.
              */
             memset (buf, 0, exts.ptr[i].length);
             dst->ops->synch_write (dst, buf, exts.ptr[i].length, offset);
-- 
2.26.2


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