On Mon, Mar 21, 2022 at 2:37 PM David Sterba <dsterba@xxxxxxxx> wrote:
>
> - allow reflinks/deduplication from two different mounts of the same
> filesystem
So I've pulled this, and it looks ok, but I'm not getting the warm and fuzzies.
In particular, I'm not seeing any commentary about different
filesystems for this.
There are several filesystems that use that ->remap_file_range()
operation, so these relaxed rules don't just affect btrfs.
Yes, yes, checking for i_sb matching does seem sensible, but I'd
*really* have liked some sign that people checked with other
filesystem maintainers and this is ok for all of them, and they didn't
make assumptions about "always same mount" rather than "always same
filesystem".
This affects at least cifs, nfs, overlayfs and ocfs2.
Adding fsdevel, and pointing to that
- if (src_file->f_path.mnt != dst_file->f_path.mnt)
+ if (file_inode(src_file)->i_sb != file_inode(dst_file)->i_sb)
change in commit 9f5710bbfd30 ("fs: allow cross-vfsmount reflink/dedupe")
And yes, there was already a comment about "Practically, they only
need to be on the same file system" from before that matches the new
behavior, but hey, comments have been known to be wrong in the past
too.
And yes, I'm also aware that do_clone_file_range() already had that
exact same i_sb check and it's not new, but since ioctl_file_clone()
cheched for the mount path, I don't think you could actually reach it
without being on the same mount.
And while discussing these sanity checks: wouldn't it make sense to
check that *both* the source file and the destination file support
that remap_file_range() op, and it's the same op?
Yes, yes, it probably always is in practice, but I could imagine some
type confusion thing. So wouldn't it be nice to also have something
like
if (dst_file->f_op != src_file->f_op)
goto out_drop_write;
in there? I'm thinking "how about dedupe from a directory to a regular
file" kind of craziness...
Linus
