Re: [PATCH 02/11] xfs: split xfs_buf_allocate_memory

From: Dave Chinner <david@fromorbit.com>
To: Christoph Hellwig <hch@lst.de>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 02/11] xfs: split xfs_buf_allocate_memory
Date: Thu, 20 May 2021 08:36:32 +1000	[thread overview]
Message-ID: <20210519223632.GC664593@dread.disaster.area> (raw)
In-Reply-To: <20210519190900.320044-3-hch@lst.de>

On Wed, May 19, 2021 at 09:08:51PM +0200, Christoph Hellwig wrote:
> Split xfs_buf_allocate_memory into one helper that allocates from
> slab and one that allocates using the page allocator.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
....
> +static int
> +xfs_buf_alloc_slab(
> +	struct xfs_buf		*bp,
> +	unsigned int		flags)
> +{

xfs_buf_alloc_kmem() or xfs_buf_alloc_heap() would be better, I
think, because it matches the flag used to indicate how the memory
associated with the buffer was allocated.

> @@ -720,9 +717,17 @@ xfs_buf_get_map(
>  	if (error)
>  		return error;
>  
> -	error = xfs_buf_allocate_memory(new_bp, flags);
> -	if (error)
> -		goto out_free_buf;
> +	/*
> +	 * For buffers that are contained within a single page, just allocate
> +	 * the memory from the heap - there's no need for the complexity of
> +	 * page arrays to keep allocation down to order 0.
> +	 */
> +	if (BBTOB(new_bp->b_length) >= PAGE_SIZE ||
> +	    xfs_buf_alloc_slab(new_bp, flags) < 0) {
> +		error = xfs_buf_alloc_pages(new_bp, flags);
> +		if (error)
> +			goto out_free_buf;
> +	}

Took me a moment to grok the logic pattern here, then I realised the
comment didn't help as it makes no indication that the heap
allocation is best effort and will fall back to pages. A small tweak
like:

	/*
	 * For buffers that fit entirely within a single page, first
	 * attempt to allocate the memory from the heap to minimise
	 * memory usage. If we can't get heap memory for these small
	 * buffers, we fall back to using the page allocator.
	 */

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com