On Sat, Oct 20, 2018 at 7:06 AM Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
>
> On Fri, Oct 19, 2018 at 11:30:18AM -0400, Olga Kornievskaia wrote:
> > Allow copy_file_range to copy between different superblocks but only
> > of the same file system types. This feature was of interest to CIFS
> > as well as NFS.
>
> > + if (file_out->f_op->copy_file_range &&
> > + (file_in->f_op->copy_file_range ==
> > + file_out->f_op->copy_file_range)) {
>
> That is seriously asking for trouble. If at some point we add
> a library function usable as ->copy_file_range() instance, this
> will turn into a hard-to-spot problem.
>
> Comparing methods like that is best avoided. If you want to compare
> fs types, do just that - it's not hard.
Another thing is the commit message claims to:
"Allow copy_file_range to copy between different superblocks but only
of the same file system types"
But what the patch actually does is:
"Allow copy_file_range() syscall to copy between different filesystems
AND allow calling the filesystems' copy_file_range() method
between different superblocks but only of the same file system types"
It's probably OK and quite useful to do the former, but maybe man page
should be fixed to explicitly mention that the copy is expected to work
across filesystems since kernel version XXX (?)
If you don't wish to change cross filesystem type behavior and only
relax cross super block limitation, then you should replace the
same inode->i_sb check above with same inode->i_sb->s_type
check instead of doing the check only for calling the filesystem
copy_file_range() method.
>
> Another potential issue here is the interplay with local filesystems
> using vfs_clone_file_prep_inodes() (or Darrick's series lifting that
> into generic code). There we assume the same block size on both
> sides; that is automatically true if they live on the same superblock,
> but with your changes it's no longer true.
Heh? I don't see that Darrick's work has anything to do with
copy_file_range(). It only touched vfs_copy_file_range() for
the ->clone_file_range() call, which Olga's patch protects with same sb
check.
Thanks,
Amir.
