Re: [PATCH 2/2] xfs_repair: distinguish true ENOMEM from "not found" in cache_node_get

["Darrick J. Wong" <djwong@kernel.org>]

On Mon, Aug 24, 2026 at 02:59:00PM -0500, user.mail wrote:
> In the cache_node_get() callchain, we may fail in cache_node_allocate
> for two distinct reasons:
>  - The cache may be at its current size limit, and must be expanded
>  - An underlying allocation may have actually failed with ENOMEM
> 
> Today, we do not distinguish these 2 failure modes and will try to
> expand the cache size even in the face of an ENOMEM, which won't help.
> We can distinguish these two failures, because:
> 
> If cache_node_allocate fails because all slots are full, c_count is
> untouched, and it is >= c_maxcount (all slots full.)
> 
> If cache_node_allocate fails due to an underlying ENOMEM, c_count is
> decremented before return and c_count will remain < c_maxcount.
> 
> So, if the cache has been fully shaken, cache_node_allocate stil
> fails, and c_count < c_maxcount, this is a true ENOMEM situation, and
> we should give up and tell the caller.
> 
> This adds yet another return value to cache_node_get (-1) for ENOMEM
> failures, and corrects the comment about the existing 0-vs-1 returns
> - though nobody looks at that return value today.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> Signed-off-by: user.mail <sandeen@redhat.com>

Dunno who "user.mail" is but I feel like I know them from somewhere ;)

> ---
>  libxfs/cache.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/libxfs/cache.c b/libxfs/cache.c
> index f8be9b89..63224e58 100644
> --- a/libxfs/cache.c
> +++ b/libxfs/cache.c
> @@ -373,8 +373,8 @@ __cache_node_purge(
>   * hit, in which case this will all be over quickly and painlessly.
>   * Otherwise, we allocate a new node, taking care not to expand the
>   * cache beyond the requested maximum size (shrink it if it would).
> - * Returns one if hit in cache, otherwise zero.  A node is _always_
> - * returned, however.
> + * Returns zero if hit in cache, otherwise return 1 for new node;
> + * allocation failures return -1 and a null nodep.
>   */
>  int
>  cache_node_get(
> @@ -474,6 +474,18 @@ next_object:
>  		 * If we exceed CACHE_MAX_PRIORITY all slots are full; grow it.
>  		 */
>  		if (priority > CACHE_MAX_PRIORITY) {
> +			/*
> +			 * Shaking failed to make room. If the cache is not
> +			 * full, expanding the limit won't help - this is a
> +			 * genuine ENOMEM. Tell the caller.
> +			 */
> +			pthread_mutex_lock(&cache->c_mutex);
> +			if (cache->c_count < cache->c_maxcount) {
> +				pthread_mutex_unlock(&cache->c_mutex);
> +				*nodep = NULL;
> +				return -1;
> +			}
> +			pthread_mutex_unlock(&cache->c_mutex);

Also seems fine to me, but nobody checks for these -1 returns.  Also,
maybe we should be "return -ENOMEM" here or something?

--D

>  			priority = 0;
>  			if (!cache_expand(cache))
>  				goto fail;
> -- 
> 2.55.0
> 
>