Re: [PATCH 5/6] XFS: remove congestion_wait() loop from kmem_alloc()

From: Dave Chinner <david@fromorbit.com>
To: NeilBrown <neilb@suse.de>
Cc: Andrew Morton <akpm@linux-foundation.org>,
	Theodore Ts'o <tytso@mit.edu>,
	Andreas Dilger <adilger.kernel@dilger.ca>,
	"Darrick J. Wong" <djwong@kernel.org>,
	Matthew Wilcox <willy@infradead.org>,
	Mel Gorman <mgorman@suse.de>, Michal Hocko <mhocko@suse.com>,
	Jonathan Corbet <corbet@lwn.net>,
	linux-xfs@vger.kernel.org, linux-ext4@vger.kernel.org,
	linux-fsdevel@vger.kernel.org, linux-nfs@vger.kernel.org,
	linux-mm@kvack.org, linux-kernel@vger.kernel.org,
	linux-doc@vger.kernel.org
Subject: Re: [PATCH 5/6] XFS: remove congestion_wait() loop from kmem_alloc()
Date: Sat, 18 Sep 2021 07:45:28 +1000	[thread overview]
Message-ID: <20210917214528.GR2361455@dread.disaster.area> (raw)
In-Reply-To: <163184741781.29351.4475236694432020436.stgit@noble.brown>

On Fri, Sep 17, 2021 at 12:56:57PM +1000, NeilBrown wrote:
> Documentation commment in gfp.h discourages indefinite retry loops on
> ENOMEM and says of __GFP_NOFAIL that it
> 
>     is definitely preferable to use the flag rather than opencode
>     endless loop around allocator.
> 
> So remove the loop, instead specifying __GFP_NOFAIL if KM_MAYFAIL was
> not given.
> 
> As we no longer have the opportunity to report a warning after some
> failures, clear __GFP_NOWARN so that the default warning (rate-limited
> to 1 ever 10 seconds) will be reported instead.
> 
> Signed-off-by: NeilBrown <neilb@suse.de>
> ---
>  fs/xfs/kmem.c |   19 ++++++-------------
>  1 file changed, 6 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/xfs/kmem.c b/fs/xfs/kmem.c
> index 6f49bf39183c..575a58e61391 100644
> --- a/fs/xfs/kmem.c
> +++ b/fs/xfs/kmem.c
> @@ -11,21 +11,14 @@
>  void *
>  kmem_alloc(size_t size, xfs_km_flags_t flags)
>  {
> -	int	retries = 0;
>  	gfp_t	lflags = kmem_flags_convert(flags);
> -	void	*ptr;
>  
>  	trace_kmem_alloc(size, flags, _RET_IP_);
>  
> -	do {
> -		ptr = kmalloc(size, lflags);
> -		if (ptr || (flags & KM_MAYFAIL))
> -			return ptr;
> -		if (!(++retries % 100))
> -			xfs_err(NULL,
> -	"%s(%u) possible memory allocation deadlock size %u in %s (mode:0x%x)",
> -				current->comm, current->pid,
> -				(unsigned int)size, __func__, lflags);
> -		congestion_wait(BLK_RW_ASYNC, HZ/50);
> -	} while (1);
> +	if (!(flags & KM_MAYFAIL)) {
> +		lflags |= __GFP_NOFAIL;
> +		lflags &= ~__GFP_NOWARN;
> +	}

This logic should really be in kmem_flags_convert() where the gfp
flags are set up. kmem_flags_convert() is only called by
kmem_alloc() now so you should just be able to hack that logic
to do exactly what is necessary.

FWIW, We've kinda not been caring about warts in this code because
the next step for kmem_alloc is to remove kmem_alloc/kmem_zalloc
completely and replace all the callers with kmalloc/kzalloc being
passed the correct gfp flags. There's about 30 kmem_alloc() callers
and 45 kmem_zalloc() calls left to be converted before kmem.[ch] can
go away completely....

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com