Re: [PATCH v10 01/10] fs: Allow fine-grained control of folio sizes

[Ryan Roberts]

On 17/07/2024 10:46, Pankaj Raghav (Samsung) wrote:
> On Tue, Jul 16, 2024 at 04:26:10PM +0100, Matthew Wilcox wrote:
>> On Mon, Jul 15, 2024 at 11:44:48AM +0200, Pankaj Raghav (Samsung) wrote:
>>> +/*
>>> + * mapping_max_folio_size_supported() - Check the max folio size supported
>>> + *
>>> + * The filesystem should call this function at mount time if there is a
>>> + * requirement on the folio mapping size in the page cache.
>>> + */
>>> +static inline size_t mapping_max_folio_size_supported(void)
>>> +{
>>> +	if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE))
>>> +		return 1U << (PAGE_SHIFT + MAX_PAGECACHE_ORDER);
>>> +	return PAGE_SIZE;
>>> +}
>>
>> There's no need for this to be part of this patch.  I've removed stuff
>> from this patch before that's not needed, please stop adding unnecessary
>> functions.  This would logically be part of patch 10.
> 
> That makes sense. I will move it to the last patch.
> 
>>
>>> +static inline void mapping_set_folio_order_range(struct address_space *mapping,
>>> +						 unsigned int min,
>>> +						 unsigned int max)
>>> +{
>>> +	if (!IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE))
>>> +		return;
>>> +
>>> +	if (min > MAX_PAGECACHE_ORDER) {
>>> +		VM_WARN_ONCE(1,
>>> +	"min order > MAX_PAGECACHE_ORDER. Setting min_order to MAX_PAGECACHE_ORDER");
>>> +		min = MAX_PAGECACHE_ORDER;
>>> +	}
>>
>> This is really too much.  It's something that will never happen.  Just
>> delete the message.
>>
>>> +	if (max > MAX_PAGECACHE_ORDER) {
>>> +		VM_WARN_ONCE(1,
>>> +	"max order > MAX_PAGECACHE_ORDER. Setting max_order to MAX_PAGECACHE_ORDER");
>>> +		max = MAX_PAGECACHE_ORDER;
>>
>> Absolutely not.  If the filesystem declares it can support a block size
>> of 4TB, then good for it.  We just silently clamp it.
> 
> Hmm, but you raised the point about clamping in the previous patches[1]
> after Ryan pointed out that we should not silently clamp the order.
> 
> ```
>> It seems strange to silently clamp these? Presumably for the bs>ps usecase,
>> whatever values are passed in are a hard requirement? So wouldn't want them to
>> be silently reduced. (Especially given the recent change to reduce the size of
>> MAX_PAGECACHE_ORDER to less then PMD size in some cases).
> 
> Hm, yes.  We should probably make this return an errno.  Including
> returning an errno for !IS_ENABLED() and min > 0.
> ```
> 
> It was not clear from the conversation in the previous patches that we
> decided to just clamp the order (like it was done before).
> 
> So let's just stick with how it was done before where we clamp the
> values if min and max > MAX_PAGECACHE_ORDER?
> 
> [1] https://lore.kernel.org/linux-fsdevel/Zoa9rQbEUam467-q@xxxxxxxxxxxxxxxxxxxx/

The way I see it, there are 2 approaches we could take:

1. Implement mapping_max_folio_size_supported(), write a headerdoc for
mapping_set_folio_order_range() that says min must be lte max, max must be lte
mapping_max_folio_size_supported(). Then emit VM_WARN() in
mapping_set_folio_order_range() if the constraints are violated, and clamp to
make it safe (from page cache's perspective). The VM_WARN()s can just be inline
in the if statements to keep them clean. The FS is responsible for checking
mapping_max_folio_size_supported() and ensuring min and max meet requirements.

2. Return an error from mapping_set_folio_order_range() (and the other functions
that set min/max). No need for warning. No state changed if error is returned.
FS can emit warning on error if it wants.

Personally I prefer option 2, but 1 is definitely less churn.

Thanks,
Ryan