Re: [RFC] mm: alloc_pages_bulk: remove assumption of populating only NULL elements

[Yunsheng Lin <linyunsheng@huawei.com>]

On 2025/2/18 5:31, Dave Chinner wrote:

...

> .....
> 
>> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
>> index 15bb790359f8..9e1ce0ab9c35 100644
>> --- a/fs/xfs/xfs_buf.c
>> +++ b/fs/xfs/xfs_buf.c
>> @@ -377,16 +377,17 @@ xfs_buf_alloc_pages(
>>  	 * least one extra page.
>>  	 */
>>  	for (;;) {
>> -		long	last = filled;
>> +		long	alloc;
>>  
>> -		filled = alloc_pages_bulk(gfp_mask, bp->b_page_count,
>> -					  bp->b_pages);
>> +		alloc = alloc_pages_bulk(gfp_mask, bp->b_page_count - refill,
>> +					 bp->b_pages + refill);
>> +		refill += alloc;
>>  		if (filled == bp->b_page_count) {
>>  			XFS_STATS_INC(bp->b_mount, xb_page_found);
>>  			break;
>>  		}
>>  
>> -		if (filled != last)
>> +		if (alloc)
>>  			continue;
> 
> You didn't even compile this code - refill is not defined
> anywhere.
> 
> Even if it did complile, you clearly didn't test it. The logic is
> broken (what updates filled?) and will result in the first
> allocation attempt succeeding and then falling into an endless retry
> loop.

Ah, the 'refill' is a typo, it should be 'filled' instead of 'refill'.
The below should fix the compile error:
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -379,9 +379,9 @@ xfs_buf_alloc_pages(
        for (;;) {
                long    alloc;

-               alloc = alloc_pages_bulk(gfp_mask, bp->b_page_count - refill,
-                                        bp->b_pages + refill);
-               refill += alloc;
+               alloc = alloc_pages_bulk(gfp_mask, bp->b_page_count - filled,
+                                        bp->b_pages + filled);
+               filled += alloc;
                if (filled == bp->b_page_count) {
                        XFS_STATS_INC(bp->b_mount, xb_page_found);
                        break;

> 
> i.e. you stepped on the API landmine of your own creation where
> it is impossible to tell the difference between alloc_pages_bulk()
> returning "memory allocation failed, you need to retry" and
> it returning "array is full, nothing more to allocate". Both these
> cases now return 0.

As my understanding, alloc_pages_bulk() will not be called when
"array is full" as the above 'filled == bp->b_page_count' checking
has ensured that if the array is not passed in with holes in the
middle for xfs.

> 
> The existing code returns nr_populated in both cases, so it doesn't
> matter why alloc_pages_bulk() returns with nr_populated != full, it
> is very clear that we still need to allocate more memory to fill it.

I am not sure if the array will be passed in with holes in the
middle for the xfs fs as mentioned above, if not, it seems to be
a typical use case like the one in mempolicy.c as below:

https://elixir.bootlin.com/linux/v6.14-rc1/source/mm/mempolicy.c#L2525

> 
> The whole point of the existing API is to prevent callers from
> making stupid, hard to spot logic mistakes like this. Forcing
> callers to track both empty slots and how full the array is itself,
> whilst also constraining where in the array empty slots can occur
> greatly reduces both the safety and functionality that
> alloc_pages_bulk() provides. Anyone that has code that wants to
> steal a random page from the array and then refill it now has a heap
> more complex code to add to their allocator wrapper.

Yes, I am agreed that it might be better to provide a common API or
wrapper if there is some clear use case that need to pass in an array
with holes in the middle by adding a new API like refill_pages_bulk()
as below.

> 
> IOWs, you just demonstrated why the existing API is more desirable
> than a highly constrained, slightly faster API that requires callers
> to get every detail right. i.e. it's hard to get it wrong with the
> existing API, yet it's so easy to make mistakes with the proposed
> API that the patch proposing the change has serious bugs in it.

IMHO, if the API is about refilling pages for the only NULL elements,
it seems better to add a API like refill_pages_bulk() for that, as
the current API seems to be prone to error of not initializing the
array to zero before calling alloc_pages_bulk().

> 
> -Dave.