Received-SPF: pass (google.com: domain of linux-kernel+bounces-107861-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Tue, 19 Mar 2024 16:46:06 +0000
From: Matthew Wilcox <willy@infradead.org>
To: Kairui Song <kasong@tencent.com>
Cc: linux-mm@kvack.org, Andrew Morton <akpm@linux-foundation.org>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/4] mm/filemap: return early if failed to allocate
 memory for split
Message-ID: <ZfnBTnXTIGck4lVR@casper.infradead.org>
References: <20240319092733.4501-1-ryncsn@gmail.com>
 <20240319092733.4501-2-ryncsn@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240319092733.4501-2-ryncsn@gmail.com>

On Tue, Mar 19, 2024 at 05:27:30PM +0800, Kairui Song wrote:
> From: Kairui Song <kasong@tencent.com>
> 
> xas_split_alloc could fail with NOMEM, and in such case, it should abort
> early instead of keep going and fail the xas_split below.
> 
> Signed-off-by: Kairui Song <kasong@tencent.com>

The usual way of programming with an xa_state is to not do any error
handling; the xas_ functions check for error and become no-ops, so we
only have to check at the end.

I think this case is worth making an exception for because we avoid
acquiring/releasing the lock.  Not that we should be getting ENOMEM
very often; we only allocate about 8 nodes usually and we get 7
nodes/page.

Acked-by: Matthew Wilcox (Oracle) <willy@infradead.org>

>  mm/filemap.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 7437b2bd75c1..f07ea0b97698 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -880,9 +880,12 @@ noinline int __filemap_add_folio(struct address_space *mapping,
>  		unsigned int order = xa_get_order(xas.xa, xas.xa_index);
>  		void *entry, *old = NULL;
>  
> -		if (order > folio_order(folio))
> +		if (order > folio_order(folio)) {
>  			xas_split_alloc(&xas, xa_load(xas.xa, xas.xa_index),
>  					order, gfp);
> +			if (xas_error(&xas))
> +				goto error;
> +		}
>  		xas_lock_irq(&xas);
>  		xas_for_each_conflict(&xas, entry) {
>  			old = entry;
> -- 
> 2.43.0
>