lkml.org 
[lkml]   [2022]   [Mar]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [GIT PULL] Btrfs updates for 5.18
On Tue, Mar 22, 2022 at 10:11 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> On Mon, Mar 21, 2022 at 2:37 PM David Sterba <dsterba@suse.com> 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.

overlayfs shouldn't have a problem with that change.

IIUC, cifs would also gain from this, because clone is implemented
on server side and even two different sb's could technically do
server side duplicate_extents.
Same goes for nfs v4.2.

There was a lot of discussion on these aspects when cross server
(i.e. cross sb) copy was implemented not so long ago.
Relaxing cross-mnt clone is nothing compared to that.


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

As the one who left this comment I can say it is based only on common
sense, similar to the rationale in this recent commit.
If it is any help, overlayfs has been doing cross mnt clones since
913b86e92e1f vfs: allow vfs_clone_file_range() across mount points
I left the comment because I did not need to take responsibility for changing
user behavior at the time, but I do not see any immediate harm from the user
behavior changes now.

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

Both S_ISDIR and !S_ISREG cases are already checked for both clone
and dedupe on both files (twice in fact), so at least that is not a concern.

There may be other reasons to worry about, but I can't think of any.

Thanks,
Amir.

\
 
 \ /
  Last update: 2022-03-22 23:10    [W:0.071 / U:0.092 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site