Re: [PATCH v7 0/4] VFS: In-kernel copy system call

[Andreas Dilger <adilger@dilger.ca>]

[-- Attachment #1: Type: text/plain, Size: 2808 bytes --]


> On Oct 24, 2015, at 10:52 AM, Eric Biggers <ebiggers3@gmail.com> wrote:
> 
> A few comments:
> 
>>      if (!(file_in->f_mode & FMODE_READ) ||
>>          !(file_out->f_mode & FMODE_WRITE) ||
>>          (file_out->f_flags & O_APPEND) ||
>>          !file_out->f_op)
>>              return -EBADF;
> 
> Isn't 'f_op' always non-NULL?
> 
> If the destination file cannot be append-only, shouldn't this be documented?

Actually, wouldn't O_APPEND only be a problem if the target file wasn't
being appended to?  In other words, if the target i_size == start offset
then it should be possible to use the copy syscall on an O_APPEND file.

Cheers, Andreas

>> 	if (inode_in->i_sb != inode_out->i_sb ||
>> 		file_in->f_path.mnt != file_out->f_path.mnt)
>> 		return -EXDEV;
> 
> Doesn't the same mount already imply the same superblock?
> 
>> /*
>> * copy_file_range() differs from regular file read and write in that it
>> * specifically allows return partial success.  When it does so is up to
>> * the copy_file_range method.
>> */
> 
> What does this mean?  I thought that read() and write() can also return partial
> success.
> 
>>      f_out = fdget(fd_out);
>>      if (!f_in.file || !f_out.file) {
>>              ret = -EBADF;
>>              goto out;
>> 	}
> 
> This looked wrong at first because it may call fdput() on a 'struct fd' that was
> not successfully acquired, but it looks like it works currently because of how
> the FDPUT_FPUT flag is used.  It may be a good idea to write it the "obvious"
> way, though (use separate labels depending on which fdput()s need to happen).
> 
> 
> Other questions:
> 
> Should FMODE_PREAD or FMODE_PWRITE access be checked if the user specifies their
> own 'off_in' or 'off_out', respectively?
> 
> What is supposed to happen if the user passes provides a file descriptor to a
> non-regular file, such as a block device or char device?
> 
> If the 'in' file has fewer than 'len' bytes remaining until EOF, what is the
> expected behavior?  It looks like the btrfs implementation has different
> behavior from the pagecache implementation.
> 
> It appears the btrfs implementation has alignment restrictions --- where is this
> documented and how will users know what alignment to use?
> 
> Are copies within the same file permitted and can the ranges overlap?  The man
> page doesn't say.
> 
> It looks like the initial patch defines __NR_copy_file_range for the ARM
> architecture but doesn't actually hook that system call up for ARM; why is that?
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 833 bytes --]