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