Re: [coreutils] Re: Feature: add ocfs2 reflink to cp(1)

[jeff.liu]

> Joel Becker wrote:
>> On Sun, Apr 04, 2010 at 11:44:48PM +0800, jeff.liu wrote:
>>> +/* This function detects whether the source and destination files
>>> +   are all resides on OCFS2.  If true, then perform the OCFS2 reflink
>>> +   operation.  Otherwise, performing btrfs clone operation.  */

>>      I don't know that the correct behavior is btrfs' when ocfs2 is
>> not detected.
> Looks I should not implement 'is_ocfs2_file()' here, it add a bit extra 
> indistinct noise.
> Now my idea is just performing either btrfs clone or ocfs2 reflink operation 
> if cp(1) invoked with
> '--reflink=[WHEN]'.
> If both of them failed no matter what the reason is, fall back to normal copy 
> process when the
> reflink mode is 'REFLINK_AUTO', or abort.
> 
>>> +/* Perform the OCFS2 CoW reflink operation if possible.
>>> +   We do not attempt to preserve security attributes for a reference
>>> +   counted link.  Instead, let 'x->preserve_xxxx' to process them if
>>> +   they are the user expected.
>>> +   Upon success, return 0, Otherwise, return -1 and set errno.  */
>>> +static inline int
>>> +reflink_file (char const *src_name, char const *dst_name,
>>> +              int src_fd, bool *new_dst)
>>> +{
>>> +#ifdef __linux__
>>> +# ifndef REFLINK_ATTR_NONE
>>> +#  define REFLINK_ATTR_NONE 0
>>> +# endif
>>      If '-p' was specified, you should honor it with
>> REFLINK_ATTR_PRESERVE.
> Thanks for pointing this out.  At first, I think the file attributes preserve 
> should managed through
> normal copy,  its wrong, I will fix it in the next patch submit.
> 
>> Joel
>>
> 
> Thanks,
> -Jeff
> 

Hello All,

This is the revised version to add ocfs2 reflink feature to cp(1) based on 
Joel's comments.

Changes:
1. Remove 'is_ocfs2_file()' function, it does not need to pre-check the file 
system type for reflink
operation.  If ocfs2 reflink ioctl(2) failed due to this issue or any other 
reason, proceed to do
btrfs clone operation.
2. Do perserve attriutes if `cp' invoked with '-p' opition.  In this 
case,'mode,ownership,timestamp'
are reflinked to destination file; security_context will be reflinked as well 
if possible.
Also, 'xattrs' are reflinked always no matter preserve_attr or preserve_none 
specified.
3. From the code layout point of view, I separated ocfs2 reflink and btrfs 
clone into 2 steps.
the reason is, ocfs2 ioctl(2) play with the destination path name instead of a 
file descriptor.
So I place its code block at the top of '*new_dst' check, otherwise, we have to 
unlink(2) the
'dst_desc' when trying ocfs2 reflink ioctl(2).


Tests against preserve option:
=============================

ACL:
---
jeff@jeff-laptop:~/opensource_dev/coreutils$ setfacl -m user:oracle:r-- /ocfs2/a
jeff@jeff-laptop:~/opensource_dev/coreutils$ ls -l /ocfs2/a
-rw-r--r--+ 1 jeff jeff 5 Apr 27 14:04 /ocfs2/a
jeff@jeff-laptop:~/opensource_dev/coreutils$ getfacl /ocfs2/a
getfacl: Removing leading '/' from absolute path names
# file: ocfs2/a
# owner: jeff
# group: jeff
user::rw-
user:oracle:r--
group::r--
mask::r--
other::r--

jeff@jeff-laptop:~/opensource_dev/coreutils$ ./src/cp --reflink=always -p 
/ocfs2/a /ocfs2/c
jeff@jeff-laptop:~/opensource_dev/coreutils$ ls -l /ocfs2/c
-rw-r--r--+ 1 jeff jeff 5 Apr 27 14:04 /ocfs2/c
jeff@jeff-laptop:~/opensource_dev/coreutils$ getfacl /ocfs2/c
getfacl: Removing leading '/' from absolute path names
# file: ocfs2/c
# owner: jeff
# group: jeff
user::rw-
user:oracle:r--
group::r--
mask::r--
other::r--

mode,ownership,timestamp:
-------------------------
root@jeff-laptop:/home/jeff/opensource_dev/coreutils# ./src/cp --reflink=always 
-p /ocfs2/a
/ocfs2/a_preserve_ownership
root@jeff-laptop:/home/jeff/opensource_dev/coreutils# ls -l /ocfs2/a
-rw-r--r--+ 1 jeff jeff 5 Apr 27 14:04 /ocfs2/a
root@jeff-laptop:/home/jeff/opensource_dev/coreutils# ls -l 
/ocfs2/a_preserve_ownership
-rw-r--r--+ 1 jeff jeff 5 Apr 27 14:04 /ocfs2/a_preserve_ownership

Security context:
-----------------
root@jeff-laptop:~# chcon oracle:object_r:tmp_t:s0 /ocfs2/a
root@jeff-laptop:~# ls -l /ocfs2/a
-rw-r--r--+ 1 jeff jeff 5 Apr 27 14:04 /ocfs2/a
root@jeff-laptop:~# ls -Z /ocfs2/a |grep oracle:object_r:tmp_t:s0
oracle:object_r:tmp_t:s0 /ocfs2/a

jeff@jeff-laptop:~/opensource_dev/coreutils$ ./src/cp --reflink=always -p 
/ocfs2/a
/ocfs2/e_preserve_security
-rw-r--r--+ 1 jeff jeff 5 Apr 27 14:04 /ocfs2/e_preserve_security
jeff@jeff-laptop:~/opensource_dev/coreutils$ ls -Z /ocfs2/e_preserve_security 
|grep
oracle:object_r:tmp_t:s0
oracle:object_r:tmp_t:s0 /ocfs2/e_preserve_security


Would you guys please review the patch which shown as following?  I will 
continue to write the test
script if you guys like the current implemention.

Any comments are appreciated!

>From 96d1d86b82ec101caeebe2c51fad6b2a52d093f5 Mon Sep 17 00:00:00 2001
From: Jie Liu <address@hidden>
Date: Tue, 27 Apr 2010 15:37:17 +0800
Subject: [PATCH 1/1] Add ocfs2 reflink to cp(1), it share the same 
'--reflink=[WHEN]' option
 with btrfs clone operation.

If cp(1) invoked with '-p' option, and the underlaying file system is OCFS2 for
both source and destination files, it will try to preserve the default
attributes(i.e. 'mode,ownership,timestamp'), xattr as well as security context 
if possbile.

Signed-off-by: Jie Liu <address@hidden>
---
 src/copy.c |   77 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 76 insertions(+), 1 deletions(-)

diff --git a/src/copy.c b/src/copy.c
index d18fd45..d2ba306 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -54,6 +54,12 @@
 #include "areadlink.h"
 #include "yesno.h"

+#if HAVE_SYS_VFS_H
+# include <sys/vfs.h>
+#else
+# include <sys/statfs.h>
+#endif
+
 #if USE_XATTR
 # include <attr/error_context.h>
 # include <attr/libattr.h>
@@ -132,6 +138,47 @@ utimens_symlink (char const *file, struct timespec const 
*timespec)
   return err;
 }

+/* Perform the OCFS2 CoW reflink ioctl(2) operation if possible.
+   When using '-p' option, the file's default attributes(i.e. mode,timestamp,
+   ownership and security context if possbile) are reflinked to the destination
+   file as well.  We will then skip over the standard preserve process for such
+   attributes.  Also, 'xattrs' are reflinked always even if 
'REFLINK_ATTR_NONE'.
+   Upon success, return 0, Otherwise, return -1 and set errno.  */
+static inline int
+reflink_file (char const *src_name, char const *dst_name,
+              bool preserve_attrs, int src_fd)
+{
+#ifdef __linux__
+# ifndef REFLINK_ATTR_NONE
+#  define REFLINK_ATTR_NONE 0
+# endif
+# ifndef REFLINK_ATTR_PRESERVE
+#  define REFLINK_ATTR_PRESERVE 1
+# endif
+# ifndef OCFS2_IOC_REFLINK
+  struct reflink_arguments {
+   uint64_t old_path;
+   uint64_t new_path;
+   uint64_t preserve;
+  };
+#  define OCFS2_IOC_REFLINK _IOW ('o', 4, struct reflink_arguments)
+# endif
+  struct reflink_arguments args = {
+    .old_path = (unsigned long) src_name,
+    .new_path = (unsigned long) dst_name,
+    .preserve = preserve_attrs ? REFLINK_ATTR_PRESERVE : REFLINK_ATTR_NONE,
+  };
+  return ioctl (src_fd, OCFS2_IOC_REFLINK, &args);
+#else
+  (void) src_name;
+  (void) dst_name;
+  (void) preserve_attrs;
+  (void) src_fd;
+  errno = ENOTSUP;
+  return -1;
+#endif
+}
+
 /* Perform the O(1) btrfs clone operation, if possible.
    Upon success, return 0.  Otherwise, return -1 and set errno.  */
 static inline int
@@ -514,6 +561,31 @@ copy_reg (char const *src_name, char const *dst_name,
       goto close_src_desc;
     }

+  bool reflink_ok = false;
+  if (x->reflink_mode)
+    {
+      /* When cp is invoked with '--reflink=[WHEN]', try to do OCFS2 reflink 
ioctl(2)
+         first, if it fails, then try Btrfs clone once again at the later time.
+         The reason to perform those operations separately is because `cp' 
will create
+         the destination file if it is a '*new_dst'.  In this case, we have to 
unlink(2)
+         it firstly, otherwise, OCFS2 reflink will fail with 'EEXIST'.  But if 
the destnation
+         file is not a '*new_dst', just let reflink ioctl(2) fails and go 
ahead.  */
+      bool preserve_attributes = (x->preserve_ownership
+                                  && x->preserve_mode
+                                  && x->preserve_timestamps);
+      reflink_ok = reflink_file (src_name, dst_name, preserve_attributes,
+                                 source_desc) == 0;
+      if (reflink_ok)
+        {
+          *new_dst = false;
+
+          /* Skip over the standard attributes preserve process
+             if reflink succeeds and they are already reflinked.  */
+          if (preserve_attributes)
+            goto close_src_desc;
+        }
+    }
+
   /* The semantics of the following open calls are mandated
      by the specs for both cp and mv.  */
   if (! *new_dst)
@@ -634,8 +706,11 @@ copy_reg (char const *src_name, char const *dst_name,
       goto close_src_and_dst_desc;
     }

-  if (x->reflink_mode)
+  if (x->reflink_mode && ! reflink_ok)
     {
+      /* It seems that the preceeding OCFS2 reflink fails, perform Btrfs
+         clone now, if it fails again and `cp' is invoked with 
'--reflink=auto',
+         fall back to a standard copy.  */
       bool clone_ok = clone_file (dest_desc, source_desc) == 0;
       if (clone_ok || x->reflink_mode == REFLINK_ALWAYS)
         {
-- 
1.5.4.3




Best Regards,
-Jeff