[RFC,cp] btrfs, nocow and cp --reflink

Commit Message

Goffredo Baroncelli May 25, 2022, 5:05 p.m. UTC

Hi All,

recently I discovered that BTRFS allow to reflink a file only if the flag FS_NOCOW_FL is the same on both source and destination.
In the end of this email I added a patch to "cp" to set the FS_NOCOW_FL flag according to the source.

Even tough this works, I am wondering if this is the expected/the least surprise behavior by/for any user. This is the reason why this email is tagged as RFC.

Without reflink, the default behavior is that the new file has the FS_NOCOW_FL flag set according to the parent directory; with this patch the flag would be the same as the source.

I am not sure that this is the correct behviour without warning the user of this change.

Another possibility, is to flip the NOCOW flag only if --reflink=always is passed.

Thoughts ?

BR
G.Baroncelli


-----

Comments

Pádraig Brady May 27, 2022, 2 p.m. UTC | #1

On 25/05/2022 18:05, Goffredo Baroncelli wrote:
> Hi All,
> 
> recently I discovered that BTRFS allow to reflink a file only if the flag FS_NOCOW_FL is the same on both source and destination.
> In the end of this email I added a patch to "cp" to set the FS_NOCOW_FL flag according to the source.
> 
> Even tough this works, I am wondering if this is the expected/the least surprise behavior by/for any user. This is the reason why this email is tagged as RFC.
> 
> Without reflink, the default behavior is that the new file has the FS_NOCOW_FL flag set according to the parent directory; with this patch the flag would be the same as the source.
> 
> I am not sure that this is the correct behviour without warning the user of this change.
> 
> Another possibility, is to flip the NOCOW flag only if --reflink=always is passed.
> 
> Thoughts ?

This flag corresponds to the 'C' chattr attribute,
to allow users to explicitly disable CoW on certain files
or files within certain dirs.

I don't think cp should be overriding that explicit config.  I.e.:
   cp --reflink=auto => try reflink but fall back to normal copy
   cp --reflink=always => try reflink and fail if not possible

We would need another option to bypass system config
(like --reflink=force), however I don't think that's
appropriate functionality for cp.

thanks,
Pádraig

Forza May 27, 2022, 2:59 p.m. UTC | #2

On 2022-05-27 16:00, Pádraig Brady wrote:
> On 25/05/2022 18:05, Goffredo Baroncelli wrote:
>> Hi All,
>>
>> recently I discovered that BTRFS allow to reflink a file only if the 
>> flag FS_NOCOW_FL is the same on both source and destination.
>> In the end of this email I added a patch to "cp" to set the 
>> FS_NOCOW_FL flag according to the source.
>>
>> Even tough this works, I am wondering if this is the expected/the 
>> least surprise behavior by/for any user. This is the reason why this 
>> email is tagged as RFC.
>>
>> Without reflink, the default behavior is that the new file has the 
>> FS_NOCOW_FL flag set according to the parent directory; with this 
>> patch the flag would be the same as the source.
>>
>> I am not sure that this is the correct behviour without warning the 
>> user of this change.
>>
>> Another possibility, is to flip the NOCOW flag only if 
>> --reflink=always is passed.
>>
>> Thoughts ?
> 
> This flag corresponds to the 'C' chattr attribute,
> to allow users to explicitly disable CoW on certain files
> or files within certain dirs.
> 
> I don't think cp should be overriding that explicit config.  I.e.:
>    cp --reflink=auto => try reflink but fall back to normal copy
>    cp --reflink=always => try reflink and fail if not possible
> 
> We would need another option to bypass system config
> (like --reflink=force), however I don't think that's
> appropriate functionality for cp.
> 
> thanks,
> Pádraig

The solution is that 'cp' should set +C on the target file before 
appending data to it. I don't think that we need any additional mode, 
but the default should be that '--reflink=auto|always' sets the fsattrs 
in the correct order on the target file. This is important for other 
fsattrs as well, such as +i (immutable), +c (compress) and +m (nocompress).

There is thread from two years about about the same issue:
https://lists.gnu.org/archive/html/coreutils/2020-05/msg00014.html

There is also an existing bug report about this issue:
https://lists.gnu.org/r/bug-coreutils/2021-06/msg00003.html

Thanks,
Forza

Patch

diff --git a/src/copy.c b/src/copy.c
index b15d91990..41df45cd5 100644
--- a/src/copy.c
+++ b/src/copy.c
@@ -22,6 +22,8 @@ 
  #include <sys/ioctl.h>
  #include <sys/types.h>
  #include <selinux/selinux.h>
+#include <sys/vfs.h>
+#include <linux/magic.h>
  
  #if HAVE_HURD_H
  # include <hurd.h>
@@ -399,6 +401,32 @@  static inline int
  clone_file (int dest_fd, int src_fd)
  {
  #ifdef FICLONE
+# ifdef __linux__
+  /* BTRFS requires that both source and dest have the same setting
+     about FS_NOCOW_FL */
+  int src_flags, dst_flags, r;
+  struct statfs sbuf;
+
+  r = fstatfs(dest_fd, &sbuf);
+  if (r < 0)
+    return r;
+  if (sbuf.f_type == BTRFS_SUPER_MAGIC || sbuf.f_type == BTRFS_TEST_MAGIC)
+    {
+      r = ioctl(src_fd, FS_IOC_GETFLAGS, &src_flags);
+      if (r < 0)
+        return r;
+      r = ioctl(dest_fd, FS_IOC_GETFLAGS, &dst_flags);
+      if (r < 0)
+        return r;
+      if ((src_flags ^ dst_flags) & FS_NOCOW_FL)
+        {
+          dst_flags ^= FS_NOCOW_FL;
+          r = ioctl(dest_fd, FS_IOC_SETFLAGS, &dst_flags);
+          if (r < 0)
+            return r;
+        }
+    }
+# endif
    return ioctl (dest_fd, FICLONE, src_fd);
  #else
    (void) dest_fd;