Ticket #1983 (closed enhancement: fixed)

Opened 14 years ago

Last modified 6 years 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 9 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 7 years ago.
UYpdated patch, using new defines.
ficlone.patch (2.2 KB) - added by gray_-_wolf 6 years ago.
(Hopefully) correct version of the patch
0001-try-FICLONE-in-copy-move.patch (2.3 KB) - added by gray_-_wolf 6 years ago.
ficlone.patch V2

Change History

comment:1 Changed 14 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 14 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 10 years ago by Alekej Serdjukov

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

comment:4 Changed 10 years ago by Alekej Serdjukov

This is more relevant now?

Changed 9 years ago by xake

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

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

UYpdated patch, using new defines.

comment:6 Changed 6 years 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 6 years ago by joost (previous) (diff)

comment:7 Changed 6 years 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 6 years ago by zaytsev (previous) (diff)

comment:8 Changed 6 years 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 6 years 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 6 years ago by gray_-_wolf (previous) (diff)

Changed 6 years ago by gray_-_wolf

(Hopefully) correct version of the patch

comment:10 Changed 6 years 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 6 years 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 6 years 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 6 years ago by gray_-_wolf

ficlone.patch V2

comment:13 Changed 6 years 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 6 years ago by gray_-_wolf

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

comment:15 follow-up: ↓ 16 Changed 6 years 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 6 years 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 6 years 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 6 years ago by andrew_b

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