Skip to contentSkip to conversation

Revert "fs/mbcache.c: make count_objects() more robust"

PATCH Applied Eric Biggers 2 participants 3 messages Jan 6, 2018 – Jan 7, 2018 VFS

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 Revert "fs/mbcache.c: make count_objects() more robust"

From: Eric Biggers <[email protected]>

This reverts commit d5dabd633922ac5ee5bcc67748f7defb8b211469.

This patch did absolutely nothing, because ->c_entry_count is unsigned.

In addition if there is a bug in how mbcache maintains its entry count, it needs to be fixed, not just hacked around. (There is no obvious bug, though.)

---
 fs/mbcache.c | 3 ---
 1 file changed, 3 deletions(-)
fs/mbcache.c +0 -3
index b8b8b9ced9f8..d818fd236787 100644
@@ -269,9 +269,6 @@ static unsigned long mb_cache_count(struct shrinker *shrink,
 	struct mb_cache *cache = container_of(shrink, struct mb_cache,
 					      c_shrink);
 
-	/* Unlikely, but not impossible */
-	if (unlikely(cache->c_entry_count < 0))
-		return 0;
 	return cache->c_entry_count;
 }
 
Signature · 1 line
2.15.1
#1 lore raw
Quoted 9 lines · Eric Biggers

From: Eric Biggers <[email protected]>

This reverts commit d5dabd633922ac5ee5bcc67748f7defb8b211469.

This patch did absolutely nothing, because ->c_entry_count is unsigned.

In addition if there is a bug in how mbcache maintains its entry count, it needs to be fixed, not just hacked around. (There is no obvious bug, though.)

Right, if we're going to add a check, we should be checking to make sure cache->c_entry_count is not zero before we decrement it in mb_cache_entry_delete().

I will note that it's quite possible the error is in do_shrink_slab() --- it's already dodgy that it assigns an unsigned long from shrinker->count_objects to a signed long. Then it multiplies it by (4
* nr_scanned) / shrinker->seeks. So there are plenty of opportunities to get the "negative objects to delete" message that has nothing to do with value returned from mb_cache_count().

Regards,

- Ted

#2 lore raw
In reply to Eric Biggers
Quoted 9 lines · Eric Biggers

From: Eric Biggers <[email protected]>

This reverts commit d5dabd633922ac5ee5bcc67748f7defb8b211469.

This patch did absolutely nothing, because ->c_entry_count is unsigned.

In addition if there is a bug in how mbcache maintains its entry count, it needs to be fixed, not just hacked around. (There is no obvious bug, though.)

Thanks, applied.

- Ted