On Tue, Sep 13, 2016 at 3:11 AM, Dave Chinner <david@xxxxxxxxxxxxx> wrote:
> On Mon, Sep 12, 2016 at 06:06:43PM +0300, Amir Goldstein wrote:
>> Use vfs_copy_file_range() helper instead of calling do_splice_direct()
>> when copying up file data.
>> When copying up within the same fs, which supports copy_file_range(),
>> fs implementation can be more efficient then do_splice_direct().
>> vfs_copy_file_range() helper falls back to do_splice_direct() if
>> it cannot use the file system's copy_file_range() implementation.
>>
>> 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.
>
> This is now stale, right? the reflink is done from the
> vfs_clone_file_range() call added in an earlier patch, not this
> change....
Well.. that depends on the definition of "now"
as patch 4/4 you are correct, but as I wrote in the cover letter
I tested patches 3,4 independently of patches 1,2
otherwise, patches 3,4 would be adding moot code
or rather a simple re-factoring.
So now it boils down to a process question:
should patches 3,4 be merged, even though no one can properly test the
'fallback to chunk copy range' case
with any of the in-tree filesystems (without modifying them first)?
I could be presenting patch 4 as a simple re-factoring
(use vfs copy range helper instead of using do_splice_direct)
which makes for a cleaner code.
But the fact that it changes behavior in a subtle way
(hopefully for the best) without anyone noticing this change far into
the future is somewhat troubling.
Your thoughts?
>
>> Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
>> ---
>> fs/overlayfs/copy_up.c | 9 +++++----
>> 1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
>> index e432d7e..32ea54f 100644
>> --- a/fs/overlayfs/copy_up.c
>> +++ b/fs/overlayfs/copy_up.c
>> @@ -159,15 +159,16 @@ static int ovl_copy_up_data(struct path *old, struct path *new, loff_t len)
>> break;
>> }
>>
>> - bytes = do_splice_direct(old_file, &old_pos,
>> - new_file, &new_pos,
>> - this_len, SPLICE_F_MOVE);
>> + bytes = vfs_copy_file_range(old_file, old_pos,
>> + new_file, new_pos,
>> + this_len, 0);
>> if (bytes <= 0) {
>> error = bytes;
>> break;
>> }
>> - WARN_ON(old_pos != new_pos);
>>
>> + old_pos += bytes;
>> + new_pos += bytes;
>> len -= bytes;
>> }
>
> Patch does not remove the
>
> /* FIXME: copy up sparse files efficiently */
>
> comment. Efficient copying of sparse files is something
> vfs_copy_file_range() should do internally....
Moved the comment into vfs_copy_file_range()
Cheers,
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
