| Message ID | 20170302160211.30451-2-kolga@netapp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Mar 02, 2017 at 11:02:09AM -0500, Olga Kornievskaia wrote: > From: Anna Schumaker <Anna.Schumaker@Netapp.com> > > Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com> > --- > fs/read_write.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/fs/read_write.c b/fs/read_write.c > index 5816d4c..1d9e305 100644 > --- a/fs/read_write.c > +++ b/fs/read_write.c > @@ -1526,6 +1526,9 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in, > if (unlikely(ret)) > return ret; > > + if (pos_in >= i_size_read(inode_in)) > + return -EINVAL; > + This ought to go in vfs_clone_file_prep_inodes. (He says, noticing that btrfs never got updated to use that validator...) --D > if (!(file_in->f_mode & FMODE_READ) || > !(file_out->f_mode & FMODE_WRITE) || > (file_out->f_flags & O_APPEND)) > -- > 1.8.3.1 > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On Mar 2, 2017, at 11:58 AM, Darrick J. Wong <darrick.wong@oracle.com> wrote: > > On Thu, Mar 02, 2017 at 11:02:09AM -0500, Olga Kornievskaia wrote: >> From: Anna Schumaker <Anna.Schumaker@Netapp.com> >> >> Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com> >> --- >> fs/read_write.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/fs/read_write.c b/fs/read_write.c >> index 5816d4c..1d9e305 100644 >> --- a/fs/read_write.c >> +++ b/fs/read_write.c >> @@ -1526,6 +1526,9 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in, >> if (unlikely(ret)) >> return ret; >> >> + if (pos_in >= i_size_read(inode_in)) >> + return -EINVAL; >> + > > This ought to go in vfs_clone_file_prep_inodes. > > (He says, noticing that btrfs never got updated to use that validator…) I apologize I’m not fully understanding the suggestion here. How btrfs is related to the check that I’m suggesting for the copy_file_range(). I don’t see how it would fix the problem for the copy_file_range(). Is the suggestion that NFS’s clone implementation is suppose to call vfs_clone_file_prep_inodes() where the check would be added and thus because vfs_copy_file_range() first decides to call clone() instead of copy() then that check would be met? > > --D > >> if (!(file_in->f_mode & FMODE_READ) || >> !(file_out->f_mode & FMODE_WRITE) || >> (file_out->f_flags & O_APPEND)) >> -- >> 1.8.3.1 >> -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 02, 2017 at 01:21:49PM -0500, Olga Kornievskaia wrote: > > > On Mar 2, 2017, at 11:58 AM, Darrick J. Wong <darrick.wong@oracle.com> wrote: > > > > On Thu, Mar 02, 2017 at 11:02:09AM -0500, Olga Kornievskaia wrote: > >> From: Anna Schumaker <Anna.Schumaker@Netapp.com> > >> > >> Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com> > >> --- > >> fs/read_write.c | 3 +++ > >> 1 file changed, 3 insertions(+) > >> > >> diff --git a/fs/read_write.c b/fs/read_write.c > >> index 5816d4c..1d9e305 100644 > >> --- a/fs/read_write.c > >> +++ b/fs/read_write.c > >> @@ -1526,6 +1526,9 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in, > >> if (unlikely(ret)) > >> return ret; > >> > >> + if (pos_in >= i_size_read(inode_in)) > >> + return -EINVAL; > >> + > > > > This ought to go in vfs_clone_file_prep_inodes. > > > > (He says, noticing that btrfs never got updated to use that validator…) > > I apologize I’m not fully understanding the suggestion here. How btrfs is > related to the check that I’m suggesting for the copy_file_range(). I don’t > see how it would fix the problem for the copy_file_range(). Never mind, I misread vfs_copy as vfs_clone and thought you were talking about something else. :( Sorry about the noise. --D > Is the suggestion that NFS’s clone implementation is suppose to call > vfs_clone_file_prep_inodes() where the check would be added and thus > because vfs_copy_file_range() first decides to call clone() instead of > copy() then that check would be met? > > > > > --D > > > >> if (!(file_in->f_mode & FMODE_READ) || > >> !(file_out->f_mode & FMODE_WRITE) || > >> (file_out->f_flags & O_APPEND)) > >> -- > >> 1.8.3.1 > >> > -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Mar 02, 2017 at 10:40:57AM -0800, Darrick J. Wong wrote: > Never mind, I misread vfs_copy as vfs_clone and thought you were talking > about something else. :( > > Sorry about the noise. And either way we'll need test coverage for copy_file_range before we do any more changes to it. I'm sick and tired of this doctoring around without having any test coverage. I've asked for it again and again and we've made very little progress despite having the code in the tree for almost a year and a half. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> On Mar 7, 2017, at 6:43 PM, Christoph Hellwig <hch@infradead.org> wrote: > > On Thu, Mar 02, 2017 at 10:40:57AM -0800, Darrick J. Wong wrote: >> Never mind, I misread vfs_copy as vfs_clone and thought you were talking >> about something else. :( >> >> Sorry about the noise. > > And either way we'll need test coverage for copy_file_range before we > do any more changes to it. I'm sick and tired of this doctoring around > without having any test coverage. I've asked for it again and again > and we've made very little progress despite having the code in the tree > for almost a year and a half. Anna has added test cases to the xfstest suite. Is there something that’s missing? -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Mar 07, 2017 at 06:46:55PM -0500, Olga Kornievskaia wrote:
> Anna has added test cases to the xfstest suite. Is there something that’s missing?
No, we still don't have that. Anna did one attempt (or maybe two) but
didn't seem to have time to follow up the review comments. Maybe you
can give it a spin to make sure we at least have basic test coverage,
including the cases mentioned in the man page like this.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
> On Mar 7, 2017, at 6:50 PM, Christoph Hellwig <hch@infradead.org> wrote: > > On Tue, Mar 07, 2017 at 06:46:55PM -0500, Olga Kornievskaia wrote: >> Anna has added test cases to the xfstest suite. Is there something that’s missing? > > No, we still don't have that. Anna did one attempt (or maybe two) but > didn't seem to have time to follow up the review comments. Maybe you > can give it a spin to make sure we at least have basic test coverage, > including the cases mentioned in the man page like this. I’m reading some mail archives and I see that Anna submitted patches for xfstests. The comment was to re-work it as a part of xfs_io command. She posted the patches for that. According to the announcement I see here: http://oss.sgi.com/archives/xfs/2016-08/msg00221.html it says it’s including the new copy_file_range command in xfs_io. I don’t run these tests so I don’t know but it look to me that the tests are in. Can you please what I’m getting wrong? Thank you. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 08, 2017 at 10:39:11AM -0500, Olga Kornievskaia wrote: > > I’m reading some mail archives and I see that Anna submitted patches for xfstests. The comment was to re-work it as a part of xfs_io command. She posted the patches for that. According to the announcement I see here: http://oss.sgi.com/archives/xfs/2016-08/msg00221.html it says it’s including the new copy_file_range command in xfs_io. I don’t run these tests so I don’t know but it look to me that the tests are in. That's xfsprogs and the xfs_io command. Now that this step is done someone (e.g. Anna or you) needs to send the patches to xfstests that use this command. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/read_write.c b/fs/read_write.c index 5816d4c..1d9e305 100644 --- a/fs/read_write.c +++ b/fs/read_write.c @@ -1526,6 +1526,9 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in, if (unlikely(ret)) return ret; + if (pos_in >= i_size_read(inode_in)) + return -EINVAL; + if (!(file_in->f_mode & FMODE_READ) || !(file_out->f_mode & FMODE_WRITE) || (file_out->f_flags & O_APPEND))