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