diff mbox

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

Message ID 20180106180654.7373-1-ebiggers3@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Eric Biggers Jan. 6, 2018, 6:06 p.m. UTC 
From: Eric Biggers <ebiggers@google.com>

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

Cc: Jan Kara <jack@suse.cz>
Cc: Jiang Biao <jiang.biao2@zte.com.cn>
Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/mbcache.c | 3 ---
 1 file changed, 3 deletions(-)

Comments

Theodore Ts'o Jan. 7, 2018, 9:04 p.m. UTC | #1 
On Sat, Jan 06, 2018 at 10:06:54AM -0800, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> 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
Theodore Ts'o Jan. 7, 2018, 9:36 p.m. UTC | #2 
On Sat, Jan 06, 2018 at 10:06:54AM -0800, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> 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
diff mbox

Patch

diff --git a/fs/mbcache.c b/fs/mbcache.c
index b8b8b9ced9f8..d818fd236787 100644
--- a/fs/mbcache.c
+++ b/fs/mbcache.c
@@ -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;
 }