On Tue, Oct 23, 2018 at 11:30 AM Olga Kornievskaia <olga.kornievskaia@xxxxxxxxx> wrote: > > 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. The reason I think we should remove the check all together is we'd allow the callers of copy_file_range() to utilize the do_splice_direct() between file system types even when copy_file_range() doesn't support cross fs copy. Isn't that beneficial? > > 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.
