Re: [PATCH] ovl: use copy_file_range for copy up if possible

[Amir Goldstein <amir73il@gmail.com>]

On Mon, Sep 12, 2016 at 9:52 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Mon, Sep 12, 2016 at 1:11 AM, Dave Chinner <david@fromorbit.com> wrote:
>> On Sat, Sep 10, 2016 at 09:54:59PM +0300, Amir Goldstein wrote:
>>> On Sat, Sep 10, 2016 at 2:52 AM, Dave Chinner <david@fromorbit.com> wrote:
>>>
>>> > You can test whether this is supported at mount time, so you do a
>>> > simply flag test at copyup to determine if a clone should be
>>> > attempted or not.
>>> >
>>>
>>> I am not sure that would not be over optimization.
>>> I am already checking for the obvious reason for clone to fail in copyup -
>>> different i_sb.
>>
>> Again, please don't do that.  Call vfs_clone_file_range() as it
>> checks a whole lot more stuff that can cause a clone to fail. And it
>> makes sure that the write references to the mnt are taken so that
>> things like freeze and remount-ro behave correctly while a clone is
>> in progress.
>>
>
> OK
>

Dave,

I just sent out v2 patch series that follows your suggestions (I hope).
Please note that inside vfs_copy_file_range() I *did* add a pre-condition
of different i_sb *before* calling into ->copy_file_range().
The reason is that not all fs check for different i_sb inside the implementation
(i.e. nfs) and since I removed same i_sb constrain from vfs_copy_file_range()
I wanted to make sure that different i_sb case always ends up with
do_splice_direct()
and never propagates into the fs implementation where consequences are unknown.

Please reply on new patch set if you disagree.
Thanks,

Amir.

>>> After all, if I just call clone_file_range() once and it fails, then we are back
>>> to copying and that is going to make the cost of calling clone insignificant.
>>
>> Apart from the fact that the ->clone_file_range() calls assume that
>> all the validity checks have already been done by the caller, which
>> you are not doing.
>>
>>> > If cloning fails or is not supported, then try vfs_copy_file_range()
>>> > to do an optimised iterative partial range file copy.  Finally, try
>>> > a slow, iterative partial range file copies using
>>> > do_splice_direct(). This part can be wholly handled by
>>> > vfs_copy_file_range() - this 'not supported' fallback doesn't need
>>> > to be implemented every time someone wants to copy data between two
>>> > files...
>>>
>>> You do realize that vfs_copy_file_range() itself does the 'not
>>> supported' fallback
>>> and if I do call it iteratively, then there will be a lot more 'not
>>> supported' attempts
>>> then there are in my current patch.
>>
>> No shit, Sherlock. But you're concentrating on the wrong thing -
>> the overhead of checking if .clone_file_range/.copy_file_range is
>> implemented and can be executed is effectively zero compared to
>> copying any amount of data.
>>
>> IOWs, Amir, you're trying to *optimise the wrong thing*. It's the
>> data copy that is costly and needs to be optimised, not the
>> iteration or the checks done to determine what type of clone/copy
>> can be executed. Shortcuts around generic helpers like you are
>> proposing are more costly in the long run because code like this is
>> much more likely to contain/mask bugs that only appear months or
>> years later when something else is changed. Case in point: the mnt
>> write references that need to be taken before calling
>> clone/copy_file_range()....
>>
>> Please, just use vfs_clone_file_range() and vfs_copy_file_range()
>> and only fall back to a slower method if the error returned is
>> -EOPNOTSUPP. For any other error, the copy should fail, not be
>> ignored.
>>
>
> Obviously, you meant to check for -EOPNOTSUPP or -EXDEV
>
>>> But regardless, as I wrote to Christoph, changing the
>>> vfs_copy_file_range() helper
>>> and changing users of do_splice to use it like you suggested sounds
>>> like it may be the right thing to do, but without consensus, I am a bit hesitant
>>> to make those changes. I am definitely willing to draft the patch and test it
>>> if I get more ACKs on the idea.
>>
>> Send a patch - that's the only way you'll get anyone to comment
>> on it.
>>
>
> Will do.
>
> Thanks,
> Amir.