On Mon, Mar 06, 2017 at 11:27:23AM -0500, Olga Kornievskaia wrote: > > > On Mar 3, 2017, at 9:10 PM, J. Bruce Fields <bfields@xxxxxxxxxxxx> > > wrote: > > > > On Fri, Mar 03, 2017 at 05:46:05PM -0500, Olga Kornievskaia wrote: > >> > >>> On Mar 3, 2017, at 4:32 PM, J. Bruce Fields <bfields@xxxxxxxxxxxx> > >>> wrote: > >>> > >>> On Fri, Mar 03, 2017 at 04:08:19PM -0500, Olga Kornievskaia wrote: > >>>> > >>>>> On Mar 3, 2017, at 3:47 PM, J. Bruce Fields > >>>>> <bfields@xxxxxxxxxxxx> wrote: > >>>>> > >>>>> On Thu, Mar 02, 2017 at 08:22:21AM -0800, Christoph Hellwig > >>>>> wrote: > >>>>>> On Thu, Mar 02, 2017 at 11:01:05AM -0500, Olga Kornievskaia > >>>>>> wrote: > >>>>>>> + if (pos_in >= i_size_read(inode_in)) + return > >>>>>>> -EINVAL; > >>>>>> > >>>>>> That's not how the syscall is supposed to work, we'd rather do > >>>>>> a short read^^^^^copy. > >>>>> > >>>>> That's what I think too, but then is COPY(2) wrong?: > >>>>> > >>>>> EINVAL Requested range extends beyond the end of the > >>>>> source file; or the flags argument is not 0. > >>>>> > >>>>> Also, copy_file_range can be implemented by ->clone_file_range, > >>>>> where these kinds of checks make more sense, I think; e.g. from > >>>>> btrfs: > >>>>> > >>>>> ret = -EINVAL; if (off + len > src->i_size || off + len > >>>>> < off) goto out_unlock; > >>>>> > >>>>> Well, so the caller just has to be prepared for either behavior, > >>>>> I guess, but that may make it more complicated to use. > >>>>> > >>>> > >>>> I’m still rather very confused again by the comment and what it > >>>> is proposing. > >>>> > >>>> There are two checks to consider for the validity of the > >>>> arguments > >>>> > >>>> 1. If the offset of the source file is beyond the end of the > >>>> source file. 2. If the offset + len is beyond the end of the > >>>> file. > >>>> > >>>> I read that the man page is talking about #2. This is actually > >>>> what the NFSv4.2 spec required for the COPY too but we’ve been > >>>> discussing that it should be a short read instead. > >>>> > >>>> This patch address is to address case #1. As far as I can tell it > >>>> applies to all file systems. > >>>> > >>>> Are you suggesting that the checks for the validity of the > >>>> arguments do not belong in VFS but instead should be in each of > >>>> the underlying file systems? > >>>> > >>>> Not all vfs_copy_file_range() are implemented via > >>>> clone_file_range(). At least I hope that “inter” NFSv4.2 COPY > >>>> will also use vfs_file_copy_range() and it would not be calling > >>>> clone(). > >>> > >>> I think it'd be acceptable for copy_file_range() to just return 0 > >>> even in your case 1. I believe that's what ordinary read and > >>> pread does. > >>> > >>> You probably can't perform it atomically with the copy, so it's > >>> possible that the size will change right after you check it. > >>> > >>> I don't see a benefit to the check. > >> > > > >> In read() you don’t specify the offset from which to read. It is > >> read from the current file descriptor offset. I don’t find the > >> comparison equal. > >> > >> It’s it more fair to compare it to lseek() which does return EINVAL > >> if you specify position beyond the end of the file. > > > > Or pread(), which takes an offset. > > > > But read() can read at an offset at the end of file, or even past > > that (if the file was truncated), and I believe it just returns 0 in > > those cases. > > > > I don’t see copy_file_range() specifying that 0 means end of the file. > Are you arguing to add that meaning to the function call? Yes. > I guess in > that case we’d need to take extra care to never return 0bytes to the > client as a “partial” copy (say due to server rebooting). Right. > Unless changed, NFS4.2 mandates the two checks that I’ve specified. I > can add the checks in the NFS implementation itself. However, we > thought at least this check belonged in the VFS layer. I’m really not > super attached to getting into VFS. Actually the 2nd check is > something that copy_file_range() man pages say should return EINVAL > but the VFS code doesn’t enforce it. Please leave those checks out. Let's try to fix the spec. --b. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
