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.
