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

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

[user.mail]

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: do not allow cache growth to overflow

[user.mail]

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.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
Signed-off-by: user.mail <sandeen@redhat.com>
---
 libxfs/cache.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/libxfs/cache.c b/libxfs/cache.c
index d5d9ba56..f8be9b89 100644
--- a/libxfs/cache.c
+++ b/libxfs/cache.c
@@ -79,16 +79,23 @@ cache_init(
 	return cache;
 }
 
-static void
+static int
 cache_expand(
 	struct cache *		cache)
 {
+	int			success = 1;
 	pthread_mutex_lock(&cache->c_mutex);
+	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;
+	} else
+		success = 0;
 	pthread_mutex_unlock(&cache->c_mutex);
+
+	return success;
 }
 
 void
@@ -468,7 +475,8 @@ next_object:
 		 */
 		if (priority > CACHE_MAX_PRIORITY) {
 			priority = 0;
-			cache_expand(cache);
+			if (!cache_expand(cache))
+				goto fail;
 		}
 	}
 
@@ -488,6 +496,10 @@ next_object:
 
 	*nodep = node;
 	return 1;
+
+fail:
+	*nodep = NULL;
+	return -1;
 }
 
 void
-- 
2.55.0

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

[user.mail]

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>
---
 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);
 			priority = 0;
 			if (!cache_expand(cache))
 				goto fail;
-- 
2.55.0

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

[Darrick J. Wong]

On Mon, Aug 24, 2026 at 02:58:59PM -0500, user.mail wrote:
> 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.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> Signed-off-by: user.mail <sandeen@redhat.com>
> ---
>  libxfs/cache.c | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/libxfs/cache.c b/libxfs/cache.c
> index d5d9ba56..f8be9b89 100644
> --- a/libxfs/cache.c
> +++ b/libxfs/cache.c
> @@ -79,16 +79,23 @@ cache_init(
>  	return cache;
>  }
>  
> -static void
> +static int
>  cache_expand(
>  	struct cache *		cache)
>  {
> +	int			success = 1;
>  	pthread_mutex_lock(&cache->c_mutex);
> +	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;

I agree that it's useful not to overflow c_maxcount, but how did you end
up with that many cached buffers?  Or is this LLM-inspired bugfixes?

Either way I'm ok with this part, so...
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

> +	} else
> +		success = 0;
>  	pthread_mutex_unlock(&cache->c_mutex);
> +
> +	return success;
>  }
>  
>  void
> @@ -468,7 +475,8 @@ next_object:
>  		 */
>  		if (priority > CACHE_MAX_PRIORITY) {
>  			priority = 0;
> -			cache_expand(cache);
> +			if (!cache_expand(cache))
> +				goto fail;
>  		}
>  	}
>  
> @@ -488,6 +496,10 @@ next_object:
>  
>  	*nodep = node;
>  	return 1;
> +
> +fail:
> +	*nodep = NULL;
> +	return -1;

...but a general comment about this patch and the next one: it seems
reasonable to make cache_node_get return a null nodep and -1 to indicate
error, but AFAICT the only caller is __cache_lookup (aka the buffer
cache), which doesn't check the return value at all, only the value of
*nodep.

I don't see a change in this series to change __cache_lookup.  Did that
get lost somewhere?

--D

>  }
>  
>  void
> -- 
> 2.55.0
> 
> 

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

[Darrick J. Wong]

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

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

[Eric Sandeen]

On 8/24/26 10:17 PM, Darrick J. Wong wrote:
> On Mon, Aug 24, 2026 at 02:58:59PM -0500, user.mail wrote:
>> 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.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> Signed-off-by: user.mail <sandeen@redhat.com>
>> ---
>>  libxfs/cache.c | 20 ++++++++++++++++----
>>  1 file changed, 16 insertions(+), 4 deletions(-)
>>
>> diff --git a/libxfs/cache.c b/libxfs/cache.c
>> index d5d9ba56..f8be9b89 100644
>> --- a/libxfs/cache.c
>> +++ b/libxfs/cache.c
>> @@ -79,16 +79,23 @@ cache_init(
>>  	return cache;
>>  }
>>  
>> -static void
>> +static int
>>  cache_expand(
>>  	struct cache *		cache)
>>  {
>> +	int			success = 1;
>>  	pthread_mutex_lock(&cache->c_mutex);
>> +	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;
> 
> I agree that it's useful not to overflow c_maxcount, but how did you end
> up with that many cached buffers?  Or is this LLM-inspired bugfixes?

See the cover letter (sorta) ;) An ENOMEM from lower layers is indistinguishable
from "cache too small" so when we ran out of memory it rapidly doubled
its way to overflow.

In a quaint throwback to simpler times, this was actually observed by a user.

I think it was actually a low max_map_count in a small vm and a large
filesystem, though I don't recall the exact details now.

> Either way I'm ok with this part, so...
> Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
> 
>> +	} else
>> +		success = 0;
>>  	pthread_mutex_unlock(&cache->c_mutex);
>> +
>> +	return success;
>>  }
>>  
>>  void
>> @@ -468,7 +475,8 @@ next_object:
>>  		 */
>>  		if (priority > CACHE_MAX_PRIORITY) {
>>  			priority = 0;
>> -			cache_expand(cache);
>> +			if (!cache_expand(cache))
>> +				goto fail;
>>  		}
>>  	}
>>  
>> @@ -488,6 +496,10 @@ next_object:
>>  
>>  	*nodep = node;
>>  	return 1;
>> +
>> +fail:
>> +	*nodep = NULL;
>> +	return -1;
> 
> ...but a general comment about this patch and the next one: it seems
> reasonable to make cache_node_get return a null nodep and -1 to indicate
> error, but AFAICT the only caller is __cache_lookup (aka the buffer
> cache), which doesn't check the return value at all, only the value of
> *nodep.

Uh, I guess that was a leftover, I thought "I should return something
different on error!" before I realized nobody was checking return val
and then didn't go back.

> I don't see a change in this series to change __cache_lookup.  Did that
> get lost somewhere?
Not really, I just changed cache_expand before I realized __cache_lookup
didn't care. We could make it a void function, I suppose, or start checking
return val instead ...

-Eric

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

[Eric Sandeen]

On 8/24/26 10:18 PM, Darrick J. Wong wrote:
> 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 ;)

I'll check my git config in that tree, oops :( Been a minute since I
sent an xfsprogs patch I guess!

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

Yeah, lazily mentioned in the commit log.

>  Also,
> maybe we should be "return -ENOMEM" here or something?

Then have callers check, etc. Do you want me to rework things to rationalize
the return values (and caller checks?)

-Eric

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

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

[Darrick J. Wong]

On Tue, Aug 25, 2026 at 10:39:08AM -0500, Eric Sandeen wrote:
> On 8/24/26 10:18 PM, Darrick J. Wong wrote:
> > 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 ;)
> 
> I'll check my git config in that tree, oops :( Been a minute since I
> sent an xfsprogs patch I guess!
> 
> >> ---
> >>  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.
> 
> Yeah, lazily mentioned in the commit log.
> 
> >  Also,
> > maybe we should be "return -ENOMEM" here or something?
> 
> Then have callers check, etc. Do you want me to rework things to rationalize
> the return values (and caller checks?)

Yeah, I think __cache_lookup should do something with the new return
codes, e.g.

	int ret = cache_node_get(bcache, key, &cn);
	if (ret < 0) {
		fprintf(stderr,
			"%s occurred, go buy a real computer!\n",
			strerror(-ret));
		abort();
	}
	if (!cn)
		return -ENOMEM;
	bp = container_of(cn, struct xfs_buf, b_node);

This can be a third patch, the first two patches look good to me aside
from having weird SoB lines.

--D

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

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

[Eric Sandeen]

On 8/25/26 10:49 AM, Darrick J. Wong wrote:
> On Tue, Aug 25, 2026 at 10:39:08AM -0500, Eric Sandeen wrote:

...

>>>  Also,
>>> maybe we should be "return -ENOMEM" here or something?
>>
>> Then have callers check, etc. Do you want me to rework things to rationalize
>> the return values (and caller checks?)
> 
> Yeah, I think __cache_lookup should do something with the new return
> codes, e.g.
> 
> 	int ret = cache_node_get(bcache, key, &cn);
> 	if (ret < 0) {
> 		fprintf(stderr,
> 			"%s occurred, go buy a real computer!\n",
> 			strerror(-ret));
> 		abort();
> 	}
> 	if (!cn)
> 		return -ENOMEM;
> 	bp = container_of(cn, struct xfs_buf, b_node);
> 
> This can be a third patch, the first two patches look good to me aside
> from having weird SoB lines.
Fair enough, I was staying in the weird "we have distinct error codes that
are never checked" world and should have fixed that up at the same time,
for tidiness.

-Eric