Subject: Re: bug#60489: Programs should exit after EIO from FICLONE or copy_file_range()

From: Pádraig Brady <[email-masked]>
Date: Mon, 2 Jan 2023 16:22:03 +0000

On 02/01/2023 13:28, Andreas Schwab wrote:
> On Jan 02 2023, Pádraig Brady wrote:
> 
>> +          /* Note error set is consistent with copy_file_range().  */
>> +          bool no_clone_attempted = errno == ENOSYS || is_ENOTSUP (errno)
>> +                                    || errno == EINVAL || errno == EBADF
>> +                                    || errno == EXDEV || errno == ETXTBSY;
> 
> Should this be refactored to avoid duplication?

Yes good call.
We should also refactor the handling of clone failure
to also apply to the usage of fclonefileat() on macOS.

Updated patch attached.

cheers,
Pádraig
From d13bde46a48036f17c3fb651977d75b8d3eb7eda Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email-masked]>
Date: Mon, 2 Jan 2023 13:07:41 +0000
Subject: [PATCH] copy: immediately fail with transient errors from FICLONE

* src/copy.c (handle_clone_fail): A new function refactored
from copy_reg() to handle failures from FICLONE or fclonefileat().
(copy_reg): Fail with all errors from FICLONE,
unless they're from the set indicating the file system
or file do not support the clone operation.
Also fail with errors from fclonefileat() if they're
from the set indicating a transient failure for the file.
(sparse_copy): Call the refactored is_CLONENOTSUP()
which is now also used by the new handle_clone_fail() function.
* NEWS: Mention the bug fix.  Also mention explicitly
the older --reflink=auto default change to aid searching.
* cfg.mk: Adjust old_NEWS_hash with `make update-NEWS-hash`.
Fixes https://bugs.gnu.org/60489
---
 NEWS       |  8 +++++
 cfg.mk     |  2 +-
 src/copy.c | 94 ++++++++++++++++++++++++++++++++++++++++--------------
 3 files changed, 79 insertions(+), 25 deletions(-)

diff --git a/NEWS b/NEWS
index f43182757..0fbfe7b09 100644
--- a/NEWS
+++ b/NEWS
@@ -13,6 +13,13 @@ GNU coreutils NEWS                                    -*- outline -*-
   'cp -rx / /mnt' no longer complains "cannot create directory /mnt/".
   [bug introduced in coreutils-9.1]
 
+  cp, mv, and install, now immediately acknowledge transient errors
+  when creating copy-on-write or cloned reflink files, on supporting
+  file systems like XFS, BTRFS, APFS, etc.
+  Previously they would have tried again with other copy methods
+  which may have resulted in data corruption.
+  [bug introduced in coreutils-7.5 and enabled by default in coreutils-9.0]
+
   'mv --backup=simple f d/' no longer mistakenly backs up d/f to f~.
   [bug introduced in coreutils-9.1]
 
@@ -283,6 +290,7 @@ GNU coreutils NEWS                                    -*- outline -*-
 ** Changes in behavior
 
   cp and install now default to copy-on-write (COW) if available.
+  I.e., cp now uses --reflink=auto mode by default.
 
   cp, install and mv now use the copy_file_range syscall if available.
   Also, they use lseek+SEEK_HOLE rather than ioctl+FS_IOC_FIEMAP on sparse
diff --git a/cfg.mk b/cfg.mk
index 179a51d1c..c9b2d4326 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -49,7 +49,7 @@ export VERBOSE = yes
 # 4914152 9e
 export XZ_OPT = -8e
 
-old_NEWS_hash = f365e0cf2c7890c617df6abe70615fce
+old_NEWS_hash = 49269d8d99f4888a4e955d28cda50091
 
 # Add an exemption for sc_makefile_at_at_check.
 _makefile_at_at_check_exceptions = ' && !/^cu_install_prog/ && !/dynamic-dep/'
diff --git a/src/copy.c b/src/copy.c
index 310c8bf14..1058b08ec 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -224,6 +224,18 @@ create_hole (int fd, char const *name, bool punch_holes, off_t size)
 }

+/* Whether the errno from FICLONE, or copy_file_range
+   indicates operation is not supported for this file or file system.  */
+
+static bool
+is_CLONENOTSUP (int err)
+{
+  return err == ENOSYS || is_ENOTSUP (err)
+         || err == EINVAL || err == EBADF
+         || err == EXDEV || err == ETXTBSY;
+}
+
+
 /* Copy the regular file open on SRC_FD/SRC_NAME to DST_FD/DST_NAME,
    honoring the MAKE_HOLES setting and using the BUF_SIZE-byte buffer
    *ABUF for temporary storage, allocating it lazily if *ABUF is null.
@@ -267,9 +279,7 @@ sparse_copy (int src_fd, int dest_fd, char **abuf, size_t buf_size,
           }
         if (n_copied < 0)
           {
-            if (errno == ENOSYS || is_ENOTSUP (errno)
-                || errno == EINVAL || errno == EBADF
-                || errno == EXDEV || errno == ETXTBSY)
+            if (is_CLONENOTSUP (errno))
               break;
 
             /* copy_file_range might not be enabled in seccomp filters,
@@ -1064,6 +1074,42 @@ infer_scantype (int fd, struct stat const *sb,
 }

+/* Handle failure from FICLONE or fclonefileat.
+   Return FALSE if it's a terminal failure for this file.  */
+
+static bool
+handle_clone_fail (int dst_dirfd, char const* dst_relname,
+                   char const* src_name, char const* dst_name,
+                   int dest_desc, bool new_dst, enum Reflink_type reflink_mode)
+{
+  /* If the clone operation is creating the destination,
+     then don't try and cater for all non transient file system errors,
+     and instead only cater for specific transient errors.  */
+  bool transient_failure;
+  if (dest_desc < 0) /* currently for fclonefileat().  */
+    transient_failure = errno == EIO || errno == ENOMEM || errno == ENOSPC;
+  else /* currently for FICLONE.  */
+    transient_failure = ! is_CLONENOTSUP (errno);
+
+  if (reflink_mode == REFLINK_ALWAYS || transient_failure)
+    error (0, errno, _("failed to clone %s from %s"),
+           quoteaf_n (0, dst_name), quoteaf_n (1, src_name));
+
+  /* Remove the destination if cp --reflink=always created it
+     but cloned no data.  */
+  if (new_dst /* currently not for fclonefileat().  */
+      && reflink_mode == REFLINK_ALWAYS
+      && ((! transient_failure) || lseek (dest_desc, 0, SEEK_END) == 0)
+      && unlinkat (dst_dirfd, dst_relname, 0) != 0 && errno != ENOENT)
+    error (0, errno, _("cannot remove %s"), quoteaf (dst_name));
+
+  if (reflink_mode == REFLINK_ALWAYS || transient_failure)
+    return false;
+
+  return true;
+}
+
+
 /* Copy a regular file from SRC_NAME to DST_NAME aka DST_DIRFD+DST_RELNAME.
    If the source file contains holes, copies holes and blocks of zeros
    in the source file as holes in the destination file.
@@ -1198,13 +1244,22 @@ copy_reg (char const *src_name, char const *dst_name,
 # ifndef CLONE_NOOWNERCOPY
 #  define CLONE_NOOWNERCOPY 0
 # endif
-      int clone_flags = x->preserve_ownership ? 0 : CLONE_NOOWNERCOPY;
+      int fc_flags = x->preserve_ownership ? 0 : CLONE_NOOWNERCOPY;
       if (data_copy_required && x->reflink_mode
           && x->preserve_mode && x->preserve_timestamps
-          && (x->preserve_ownership || CLONE_NOOWNERCOPY)
-          && (fclonefileat (source_desc, dst_dirfd, dst_relname, clone_flags)
-              == 0))
-        goto close_src_desc;
+          && (x->preserve_ownership || CLONE_NOOWNERCOPY))
+        {
+          if (fclonefileat (source_desc, dst_dirfd, dst_relname, fc_flags) == 0)
+            goto close_src_desc;
+          else if (! handle_clone_fail (dst_dirfd, dst_relname, src_name,
+                                        dst_name,
+                                        -1, false /* We didn't create dst  */,
+                                        x->reflink_mode))
+            {
+              return_val = false;
+              goto close_src_desc;
+            }
+        }
 #endif
 
       /* To allow copying xattrs on read-only files, create with u+w.
@@ -1275,23 +1330,14 @@ copy_reg (char const *src_name, char const *dst_name,
     {
       if (clone_file (dest_desc, source_desc) == 0)
         data_copy_required = false;
-      else if (x->reflink_mode == REFLINK_ALWAYS)
+      else
         {
-          error (0, errno, _("failed to clone %s from %s"),
-                 quoteaf_n (0, dst_name), quoteaf_n (1, src_name));
-
-          /* Remove the destination if cp --reflink=always created it
-             but cloned no data.  If clone_file failed with
-             EOPNOTSUPP, EXDEV or EINVAL no data were copied so do not
-             go to the expense of lseeking.  */
-          if (*new_dst
-              && (is_ENOTSUP (errno) || errno == EXDEV || errno == EINVAL
-                  || lseek (dest_desc, 0, SEEK_END) == 0)
-              && unlinkat (dst_dirfd, dst_relname, 0) != 0 && errno != ENOENT)
-            error (0, errno, _("cannot remove %s"), quoteaf (dst_name));
-
-          return_val = false;
-          goto close_src_and_dst_desc;
+          if (! handle_clone_fail (dst_dirfd, dst_relname, src_name, dst_name,
+                                   dest_desc, *new_dst, x->reflink_mode))
+           {
+             return_val = false;
+             goto close_src_and_dst_desc;
+           }
         }
     }
 
-- 
2.26.2