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

From: Luis Henriques
Date: Fri Feb 12 2021 - 10:01:39 EST


Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> writes:

> On Fri, Feb 12, 2021 at 12:41:48PM +0000, Luis Henriques wrote:
>> Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> writes:
...
>> >> >> Our option now are:
>> >> >> - Restore the cross-fs restriction into generic_copy_file_range()
>> >> >
>> >> > Yes.
>> >> >
>> >>
>> >> Restoring this restriction will actually change the current cephfs CFR
>> >> behaviour. Since that commit we have allowed doing remote copies between
>> >> different filesystems within the same ceph cluster. See commit
>> >> 6fd4e6348352 ("ceph: allow object copies across different filesystems in
>> >> the same cluster").
>> >>
>> >> Although I'm not aware of any current users for this scenario, the
>> >> performance impact can actually be huge as it's the difference between
>> >> asking the OSDs for copying a file and doing a full read+write on the
>> >> client side.
>> >
>> > Regression in performance is ok if it fixes a regression for things that
>> > used to work just fine in the past :)
>> >
>> > First rule, make it work.
>>
>> 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);
>> }
>>
>> /*
>
> That would make much more sense to me.

Great. I can send a proper patch with changelog, if this is the really
what we want. But I would rather hear from others first. I guess that at
least the NFS devs have something to say here.

Cheers,
--
Luis