* [PATCH v5 8/9] vfs: allow copy_file_range to copy across devices @ 2019-06-04 13:56 Amir Goldstein 2019-06-04 20:42 ` Olga Kornievskaia 0 siblings, 1 reply; 4+ messages in thread From: Amir Goldstein @ 2019-06-04 13:56 UTC (permalink / raw) To: Darrick J . Wong Cc: Dave Chinner, Christoph Hellwig, linux-xfs, Olga Kornievskaia, Luis Henriques, Al Viro, linux-fsdevel, linux-api, ceph-devel, linux-nfs, linux-cifs, Steve French, Dave Chinner We want to enable cross-filesystem copy_file_range functionality where possible, so push the "same superblock only" checks down to the individual filesystem callouts so they can make their own decisions about cross-superblock copy offload and fallack to generic_copy_file_range() for cross-superblock copy. [Amir] We do not call ->remap_file_range() in case the files are not on the same sb and do not call ->copy_file_range() in case the files do not belong to the same filesystem driver. This changes behavior of the copy_file_range(2) syscall, which will now allow cross filesystem in-kernel copy. CIFS already supports cross-superblock copy, between two shares to the same server. This functionality will now be available via the copy_file_range(2) syscall. Cc: Steve French <stfrench@microsoft.com> Signed-off-by: Dave Chinner <dchinner@redhat.com> Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- Darrick, Per feedback from Olga, I am sending a modified version of this patch to address cross file_system_type copy issue in nfs. For the sake of global warming I am not re-posting the entire patch set. I removed your RVB because of the change. Thanks, Amir. Changes since v4: - Check "same filesystem driver" by comapring ->copy_file_range() function pointer fs/ceph/file.c | 4 +++- fs/cifs/cifsfs.c | 2 +- fs/fuse/file.c | 5 ++++- fs/nfs/nfs4file.c | 5 ++++- fs/read_write.c | 18 ++++++++++++------ 5 files changed, 24 insertions(+), 10 deletions(-) diff --git a/fs/ceph/file.c b/fs/ceph/file.c index e87f7b2023af..4cd41ed5cc53 100644 --- a/fs/ceph/file.c +++ b/fs/ceph/file.c @@ -1909,6 +1909,8 @@ static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off, if (src_inode == dst_inode) return -EINVAL; + if (src_inode->i_sb != dst_inode->i_sb) + return -EXDEV; if (ceph_snap(dst_inode) != CEPH_NOSNAP) return -EROFS; @@ -2109,7 +2111,7 @@ static ssize_t ceph_copy_file_range(struct file *src_file, loff_t src_off, ret = __ceph_copy_file_range(src_file, src_off, dst_file, dst_off, len, flags); - if (ret == -EOPNOTSUPP) + if (ret == -EOPNOTSUPP || ret == -EXDEV) ret = generic_copy_file_range(src_file, src_off, dst_file, dst_off, len, flags); return ret; diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c index c65823270313..f11eea6125c1 100644 --- a/fs/cifs/cifsfs.c +++ b/fs/cifs/cifsfs.c @@ -1149,7 +1149,7 @@ static ssize_t cifs_copy_file_range(struct file *src_file, loff_t off, len, flags); free_xid(xid); - if (rc == -EOPNOTSUPP) + if (rc == -EOPNOTSUPP || rc == -EXDEV) rc = generic_copy_file_range(src_file, off, dst_file, destoff, len, flags); return rc; diff --git a/fs/fuse/file.c b/fs/fuse/file.c index e03901ae729b..569baf286835 100644 --- a/fs/fuse/file.c +++ b/fs/fuse/file.c @@ -3126,6 +3126,9 @@ static ssize_t __fuse_copy_file_range(struct file *file_in, loff_t pos_in, if (fc->no_copy_file_range) return -EOPNOTSUPP; + if (file_inode(file_in)->i_sb != file_inode(file_out)->i_sb) + return -EXDEV; + inode_lock(inode_out); if (fc->writeback_cache) { @@ -3182,7 +3185,7 @@ static ssize_t fuse_copy_file_range(struct file *src_file, loff_t src_off, ret = __fuse_copy_file_range(src_file, src_off, dst_file, dst_off, len, flags); - if (ret == -EOPNOTSUPP) + if (ret == -EOPNOTSUPP || ret == -EXDEV) ret = generic_copy_file_range(src_file, src_off, dst_file, dst_off, len, flags); return ret; diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c index 4842f3ab3161..f4157eb1f69d 100644 --- a/fs/nfs/nfs4file.c +++ b/fs/nfs/nfs4file.c @@ -133,6 +133,9 @@ static ssize_t __nfs4_copy_file_range(struct file *file_in, loff_t pos_in, struct file *file_out, loff_t pos_out, size_t count, unsigned int flags) { + /* Only offload copy if superblock is the same */ + if (file_inode(file_in)->i_sb != file_inode(file_out)->i_sb) + return -EXDEV; if (!nfs_server_capable(file_inode(file_out), NFS_CAP_COPY)) return -EOPNOTSUPP; if (file_inode(file_in) == file_inode(file_out)) @@ -148,7 +151,7 @@ static ssize_t nfs4_copy_file_range(struct file *file_in, loff_t pos_in, ret = __nfs4_copy_file_range(file_in, pos_in, file_out, pos_out, count, flags); - if (ret == -EOPNOTSUPP) + if (ret == -EOPNOTSUPP || ret == -EXDEV) ret = generic_copy_file_range(file_in, pos_in, file_out, pos_out, count, flags); return ret; diff --git a/fs/read_write.c b/fs/read_write.c index cec7e7b1f693..bb594c8f4404 100644 --- a/fs/read_write.c +++ b/fs/read_write.c @@ -1599,7 +1599,16 @@ static ssize_t do_copy_file_range(struct file *file_in, loff_t pos_in, struct file *file_out, loff_t pos_out, size_t len, unsigned int flags) { - if (file_out->f_op->copy_file_range) + /* + * Although we now allow filesystems to handle cross sb copy, passing + * a file of the wrong filesystem type to filesystem driver can result + * in an attempt to dereference the wrong type of ->private_data, so + * avoid doing that until we really have a good reason. + * NFS has several different file_system_type's, but they all end up + * using the same ->copy_file_range() function pointer. + */ + if (file_out->f_op->copy_file_range && + file_out->f_op->copy_file_range == file_in->f_op->copy_file_range) return file_out->f_op->copy_file_range(file_in, pos_in, file_out, pos_out, len, flags); @@ -1622,10 +1631,6 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in, if (flags != 0) return -EINVAL; - /* this could be relaxed once a method supports cross-fs copies */ - if (file_inode(file_in)->i_sb != file_inode(file_out)->i_sb) - return -EXDEV; - ret = generic_copy_file_checks(file_in, pos_in, file_out, pos_out, &len, flags); if (unlikely(ret)) @@ -1648,7 +1653,8 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in, * Try cloning first, this is supported by more file systems, and * more efficient if both clone and copy are supported (e.g. NFS). */ - if (file_in->f_op->remap_file_range) { + if (file_in->f_op->remap_file_range && + file_inode(file_in)->i_sb == file_inode(file_out)->i_sb) { loff_t cloned; cloned = file_in->f_op->remap_file_range(file_in, pos_in, -- 2.17.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v5 8/9] vfs: allow copy_file_range to copy across devices 2019-06-04 13:56 [PATCH v5 8/9] vfs: allow copy_file_range to copy across devices Amir Goldstein @ 2019-06-04 20:42 ` Olga Kornievskaia 2019-06-06 0:31 ` Steve French 0 siblings, 1 reply; 4+ messages in thread From: Olga Kornievskaia @ 2019-06-04 20:42 UTC (permalink / raw) To: Amir Goldstein Cc: Darrick J . Wong, Dave Chinner, Christoph Hellwig, linux-xfs, Luis Henriques, Al Viro, linux-fsdevel, Linux API, ceph-devel, linux-nfs, CIFS, Steve French, Dave Chinner On Tue, Jun 4, 2019 at 9:56 AM Amir Goldstein <amir73il@gmail.com> wrote: > > We want to enable cross-filesystem copy_file_range functionality > where possible, so push the "same superblock only" checks down to > the individual filesystem callouts so they can make their own > decisions about cross-superblock copy offload and fallack to > generic_copy_file_range() for cross-superblock copy. > > [Amir] We do not call ->remap_file_range() in case the files are not > on the same sb and do not call ->copy_file_range() in case the files > do not belong to the same filesystem driver. > > This changes behavior of the copy_file_range(2) syscall, which will > now allow cross filesystem in-kernel copy. CIFS already supports > cross-superblock copy, between two shares to the same server. This > functionality will now be available via the copy_file_range(2) syscall. > > Cc: Steve French <stfrench@microsoft.com> > Signed-off-by: Dave Chinner <dchinner@redhat.com> > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > --- > > Darrick, > > Per feedback from Olga, I am sending a modified version of this patch > to address cross file_system_type copy issue in nfs. > Thanks Amir, this works for NFS with referrals. > For the sake of global warming I am not re-posting the entire patch set. > I removed your RVB because of the change. > > Thanks, > Amir. > > Changes since v4: > - Check "same filesystem driver" by comapring ->copy_file_range() > function pointer > > fs/ceph/file.c | 4 +++- > fs/cifs/cifsfs.c | 2 +- > fs/fuse/file.c | 5 ++++- > fs/nfs/nfs4file.c | 5 ++++- > fs/read_write.c | 18 ++++++++++++------ > 5 files changed, 24 insertions(+), 10 deletions(-) > > diff --git a/fs/ceph/file.c b/fs/ceph/file.c > index e87f7b2023af..4cd41ed5cc53 100644 > --- a/fs/ceph/file.c > +++ b/fs/ceph/file.c > @@ -1909,6 +1909,8 @@ static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off, > > if (src_inode == dst_inode) > return -EINVAL; > + if (src_inode->i_sb != dst_inode->i_sb) > + return -EXDEV; > if (ceph_snap(dst_inode) != CEPH_NOSNAP) > return -EROFS; > > @@ -2109,7 +2111,7 @@ static ssize_t ceph_copy_file_range(struct file *src_file, loff_t src_off, > ret = __ceph_copy_file_range(src_file, src_off, dst_file, dst_off, > len, flags); > > - if (ret == -EOPNOTSUPP) > + if (ret == -EOPNOTSUPP || ret == -EXDEV) > ret = generic_copy_file_range(src_file, src_off, dst_file, > dst_off, len, flags); > return ret; > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c > index c65823270313..f11eea6125c1 100644 > --- a/fs/cifs/cifsfs.c > +++ b/fs/cifs/cifsfs.c > @@ -1149,7 +1149,7 @@ static ssize_t cifs_copy_file_range(struct file *src_file, loff_t off, > len, flags); > free_xid(xid); > > - if (rc == -EOPNOTSUPP) > + if (rc == -EOPNOTSUPP || rc == -EXDEV) > rc = generic_copy_file_range(src_file, off, dst_file, > destoff, len, flags); > return rc; > diff --git a/fs/fuse/file.c b/fs/fuse/file.c > index e03901ae729b..569baf286835 100644 > --- a/fs/fuse/file.c > +++ b/fs/fuse/file.c > @@ -3126,6 +3126,9 @@ static ssize_t __fuse_copy_file_range(struct file *file_in, loff_t pos_in, > if (fc->no_copy_file_range) > return -EOPNOTSUPP; > > + if (file_inode(file_in)->i_sb != file_inode(file_out)->i_sb) > + return -EXDEV; > + > inode_lock(inode_out); > > if (fc->writeback_cache) { > @@ -3182,7 +3185,7 @@ static ssize_t fuse_copy_file_range(struct file *src_file, loff_t src_off, > ret = __fuse_copy_file_range(src_file, src_off, dst_file, dst_off, > len, flags); > > - if (ret == -EOPNOTSUPP) > + if (ret == -EOPNOTSUPP || ret == -EXDEV) > ret = generic_copy_file_range(src_file, src_off, dst_file, > dst_off, len, flags); > return ret; > diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c > index 4842f3ab3161..f4157eb1f69d 100644 > --- a/fs/nfs/nfs4file.c > +++ b/fs/nfs/nfs4file.c > @@ -133,6 +133,9 @@ static ssize_t __nfs4_copy_file_range(struct file *file_in, loff_t pos_in, > struct file *file_out, loff_t pos_out, > size_t count, unsigned int flags) > { > + /* Only offload copy if superblock is the same */ > + if (file_inode(file_in)->i_sb != file_inode(file_out)->i_sb) > + return -EXDEV; > if (!nfs_server_capable(file_inode(file_out), NFS_CAP_COPY)) > return -EOPNOTSUPP; > if (file_inode(file_in) == file_inode(file_out)) > @@ -148,7 +151,7 @@ static ssize_t nfs4_copy_file_range(struct file *file_in, loff_t pos_in, > > ret = __nfs4_copy_file_range(file_in, pos_in, file_out, pos_out, count, > flags); > - if (ret == -EOPNOTSUPP) > + if (ret == -EOPNOTSUPP || ret == -EXDEV) > ret = generic_copy_file_range(file_in, pos_in, file_out, > pos_out, count, flags); > return ret; > diff --git a/fs/read_write.c b/fs/read_write.c > index cec7e7b1f693..bb594c8f4404 100644 > --- a/fs/read_write.c > +++ b/fs/read_write.c > @@ -1599,7 +1599,16 @@ static ssize_t do_copy_file_range(struct file *file_in, loff_t pos_in, > struct file *file_out, loff_t pos_out, > size_t len, unsigned int flags) > { > - if (file_out->f_op->copy_file_range) > + /* > + * Although we now allow filesystems to handle cross sb copy, passing > + * a file of the wrong filesystem type to filesystem driver can result > + * in an attempt to dereference the wrong type of ->private_data, so > + * avoid doing that until we really have a good reason. > + * NFS has several different file_system_type's, but they all end up > + * using the same ->copy_file_range() function pointer. > + */ > + if (file_out->f_op->copy_file_range && > + file_out->f_op->copy_file_range == file_in->f_op->copy_file_range) > return file_out->f_op->copy_file_range(file_in, pos_in, > file_out, pos_out, > len, flags); > @@ -1622,10 +1631,6 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in, > if (flags != 0) > return -EINVAL; > > - /* this could be relaxed once a method supports cross-fs copies */ > - if (file_inode(file_in)->i_sb != file_inode(file_out)->i_sb) > - return -EXDEV; > - > ret = generic_copy_file_checks(file_in, pos_in, file_out, pos_out, &len, > flags); > if (unlikely(ret)) > @@ -1648,7 +1653,8 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in, > * Try cloning first, this is supported by more file systems, and > * more efficient if both clone and copy are supported (e.g. NFS). > */ > - if (file_in->f_op->remap_file_range) { > + if (file_in->f_op->remap_file_range && > + file_inode(file_in)->i_sb == file_inode(file_out)->i_sb) { > loff_t cloned; > > cloned = file_in->f_op->remap_file_range(file_in, pos_in, > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v5 8/9] vfs: allow copy_file_range to copy across devices 2019-06-04 20:42 ` Olga Kornievskaia @ 2019-06-06 0:31 ` Steve French 2019-06-06 5:27 ` Amir Goldstein 0 siblings, 1 reply; 4+ messages in thread From: Steve French @ 2019-06-06 0:31 UTC (permalink / raw) To: Olga Kornievskaia Cc: Amir Goldstein, Darrick J . Wong, Dave Chinner, Christoph Hellwig, linux-xfs, Luis Henriques, Al Viro, linux-fsdevel, Linux API, ceph-devel, linux-nfs, CIFS, Steve French, Dave Chinner I tried the patchset and new xfstests with cifs.ko and looks like the copy_file_range patches are fine 1) xfs_io copy_range worked fine (once I fixed a bug I found in cifs.ko in the "match_server" function) 2) the new xfstest 989 requires other features (swapfiles) which I can add and 990 requires new xfsprogs presumably (xfs_io in current Ubuntu doesn't seem to include xfs_io chmod) generic/989 [not run] swapfiles are not supported generic/990 [not run] xfs_io chmod support is missing Are there other xfstests that you would like me to run (other than the 3 in the 900 series)? On Tue, Jun 4, 2019 at 3:43 PM Olga Kornievskaia <olga.kornievskaia@gmail.com> wrote: > > On Tue, Jun 4, 2019 at 9:56 AM Amir Goldstein <amir73il@gmail.com> wrote: > > > > We want to enable cross-filesystem copy_file_range functionality > > where possible, so push the "same superblock only" checks down to > > the individual filesystem callouts so they can make their own > > decisions about cross-superblock copy offload and fallack to > > generic_copy_file_range() for cross-superblock copy. > > > > [Amir] We do not call ->remap_file_range() in case the files are not > > on the same sb and do not call ->copy_file_range() in case the files > > do not belong to the same filesystem driver. > > > > This changes behavior of the copy_file_range(2) syscall, which will > > now allow cross filesystem in-kernel copy. CIFS already supports > > cross-superblock copy, between two shares to the same server. This > > functionality will now be available via the copy_file_range(2) syscall. > > > > Cc: Steve French <stfrench@microsoft.com> > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > --- > > > > Darrick, > > > > Per feedback from Olga, I am sending a modified version of this patch > > to address cross file_system_type copy issue in nfs. > > > > Thanks Amir, this works for NFS with referrals. > > > For the sake of global warming I am not re-posting the entire patch set. > > I removed your RVB because of the change. > > > > Thanks, > > Amir. > > > > Changes since v4: > > - Check "same filesystem driver" by comapring ->copy_file_range() > > function pointer > > > > fs/ceph/file.c | 4 +++- > > fs/cifs/cifsfs.c | 2 +- > > fs/fuse/file.c | 5 ++++- > > fs/nfs/nfs4file.c | 5 ++++- > > fs/read_write.c | 18 ++++++++++++------ > > 5 files changed, 24 insertions(+), 10 deletions(-) > > > > diff --git a/fs/ceph/file.c b/fs/ceph/file.c > > index e87f7b2023af..4cd41ed5cc53 100644 > > --- a/fs/ceph/file.c > > +++ b/fs/ceph/file.c > > @@ -1909,6 +1909,8 @@ static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off, > > > > if (src_inode == dst_inode) > > return -EINVAL; > > + if (src_inode->i_sb != dst_inode->i_sb) > > + return -EXDEV; > > if (ceph_snap(dst_inode) != CEPH_NOSNAP) > > return -EROFS; > > > > @@ -2109,7 +2111,7 @@ static ssize_t ceph_copy_file_range(struct file *src_file, loff_t src_off, > > ret = __ceph_copy_file_range(src_file, src_off, dst_file, dst_off, > > len, flags); > > > > - if (ret == -EOPNOTSUPP) > > + if (ret == -EOPNOTSUPP || ret == -EXDEV) > > ret = generic_copy_file_range(src_file, src_off, dst_file, > > dst_off, len, flags); > > return ret; > > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c > > index c65823270313..f11eea6125c1 100644 > > --- a/fs/cifs/cifsfs.c > > +++ b/fs/cifs/cifsfs.c > > @@ -1149,7 +1149,7 @@ static ssize_t cifs_copy_file_range(struct file *src_file, loff_t off, > > len, flags); > > free_xid(xid); > > > > - if (rc == -EOPNOTSUPP) > > + if (rc == -EOPNOTSUPP || rc == -EXDEV) > > rc = generic_copy_file_range(src_file, off, dst_file, > > destoff, len, flags); > > return rc; > > diff --git a/fs/fuse/file.c b/fs/fuse/file.c > > index e03901ae729b..569baf286835 100644 > > --- a/fs/fuse/file.c > > +++ b/fs/fuse/file.c > > @@ -3126,6 +3126,9 @@ static ssize_t __fuse_copy_file_range(struct file *file_in, loff_t pos_in, > > if (fc->no_copy_file_range) > > return -EOPNOTSUPP; > > > > + if (file_inode(file_in)->i_sb != file_inode(file_out)->i_sb) > > + return -EXDEV; > > + > > inode_lock(inode_out); > > > > if (fc->writeback_cache) { > > @@ -3182,7 +3185,7 @@ static ssize_t fuse_copy_file_range(struct file *src_file, loff_t src_off, > > ret = __fuse_copy_file_range(src_file, src_off, dst_file, dst_off, > > len, flags); > > > > - if (ret == -EOPNOTSUPP) > > + if (ret == -EOPNOTSUPP || ret == -EXDEV) > > ret = generic_copy_file_range(src_file, src_off, dst_file, > > dst_off, len, flags); > > return ret; > > diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c > > index 4842f3ab3161..f4157eb1f69d 100644 > > --- a/fs/nfs/nfs4file.c > > +++ b/fs/nfs/nfs4file.c > > @@ -133,6 +133,9 @@ static ssize_t __nfs4_copy_file_range(struct file *file_in, loff_t pos_in, > > struct file *file_out, loff_t pos_out, > > size_t count, unsigned int flags) > > { > > + /* Only offload copy if superblock is the same */ > > + if (file_inode(file_in)->i_sb != file_inode(file_out)->i_sb) > > + return -EXDEV; > > if (!nfs_server_capable(file_inode(file_out), NFS_CAP_COPY)) > > return -EOPNOTSUPP; > > if (file_inode(file_in) == file_inode(file_out)) > > @@ -148,7 +151,7 @@ static ssize_t nfs4_copy_file_range(struct file *file_in, loff_t pos_in, > > > > ret = __nfs4_copy_file_range(file_in, pos_in, file_out, pos_out, count, > > flags); > > - if (ret == -EOPNOTSUPP) > > + if (ret == -EOPNOTSUPP || ret == -EXDEV) > > ret = generic_copy_file_range(file_in, pos_in, file_out, > > pos_out, count, flags); > > return ret; > > diff --git a/fs/read_write.c b/fs/read_write.c > > index cec7e7b1f693..bb594c8f4404 100644 > > --- a/fs/read_write.c > > +++ b/fs/read_write.c > > @@ -1599,7 +1599,16 @@ static ssize_t do_copy_file_range(struct file *file_in, loff_t pos_in, > > struct file *file_out, loff_t pos_out, > > size_t len, unsigned int flags) > > { > > - if (file_out->f_op->copy_file_range) > > + /* > > + * Although we now allow filesystems to handle cross sb copy, passing > > + * a file of the wrong filesystem type to filesystem driver can result > > + * in an attempt to dereference the wrong type of ->private_data, so > > + * avoid doing that until we really have a good reason. > > + * NFS has several different file_system_type's, but they all end up > > + * using the same ->copy_file_range() function pointer. > > + */ > > + if (file_out->f_op->copy_file_range && > > + file_out->f_op->copy_file_range == file_in->f_op->copy_file_range) > > return file_out->f_op->copy_file_range(file_in, pos_in, > > file_out, pos_out, > > len, flags); > > @@ -1622,10 +1631,6 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in, > > if (flags != 0) > > return -EINVAL; > > > > - /* this could be relaxed once a method supports cross-fs copies */ > > - if (file_inode(file_in)->i_sb != file_inode(file_out)->i_sb) > > - return -EXDEV; > > - > > ret = generic_copy_file_checks(file_in, pos_in, file_out, pos_out, &len, > > flags); > > if (unlikely(ret)) > > @@ -1648,7 +1653,8 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in, > > * Try cloning first, this is supported by more file systems, and > > * more efficient if both clone and copy are supported (e.g. NFS). > > */ > > - if (file_in->f_op->remap_file_range) { > > + if (file_in->f_op->remap_file_range && > > + file_inode(file_in)->i_sb == file_inode(file_out)->i_sb) { > > loff_t cloned; > > > > cloned = file_in->f_op->remap_file_range(file_in, pos_in, > > -- > > 2.17.1 > > -- Thanks, Steve ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v5 8/9] vfs: allow copy_file_range to copy across devices 2019-06-06 0:31 ` Steve French @ 2019-06-06 5:27 ` Amir Goldstein 0 siblings, 0 replies; 4+ messages in thread From: Amir Goldstein @ 2019-06-06 5:27 UTC (permalink / raw) To: Steve French Cc: Olga Kornievskaia, Darrick J . Wong, Dave Chinner, Christoph Hellwig, linux-xfs, Luis Henriques, Al Viro, linux-fsdevel, Linux API, ceph-devel, linux-nfs, CIFS, Steve French, Dave Chinner On Thu, Jun 6, 2019 at 2:31 AM Steve French <smfrench@gmail.com> wrote: > > I tried the patchset and new xfstests with cifs.ko and looks like the > copy_file_range patches are fine Glad to hear! Just to clarify that you are referring to the latest patch version. If you took patch v5 (that you replied to) and the rest of the patches from v4, that was correct. You can compare to my github branch if in any doubt (or just test my branch): https://github.com/amir73il/linux/commits/copy_file_range-v5 NOTE that the github branch has a CIFS patch that was not included in v4 posting to Darrick ("cifs: copy_file_range needs to strip setuid bits ..."), because I decided this patch should be sent directly to you later. Please do include this patch in you CIFS tests. > > 1) xfs_io copy_range worked fine (once I fixed a bug I found in > cifs.ko in the "match_server" function) > 2) the new xfstest 989 requires other features (swapfiles) which I can > add and 990 requires new xfsprogs presumably (xfs_io in current Ubuntu > doesn't seem to include xfs_io chmod) It's true, you would need minimum version xfs_io 4.20 to run that test, but I am less worried about testing bounds checks with CIFS, as all bounds checks are performed before entering cifs code. BTW, checking for xfs_io chmod was in xfstests patches v2: https://github.com/amir73il/xfstests/commits/copy_file_range-v2 I since posted v3. Actually not much changed, but checking for an even newer (only just posted) feature of xfs_io: https://github.com/amir73il/xfstests/commits/copy_file_range-v3 So you are not any better with this version w.r.t test 990. > generic/989 [not run] swapfiles are not supported Expected not to run on cifs > generic/990 [not run] xfs_io chmod support is missing Expected not to with old xfs_io > > Are there other xfstests that you would like me to run (other than the > 3 in the 900 series)? Yes, please run tests group "-g copy_range", namely 430..434 Also, the "900 series" had 4 tests not 3, so not sure which tests did run? Did 991 (cross-device copy) run? That is THE test to run and cifs is THE filesystem to test with to actually exercise filesystem implementation of server side cross-device (i.e. cross-share) copy. Thanks! Amir. > On Tue, Jun 4, 2019 at 3:43 PM Olga Kornievskaia > <olga.kornievskaia@gmail.com> wrote: > > > > On Tue, Jun 4, 2019 at 9:56 AM Amir Goldstein <amir73il@gmail.com> wrote: > > > > > > We want to enable cross-filesystem copy_file_range functionality > > > where possible, so push the "same superblock only" checks down to > > > the individual filesystem callouts so they can make their own > > > decisions about cross-superblock copy offload and fallack to > > > generic_copy_file_range() for cross-superblock copy. > > > > > > [Amir] We do not call ->remap_file_range() in case the files are not > > > on the same sb and do not call ->copy_file_range() in case the files > > > do not belong to the same filesystem driver. > > > > > > This changes behavior of the copy_file_range(2) syscall, which will > > > now allow cross filesystem in-kernel copy. CIFS already supports > > > cross-superblock copy, between two shares to the same server. This > > > functionality will now be available via the copy_file_range(2) syscall. > > > > > > Cc: Steve French <stfrench@microsoft.com> > > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > > --- > > > > > > Darrick, > > > > > > Per feedback from Olga, I am sending a modified version of this patch > > > to address cross file_system_type copy issue in nfs. > > > > > > > Thanks Amir, this works for NFS with referrals. > > > > > For the sake of global warming I am not re-posting the entire patch set. > > > I removed your RVB because of the change. > > > > > > Thanks, > > > Amir. > > > > > > Changes since v4: > > > - Check "same filesystem driver" by comapring ->copy_file_range() > > > function pointer > > > > > > fs/ceph/file.c | 4 +++- > > > fs/cifs/cifsfs.c | 2 +- > > > fs/fuse/file.c | 5 ++++- > > > fs/nfs/nfs4file.c | 5 ++++- > > > fs/read_write.c | 18 ++++++++++++------ > > > 5 files changed, 24 insertions(+), 10 deletions(-) > > > > > > diff --git a/fs/ceph/file.c b/fs/ceph/file.c > > > index e87f7b2023af..4cd41ed5cc53 100644 > > > --- a/fs/ceph/file.c > > > +++ b/fs/ceph/file.c > > > @@ -1909,6 +1909,8 @@ static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off, > > > > > > if (src_inode == dst_inode) > > > return -EINVAL; > > > + if (src_inode->i_sb != dst_inode->i_sb) > > > + return -EXDEV; > > > if (ceph_snap(dst_inode) != CEPH_NOSNAP) > > > return -EROFS; > > > > > > @@ -2109,7 +2111,7 @@ static ssize_t ceph_copy_file_range(struct file *src_file, loff_t src_off, > > > ret = __ceph_copy_file_range(src_file, src_off, dst_file, dst_off, > > > len, flags); > > > > > > - if (ret == -EOPNOTSUPP) > > > + if (ret == -EOPNOTSUPP || ret == -EXDEV) > > > ret = generic_copy_file_range(src_file, src_off, dst_file, > > > dst_off, len, flags); > > > return ret; > > > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c > > > index c65823270313..f11eea6125c1 100644 > > > --- a/fs/cifs/cifsfs.c > > > +++ b/fs/cifs/cifsfs.c > > > @@ -1149,7 +1149,7 @@ static ssize_t cifs_copy_file_range(struct file *src_file, loff_t off, > > > len, flags); > > > free_xid(xid); > > > > > > - if (rc == -EOPNOTSUPP) > > > + if (rc == -EOPNOTSUPP || rc == -EXDEV) > > > rc = generic_copy_file_range(src_file, off, dst_file, > > > destoff, len, flags); > > > return rc; > > > diff --git a/fs/fuse/file.c b/fs/fuse/file.c > > > index e03901ae729b..569baf286835 100644 > > > --- a/fs/fuse/file.c > > > +++ b/fs/fuse/file.c > > > @@ -3126,6 +3126,9 @@ static ssize_t __fuse_copy_file_range(struct file *file_in, loff_t pos_in, > > > if (fc->no_copy_file_range) > > > return -EOPNOTSUPP; > > > > > > + if (file_inode(file_in)->i_sb != file_inode(file_out)->i_sb) > > > + return -EXDEV; > > > + > > > inode_lock(inode_out); > > > > > > if (fc->writeback_cache) { > > > @@ -3182,7 +3185,7 @@ static ssize_t fuse_copy_file_range(struct file *src_file, loff_t src_off, > > > ret = __fuse_copy_file_range(src_file, src_off, dst_file, dst_off, > > > len, flags); > > > > > > - if (ret == -EOPNOTSUPP) > > > + if (ret == -EOPNOTSUPP || ret == -EXDEV) > > > ret = generic_copy_file_range(src_file, src_off, dst_file, > > > dst_off, len, flags); > > > return ret; > > > diff --git a/fs/nfs/nfs4file.c b/fs/nfs/nfs4file.c > > > index 4842f3ab3161..f4157eb1f69d 100644 > > > --- a/fs/nfs/nfs4file.c > > > +++ b/fs/nfs/nfs4file.c > > > @@ -133,6 +133,9 @@ static ssize_t __nfs4_copy_file_range(struct file *file_in, loff_t pos_in, > > > struct file *file_out, loff_t pos_out, > > > size_t count, unsigned int flags) > > > { > > > + /* Only offload copy if superblock is the same */ > > > + if (file_inode(file_in)->i_sb != file_inode(file_out)->i_sb) > > > + return -EXDEV; > > > if (!nfs_server_capable(file_inode(file_out), NFS_CAP_COPY)) > > > return -EOPNOTSUPP; > > > if (file_inode(file_in) == file_inode(file_out)) > > > @@ -148,7 +151,7 @@ static ssize_t nfs4_copy_file_range(struct file *file_in, loff_t pos_in, > > > > > > ret = __nfs4_copy_file_range(file_in, pos_in, file_out, pos_out, count, > > > flags); > > > - if (ret == -EOPNOTSUPP) > > > + if (ret == -EOPNOTSUPP || ret == -EXDEV) > > > ret = generic_copy_file_range(file_in, pos_in, file_out, > > > pos_out, count, flags); > > > return ret; > > > diff --git a/fs/read_write.c b/fs/read_write.c > > > index cec7e7b1f693..bb594c8f4404 100644 > > > --- a/fs/read_write.c > > > +++ b/fs/read_write.c > > > @@ -1599,7 +1599,16 @@ static ssize_t do_copy_file_range(struct file *file_in, loff_t pos_in, > > > struct file *file_out, loff_t pos_out, > > > size_t len, unsigned int flags) > > > { > > > - if (file_out->f_op->copy_file_range) > > > + /* > > > + * Although we now allow filesystems to handle cross sb copy, passing > > > + * a file of the wrong filesystem type to filesystem driver can result > > > + * in an attempt to dereference the wrong type of ->private_data, so > > > + * avoid doing that until we really have a good reason. > > > + * NFS has several different file_system_type's, but they all end up > > > + * using the same ->copy_file_range() function pointer. > > > + */ > > > + if (file_out->f_op->copy_file_range && > > > + file_out->f_op->copy_file_range == file_in->f_op->copy_file_range) > > > return file_out->f_op->copy_file_range(file_in, pos_in, > > > file_out, pos_out, > > > len, flags); > > > @@ -1622,10 +1631,6 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in, > > > if (flags != 0) > > > return -EINVAL; > > > > > > - /* this could be relaxed once a method supports cross-fs copies */ > > > - if (file_inode(file_in)->i_sb != file_inode(file_out)->i_sb) > > > - return -EXDEV; > > > - > > > ret = generic_copy_file_checks(file_in, pos_in, file_out, pos_out, &len, > > > flags); > > > if (unlikely(ret)) > > > @@ -1648,7 +1653,8 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in, > > > * Try cloning first, this is supported by more file systems, and > > > * more efficient if both clone and copy are supported (e.g. NFS). > > > */ > > > - if (file_in->f_op->remap_file_range) { > > > + if (file_in->f_op->remap_file_range && > > > + file_inode(file_in)->i_sb == file_inode(file_out)->i_sb) { > > > loff_t cloned; > > > > > > cloned = file_in->f_op->remap_file_range(file_in, pos_in, > > > -- > > > 2.17.1 > > > > > > > -- > Thanks, > > Steve ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-06-06 5:27 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-06-04 13:56 [PATCH v5 8/9] vfs: allow copy_file_range to copy across devices Amir Goldstein 2019-06-04 20:42 ` Olga Kornievskaia 2019-06-06 0:31 ` Steve French 2019-06-06 5:27 ` Amir Goldstein
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).