Re: [PATCH 15/32] btrfs: introduce a helper to determine if the sectorsize is smaller than PAGE_SIZE

[Qu Wenruo <wqu@suse.com>]

On 2020/11/11 上午9:34, Qu Wenruo wrote:
> 
> 
> On 2020/11/10 下午10:53, David Sterba wrote:
>> On Sat, Nov 07, 2020 at 08:00:26AM +0800, Qu Wenruo wrote:
>>>>>>> +static inline bool btrfs_is_subpage(struct btrfs_fs_info *fs_info)
>>>>>>> +{
>>>>>>> +	return (fs_info->sectorsize < PAGE_SIZE);
>>>>>>> +}
>>>>>>
>>>>>> This is conceptually wrong. The filesystem shouldn't care whether we are
>>>>>> diong subpage blocksize io or not. I.e it should be implemented in such
>>>>>> a way so that everything " just works". All calculation should be
>>>>>> performed based on the fs_info::sectorsize and we shouldn't care what
>>>>>> the value of PAGE_SIZE is. The central piece becomes sectorsize.
>>>>>
>>>>> Nope, as long as we're using things like bio, we can't avoid the
>>>>> restrictions from page.
>>>>>
>>>>> I can't get your point at all, I see nothing wrong here, especially when
>>>>> we still need to handle page lock for a lot of things.
>>>>>
>>>>> Furthermore, this thing is only used inside btrfs, how could this be
>>>>> *conectpionally* wrong?
>>>>
>>>> As Nik said, it should be built around sectorsize (even if some other
>>>> layers work with pages or bios). Conceptually wrong is adding special
>>>> cases instead of generalizing or abstracting the code so it also
>>>> supports pagesize != sectorsize.
>>>>
>>> Really? For later patches you will see some unavoidable difference anyway.
>>
>> Yeah some of the new sector/page combinations will need some thinking
>> how to handle them without sacrificing code quality.
>>
>>> One example is page->private for metadata.
>>> For regular case, page-private is a pointer to eb, which is never
>>> feasible for subpage case.
>>>
>>> It's OK to be ideal, but not OK to be too ideal.
>>
>> I'm always trying to take the practical approach because with a long
>> development period and with many people contributing and with doing too
>> many compromises the code becomes way below the ideal. You may have
>> heared yourself or others bitching about some old code, but we have
>> enough group knowledge and experience not to let bad coding patterns
>> continue coming back once painfully cleaned up.
>>
> Yeah, I totally understand that.
> 
> But here we have to do trade-off call for page->private anyway.
> 
> Either we:
> - Do special handling for btrfs subpage support
>   This means, for subpage, page->private will be handled specially,
>   while regular page size will stay mostly the same.
>   This doesn't touch the existing behavior, except one extra if () check
>   on certain low-level functions.
> 
>   For subpage, page->private will be used for extra info, like various
>   bitmap, and reader/writer counts. Just like iomap_page.
>   This would be the "code quality" impact.
> 
> - Do no special handling, unifying to subpage behavior
>   This means, we will allocate extra memory for each data page no matter
>   the page size/sector size combination.
>   Obviously, it would cost extra memory usage for each data page.
>   And if we had any bug in subpage support, no one can survive.
> 
> Thus I take the poison of the first method.
> Also to reduce the impact, all btrfs_is_subpage() check is in some lower
> level function.
> You won't see much btrfs_is_subpage() check in some common functions,
> but all hidden in some helpers.
> 
> I doubt if this would really impact the code quality.

The current example for such btrfs_is_subpage() usage can be found here:
https://github.com/adam900710/linux/commit/887779f8c0a64a6c7ad6f34911aaf88c9f6901bb

The related functions, begin_page_read() and end_page_read() are the the
main function to handle subpage and regular cases differently.

Please take a look at it to see if it's acceptable or not.

Thanks,
Qu

> 
> Thanks,
> Qu
>