Re: [PATCH v2] mm/readahead: Fix large folio support in async readahead

[David Hildenbrand <david@redhat.com>]

>> Sorry, but this code is getting quite confusing, especially with such
>> misleading "large folio" comments.
>>
>> Even without MADV_HUGEPAGE we will be allocating large folios, as
>> emphasized by Willy [1]. So the only thing MADV_HUGEPAGE controls is
>> *which* large folios we allocate. .. as Willy says [2]: "We were only
>> intending to breach the 'max' for the MADV_HUGE case, not for all cases."
>>
>> I have no idea how *anybody* should derive from the code here that we
>> treat MADV_HUGEPAGE in a special way.
>>
>> Simply completely confusing.
>>
>> My interpretation of "I don't know if we should try to defend a stupid
>> sysadmin against the consequences of their misconfiguration like this"
>> means" would be "drop this patch and don't change anything".
> 
> Without this change, large folios won’t function as expected.
> Currently, to support MADV_HUGEPAGE, you’d need to set readahead_kb to
> 2MB, 4MB, or more. However, many applications run without
 > MADV_HUGEPAGE, and a larger readahead_kb might not be optimal for> them.

Someone configured: "Don't readahead more than 128KiB"

And then we complain why we "don't readahead more than 128KiB".

:)

"mm/filemap: Support VM_HUGEPAGE for file mappings" talks about "even if 
we have no history of readahead being successful".

So not about exceeding the configured limit, but exceeding the 
"readahead history".

So I consider VM_HUGEPAGE the sign here to "ignore readahead history" 
and not to "violate the config".

But that's just my opinion.

> 
>>
>> No changes to API, no confusing code.
> 
> New features like large folios can often create confusion with
> existing rules or APIs, correct?

We should not try making it even more confusing, if possible.

> 
>>
>> Maybe pr_info_once() when someone uses MADV_HUGEPAGE with such backends
>> to tell the sysadmin that something stupid is happening ...
> 
> It's not a flawed setup; it's just that this new feature doesn’t work
> well with the existing settings, and updating those settings to
> accommodate it isn't always feasible.
I don't agree. But it really is Willy's take.

The code, as it stands is confusing and nobody will be able to figure 
out how MADV_HUGEPAGE comes into play here and why we suddenly exceed 
"max/config" simply because "cur" is larger than max.

For example, in the code

ra->size = start - index;	/* old async_size */
ra->size += req_count;
ra->size = get_next_ra_size(ra, max_pages);

What happens if ra->size was at max, then we add "req_count" and 
suddenly we exceed "max" and say "well, sure that's fine now". Even if 
MADV_HUGEPAGE was never involved? Maybe it cannot happen, but it sure is 
confusing.


Not to mention that "It's worth noting that if read_ahead_kb is set to a 
larger value that isn't aligned with huge page sizes (e.g., 4MB + 
128KB), it may still fail to map to hugepages." sounds very odd :(

-- 
Cheers,

David / dhildenb