Ticket #1983 (closed enhancement: fixed)

Opened 9 years ago

Last modified 3 weeks ago

Add btrfs' file clone operation

Reported by: sfionov Owned by: andrew_b
Priority: minor Milestone: 4.8.22
Component: mc-vfs Version: master
Keywords: Cc: deletesoftware@…, wolf+mc@…
Blocked By: Blocking:
Branch state: merged Votes for changeset: committed-master

Description

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.

Attachments

0001-Uglybtrfsclone.patch (2.3 KB) - added by xake 3 years ago.
A RFC for a possible implementation of this. Tested. Please review/comment.
0001-Added-support-for-reflink-clone-with-BTRFS.patch (2.5 KB) - added by xake 15 months ago.
UYpdated patch, using new defines.
ficlone.patch (2.2 KB) - added by gray_-_wolf 7 weeks ago.
(Hopefully) correct version of the patch
0001-try-FICLONE-in-copy-move.patch (2.3 KB) - added by gray_-_wolf 7 weeks ago.
ficlone.patch V2

Change History

comment:1 Changed 9 years ago by andrew_b

  • Priority changed from major to minor
  • Component changed from mc-core to mc-vfs
  • Milestone changed from 4.7 to Future Releases

comment:2 Changed 9 years ago by 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.

comment:3 Changed 4 years ago by Alekej Serdjukov

  • Cc deletesoftware@… added
  • Branch state set to no branch

comment:4 Changed 4 years ago by Alekej Serdjukov

This is more relevant now?

Changed 3 years ago by xake

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

comment:5 Changed 2 years ago by 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.

Changed 15 months ago by xake

UYpdated patch, using new defines.

comment:6 Changed 11 months ago by 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.

Last edited 11 months ago by joost (previous) (diff)

comment:7 Changed 10 months ago by 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
Last edited 10 months ago by zaytsev (previous) (diff)

comment:8 Changed 2 months ago by gray_-_wolf

  • Cc wolf+mc@… added

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?
  1. 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.

comment:9 Changed 2 months ago by gray_-_wolf

Also, the

#else 
  (void) dest_vfs_fd; 
  (void) src_vfs_fd; 
  return 0; 
#endif 

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

Last edited 2 months ago by gray_-_wolf (previous) (diff)

Changed 7 weeks ago by gray_-_wolf

(Hopefully) correct version of the patch

comment:10 Changed 7 weeks ago by gray_-_wolf

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

comment:11 Changed 7 weeks ago by 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.

comment:12 Changed 7 weeks ago by 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.

Changed 7 weeks ago by gray_-_wolf

ficlone.patch V2

comment:13 Changed 6 weeks ago by andrew_b

  • Status changed from new to accepted
  • Owner set to andrew_b
  • Version changed from version not selected to master
  • Branch state changed from no branch to on review
  • Milestone changed from Future Releases to 4.8.22

Branch: 1983_btrfs_clone
Initial changeset:30ca5a53ddb7de8392cceadeae618cc03cf61274

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

comment:14 Changed 5 weeks ago by gray_-_wolf

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

comment:15 follow-up: ↓ 16 Changed 3 weeks ago by 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.

comment:16 in reply to: ↑ 15 Changed 3 weeks ago by andrew_b

  • Votes for changeset set to gray_-_wolf andrew_b
  • Branch state changed from on review to approved

Replying to gray_-_wolf:

So I would say it works fine.

Thanks! Applying...

comment:17 Changed 3 weeks ago by andrew_b

  • Status changed from accepted to testing
  • Votes for changeset changed from gray_-_wolf andrew_b to committed-master
  • Resolution set to fixed
  • Branch state changed from approved to merged

Merge to master: [388dad98996de2a493284143ded3859435e81733].

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

comment:18 Changed 3 weeks ago by andrew_b

  • Status changed from testing to closed
Note: See TracTickets for help on using tickets.