On 2026-09-02 15:00, Pádraig Brady wrote:
On 02/09/2026 20:47, Paul Eggert wrote:
Don't we need to be careful with some errors as per
https://github.com/coreutils/coreutils/commit/f6c93f334
https://bugs.gnu.org/60489 ?
Thanks for bringing that up; I'd forgotten that. Unfortunately, though, bug#60489 seems
to have given us some incorrect advice. In it, Noah Misch wrote "One could argue
that ENOSPC also warrants termination, since no fallback reduces space usage." - but
in this thread people are saying that the fallback read+write can work even when
ioctl+FICLONE fails with ENOSPC.
Also, Noah's bug#60489 comments about EIO suggest that when EIO occurs, the
entire file or maybe even the entire file system (not just this open file
descriptor) is at risk of being inconsistent, which is a kernel bug that we
can't reasonably expect cp to work around.
All that being said, it does seem prudent to go back to having cp give up on
the file entirely (not just revert to read+write) if FICLONE fails with EIO. I
did that by installing the attached.
Also this removes partially cloned files on error,
rather than just empty files. ?
Why keep partially cloned files that we just created? ioctl + FICLONE is
supposed to be atomic, and if a kernel bug makes it non-atomic, the removal is
supposed to work around the bug. Why should the workaround go to the extra work
of checking that the file is empty before unlinking it? That loses the pretense
of atomicity that the workaround should strive for. And particularly with EIO,
the file is in a possibly-inconsistent state so leaving it around could be
hazardous.
I'm a little worried about about giving worrying errors here
rather than an abstract "unsupported". Perhaps map is_CLONENOTSUP()
to "unsupported" and others to their errno?
--debug is *supposed* to worry people (:-), and I thought it better to put a
bit more info into the debugging output. But please feel free to adjust the
--debug diagnostics, though I'm not sure is_CLONENOTSUP is completely apropos
for doing so as is_CLONENOTSUP (which is not aptly named any more) is about
copy_file_range, not about ioctl with FICLONE.
From 1ee46ac2c990a6a3b10737d59d8cb358b59af5df Mon Sep 17 00:00:00 2001
From: Paul Eggert <[email protected]>
Date: Thu, 3 Sep 2026 00:52:39 -0700
Subject: [PATCH] cp: ioctl+FICLONE EIO now fails without fallback
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Problem reported by Pádraig Brady (bug#81776#16).
* src/copy.c (handle_clone_fail): Be more cautious if
ioctl+FICLONE failed with EIO; do not fall back to read+write,
even if --reflink=auto is in effect.
---
NEWS | 2 +-
src/copy.c | 20 +++++++++++++-------
2 files changed, 14 insertions(+), 8 deletions(-)
diff --git a/NEWS b/NEWS
index 0e4ad1d94..d0d765523 100644
--- a/NEWS
+++ b/NEWS
@@ -13,7 +13,7 @@ GNU coreutils NEWS -*- outline -*-
[This bug was present in "the beginning".]
'cp`, 'install', and 'mv' now fall back to a standard copy
- if a --reflink=auto clone fails for any reason, including ENOSPC.
+ if a --reflink=auto clone fails due to EDQUOT, ENOMEM, or ENOSPC.
E.g., with XFS, a clone can exhaust metadata space in an allocation
group, while a standard copy works.
[bug introduced in coreutils-9.2]
diff --git a/src/copy.c b/src/copy.c
index ce6aa8e1a..aedbd0dc3 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -673,25 +673,31 @@ handle_clone_fail (int dst_dirfd, char const *dst_relname,
char const *src_name, char const *dst_name,
bool new_dst, enum Reflink_type reflink_mode)
{
- /* Record failure for debugging, but return false only if --reflink=always.
- No errno value is serious enough to give up on read+write copying,
+ /* Record errno for debugging, but diagnose the failure and return
+ false only if either --reflink=always, or errno is so serious
+ that we should not fall back on read+write copying,
which can succeed even if cloning fails due to ENOSPC etc.
E.g., XFS has Allocation Groups where a clone may fail but a copy
- (to other groups) may succeed. */
+ (to other groups) may succeed. The only known potentially
+ serious errno value is EIO, due to Linux kernel bugs reported
+ in 2023 <https://bugs.gnu.org/60489>. */
copy_debug.reflink = errno;
-
- if (reflink_mode != REFLINK_ALWAYS)
+ if (! (reflink_mode == REFLINK_ALWAYS || errno == EIO))
return true;
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. */
+ /* Remove the destination if cloning created it but later failed,
+ either due to EIO, or to any error if --reflink=always. The need
+ for removal is a Linux kernel bug, as ioctl with FICLONE is
+ supposed to be atomic. We don't know whether currently supported
+ kernels have the problem, so play it safe. */
if (new_dst /* currently not for fclonefileat(). */
&& unlinkat (dst_dirfd, dst_relname, 0) < 0 && errno != ENOENT)
error (0, errno, _("cannot remove %s"), quoteaf (dst_name));
+
return false;
}
--
2.53.0
