Re: [PATCH v2 2/4] ovl: use vfs_clone_file_range() for copy up if possible

[Amir Goldstein <amir73il@gmail.com>]

On Tue, Sep 13, 2016 at 3:02 AM, Dave Chinner <david@fromorbit.com> wrote:
> On Mon, Sep 12, 2016 at 06:06:41PM +0300, Amir Goldstein wrote:
>> When copying up within the same fs, try to use vfs_clone_file_range().
>> This is very efficient when lower and upper are on the same fs
>> with file reflink support. If vfs_clone_file_range() fails because
>> lower and upper are not on the same fs or if fs has no reflink support,
>> copy up falls back to the regular data copy code.
>>
>> Tested correct behavior when lower and upper are on:
>> 1. same ext4 (copy)
>> 2. same xfs + reflink patches + mkfs.xfs (copy)
>> 3. same xfs + reflink patches + mkfs.xfs -m reflink=1 (clone)
>> 4. different xfs + reflink patches + mkfs.xfs -m reflink=1 (copy)
>>
>> For comparison, on my laptop, xfstest overlay/001 (copy up of large
>> sparse files) takes less than 1 second in the xfs reflink setup vs.
>> 25 seconds on the rest of the setups.
>>
>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>> ---
>>  fs/overlayfs/copy_up.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
>> index 43fdc27..e432d7e 100644
>> --- a/fs/overlayfs/copy_up.c
>> +++ b/fs/overlayfs/copy_up.c
>> @@ -136,6 +136,16 @@ static int ovl_copy_up_data(struct path *old, struct path *new, loff_t len)
>>               goto out_fput;
>>       }
>>
>> +     /*
>> +      * Try to use clone_file_range to clone up within the same fs.
>> +      * On failure to clone entire file, fallback to copy loop.
>> +      */
>> +     error = vfs_clone_file_range(old_file, 0, new_file, 0, len);
>> +     if (error == -EXDEV || error == -EOPNOTSUPP)
>> +             error = 0;
>> +     else
>> +             len = 0;
>> +
>>       /* FIXME: copy up sparse files efficiently */
>>       while (len) {
>>               size_t this_len = OVL_COPY_UP_CHUNK_SIZE;
>
> That's nasty. Just use a goto to skip the code that doesn't need to
> be run if vfs_clone_file_range() succeeds. This is much better, IMO:

OK, but I prefer to write up explicit comments for all cases.

>
>         if (!error)
>                 goto out_done;
>         if (error != -EXDEV && error != -EOPNOTSUPP)
>                 goto out_error;
>
>         /* Can't clone, so now we try to copy the data */
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@fromorbit.com