Re: [PATCH v2] btrfs: do not wait for short bulk allocation

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

在 2024/4/5 06:27, David Sterba 写道:
> On Fri, Mar 29, 2024 at 06:59:57AM +1030, Qu Wenruo wrote:
>>>> This patch would only call memalloc_retry_wait() if failed to allocate
>>>> any page for tree block allocation (which goes with __GFP_NOFAIL and may
>>>> not need the special handling anyway), and reduce the latency for
>>>> btrfs_alloc_page_array().
>>>
>>> Is this saying that it can fail with GFP_NOFAIL?
>>
>> I'd say no, but never say no to memory allocation failure.
>
> It's contradicting and looks confusing in the code, either it fails or
> not.
>
>>>> Reported-by: Julian Taylor <julian.taylor@1und1.de>
>>>> Tested-by: Julian Taylor <julian.taylor@1und1.de>
>>>> Link: https://lore.kernel.org/all/8966c095-cbe7-4d22-9784-a647d1bf27c3@1und1.de/
>>>> Fixes: 395cb57e8560 ("btrfs: wait between incomplete batch memory allocations")
>>>> Reviewed-by: Filipe Manana <fdmanana@suse.com>
>>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>>> ---
>>>> Changelog:
>>>> v2:
>>>> - Still use bulk allocation function
>>>>     Since alloc_pages_bulk_array() would fall back to single page
>>>>     allocation by itself, there is no need to go alloc_page() manually.
>>>>
>>>> - Update the commit message to indicate other fses do not call
>>>>     memalloc_retry_wait() unconditionally
>>>>     In fact, they only call it when they need to retry hard and can not
>>>>     really fail.
>>>> ---
>>>>    fs/btrfs/extent_io.c | 22 +++++++++-------------
>>>>    1 file changed, 9 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
>>>> index 7441245b1ceb..c96089b6f388 100644
>>>> --- a/fs/btrfs/extent_io.c
>>>> +++ b/fs/btrfs/extent_io.c
>>>> @@ -681,31 +681,27 @@ static void end_bbio_data_read(struct btrfs_bio *bbio)
>>>>    int btrfs_alloc_page_array(unsigned int nr_pages, struct page **page_array,
>>>>    			   gfp_t extra_gfp)
>>>>    {
>>>> +	const gfp_t gfp = GFP_NOFS | extra_gfp;
>>>>    	unsigned int allocated;
>>>>
>>>>    	for (allocated = 0; allocated < nr_pages;) {
>>>>    		unsigned int last = allocated;
>>>>
>>>> -		allocated = alloc_pages_bulk_array(GFP_NOFS | extra_gfp,
>>>> -						   nr_pages, page_array);
>>>> +		allocated = alloc_pages_bulk_array(gfp, nr_pages, page_array);
>>>> +		if (unlikely(allocated == last)) {
>>>> +			/* Can not fail, wait and retry. */
>>>> +			if (extra_gfp & __GFP_NOFAIL) {
>>>> +				memalloc_retry_wait(GFP_NOFS);
>>>
>>> Can this happen? alloc_pages_bulk_array() should not fail when
>>> GFP_NOFAIL is passed, there are two allocation phases in
>>> __alloc_pages_bulk() and if it falls back to __alloc_pages() + NOFAIL it
>>> will not fail ... so what's the point of the retry?
>>
>> Yeah, that's also one of my concern.
>>
>> Unlike other fses, btrfs utilizes NOFAIL for metadata memory allocation,
>> meanwhile others do not.
>> E.g. XFS always do the retry wait even the allocation does not got a
>> page allocated. (aka, another kind of NOFAIL).
>>
>> If needed, I can drop the retry part completely.
>
> The retry logic could make sense for the normal allocations (ie. without
> NOFAIL), but as it is now it's confusing. If memory management and
> allocator says something does not fail then we should take it as such,
> same what we do with bioset allocations of bios and do not expect them
> to fail.

Got it, would remove the retry part completely.

Thanks,
Qu