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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Sep 12, 2016 at 1:11 AM, Dave Chinner <david@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> 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

>> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Filesystems Devel]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux