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

From: David Sterba <dsterba@suse.cz>
To: Qu Wenruo <quwenruo.btrfs@gmx.com>
Cc: dsterba@suse.cz, Qu Wenruo <wqu@suse.com>,
	linux-btrfs@vger.kernel.org,
	Julian Taylor <julian.taylor@1und1.de>,
	Filipe Manana <fdmanana@suse.com>
Subject: Re: [PATCH v2] btrfs: do not wait for short bulk allocation
Date: Thu, 4 Apr 2024 21:57:10 +0200	[thread overview]
Message-ID: <20240404195710.GL14596@twin.jikos.cz> (raw)
In-Reply-To: <ec65443a-b7ba-4c60-9cbd-23ffd45c8994@gmx.com>

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.