Add btrfs' file clone operation

[sfionov]

From Wikipedia: "Btrfs provides a clone operation which atomically creates a copy-on-write snapshot of a file, support for which was added to GNU coreutils 7.5."

In coreutils this feature may be used with cp --reflink file1 file2

Syntax is: ioctl(dest_desc, BTRFS_IOC_CLONE, src_desc), where dest_desc and src_desc are file descriptors.

It would be nice to utilize this feature in MC, for example, as "Copy-on-write" switch in copy file dialog (appearing when source and destination is on one Btrfs filesystem) or as a separate operation in UI.

[Wiseman1024]

This could be interesting, especially if mc is smart enough to work as cp --reflink=auto where a normal copy will be made if the file cannot be COW'd in the desired destination.

In other words, it'd be good to make it a "Copy on write if possible" switch.

[Alekej Serdjukov]

This is more relevant now?

[xake]

A RFC for a possible implementation of this. Tested. Please review/comment.

[AleXoundOS]

XFS is also on the way to support copy-on-write.
For reference ​http://lkml.iu.edu/hypermail/linux/kernel/1610.1/01939.html: many essential XFS changes will be merged into Linux kernel 4.9 soon.
Now it's not Btrfs only feature. So there is a higher demand for MC to support reflink copying.

[xake]

UYpdated patch, using new defines.

[joost]

There is a problem with the patch.
When I do a file move (F6 key) between two subvolumes on the same filesystem, then the actual result is a file copy (COW). The source file still exists.
Otherwise it seems to work fine.

Edit:
Adding "return_status = FILE_CONT;" before the "goto ret;" seems to fix it.

[joost]

I have been using the patch with my modification for a while without any problems.

Here's the last diff section highlighting the change I made:

diff --git a/src/filemanager/file.c b/src/filemanager/file.c
index b61821a86..708967e00 100644
--- src/filemanager/file.c
+++ src/filemanager/file.c
@@ -1738,6 +1738,'''14''' @@ copy_file_file (file_op_total_context_t * tctx, file_op_context_t * ctx,
     appending = ctx->do_append;
     ctx->do_append = FALSE;
 
+    /* Try clone the file first */
+    if (vfs_clone_file(dest_desc,src_desc) == 0)
+    {
+        dst_status = DEST_FULL;
'''+        return_status = FILE_CONT;'''
+        goto ret;
+    }
+
     /* Find out the optimal buffer size.  */
     while (mc_fstat (dest_desc, &dst_stat) != 0)
     {
-- 
2.13.0

[gray_-_wolf]

Since I'm using BTRFS quite a lot, I would really appreciate this feature, so I'm considering building my own mc with this patch instead of using one from the distribution.

However before I do that, I want to make sure I understand what this patch is about. I do not fully understand these line:

dest_class = vfs_class_find_by_handle (dest_vfs_fd, &dest_fd); 
if ((dest_class->flags & VFSF_LOCAL) == 0 || dest_fd == NULL) 
  return 0; 
 
src_class = vfs_class_find_by_handle (src_vfs_fd, &src_fd); 
if ((src_class->flags & VFSF_LOCAL) == 0 || src_fd == NULL) 
  return 0; 

If my understanding is correct, this checks if FICLONE should work and if not (not local or null) return 0; without even trying the ioctl call.

Here are two things I don't understand:

1. Why do this at all? Why not just call ioctl and let it fail?

2. Is return 0; the correct thing to do? Calling code uses

if (vfs_clone_file(dest_desc,src_desc) == 0) 

to check if clone was success. Shouldn't it therefore be return -1; to signal that the clone failed and normal copy should be done?

Also, could this be done in time for 4.8.22? Seems like very useful feature.

[gray_-_wolf]

Also, the

#else

always returns 0 meaning the clone always succeedes on anything not linux? That seems... weird.

[gray_-_wolf]

(Hopefully) correct version of the patch

[gray_-_wolf]

I believe current patches are not entirelly correct, I've attached mine with few fixes (mainly return codes).

[andrew_b]

if ((dest_fd == NULL || dest_class->flags & VFSF_LOCAL) == 0)

Something is incorrect here.

if ((src_fd == NULL || src_class->flags & VFSF_LOCAL) == 0)

Likewise.

#else 
    (void) dest_vfs_fd; 
    (void) src_vfs_fd; 
    return -1; 
#endif

Perhaps, like ​coreutils, we should set errno in this case. Also we should set errno in other cases where -1 is returned.

[gray_-_wolf]

Something is incorrect here.

You are ofc right, I've made a mistake when moving the == NULL check to the front. Parentheses should be fixed now.

should set errno in this case

Don't see reason why not. mc is not checking the errno but still why not. I went with EOPNOTSUPP (​http://man7.org/linux/man-pages/man2/ioctl_ficlonerange.2.html) in all cases. While technically we could return EXDEV if only one of the check ifs is true, I don't think it warrants the added complexity.

[gray_-_wolf]

ficlone.patch V2

[andrew_b]

Branch: 1983_btrfs_clone
Initial changeset:30ca5a53ddb7de8392cceadeae618cc03cf61274

Please test. I don't use the BTRFS and I cannot test this branch completely myself.

[gray_-_wolf]

Sorry for the delay, will get to it over the weekend.

[gray_-_wolf]

So I've been using patched version for some time now doing sorting my data store and did not notice anything weird after moving few 100GBs of data around. So I would say it works fine.

[andrew_b]

Replying to gray_-_wolf:

So I would say it works fine.

Thanks! Applying...

[andrew_b]

Merge to master: [388dad98996de2a493284143ded3859435e81733].

git log --pretty=oneline 5f80acd..388dad9