File cloning improvements by tuffnatty · Pull Request #5168 · MidnightCommander/mc

tuffnatty · 2026-09-27T21:39:55Z

Proposed changes

Add support for copy_file_range(2) on Linux 5.19+. It fixes file cloning between datasets on ZFS on Linux.
There are cases where cloning the entire file in a single syscall takes considerable time, for example, the FICLONERANGE ioctl does not fail with NFS (before version 4.2 which has introduced server copy offload) but visually freezes until the whole file is copied (probably related: Wrong information on copy/move window on zfs with deduplication on #4606). Refactor range cloning path to work in chunks (of size increasing until it hurts progress reporting) and report progress.
Enabled file cloning for temporary files created during VFS copy

Checklist

I have referenced the issue(s) resolved by this PR (if any)
I have signed-off my contribution with git commit --amend -s
Lint and unit tests pass locally with my changes (make indent && make check)
I have added tests that prove my fix is effective or that my feature works
I have added the necessary documentation (if appropriate)

ossilator

too bad the second commit seems necessary. ah, well.

the last one has a rather sub-optimal commit message.

tuffnatty · 2026-09-28T11:57:19Z

@ossilator I've merged your suggestions in and tried to improve the third commit message.

tuffnatty · 2026-09-28T12:50:43Z

@ossilator Unfortunately clang-format on CI is against indenting the preprocessor directives.

zyv · 2026-09-28T13:27:00Z

@ossilator Unfortunately clang-format on CI is against indenting the preprocessor directives.

As I was configuring clang-format, I wanted to enable auto-indentation of preprocessor directives (IndentPPDirectives: AfterHash or BeforeHash - my preference), but Andrew didn't want it because it causes indentation for constructs like this when the highly nested directives are mixed with the code:

163 #        ifdef NCURSES_VERSION_MINOR
164     printf (".%d", NCURSES_VERSION_MINOR);
165 #        endif

I'd rather fix those places in the code, but at that point it was more important for me to introduce clang-format at all, rather than endlessly argue about style details... I don't know if it can/should be revisited now.

tuffnatty · 2026-09-28T13:36:50Z

I personally find that the benefits of unified formatting don't justify the harm that forced reformatting of existing code does to software archaeology.

I have tried to use blank lines again, but more sparingly, which could improve the readability of the fragment.

zyv · 2026-09-28T13:48:48Z

I personally find that the benefits of unified formatting don't justify the harm that forced reformatting of existing code does to software archaeology.

You're entitled to have your own opinion on the subject, and we'll have to disagree here.

zyv

I've just run another experiment, as in, what would it take to review something like that halfway properly according to my standards with Opus 5.5; see the findings below. In addition to tracing code paths, I had it build and run-test the PR on Linux.

It's quite difficult to explain everything using suggestions, but just patching without context is also hard on the submitter. Also, I do not think that posting the extended Opus blurbs unedited would be of much help, and there is no way I can edit it in a reasonable shape.

Thus, my idea is as follows - I've added suggestions with explanations to illustrate what I perceive as valid the problems, and added a separate commit that implements all of the suggestions.

Two pre-existing issues are fixed in separate commits, and unit tests are added showing the failures in the submitted code and providing basic coverage for the new loop.

Unfortunately, even with Claude's help, it took almost 4 hours. I will still try to use the cloud credits my friend @etnw kindly agreed to contribute towards the project (many thanks!), but my capacity remains limited due to exams; for PRs of this complexity, expect inordinate delays, as before :(

Now, I think I'd turn the tables on you @tuffnatty - please re-check everything and tell me what do you think about TODOs.

Hope that helps nevertheless...

ID	Finding	Status
F1	[ Append ] with file cloning on (default) overwrites the local destination from offset 0, see unit test	Fixed
F2	[ Append ] to `ftp://` / `sh://` with cloning on replaces the remote file	Fixed
F3	Cloning into USETMP temp files passes a pointer's low 32 bits as an fd	Fixed
F4	`vfs_clone_file()` returns -1 after a successful FICLONERANGE (`SSIZE_MAX` truncated to `int`)	Fixed, TODO
F5	Preallocation now runs before the clone attempt	TODO
F6	`#define _FILE_OFFSET_BITS 64` is a no-op; 32 bit `off_t` builds pass `off_t ` to an `__off64_t ` prototype	Fixed
F7	Return -1 with stale `errno`	Fixed
F8	Chunk size only grows: calls take up to about 2 s and never shrink when throughput drops	I'd leave it
F9	On Linux, "Use COW file cloning" now also means an in-kernel byte copy on non-reflink filesystems	TODO
F10	`while` loop used as an `if`; non-`const` path parameters	Fixed
F11	No tests for the new copy loop; CI stays green with F1/F2	Fixed
F12	Preallocation corrupts the destination in Append and Reget modes	Fixed
F13	Shell VFS built-in `send` and `append` fallback scripts are swapped	Fix in #5155

zyv · 2026-09-29T09:19:04Z

+        off_t src_offset = ctx->do_reget;
+        off_t dst_offset = ctx->do_reget;


Already added to my corrections batch, see unit test.

According to my understanding, [ Append ] with file cloning enabled now overwrites the destination from offset 0:

For [ Append ], ctx->do_reget is 0

FICLONERANGE and copy_file_range() use the explicit offsets and ignore the fd position, so mc_lseek (dest_desc, 0, SEEK_END) above does not help

dst_stat is taken after that seek, so its size is the append position (and equals do_reget for [ Reget ])

Suggested change

off_t src_offset = ctx->do_reget;

off_t dst_offset = ctx->do_reget;

off_t src_offset = ctx->do_reget;

off_t dst_offset = appending ? dst_stat.st_size : 0;

zyv · 2026-09-29T09:33:42Z

    else
        open_flags |= O_CREAT | O_TRUNC;


Already added to my corrections batch.

[ Append ] to ftp:// or sh:// now replaces the remote file; minimal fix below to illustrate the problem, a cleaner variant decides try_cloning once before open_flags and reuses it later:

Suggested change

else

open_flags |= O_CREAT | O_TRUNC;

else

open_flags |= O_CREAT | O_TRUNC;

#ifdef HAVE_FILE_CLONING_BY_RANGE

// Only local files can be cloned. Other VFSes (ftpfs, shell) append only with O_APPEND

if (dst_exists && ctx->do_append && !vfs_cloning_supported (src_vpath, dst_vpath))

open_flags |= O_APPEND;

#endif

zyv · 2026-09-29T10:06:52Z

+gboolean
+vfs_cloning_supported (vfs_path_t *src_vpath, vfs_path_t *dst_vpath)
+{
+    if (!vfs_file_is_local (src_vpath))
+        return FALSE;
+    if (vfs_file_is_local (dst_vpath))
+        return TRUE;
+    if ((vfs_file_class_flags (dst_vpath) & VFSF_USETMP) != 0)
+        return TRUE;
+    return FALSE;
+}


Already added to my corrections batch, see unit test.

Only localfs stores an int * as the handle's fsinfo, for ftpfs the write handle is usually the data socket. Suggest limiting cloning to local files, i.e. dropping the USETMP part.

Suggested change

gboolean

vfs_cloning_supported (vfs_path_t *src_vpath, vfs_path_t *dst_vpath)

{

if (!vfs_file_is_local (src_vpath))

return FALSE;

if (vfs_file_is_local (dst_vpath))

return TRUE;

if ((vfs_file_class_flags (dst_vpath) & VFSF_USETMP) != 0)

return TRUE;

return FALSE;

}

gboolean

vfs_cloning_supported (const vfs_path_t *src_vpath, const vfs_path_t *dst_vpath)

{

return vfs_file_is_local (src_vpath) && vfs_file_is_local (dst_vpath);

}

zyv · 2026-09-29T10:11:30Z

 #if defined(FICLONERANGE)
    {
-        struct file_clone_range fcr = {
-            .src_fd = *(int *) src_fd,
-            .src_offset = in_offset,
-            .src_length = 0,
-            .dest_offset = out_offset,
-        };
-
-        return ioctl (*(int *) dest_fd, FICLONERANGE, &fcr);
+        int rc = mc_copy_file_range_ficlonerange (*(int *) src_fd, &in_offset, *(int *) dest_fd,
+                                                  &out_offset, SSIZE_MAX);
+
+#if defined(HAVE_COPY_FILE_RANGE)
+        if (rc != -1)
+#endif
+            return rc;
+        /* Proceed with copy_file_range() */
    }
-#elif defined(COPY_FILE_RANGE_CLONE)
+#endif


Already added to my corrections batch.

vfs_clone_file() returns -1 on LP64 after a successful FICLONERANGE as mc_copy_file_range_ficlonerange (..., SSIZE_MAX) returns SSIZE_MAX for a whole file clone and SSIZE_MAX is then stored in int rc.

This PR removes the last vfs_clone_file() caller, so instead of fixing, I would suggest deleting vfs_clone_file() and the tests.

zyv · 2026-09-29T10:18:50Z

+            break;
+        }
+
+        if (copy_method == NULL)  // cloning has failed, fallback to normal copy


TODO

According to my understanding, preallocation now runs before the clone attempt. Suggest preallocating only when falling back to read/write.

zyv · 2026-09-29T10:22:39Z

        [
            AC_MSG_RESULT(no)
        ])
+    AC_CHECK_FUNCS(copy_file_range)  # copy_file_range(2) (Linux)


Already added to my corrections batch.

glibc declares copy_file_range() with __off64_t * arguments. With a 32-bit off_t (for example i686 with --disable-largefile) passing off_t * is an incompatible pointer, which is an error by default with GCC 14. Only enable it with a 64-bit off_t (verified: with ac_cv_sizeof_off_t=4 the check is skipped, with 8 it is still found):

Suggested change

AC_CHECK_FUNCS(copy_file_range) # copy_file_range(2) (Linux)

# copy_file_range(2) (Linux): glibc declares it with off64_t *, so require a 64-bit off_t

AS_IF([test "x$ac_cv_sizeof_off_t" = x8], [AC_CHECK_FUNCS(copy_file_range)])

zyv · 2026-09-29T10:27:45Z

+#include <unistd.h>  // copy_file_range(), COPY_FILE_RANGE_CLONE
+
+#if !defined(COPY_FILE_RANGE_CLONE)
+#define COPY_FILE_RANGE_CLONE 0  // shim for Linux


FYI, it seems that with this "shim", "Use COW file cloning" also enables plain in-kernel copies on Linux.

On Linux the flags argument must be 0, so copy_file_range() is not clone only: on a non-reflink filesystem (ext4) it copies the data in the kernel.

That is a welcome speedup, but it changes what the option means; worth saying so in the commit message (hello, @ossilator!) and the option's help, and calling this a flags value rather than a "shim".

On FreeBSD builds without COPY_FILE_RANGE_CLONE this also should turn on range cloning (previously vfs_clone_file() had no return on that path at all).

Yes, and this provokes a discussion how we should call it in the interface. Kernel copy offload seems too technical, Use COW file cloning is no longer exact....

Exactly; to me, "kernel copy offloading" sounds okay - clear in terms of mechanism and effect, but I agree that it's very technical. OTOH, who is mc's audience these days? I can live with COW if everybody objects, but I wanted to have the discussion nevertheless. What's your opinion?

does this naming even reflect on anything user-visible?

does this naming even reflect on anything user-visible?

To me, "COW cloning" means "will use less space and take less time," and "kernel copy offloading" means "will take less time, but possibly not less space".

yes, but that misses the point. the comment is on a define, something no end-user cares about. i'm asking whether there are actually relevant instances (and whether they'd be affected by this internal change).

tuffnatty · 2026-09-29T11:25:58Z

@zyv the shell send/append swap commit seems unrelated and imho is resolved much better with #5155.

tuffnatty · 2026-09-29T13:11:17Z

F4 seems to be already fixed and not TODO.
I thought about removing vfs_clone_file() altogether, perhaps there's no sense in maintaining it for some internal usage where no progress indicator is needed.
I've fixed F3 to re-enable VFSF_USETMP destinations.

tuffnatty · 2026-09-29T13:30:39Z

And, sorry, I have pushed the branch with removed F13 commit.

zyv · 2026-09-29T15:07:10Z

@zyv the shell send/append swap commit seems unrelated and imho is resolved much better with #5155.

Well, it's marginally related, which is why I added it, but it is true that it has nothing to do with your changes.

It was discovered when I tasked the agent to check the append / reget code paths, and it found the descriptor issue with non-local VFS, as well as noticed that the two scripts are swapped, because it ran tests from the local build without installing it first.

I would prefer to keep the commit to fix it in master when this PR is merged until we get to #5155 if this is fine with you. If you object on principle, well, then let's drop it.

Here is the commit for you to restore:

From fe37e391302516920fa0137b1109c967704d5835 Mon Sep 17 00:00:00 2001
From: "Yury V. Zaytsev" <yury@shurup.com>
Date: Tue, 29 Sep 2026 09:01:14 +0000
Subject: [PATCH] shell: swap built-in default send and append scripts

The built-in fallbacks used when the helper scripts are not installed
were swapped: the default 'send' script appended to the target and the
default 'append' script truncated it, the opposite of the send and
append helpers.

Assisted-By: Claude Opus 5.5
Signed-off-by: Yury V. Zaytsev <yury@shurup.com>
---
 src/vfs/shell/shelldef.h | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/src/vfs/shell/shelldef.h b/src/vfs/shell/shelldef.h
index 771ae8e40..4c119ad25 100644
--- a/src/vfs/shell/shelldef.h
+++ b/src/vfs/shell/shelldef.h
@@ -149,20 +149,6 @@
 
 /* default 'stor'  script */
 #define VFS_SHELL_SEND_DEF_CONTENT                                                                 \
-    ""                                                                                             \
-    "FILENAME=\"/${SHELL_FILENAME}\"\n"                                                            \
-    "FILESIZE=${SHELL_FILESIZE}\n"                                                                 \
-    "echo \"### 001\"\n"                                                                           \
-    "{\n"                                                                                          \
-    "    while [ $FILESIZE -gt 0 ]; do\n"                                                          \
-    "        cnt=`expr \\( $FILESIZE + 255 \\) / 256`\n"                                           \
-    "        n=`dd bs=256 count=$cnt | tee -a \"${FILENAME}\" | wc -c`\n"                          \
-    "        FILESIZE=`expr $FILESIZE - $n`\n"                                                     \
-    "    done\n"                                                                                   \
-    "}; echo \"### 200\"\n"
-
-/* default 'appe'  script */
-#define VFS_SHELL_APPEND_DEF_CONTENT                                                               \
     ""                                                                                             \
     "FILENAME=\"/${SHELL_FILENAME}\"\n"                                                            \
     "FILESIZE=${SHELL_FILESIZE}\n"                                                                 \
@@ -183,6 +169,20 @@
     "    done\n"                                                                                   \
     "}; echo \"### 200\"\n"
 
+/* default 'appe'  script */
+#define VFS_SHELL_APPEND_DEF_CONTENT                                                               \
+    ""                                                                                             \
+    "FILENAME=\"/${SHELL_FILENAME}\"\n"                                                            \
+    "FILESIZE=${SHELL_FILESIZE}\n"                                                                 \
+    "echo \"### 001\"\n"                                                                           \
+    "{\n"                                                                                          \
+    "    while [ $FILESIZE -gt 0 ]; do\n"                                                          \
+    "        cnt=`expr \\( $FILESIZE + 255 \\) / 256`\n"                                           \
+    "        n=`dd bs=256 count=$cnt | tee -a \"${FILENAME}\" | wc -c`\n"                          \
+    "        FILESIZE=`expr $FILESIZE - $n`\n"                                                     \
+    "    done\n"                                                                                   \
+    "}; echo \"### 200\"\n"
+
 /* default 'info'  script */
 #define VFS_SHELL_INFO_DEF_CONTENT                                                                 \
     ""                                                                                             \
-- 
2.50.1 (Apple Git-155)

zyv · 2026-09-29T15:11:01Z

F4 seems to be already fixed and not TODO.

Yes, you are right. I've gotten overwhelmed by the amount of findings. I didn't want to add this hunk because I'd rather remove the function instead, but it slipped through.

I thought about removing vfs_clone_file() altogether, perhaps there's no sense in maintaining it for some internal usage where no progress indicator is needed.

I would remove it, because I'm sure that if it's not called from anywhere, it will just rot and confuse. If needed, it can always be restored from VCS/PR.

I've fixed F3 to re-enable VFSF_USETMP destinations.

Let me know when, in as far as you are concerned, you are done with everything. I'll see about re-reviewing the PR. I'm not sure if anyone else wants to have a look.

tuffnatty · 2026-09-29T15:22:59Z

I would prefer to keep the commit to fix it in master when this PR is merged until we get to #5155 if this is fine with you. If you object on principle, well, then let's drop it.

Why not just commit it to master (as a separate PR?) and forget it? I do not object to the patch, I just find it's only marginally related to this PR and needs to be done anyway, it's a fix.

zyv · 2026-09-29T15:28:31Z

I would prefer to keep the commit to fix it in master when this PR is merged until we get to #5155 if this is fine with you. If you object on principle, well, then let's drop it.

Why not just commit it to master (as a separate PR?) and forget it? I do not object to the patch, I just find it's only marginally related to this PR and needs to be done anyway, it's a fix.

Okay, then keep it dropped. I pasted the patch in #5155, and we'll take care of that later.

tuffnatty · 2026-10-03T15:15:04Z

About F9. The situation is a bit more complicated:

On Oracle's Solaris and MacOS, we can either old-school copy or use a syscall for whole file cloning.
On FreeBSD, we can old-school copy, explicitly clone ranges (COPY_FILE_RANGE_CLONE since FreeBSD 15), or plain kernel-copy-offload (copy_file_range() with flags=0) which would also clone when it can.
On Linux, FICLONE/FICLONERANGE ioctl does not only clone on supported local filesystems, it works at least with NFS and SMB as well (which I would not call COW file cloning, rather server copy offload) - but returns EXDEV on trying to clone between different filesystems. And copy_file_range() allows to clone between different ZFS datasets within the same pool, but may enforce kernel copy offload in other cases (which ones? The documentation is lacking in this regard).

So the range of options differs between platforms.

This PR changes the behavior for the filesystems that don't support file cloning, enforcing them to use kernel copy offload - for a benefit of increased performance and cross-dataset ZFS clone support.

As a user, I usually don't care about the benefits of kernel copy offload; I would only like a guarantee that when COW cloning is applied (which means I don't actually have two separate copies of the data) it would not surprise me, as I have the checkbox selected.

I have a couple of ideas which I would like to get some feedback for.

Idea 1. Should we make the copy_file_range() method configurable as a separate option ("Use kernel copy offload", enabled only when "Use file cloning" is enabled, available only on Linux and FreeBSD, required to get cross-dataset ZFS file cloning under Linux)?

Idea 2. Fallback to copy_file_range() on Linux only when FICLONERANGE has already returned a EXDEV. Perhaps it would take care of the problem of unwanted kernel copy offload and preserve most of the existing behaviour, while still adding cross-ZFS-dataset cloning support. Could it be the better solution tactically?

tuffnatty · 2026-10-04T00:40:47Z

I have just implemented and pushed the second option, that is, on Linux, only try copy_file_range() after FICLONERANGE returns EXDEV; on FreeBSD pre-COPY_FILE_RANGE_CLONE, switch off cloning support; the 14.x legacy version where this matters will probably go out of support sooner than MC gets a new release.

This way, we don't do non-cloning kernel copy offload, thus keeping the most of existing behaviour.

zyv · 2026-10-04T05:54:49Z

I have a couple of ideas which I would like to get some feedback for.

Idea 1. Should we make the copy_file_range() method configurable as a separate option ("Use kernel copy offload", enabled only when "Use file cloning" is enabled, available only on Linux and FreeBSD, required to get cross-dataset ZFS file cloning under Linux)?

Idea 2. Fallback to copy_file_range() on Linux only when FICLONERANGE has already returned a EXDEV. Perhaps it would take care of the problem of unwanted kernel copy offload and preserve most of the existing behaviour, while still adding cross-ZFS-dataset cloning support. Could it be the better solution tactically?

In as far as I'm concerned, I'm not really sure as to why the user would want to do COW, but avoid copy offloading. This is why my suggestion was to rename the option to "Kernel copy offloading" and change the semantics.

My understanding is that your current version keeps the option name clear, which is a valid approach for me as well, but I wonder why that would be advantageous. I think that one could argue that in practice, one really wants to differentiate on space usage, but this isn't really guaranteed, even for COW file systems, is it?

One thing that I don't really like is the proliferation of detailed and obscure options, and something that you have to switch on for improvements (unless this is really dangerous, like buffered writes). That's why I believe that unless there are good reasons for it, I'd just always use whatever acceleration or compaction the kernel offers by default, and let the user turn it off if they see it as problematic for any reason.

tuffnatty · 2026-10-04T11:38:21Z

One thing that I don't really like is the proliferation of detailed and obscure options, and something that you have to switch on for improvements (unless this is really dangerous, like buffered writes).

And I can assure you that ZFS block cloning had been quite dangerous until recently (as any other filesystem feature that's not been 10 years battle-tested), that's why I don't like the option hiding.

I also don't know why the user wouldn't want kernel copy offloading (except disabling COW). But introducing non-COW kernel copy offload is too much of a behaviour change, and too much discussions and reformulations and struggle which I had not planned for this PR.

zyv · 2026-10-04T12:01:14Z

One thing that I don't really like is the proliferation of detailed and obscure options, and something that you have to switch on for improvements (unless this is really dangerous, like buffered writes).

And I can assure you that ZFS block cloning had been quite dangerous until recently (as any other filesystem feature that's not been 10 years battle-tested), that's why I don't like the option hiding.

I also don't know why the user wouldn't want kernel copy offloading (except disabling COW). But introducing non-COW kernel copy offload is too much of a behaviour change, and too much discussions and reformulations and struggle which I had not planned for this PR.

Alright, let's leave the semantics as they are for now (COW-only or nothing), and then replace it with offloading later... or not. Otherwise, this is ready for re-review and merge in as far as you are concerned?

ossilator · 2026-10-04T12:14:42Z

And I can assure you that ZFS block cloning had been quite dangerous until recently

which means that this use case absolutely shouldn't be an option, but something mc decides automatically. the COW option can be offered when it is known to work and be safe.

But introducing non-COW kernel copy offload is too much of a behaviour change,

i don't see how it's a behavior change at all. it's an implementation detail, and a rather minor one in the big picture of things.

posix_fallocate() extends the file to offset + len; in append and reget modes the destination was extended to the source size before copying, and the data was then appended after the preallocated area: the result was too long and had a zero-filled gap (reget), or preallocation failed with EINVAL when the source was smaller than the destination (append). Assisted-By: Claude Opus 5.5 Signed-off-by: Yury V. Zaytsev <yury@shurup.com>

…(15.0+) Otherwise copy_file_range() with flags=0 performs a kernel copy offload without block cloning guarantee. Signed-off-by: Phil Krylov <phil@krylov.eu>

This allows to make cross-dataset file clones on ZFS on Linux. If ioctl(FICLONERANGE) sets EXDEV, then try copy_file_range(), otherwise skip it as it could result in a non-cloning kernel copy offload. Before Linux 5.19, copy_file_range() had bugs and inconsistent behaviour, so guard it with a runtime version check. On Linux, use copy_file_range() only with 64-bit off_t. Signed-off-by: Phil Krylov <phil@krylov.eu> Co-authored-by: Yury V. Zaytsev <yury@shurup.com> Assisted-By: Claude Opus 5.5

There are cases where cloning the entire file in a single syscall still takes considerable time, for example, the FICLONERANGE Linux ioctl on a pre-4.2 NFS filesystem. Refactor range cloning path to work in chunks (of size increasing until it hurts progress reporting) and report progress. The new workflow is: 1. determine if we should try cloning for the given paths and append mode; 2. if cloning in append mode, open with O_WRONLY+lseek instead of O_APPEND; 3. if cloning, determine the copy method by trying to clone the first chunk. On Linux, if FICLONERANGE fails with EXDEV, retry with copy_file_range(); 4. if both copy methods have failed, switch to plain copy mode and only now perform preallocation; 5. in the main copy loop, if cloning, increase copy chunk size until it hurts progress bar smoothness. Signed-off-by: Phil Krylov <phil@krylov.eu> Co-authored-by: Yury V. Zaytsev <yury@shurup.com> Assisted-By: Claude Opus 5.5

Signed-off-by: Phil Krylov <phil@krylov.eu>

Assisted-By: Claude Opus 5.5 Signed-off-by: Yury V. Zaytsev <yury@shurup.com>

Signed-off-by: Phil Krylov <phil@krylov.eu>

tuffnatty · 2026-10-04T23:13:44Z

which means that this use case absolutely shouldn't be an option, but something mc decides automatically. the COW option can be offered when it is known to work and be safe.

Midnight Commander is not in a position where it can decide what is safe or unsafe. It has no mechanisms for this, is released quite rarely, and it's just not its job. An OS/FS may offer the API in the belief it's safe; then bugs happen, and the feature is no longer safe. How should MC handle the situation? It's just not possible.

i don't see how it's a behavior change at all. it's an implementation detail, and a rather minor one in the big picture of things.
OK, I do not want to change this implementation detail.

tuffnatty · 2026-10-04T23:21:50Z

@zyv

Alright, let's leave the semantics as they are for now (COW-only or nothing), and then replace it with offloading later... or not. Otherwise, this is ready for re-review and merge in as far as you are concerned?

Yes, I believe it's ready now. I've added some tests for the "copy methods" and moved preallocation after copy method selection. Also, I've separated the commits with fixes to bugs currently in master and moved them to the top, so that they can theoretically be merged without waiting for the rest of the PR.

ossilator · 2026-10-05T07:54:57Z

Midnight Commander is not in a position where it can decide what is safe or unsafe.

neither is the user, realistically speaking.

It has no mechanisms for this

it can do the same os+version check you expect from the user.

tuffnatty · 2026-10-05T08:29:44Z

Midnight Commander is not in a position where it can decide what is safe or unsafe.

neither is the user, realistically speaking.

It has no mechanisms for this

it can do the same os+version check you expect from the user.

Version of every OS and of every filesystem driver? And proactively disable file cloning after thorough code review? I don't think so. It's much better to give the user a choice and let them take the responsibility for their decisions.

ossilator · 2026-10-05T08:55:12Z

and of every filesystem driver?

it doesn't have to be that precise. it's not critical if the test produces false positives.

And proactively disable file cloning after thorough code review?

yes. it's not like this is happening at such a massive scale that it would be an actual problem.

It's much better to give the user a choice and let them take the responsibility for their decisions.

this is about as responsible as handing out loaded guns to children.

the absolute minimum of acceptable behavior is pro-actively sharing our knowledge of the feature's danger, i.e. showing a banner "WARNING: your OS version is known to cause file system corruption under some circumstances when CoW is used" next to the checkbox (when it is checked).

tuffnatty · 2026-10-05T09:14:16Z

And proactively disable file cloning after thorough code review?

yes. it's not like this is happening at such a massive scale that it would be an actual problem.

Well, we need to build in Claude, then...

It's much better to give the user a choice and let them take the responsibility for their decisions.

this is about as responsible as handing out loaded guns to children.

the absolute minimum of acceptable behavior is pro-actively sharing our knowledge of the feature's danger, i.e. showing a banner "WARNING: your OS version is known to cause file system corruption under some circumstances when CoW is used" next to the checkbox (when it is checked).

OK, how about: "WARNING: Midnight Commander has 658 unclosed bugs in its bug tracker and is written in C by several teams changing over 35 years. It may surely eat your data: . It may also interact with bugs of your OS, your FS, remote OSes an FSes in a way unforeseen by the developers. Just close it and use command-line instead, it does a better job of protecting your data [questionable]."

ossilator · 2026-10-05T10:03:52Z

OK, how about:

you're not making things better by trying to take it ad absurdum. there is a difference between warning about a known specific danger and a general disclaimer (which is already present at various places).

tuffnatty · 2026-10-05T10:17:09Z

@ossilator Sorry for not making things better. Basically, I think that the discussion of the safety of the feature, default setting and user interaction does not really belong to this PR. #5140 is a better place.

github-actions Bot added this to the Future Releases milestone Sep 27, 2026

github-actions Bot added needs triage Needs triage by maintainers prio: medium Has the potential to affect progress labels Sep 27, 2026

tuffnatty force-pushed the linux-copy-file-range branch from 98cb13d to 524287a Compare September 28, 2026 09:13

ossilator reviewed Sep 28, 2026

View reviewed changes

Comment thread lib/vfs/vfs.c

Comment thread lib/vfs/vfs.c Outdated

tuffnatty force-pushed the linux-copy-file-range branch from 524287a to 9770c29 Compare September 28, 2026 11:55

tuffnatty force-pushed the linux-copy-file-range branch from 9770c29 to e479b2b Compare September 28, 2026 12:31

tuffnatty force-pushed the linux-copy-file-range branch from e479b2b to 8ae7070 Compare September 28, 2026 13:10

tuffnatty force-pushed the linux-copy-file-range branch from 8ae7070 to 974a8a7 Compare September 28, 2026 17:23

zyv added area: core Issues not related to a specific subsystem and removed needs triage Needs triage by maintainers labels Sep 29, 2026

zyv requested changes Sep 29, 2026

View reviewed changes

tuffnatty force-pushed the linux-copy-file-range branch from fe37e39 to 150fb9e Compare September 29, 2026 13:08

zyv mentioned this pull request Sep 29, 2026

vfs/shell: autogenerate shelldef.h, removing code duplication #5155

Open

3 tasks done

tuffnatty force-pushed the linux-copy-file-range branch 2 times, most recently from 22e4a61 to 16061db Compare October 3, 2026 15:10

tuffnatty force-pushed the linux-copy-file-range branch from 16061db to 0cd7ba4 Compare October 4, 2026 00:31

tuffnatty force-pushed the linux-copy-file-range branch from 0cd7ba4 to 9210498 Compare October 4, 2026 00:34

zyv and others added 3 commits October 4, 2026 17:04

(fix) file cloning: Require COPY_FILE_RANGE_CLONE support on FreeBSD …

7303d00

…(15.0+) Otherwise copy_file_range() with flags=0 performs a kernel copy offload without block cloning guarantee. Signed-off-by: Phil Krylov <phil@krylov.eu>

tuffnatty force-pushed the linux-copy-file-range branch 2 times, most recently from d844ffd to 043cac3 Compare October 4, 2026 22:50

tuffnatty and others added 4 commits October 5, 2026 00:51

tests: add regression tests for mc_copy_file_range_*()

43c00c5

Signed-off-by: Phil Krylov <phil@krylov.eu>

tests: add copy_file_file() regression tests

20e17aa

Assisted-By: Claude Opus 5.5 Signed-off-by: Yury V. Zaytsev <yury@shurup.com>

Enabled file cloning for temporary files created during VFS copy

cacf21c

Signed-off-by: Phil Krylov <phil@krylov.eu>

tuffnatty force-pushed the linux-copy-file-range branch from 043cac3 to cacf21c Compare October 4, 2026 23:06

		off_t src_offset = ctx->do_reget;
		off_t dst_offset = ctx->do_reget;

-    else
-        open_flags |= O_CREAT | O_TRUNC;
+    else
+        open_flags |= O_CREAT | O_TRUNC;
+#ifdef HAVE_FILE_CLONING_BY_RANGE
+    // Only local files can be cloned. Other VFSes (ftpfs, shell) append only with O_APPEND
+    if (dst_exists && ctx->do_append && !vfs_cloning_supported (src_vpath, dst_vpath))
+        open_flags |= O_APPEND;
+#endif

	AC_CHECK_FUNCS(copy_file_range) # copy_file_range(2) (Linux)
	# copy_file_range(2) (Linux): glibc declares it with off64_t *, so require a 64-bit off_t
	AS_IF([test "x$ac_cv_sizeof_off_t" = x8], [AC_CHECK_FUNCS(copy_file_range)])

Conversation

tuffnatty commented Sep 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes

Checklist

Uh oh!

ossilator left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tuffnatty commented Sep 28, 2026

Uh oh!

tuffnatty commented Sep 28, 2026

Uh oh!

zyv commented Sep 28, 2026

Uh oh!

tuffnatty commented Sep 28, 2026

Uh oh!

zyv commented Sep 28, 2026

Uh oh!

zyv left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zyv Sep 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tuffnatty commented Sep 29, 2026

Uh oh!

tuffnatty commented Sep 29, 2026

Uh oh!

tuffnatty commented Sep 29, 2026

Uh oh!

zyv commented Sep 29, 2026

Uh oh!

zyv commented Sep 29, 2026

Uh oh!

tuffnatty commented Sep 29, 2026

Uh oh!

zyv commented Sep 29, 2026

Uh oh!

tuffnatty commented Oct 3, 2026

Uh oh!

tuffnatty commented Oct 4, 2026

Uh oh!

zyv commented Oct 4, 2026

Uh oh!

tuffnatty commented Oct 4, 2026

Uh oh!

zyv commented Oct 4, 2026

Uh oh!

ossilator commented Oct 4, 2026

Uh oh!

tuffnatty commented Oct 4, 2026

tuffnatty commented Sep 27, 2026 •

edited

Loading

zyv left a comment •

edited

Loading

zyv Sep 29, 2026 •

edited

Loading