On 14/02/2023 06:12, Paul Eggert wrote:
On 2023-02-13 09:16, George Valkov wrote:
1. We apply my original patch to disable sparse-copy on macOS. Otherwise since
fclonefileat is not used whenever we overwrite a file, corruption will still
occur.
I'm not entirely sold on this patch, because I still don't understand
what's happening. The original bug report in
<https://github.com/openwrt/openwrt/pull/11960> basically says only "it
doesn't work", and I'd like to know more.
Part of the reason I'm hesitating is that there are multiple ways of
interpreting what SEEK_HOLE and SEEK_DATA mean, and perhaps it's just
that Apple has come up with yet another way to interpret it, which we
need to know about.
I agree it would be good to know more.
However given it works on HFS but fails on APFS suggests a file system specific
issue,
rather than an API mismatch issue (over what we're already catering for on
macOS).
I suspect it's a similar issue to the one that openzfs had:
https://github.com/openzfs/zfs/issues/11900
Given how closed / uncommunicative Apple are in general
and specifically for this already reported bug,
I'm inclined to disable SEEK_DATA on __APPLE__ for the upcoming release.
Also we've mitigated the impact somewhat by enabling fclonefileat() in more
cases.
Another reason to hesitate is that GNU coreutils is not the only core
program that uses SEEK_HOLE and SEEK_DATA. If this really is a generic
Apple problem we need to know which Apple releases have it, so that we
can program around it at the Gnulib level instead of mucking about with
each individual program.
Yes that would be best if possible.
I've attached the 3 latest patches I'm considering in this area.
I presume you're OK with your amended fclonefileat() improvement one?
thanks,
Pádraig
From 0a590853b159d4eda90b1d3a83fc1859356727ac Mon Sep 17 00:00:00 2001
From: Paul Eggert <egg...@cs.ucla.edu>
Date: Fri, 10 Feb 2023 13:34:54 -0800
Subject: [PATCH 1/3] copy: improve use of fclonefileat
* src/copy.c (copy_reg): Use CLONE_ACL if available and working.
If the only problem with fclonefileat is that it would create the
file with the wrong timestamp, or with too few permissions,
do that but fix the timestamp and permissions afterwards,
rather than falling back on a traditional copy.
(CLONE_ACL) [HAVE_FCLONEFILEAT && !USE_XATTR]: Default to 0.
---
NEWS | 3 +++
src/copy.c | 53 +++++++++++++++++++++++++++++++++++++++++++++--------
2 files changed, 48 insertions(+), 8 deletions(-)
diff --git a/NEWS b/NEWS
index 3d0ede150..cbc614c2e 100644
--- a/NEWS
+++ b/NEWS
@@ -124,6 +124,9 @@ GNU coreutils NEWS -*- outline -*-
and possibly employing copy offloading or reflinking,
for the non sparse portion of such sparse files.
+ On macOS, cp creates a copy-on-write clone in more cases.
+ Previously cp would only do this when preserving mode and timestamps.
+
date --debug now diagnoses if multiple --date or --set options are
specified, as only the last specified is significant in that case.
diff --git a/src/copy.c b/src/copy.c
index dfbb557de..8370f55bd 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -1146,6 +1146,7 @@ copy_reg (char const *src_name, char const *dst_name,
union scan_inference scan_inference;
bool return_val = true;
bool data_copy_required = x->data_copy_required;
+ bool mode_already_preserved = false;
bool preserve_xattr = USE_XATTR & x->preserve_xattr;
source_desc = open (src_name,
@@ -1244,17 +1245,48 @@ copy_reg (char const *src_name, char const *dst_name,
if (*new_dst)
{
#if HAVE_FCLONEFILEAT && !USE_XATTR
-/* CLONE_NOOWNERCOPY only available on macos >= 10.13. */
+# ifndef CLONE_ACL
+# define CLONE_ACL 0 /* Added in macOS 12.6. */
+# endif
# ifndef CLONE_NOOWNERCOPY
-# define CLONE_NOOWNERCOPY 0
+# define CLONE_NOOWNERCOPY 0 /* Added in macOS 10.13. */
# endif
- int fc_flags = x->preserve_ownership ? 0 : CLONE_NOOWNERCOPY;
+ mode_t cloned_mode_bits = S_ISVTX | S_IRWXUGO;
+ mode_t cloned_mode = src_mode & cloned_mode_bits;
+ int fc_flags = ((x->preserve_mode ? CLONE_ACL : 0)
+ | (x->preserve_ownership ? 0 : CLONE_NOOWNERCOPY));
if (data_copy_required && x->reflink_mode
- && x->preserve_mode && x->preserve_timestamps
+ && (x->preserve_mode || ! (cloned_mode & ~dst_mode))
&& (x->preserve_ownership || CLONE_NOOWNERCOPY))
{
- if (fclonefileat (source_desc, dst_dirfd, dst_relname, fc_flags) == 0)
- goto close_src_desc;
+ int s = fclonefileat (source_desc, dst_dirfd, dst_relname, fc_flags);
+ if (s != 0 && (fc_flags & CLONE_ACL) != 0 && errno == EINVAL)
+ {
+ fc_flags &= ~CLONE_ACL;
+ s = fclonefileat (source_desc, dst_dirfd, dst_relname, fc_flags);
+ }
+ if (s == 0)
+ {
+ if (!x->preserve_timestamps)
+ {
+ struct timespec timespec[2];
+ timespec[0].tv_nsec = timespec[1].tv_nsec = UTIME_NOW;
+ if (utimensat (dst_dirfd, dst_relname, timespec,
+ AT_SYMLINK_NOFOLLOW)
+ != 0)
+ {
+ error (0, errno, _("updating times for %s"),
+ quoteaf (dst_name));
+ return_val = false;
+ }
+ }
+ mode_already_preserved = (fc_flags & CLONE_ACL) != 0
+ && cloned_mode == src_mode;
+ dest_desc = -1;
+ omitted_permissions = dst_mode & ~cloned_mode;
+ extra_permissions = dst_mode & ~ cached_umask ();
+ goto set_dest_mode;
+ }
else if (! handle_clone_fail (dst_dirfd, dst_relname, src_name,
dst_name,
-1, false /* We didn't create dst */,
@@ -1485,9 +1517,14 @@ copy_reg (char const *src_name, char const *dst_name,
set_author (dst_name, dest_desc, src_sb);
+#if HAVE_FCLONEFILEAT && !USE_XATTR
+set_dest_mode:
+#endif
if (x->preserve_mode || x->move_mode)
{
- if (copy_acl (src_name, source_desc, dst_name, dest_desc, src_mode) != 0
+ if (!mode_already_preserved
+ && (copy_acl (src_name, source_desc, dst_name, dest_desc, src_mode)
+ != 0)
&& x->require_preserve)
return_val = false;
}
@@ -1517,7 +1554,7 @@ copy_reg (char const *src_name, char const *dst_name,
}
close_src_and_dst_desc:
- if (close (dest_desc) < 0)
+ if (0 <= dest_desc && close (dest_desc) < 0)
{
error (0, errno, _("failed to close %s"), quoteaf (dst_name));
return_val = false;
--
2.26.2
From 0ae2f8fe514d10d12e8f640c53abeb1143e1d0c1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <p...@draigbrady.com>
Date: Mon, 13 Feb 2023 19:02:12 +0000
Subject: [PATCH 2/3] tests: allow copying through dangling symlinks on macOS
* tests/cp/thru-dangling.sh: fclonefileat() can copy
through dangling symlinks robustly, so don't enforce
a failure in this case on macOS.
---
tests/cp/thru-dangling.sh | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/tests/cp/thru-dangling.sh b/tests/cp/thru-dangling.sh
index 1cbb12d11..60d417886 100755
--- a/tests/cp/thru-dangling.sh
+++ b/tests/cp/thru-dangling.sh
@@ -26,13 +26,19 @@ echo "cp: not writing through dangling symlink 'dangle'" \
> exp-err || framework_failure_
-# Starting with 6.9.90, this usage fails, by default:
-for opt in '' '-f'; do
- returns_ 1 cp $opt f dangle > err 2>&1 || fail=1
- compare exp-err err || fail=1
- test -f no-such && fail=1
-done
-
+# macOS may use fclonefileat() which does copy through the dangling symlink
+case $host_triplet in
+ *darwin*) skip_dangling=1 ;;
+esac
+
+if test -z "$skip_dangling"; then
+ # Starting with 6.9.90, this usage fails, by default:
+ for opt in '' '-f'; do
+ returns_ 1 cp $opt f dangle > err 2>&1 || fail=1
+ compare exp-err err || fail=1
+ test -f no-such && fail=1
+ done
+fi
# But you can set POSIXLY_CORRECT to get the historical behavior.
env POSIXLY_CORRECT=1 cp f dangle > out 2>&1 || fail=1
--
2.26.2
From a3826d76199071c287086107abeba12f9335fda0 Mon Sep 17 00:00:00 2001
From: George Valkov <gval...@gmail.com>
Date: Mon, 13 Feb 2023 18:50:49 +0000
Subject: [PATCH 3/3] copy: disable sparse copy on macOS
Due to a bug in macOS, sparse copies are corrupted on virtual disks
formatted with APFS. HFS is not affected. Affected are coreutils
install, and gcp when compiled with SEEK_HOLE, as well as macOS Finder.
While reading the entire file returns valid data, scanning for
allocated segments may return holes where valid data is present.
In this case a sparse copy does not contain these segments and returns
zeroes instead. Once the virtual disk is dismounted and then
mounted again, a sparse copy produces correct results.
This breaks OpenWRT build on macOS. Details:
https://github.com/openwrt/openwrt/pull/11960
https://github.com/openwrt/openwrt/pull/11960#issuecomment-1423185579
* NEWS: Mention the bug fix.
* src/copy.c (infer_scantype): Avoid SEEK_DATA on __APPLE__.
* tests/seek-data-capable: Likewise.
Addresses https://bugs.gnu.org/61386
---
NEWS | 4 ++++
src/copy.c | 3 ++-
tests/seek-data-capable | 3 +++
3 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/NEWS b/NEWS
index cbc614c2e..8f50c0b8e 100644
--- a/NEWS
+++ b/NEWS
@@ -29,6 +29,10 @@ GNU coreutils NEWS -*- outline -*-
falling back from copy_file_range to a better supported standard copy.
[issue introduced in coreutils-9.0]
+ cp, mv, and install now avoid the use of lseek+SEEK_HOLE on macOS,
+ as data corruption was seen in some cases on APFS file systems.
+ [issue introduced in coreutils-9.0]
+
'mv --backup=simple f d/' no longer mistakenly backs up d/f to f~.
[bug introduced in coreutils-9.1]
diff --git a/src/copy.c b/src/copy.c
index 8370f55bd..e98a36b13 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -1062,7 +1062,8 @@ infer_scantype (int fd, struct stat const *sb,
&& ST_NBLOCKS (*sb) < sb->st_size / ST_NBLOCKSIZE))
return PLAIN_SCANTYPE;
-#ifdef SEEK_HOLE
+#if defined SEEK_HOLE && !defined __APPLE__
+ /* Avoid on macOS (APFS) due to bug #61386. */
off_t ext_start = lseek (fd, 0, SEEK_DATA);
if (0 <= ext_start || errno == ENXIO)
{
diff --git a/tests/seek-data-capable b/tests/seek-data-capable
index cc6372214..275a46464 100644
--- a/tests/seek-data-capable
+++ b/tests/seek-data-capable
@@ -4,6 +4,9 @@ import sys, os, errno, platform
if len(sys.argv) != 2:
sys.exit(1)
+if platform.system() == "Darwin":
+ sys.exit(1) # as per bug 61386
+
if not hasattr(os, 'SEEK_DATA'):
# Not available on python 2, or on darwin python 3
# Also Darwin swaps SEEK_DATA/SEEK_HOLE definitions
--
2.26.2
