Re: [PATCH v5 8/9] vfs: Add vfs_copy_file_range() support for pagecache copies

[Anna Schumaker <Anna.Schumaker-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org>]

On 10/09/2015 07:15 AM, Pádraig Brady wrote:
> On 08/10/15 02:40, Neil Brown wrote:
>> Anna Schumaker <Anna.Schumaker-HgOvQuBEEgTQT0dZR+AlfA@public.gmane.org> writes:
>>
>>> @@ -1338,34 +1362,26 @@ ssize_t vfs_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)
>>>  {
>>> -	struct inode *inode_in;
>>> -	struct inode *inode_out;
>>>  	ssize_t ret;
>>>  
>>> -	if (flags)
>>> +	/* Flags should only be used exclusively. */
>>> +	if ((flags & COPY_FR_COPY) && (flags & ~COPY_FR_COPY))
>>> +		return -EINVAL;
>>> +	if ((flags & COPY_FR_REFLINK) && (flags & ~COPY_FR_REFLINK))
>>> +		return -EINVAL;
>>> +	if ((flags & COPY_FR_DEDUP) && (flags & ~COPY_FR_DEDUP))
>>>  		return -EINVAL;
>>>  
>>
>> Do you also need:
>>
>>    if (flags & ~(COPY_FR_COPY | COPY_FR_REFLINK | COPY_FR_DEDUP))
>>    	return -EINVAL;
>>
>> so that future user-space can test if the kernel supports new flags?
> 
> Seems like a good idea, yes.
> 
> Also that got me thinking about COPY_FR_SPARSE.
> What's the current behavior when copying a sparse range?
> Is the hole propagated by default (good), or is it expanded?

I haven't tried it, but I think the hole would be expanded :(.  I'm having splice() handle the pagecache copy part, and (as far as I know) splice() doesn't know anything about sparse files.  I might be able to put in some kind of fallocate() / splice() loop to copy the range in multiple pieces.

I don't want to add COPY_FR_SPARSE_AUTO, because then the kernel will have to determine how best to interpret "auto".  I'm more inclined to add a single COPY_FR_SPARSE flag to enable creating sparse files, and then have the application tell us what to do for any given range.

Anna

> 
> Note cp(1) has --sparse={never,auto,always}. Auto is the default,
> so it would be good I think if that was the default mode for copy_file_range().
> With other sparse modes, we'd have to avoid copy_file_range() unless
> there was control possible with COPY_FR_SPARSE_{AUTO,NONE,ALWAYS}.
> Note currently cp --sparse=always will detect runs of zeros and also
> avoid speculative preallocation by using fallocate (fd, FALLOC_FL_PUNCH_HOLE, ...)
> 
> thanks,
> Pádraig.
>