[PATCH 0/2 V2] xfs_repair: improve caching in the face of ENOMEM

[PATCH 0/2 V2] xfs_repair: improve caching in the face of ENOMEM

[Eric Sandeen]

V2:
- Flip the order of the patches; put the bugfix first and the
  cache expansion guard second, at which point it's just defensive.

- Be more explicit about an enomem return due to a real allocation
  failure; don't try to figure that out indirectly as before.

- Actually care about the return values as pointed out by djwong.

There are problems with how we react to cache allocation failures that
are more apparent before:
6b6239689e36 ("libxfs: unmap xmbuf pages to avoid disaster")
landed, but with enough pressure they show up again.

Two problems: cache_expand has no bounds checking, so call it enough
and c_maxcount will overflow, which typically results in an infinite
loop.

That can be triggered by the fact that today, an underlying ENOMEM
can look like a "cache too small" return, so we double when that
won't help at all. Do that enough times and kaboom.

I can trigger this with something like:

mkfs.xfs -d file,name=testfile.img,agcount=10000,size=1t
echo 1000 > /proc/sys/vm/max_map_count
xfs_repair -n -P -m 1280 testfile.img

to force an ENOMEM from the max map count, though the actual
max_map_count value needed varies depending on the machine I'm on,
for some reason.

These were quickly tested with ./check -g quick

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

[Eric Sandeen]

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)
 {
 	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;
 		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

[PATCH 2/2] xfs_repair: do not allow cache growth to overflow

[Eric Sandeen]

Nothing stops cache_expand() from growing c_maxcount (via *2)
repeatedly until it overflows.

Add a bounds check here and return failure if for any reason we try to
grow too much.

With the prior fixes, we should never get this far, but it's worth
defending against anyway.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---
 libxfs/cache.c | 30 +++++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 7 deletions(-)

diff --git a/libxfs/cache.c b/libxfs/cache.c
index 21e4c0c0..2339fa7d 100644
--- a/libxfs/cache.c
+++ b/libxfs/cache.c
@@ -79,16 +79,28 @@ cache_init(
 	return cache;
 }
 
-static void
+/*
+ * Double the cache size limit, and return success (or not)
+ */
+static bool
 cache_expand(
 	struct cache *		cache)
 {
+	bool			expanded = false;
+
 	pthread_mutex_lock(&cache->c_mutex);
+	/* do not overflow c_maxcount */
+	if (cache->c_maxcount <= UINT_MAX / 2) {
 #ifdef CACHE_DEBUG
-	fprintf(stderr, "doubling cache size to %u\n", 2 * cache->c_maxcount);
+		fprintf(stderr, "doubling cache size to %u\n",
+			2 * cache->c_maxcount);
 #endif
-	cache->c_maxcount *= 2;
+		cache->c_maxcount *= 2;
+		expanded = true;
+	}
 	pthread_mutex_unlock(&cache->c_mutex);
+
+	return expanded;
 }
 
 void
@@ -378,9 +390,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 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.
+ * Returns zero if hit in cache, one if a new node was allocated, or
+ * -1 if a new node cannot be obtained.
  */
 int
 cache_node_get(
@@ -494,8 +505,13 @@ next_object:
 				ret = -1;
 				goto out;
 			}
+			/* Fail if we can't expand the cache any futher. */
+			if (!cache_expand(cache)) {
+				node = NULL;
+				ret = -1;
+				goto out;
+			}
 			priority = 0;
-			cache_expand(cache);
 		}
 	}
 
-- 
2.55.0

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

[Darrick J. Wong]

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
> 
> 

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

[Eric Sandeen]

On 9/4/26 11:20 AM, Darrick J. Wong wrote:
> 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.

yeah I looked for ERR_PTR and we don't really do that.

> 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?

As you wish :)

...

> 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.
OK, I can see how that looks. Dragged into some other work for a while
now, though. :(

-Eric