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

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

On Thu, Sep 03, 2026 at 01:42:43PM -0500, Eric Sandeen wrote:
> When cache_node_get() needs a new node it calls cache_node_allocate(),
> which returns NULL for two very different reasons:
> 
>  - the cache is already full (c_count has reached c_maxcount), or
>  - an underlying allocation actually failed with ENOMEM.
> 
> These require different responses. A full cache should be shaken to free
> slots and, failing that, expanded. An actual ENOMEM should be returned
> to the caller to deal with.
> 
> To achieve this, make cache_node_allocate() say which case occurred via
> a new *enomem parameter, set true only for a real allocation failure.
> Cache shaking still happens before returning ENOMEM, as this may actually
> free memory and allow new allocations to succeed.
> 
> Also fix the stale return-value comment, which had the hit/new-node
> cases backwards.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
>  libxfs/cache.c | 39 ++++++++++++++++++++++++++++++++++-----
>  1 file changed, 34 insertions(+), 5 deletions(-)
> 
> diff --git a/libxfs/cache.c b/libxfs/cache.c
> index d5d9ba56..21e4c0c0 100644
> --- a/libxfs/cache.c
> +++ b/libxfs/cache.c
> @@ -284,15 +284,26 @@ cache_shake(
>  /*
>   * Allocate a new hash node (updating atomic counter in the process),
>   * unless doing so will push us over the maximum cache size.
> + *
> + * Returns NULL on failure.  A NULL return can mean two different things:
> + * either the cache is already full (c_count has reached c_maxcount), or
> + * the underlying allocation genuinely failed with ENOMEM.  These call for
> + * very different responses from the caller, so report which one happened
> + * via *enomem: it is set true only for a real allocation failure, and
> + * left false when we cannot allocate another node in the current cache
> + * size.
>   */
>  static struct cache_node *
>  cache_node_allocate(
>  	struct cache *		cache,
> -	cache_key_t		key)
> +	cache_key_t		key,
> +	bool			*enomem)

Hrmm.  Having ENOMEM as an outparam is a little weird, but I suppose we
can't do the ERR_PTR stuff that the kernel does.

OTOH I guess you could set errno here and redefine the return value as:

"This function returns a pointer to a cache object in the case of a hit.
For a miss, it returns NULL with errno set to 0.  For any other runtime
error, it returns NULL with errno set to a nonzero value."

Hrm?

>  {
>  	unsigned int		nodesfree;
>  	struct cache_node *	node;
>  
> +	*enomem = false;
> +
>  	pthread_mutex_lock(&cache->c_mutex);
>  	nodesfree = (cache->c_count < cache->c_maxcount);
>  	if (nodesfree) {
> @@ -309,6 +320,7 @@ cache_node_allocate(
>  		pthread_mutex_lock(&cache->c_mutex);
>  		cache->c_count--;
>  		pthread_mutex_unlock(&cache->c_mutex);
> +		*enomem = true;
>  		return NULL;
>  	}
>  	pthread_mutex_init(&node->cn_mutex, NULL);
> @@ -366,8 +378,9 @@ __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, one if a new node was allocated.  If
> + * allocation fails with a genuine ENOMEM we return -1 with a NULL
> + * *nodep; in every other case a node is returned.
>   */
>  int
>  cache_node_get(
> @@ -384,6 +397,8 @@ cache_node_get(
>  	unsigned int		hashidx;
>  	int			priority = 0;
>  	int			purged = 0;
> +	int			ret;
> +	bool			enomem = false;
>  
>  	hashidx = cache->hash(key, cache->c_hashsize, cache->c_hashshift);
>  	hash = cache->c_hash + hashidx;
> @@ -457,7 +472,7 @@ next_object:
>  		/*
>  		 * not found, allocate a new entry
>  		 */
> -		node = cache_node_allocate(cache, key);
> +		node = cache_node_allocate(cache, key, &enomem);
>  		if (node)
>  			break;

I guess the awkward part here is that you then need extra stupid
variables and logic:

		switch (errno) {
		case ENOMEM:
			enomem = true;
			break;
		case 0:
			break;
		default:
			/* what the heck?? */
			break;
		}

I think I'd almost rather we change cache_node_allocate to return int
and the cache node as an outparam since we do that elsewhere, but
<shrug> I'll defer to Andrey.

--D

>  		priority = cache_shake(cache, priority, false);
> @@ -467,6 +482,18 @@ next_object:
>  		 * If we exceed CACHE_MAX_PRIORITY all slots are full; grow it.
>  		 */
>  		if (priority > CACHE_MAX_PRIORITY) {
> +			/*
> +			 * We've shaken every priority level and still can't
> +			 * allocate.  If the last attempt failed with a real
> +			 * ENOMEM rather than simply a full cache, then neither
> +			 * shaking nor growing the cache can help - give up and
> +			 * let the caller handle the failure.
> +			 */
> +			if (enomem) {
> +				node = NULL;
> +				ret = -1;
> +				goto out;
> +			}
>  			priority = 0;
>  			cache_expand(cache);
>  		}
> @@ -479,7 +506,9 @@ next_object:
>  	hash->ch_count++;
>  	list_add(&node->cn_hash, &hash->ch_list);
>  	pthread_mutex_unlock(&hash->ch_mutex);
> +	ret = 1;
>  
> +out:
>  	if (purged) {
>  		pthread_mutex_lock(&cache->c_mutex);
>  		cache->c_count -= purged;
> @@ -487,7 +516,7 @@ next_object:
>  	}
>  
>  	*nodep = node;
> -	return 1;
> +	return ret;
>  }
>  
>  void
> -- 
> 2.55.0
> 
>