On Tue, Oct 23, 2018 at 11:03 AM Olga Kornievskaia <olga.kornievskaia@xxxxxxxxx> wrote: > > On Tue, Oct 23, 2018 at 2:05 AM Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > > > On Tue, Oct 23, 2018 at 2:39 AM Jeff Layton <jlayton@xxxxxxxxxx> wrote: > > > > > > On Mon, 2018-10-22 at 15:34 -0400, Olga Kornievskaia wrote: > > > > On Mon, Oct 22, 2018 at 3:06 PM Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote: > > > > > > > > > > On Mon, Oct 22, 2018 at 02:45:04PM -0400, Olga Kornievskaia wrote: > > > > > > On Sat, Oct 20, 2018 at 4:54 AM Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > > > > > > 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. > > > > > > > > > > > > Thank you for the feedback. In the next version, I will remove the > > > > > > check for the functions and instead check for the same file system > > > > > > types. > > > > > > > > > > Jeff and I agree that this is the wrong way to go. Instead, the > > > > > cross-device check should be in the individual instances, not the top > > > > > level code. > > > > > > > > So remove the check all together for the VFS (that was my original > > > > patch to begin with (like #1 not this one). So am I missing the point > > > > again, I keep getting different corrections every time. > > > > > > Sorry if I wasn't clear before: > > > > > > Basically, I think Willy and I are both envisioning that some > > > copy_file_range implementations may eventually not be subject to the > > > limitations of the checks you're adding. > > > > > > > Yes. Eventually. And even Matthew is (quote) "dubious" about that ever > > happening. Changing the interface as Matthew proposed has a price > > and we need to compare this price to the alleged backporting price > > that nobody may ever need to pay. > > > > As far as I can tell, passing a struct file * on a file_operations method > > that does not belong to that filesystem in unprecedented (*) and is a far > > more lethal landmine than the alleged backporting landmine. > > > > (*) prior to v4.19-rc1, filesystems could get an overlayfs file, but > > file_inode(file) has always belonged to the filesystem. > > > > Olga, > > > > I do not strongly object to Matthew's proposal, so don't feel > > obligated to choose my side of the argument. I am just trying > > to offer a different perspective. > > > > In any case, my outstanding concerns with the patch are: > > > > 1. If you change syscall to support cross fs type copy (which is > > good IMO) need to document that in commit message > > and possibly follow up later with a note in man page > > > > 2. Commit message says: > > "This feature was of interest of ... NFS" > > "This feature is needed by NFSv4.2..." > > "NFS will allow for copies between different NFS servers." > > It is not clear to me if we are talking about present of future > > NFSv4.2 code. If NFSv4.2 currently does not support cross > > sb copy (??) than your patch need to enforce same sb > > in nfs4_copy_file_range(). If it does support cross sb copy > > than please edit the commit message to make that clear. > > I personally agree with Amir. I think it's far fetched that a file > system would know how to handle something that's not of its type. When > the copy_file_range() was checked in, there was a comment above the > superblock check saying "we might want to relax this in the future". > It deemed appropriate then to enforce the check since none of the file > systems used it. Now, the future is here, and we are removing the > check but proposing a different once because again the future isn't > here and having a single check simplifies the code. Sorry Ok I wrote this too fast. I think I'm changing my mind and siding with the check by the file system. > But I don't feel strongly about the check (or rather the location of > it VFS vs each FS) and what I ultimately need is to removed same sb > check. It sounds like if Amir isn't objecting, then the check for same > file system type should be removed from VFS. And, for each of the file > systems that currently support copy_file_range() -- CIFS, NFS, and > overlayfs -- I need to modify them and add a check for the same > fs_type. > > Amir to answer your question, only NFSv4.2 has copy_offload > functionality (not earlier NFS versions). Furthermore, existing > upstream only supports same sb copy offload. What this patch series > adds is support for copy offload across different superblocks but NFS > will not support (and would need a check) for copy offload across > different file system types. Also I kinda stand behind the ideas > stated: 1) this is of interest to NFS (where NFS here is to represent > a community, and CIFS is used to represent the other community). 2) > NFSv4.2 copy offload a specific feature that needs this functionality. > 3rd statement is bad. Only NFSv4.2 will allow copies between different > NFS servers (ie., after this patch +series), the emphasis was on "will > allow" meaning what this patch will allow to be done (ie, patch's > purpose). Or also, if the NFS server exports different exports, then a > copy between them (assuming exports of the same file system type). > > In the next version of the patch, I'll do my best to specified what > changed as the consequence of removing the cross sb check (ie, file > system types of the passed in file can be from different file > systems). I will add wording to the man page and add the suggested > wording to the "porting" file.
