Re: [PATCH 1/6] fs: Add flag to file_system_type to indicate content is generated

[Luis Henriques]

Luis Henriques <lhenriques@xxxxxxx> writes:

> Amir Goldstein <amir73il@xxxxxxxxx> writes:
>
>> On Fri, Feb 12, 2021 at 2:40 PM Luis Henriques <lhenriques@xxxxxxx> wrote:
> ...
>>> Sure, I just wanted to point out that *maybe* there are other options than
>>> simply reverting that commit :-)
>>>
>>> Something like the patch below (completely untested!) should revert to the
>>> old behaviour in filesystems that don't implement the CFR syscall.
>>>
>>> Cheers,
>>> --
>>> Luis
>>>
>>> diff --git a/fs/read_write.c b/fs/read_write.c
>>> index 75f764b43418..bf5dccc43cc9 100644
>>> --- a/fs/read_write.c
>>> +++ b/fs/read_write.c
>>> @@ -1406,8 +1406,11 @@ static ssize_t do_copy_file_range(struct file *file_in, loff_t pos_in,
>>>                                                        file_out, pos_out,
>>>                                                        len, flags);
>>>
>>> -       return generic_copy_file_range(file_in, pos_in, file_out, pos_out, len,
>>> -                                      flags);
>>> +       if (file_inode(file_in)->i_sb != file_inode(file_out)->i_sb)
>>> +               return -EXDEV;
>>> +       else
>>> +               generic_copy_file_range(file_in, pos_in, file_out, pos_out, len,
>>> +                                       flags);
>>>  }
>>>
>>
>> Which kernel is this patch based on?
>
> It was v5.11-rc7.
>
>> At this point, I am with Dave and Darrick on not falling back to
>> generic_copy_file_range() at all.
>>
>> We do not have proof of any workload that benefits from it and the
>> above patch does not protect from a wierd use case of trying to copy a file
>> from sysfs to sysfs.
>>
>
> Ok, cool.  I can post a new patch doing just that.  I guess that function
> do_copy_file_range() can be dropped in that case.
>
>> I am indecisive about what should be done with generic_copy_file_range()
>> called as fallback from within filesystems.
>>
>> I think the wise choice is to not do the fallback in any case, but this is up
>> to the specific filesystem maintainers to decide.
>
> I see what you mean.  You're suggesting to have userspace handle all the
> -EOPNOTSUPP and -EXDEV errors.  Would you rather have a patch that also
> removes all the calls to generic_copy_file_range() function?  And that
> function can also be deleted too, of course.

Here's a first stab at this patch.  Hopefully I didn't forgot anything
here.  Let me know if you prefer the more conservative approach, i.e. not
touching any of the filesystems and let them use generic_copy_file_range.

Once everyone agrees on the final solution, I can follow-up with the
manpages update.

Cheers,
-- 
Luis