fix return value for mb_cache_shrink_fn when nr_to_scan > 0

PATCH In review Wang Sheng-Hui 5 participants 8 messages Jul 19, 2010 – Jul 22, 2010

Subsystems (5)

Memory management VFS Kernel janitors ext4 file system +1 more subsystems

lore

Share & tools

lore

Wang Sheng-Hui

author

2010-07-21 10:53 UTC #0 lore raw

PATCH 1/1 fix return value for mb_cache_shrink_fn when nr_to_scan > 0

Sorry. regerated the patch, please check it.
I wrapped most code in single pair of spinlock ops for 2 reasons:
1) get spinlock 2 times seems time consuming
2) use single pair of spinlock ops can keep "count" consistent for the shrink operation. 2 pairs may
get some new ces created by other processes.

Signed-off-by Wang Sheng-Hui <[email protected]>

---
 fs/mbcache.c |   24 ++++++++++++------------
 1 files changed, 12 insertions(+), 12 deletions(-)

fs/mbcache.c +12 ~~-11~~

index ec88ff3..ee57aa3 100644
@@ -201,21 +201,15 @@ mb_cache_shrink_fn(int nr_to_scan, gfp_t gfp_mask)
 {
 	LIST_HEAD(free_list);
 	struct list_head *l, *ltmp;
+	struct mb_cache *cache;
 	int count = 0;
 
-	spin_lock(&mb_cache_spinlock);
-	list_for_each(l, &mb_cache_list) {
-		struct mb_cache *cache -			list_entry(l, struct mb_cache, c_cache_list);
-		mb_debug("cache %s (%d)", cache->c_name,
-			  atomic_read(&cache->c_entry_count));
-		count += atomic_read(&cache->c_entry_count);
-	}
 	mb_debug("trying to free %d entries", nr_to_scan);
-	if (nr_to_scan = 0) {
-		spin_unlock(&mb_cache_spinlock);
+
+	spin_lock(&mb_cache_spinlock);
+	if (nr_to_scan = 0)
 		goto out;
-	}
+
 	while (nr_to_scan-- && !list_empty(&mb_cache_lru_list)) {
 		struct mb_cache_entry *ce  			list_entry(mb_cache_lru_list.next,

@@ -223,12 +217,18 @@ mb_cache_shrink_fn(int nr_to_scan, gfp_t gfp_mask)
 		list_move_tail(&ce->e_lru_list, &free_list);
 		__mb_cache_entry_unhash(ce);
 	}
-	spin_unlock(&mb_cache_spinlock);
 	list_for_each_safe(l, ltmp, &free_list) {
 		__mb_cache_entry_forget(list_entry(l, struct mb_cache_entry,
 						   e_lru_list), gfp_mask);
 	}
 out:
+	list_for_each_entry(cache, &mb_cache_list, c_cache_list) {
+		mb_debug("cache %s (%d)", cache->c_name,
+			  atomic_read(&cache->c_entry_count));
+		count += atomic_read(&cache->c_entry_count);
+	}
+	spin_unlock(&mb_cache_spinlock);
+
 	return (count / 100) * sysctl_vfs_cache_pressure;
 }

Signature · 1 line

1.6.3.3

Eric Sandeen

2010-07-21 14:00 UTC #1 lore raw

Quoted 7 lines · Wang Sheng-Hui

Sorry. regerated the patch, please check it.
I wrapped most code in single pair of spinlock ops for 2 reasons:
1) get spinlock 2 times seems time consuming
2) use single pair of spinlock ops can keep "count" consistent for the shrink operation. 2 pairs may
get some new ces created by other processes.

Sorry, this patch appears to have whitespace cut & paste mangling.

More comments below.

Quoted 35 lines

Signed-off-by Wang Sheng-Hui <[email protected]>

---
fs/mbcache.c | 24 ++++++++++++------------

1 files changed, 12 insertions(+), 12 deletions(-)

fs/mbcache.c +5 ~~-10~~

index ec88ff3..ee57aa3 100644
@@ -201,21 +201,15 @@ mb_cache_shrink_fn(int nr_to_scan, gfp_t gfp_mask)
 {
    LIST_HEAD(free_list);
    struct list_head *l, *ltmp;
+    struct mb_cache *cache;
    int count = 0;
 
-    spin_lock(&mb_cache_spinlock);
-    list_for_each(l, &mb_cache_list) {
-        struct mb_cache *cache > -            list_entry(l, struct mb_cache, c_cache_list);
-        mb_debug("cache %s (%d)", cache->c_name,
-              atomic_read(&cache->c_entry_count));
-        count += atomic_read(&cache->c_entry_count);
-    }
    mb_debug("trying to free %d entries", nr_to_scan);
-    if (nr_to_scan = 0) {
-        spin_unlock(&mb_cache_spinlock);
+
+    spin_lock(&mb_cache_spinlock);
+    if (nr_to_scan = 0)
        goto out;
-    }
+
    while (nr_to_scan-- && !list_empty(&mb_cache_lru_list)) {
        struct mb_cache_entry *ce >             list_entry(mb_cache_lru_list.next,

fs/mbcache.c mb_cache_shrink_fn(int nr_to_scan, gfp_t gfp_mask) · line 226

Quoted 4 lines

fs/mbcache.c +0 -1

@@ -223,12 +217,18 @@ mb_cache_shrink_fn(int nr_to_scan, gfp_t gfp_mask)
        list_move_tail(&ce->e_lru_list, &free_list);
        __mb_cache_entry_unhash(ce);
    }
-    spin_unlock(&mb_cache_spinlock);

you can't do this because

fs/mbcache.c mb_cache_shrink_fn(int nr_to_scan, gfp_t gfp_mask) · line 221

Quoted 2 lines

fs/mbcache.c +0 -0

    list_for_each_safe(l, ltmp, &free_list) {
        __mb_cache_entry_forget(list_entry(l, struct mb_cache_entry,

this takes the spinlock too and you'll deadlock.

Did you test this patch?

-Eric

Quoted 13 lines

fs/mbcache.c +0 -0

                           e_lru_list), gfp_mask);
    }

out:

fs/mbcache.c +7 -0

+    list_for_each_entry(cache, &mb_cache_list, c_cache_list) {
+        mb_debug("cache %s (%d)", cache->c_name,
+              atomic_read(&cache->c_entry_count));
+        count += atomic_read(&cache->c_entry_count);
+    }
+    spin_unlock(&mb_cache_spinlock);
+
    return (count / 100) * sysctl_vfs_cache_pressure;

}

Andreas Gruenbacher

2010-07-19 16:19 UTC #2 lore raw

PATCH 1/2 mbcache: Remove unused features

The mbcache code was written to support a variable number of indexes, but all the existing users use exactly one index. Simplify to code to support only that case.

There are also no users of the cache entry free operation, and none of the users keep extra data in cache entries. Remove those features as well.

Signed-off-by Andreas Gruenbacher <[email protected]>

---
 fs/ext2/xattr.c         |   12 ++--
 fs/ext3/xattr.c         |   12 ++--
 fs/ext4/xattr.c         |   12 ++--
 fs/mbcache.c            |  141 +++++++++++++---------------------------------
 include/linux/mbcache.h |   20 ++-----
 5 files changed, 60 insertions(+), 137 deletions(-)

fs/ext2/xattr.c +5 -7

index 7c39157..af59915 100644
@@ -838,7 +838,7 @@ ext2_xattr_cache_insert(struct buffer_head *bh)
 	ce = mb_cache_entry_alloc(ext2_xattr_cache, GFP_NOFS);
 	if (!ce)
 		return -ENOMEM;
-	error = mb_cache_entry_insert(ce, bh->b_bdev, bh->b_blocknr, &hash);
+	error = mb_cache_entry_insert(ce, bh->b_bdev, bh->b_blocknr, hash);
 	if (error) {
 		mb_cache_entry_free(ce);
 		if (error = -EBUSY) {

@@ -912,8 +912,8 @@ ext2_xattr_cache_find(struct inode *inode, struct ext2_xattr_header *header)
 		return NULL;  /* never share */
 	ea_idebug(inode, "looking for cached blocks [%x]", (int)hash);
 again:
-	ce = mb_cache_entry_find_first(ext2_xattr_cache, 0,
-				       inode->i_sb->s_bdev, hash);
+	ce = mb_cache_entry_find_first(ext2_xattr_cache, inode->i_sb->s_bdev,
+				       hash);
 	while (ce) {
 		struct buffer_head *bh;

@@ -945,7 +945,7 @@ again:
 			unlock_buffer(bh);
 			brelse(bh);
 		}
-		ce = mb_cache_entry_find_next(ce, 0, inode->i_sb->s_bdev, hash);
+		ce = mb_cache_entry_find_next(ce, inode->i_sb->s_bdev, hash);
 	}
 	return NULL;
 }

@@ -1021,9 +1021,7 @@ static void ext2_xattr_rehash(struct ext2_xattr_header *header,
 int __init
 init_ext2_xattr(void)
 {
-	ext2_xattr_cache = mb_cache_create("ext2_xattr", NULL,
-		sizeof(struct mb_cache_entry) +
-		sizeof(((struct mb_cache_entry *) 0)->e_indexes[0]), 1, 6);
+	ext2_xattr_cache = mb_cache_create("ext2_xattr", 6);
 	if (!ext2_xattr_cache)
 		return -ENOMEM;
 	return 0;

fs/ext3/xattr.c +5 -7

index 71fb8d6..e69dc6d 100644
@@ -1139,7 +1139,7 @@ ext3_xattr_cache_insert(struct buffer_head *bh)
 		ea_bdebug(bh, "out of memory");
 		return;
 	}
-	error = mb_cache_entry_insert(ce, bh->b_bdev, bh->b_blocknr, &hash);
+	error = mb_cache_entry_insert(ce, bh->b_bdev, bh->b_blocknr, hash);
 	if (error) {
 		mb_cache_entry_free(ce);
 		if (error = -EBUSY) {

@@ -1211,8 +1211,8 @@ ext3_xattr_cache_find(struct inode *inode, struct ext3_xattr_header *header,
 		return NULL;  /* never share */
 	ea_idebug(inode, "looking for cached blocks [%x]", (int)hash);
 again:
-	ce = mb_cache_entry_find_first(ext3_xattr_cache, 0,
-				       inode->i_sb->s_bdev, hash);
+	ce = mb_cache_entry_find_first(ext3_xattr_cache, inode->i_sb->s_bdev,
+				       hash);
 	while (ce) {
 		struct buffer_head *bh;

@@ -1237,7 +1237,7 @@ again:
 			return bh;
 		}
 		brelse(bh);
-		ce = mb_cache_entry_find_next(ce, 0, inode->i_sb->s_bdev, hash);
+		ce = mb_cache_entry_find_next(ce, inode->i_sb->s_bdev, hash);
 	}
 	return NULL;
 }

@@ -1313,9 +1313,7 @@ static void ext3_xattr_rehash(struct ext3_xattr_header *header,
 int __init
 init_ext3_xattr(void)
 {
-	ext3_xattr_cache = mb_cache_create("ext3_xattr", NULL,
-		sizeof(struct mb_cache_entry) +
-		sizeof(((struct mb_cache_entry *) 0)->e_indexes[0]), 1, 6);
+	ext3_xattr_cache = mb_cache_create("ext3_xattr", 6);
 	if (!ext3_xattr_cache)
 		return -ENOMEM;
 	return 0;

fs/ext4/xattr.c +5 -7

index 0433800..1c93198 100644
@@ -1418,7 +1418,7 @@ ext4_xattr_cache_insert(struct buffer_head *bh)
 		ea_bdebug(bh, "out of memory");
 		return;
 	}
-	error = mb_cache_entry_insert(ce, bh->b_bdev, bh->b_blocknr, &hash);
+	error = mb_cache_entry_insert(ce, bh->b_bdev, bh->b_blocknr, hash);
 	if (error) {
 		mb_cache_entry_free(ce);
 		if (error = -EBUSY) {

@@ -1490,8 +1490,8 @@ ext4_xattr_cache_find(struct inode *inode, struct ext4_xattr_header *header,
 		return NULL;  /* never share */
 	ea_idebug(inode, "looking for cached blocks [%x]", (int)hash);
 again:
-	ce = mb_cache_entry_find_first(ext4_xattr_cache, 0,
-				       inode->i_sb->s_bdev, hash);
+	ce = mb_cache_entry_find_first(ext4_xattr_cache, inode->i_sb->s_bdev,
+				       hash);
 	while (ce) {
 		struct buffer_head *bh;

@@ -1515,7 +1515,7 @@ again:
 			return bh;
 		}
 		brelse(bh);
-		ce = mb_cache_entry_find_next(ce, 0, inode->i_sb->s_bdev, hash);
+		ce = mb_cache_entry_find_next(ce, inode->i_sb->s_bdev, hash);
 	}
 	return NULL;
 }

@@ -1591,9 +1591,7 @@ static void ext4_xattr_rehash(struct ext4_xattr_header *header,
 int __init
 init_ext4_xattr(void)
 {
-	ext4_xattr_cache = mb_cache_create("ext4_xattr", NULL,
-		sizeof(struct mb_cache_entry) +
-		sizeof(((struct mb_cache_entry *) 0)->e_indexes[0]), 1, 6);
+	ext4_xattr_cache = mb_cache_create("ext4_xattr", 6);
 	if (!ext4_xattr_cache)
 		return -ENOMEM;
 	return 0;

fs/mbcache.c +40 ~~-100~~

273 lines not on this page · raw message

include/linux/mbcache.h +5 ~~-15~~

index a09b84e..54cbbac 100644
@@ -4,9 +4,6 @@
   (C) 2001 by Andreas Gruenbacher, <[email protected]>
 */
 
-/* Hardwire the number of additional indexes */
-#define MB_CACHE_INDEXES_COUNT 1
-
 struct mb_cache_entry {
 	struct list_head		e_lru_list;
 	struct mb_cache			*e_cache;

@@ -18,17 +15,12 @@ struct mb_cache_entry {
 	struct {
 		struct list_head	o_list;
 		unsigned int		o_key;
-	} e_indexes[0];
-};
-
-struct mb_cache_op {
-	int (*free)(struct mb_cache_entry *, gfp_t);
+	} e_index;
 };
 
 /* Functions on caches */
 
-struct mb_cache * mb_cache_create(const char *, struct mb_cache_op *, size_t,
-				  int, int);
+struct mb_cache *mb_cache_create(const char *, int);
 void mb_cache_shrink(struct block_device *);
 void mb_cache_destroy(struct mb_cache *);

@@ -36,17 +28,15 @@ void mb_cache_destroy(struct mb_cache *);
 
 struct mb_cache_entry *mb_cache_entry_alloc(struct mb_cache *, gfp_t);
 int mb_cache_entry_insert(struct mb_cache_entry *, struct block_device *,
-			  sector_t, unsigned int[]);
+			  sector_t, unsigned int);
 void mb_cache_entry_release(struct mb_cache_entry *);
 void mb_cache_entry_free(struct mb_cache_entry *);
 struct mb_cache_entry *mb_cache_entry_get(struct mb_cache *,
 					  struct block_device *,
 					  sector_t);
-#if !defined(MB_CACHE_INDEXES_COUNT) || (MB_CACHE_INDEXES_COUNT > 0)
-struct mb_cache_entry *mb_cache_entry_find_first(struct mb_cache *cache, int,
+struct mb_cache_entry *mb_cache_entry_find_first(struct mb_cache *cache,
 						 struct block_device *, 
 						 unsigned int);
-struct mb_cache_entry *mb_cache_entry_find_next(struct mb_cache_entry *, int,
+struct mb_cache_entry *mb_cache_entry_find_next(struct mb_cache_entry *,
 						struct block_device *, 
 						unsigned int);
-#endif

Signature · 1 line

1.7.2.rc3.57.g77b5b

Andreas Dilger

2010-07-21 23:18 UTC #3 lore raw

Re: [PATCH 1/2] mbcache: Remove unused features

Quoted 7 lines · Andreas Gruenbacher

The mbcache code was written to support a variable number of indexes, but all the existing users use exactly one index. Simplify to code to support only that case.
There are also no users of the cache entry free operation, and none of the users keep extra data in cache entries. Remove those features as well.

Is it possible to allow mbcache to be disabled, either for the whole kernel, on a per-filesystem basis, or adaptively if the cache hit rate is very low (any of these is fine, not all of them).

The reason I ask is because under some uses mbcache is adding significant overhead for very little/no benefit. If the xattr blocks are not shared, then every xattr is stored in a separate entry, and there is a single spinlock protecting the whole mbcache for all filesystems.

On systems with large memory for the buffer cache (6M+ buffer heads, 5M inodes in memory) there are very long hash chains to be searched, and this slows down filesystem performance dramatically. We became aware of this problem because of NMIs triggering due to long spinlock hold times in mb_cache_entry_insert() on a server with 32GB of RAM.

To reproduce the problem, a simple test can be done with a bunch of kernel source trees (not hard-linked trees though, must be unpacked separately):

$ for i in linux-* ; do
    time du ${i}
  done

gives :
8s for the first one
12s for the 10th
27s for the 25th
48s for the 50th
95s for the 100th
=> this is strictly linear

opreport -l vmlinux show :
68.12% in mb_cache_entry_insert
21.71% in mb_cache_entry_release
4.27% in mb_cache_entry_free
1.49% in mb_cache_entry_find_first
0.82% in __mb_cache_entry_find

(see https://bugzilla.lustre.org/show_bug.cgi?id=22771 for full details)

I don't think fixing the mbcache to be more efficient (more buckets, more locks, etc) is really solving the problem which is that mbcache is adding overhead without value in these situations.

Attached is a patch that allows manually disabling mbcache on a per-filesystem basis with a mount option. Better would be to automatically disable it if e.g. some hundreds or thousands of objects were inserted into the cache and there was < 1% cache hit rate. That would help everyone, even those people who don't know they have a problem.

Cheers, Andreas

Andreas Gruenbacher

2010-07-22 00:07 UTC #4 lore raw

Re: [PATCH 1/2] mbcache: Remove unused features

Quoted 6 lines

From https://bugzilla.lustre.org/show_bug.cgi?id=22771#c24 :
On our production system, the hash table contain 64 entries (6 bits) for a cache of 2307267 entries.
A count in each list give a good load balance : number of entries vary between 35782 to 36496 while the optimal repartition is 2307267 / 64 = 36051.

Hehe, I like that sense of humor :)

Quoted 3 lines · On Thursday 22 July 2010 01:18:39 Andreas Dilger wrote:

Is it possible to allow mbcache to be disabled, either for the whole kernel, on a per-filesystem basis, or adaptively if the cache hit rate is very low (any of these is fine, not all of them).

We could do that, but making the cache not degrade so badly would be a good idea in any case. The number of buckets is currently fixed for ext[234] so it would make sense to either make that number dynamic or limit the maximum number of cache entries. The latter will probably be good enough for most workloads.

Quoted 6 lines

Attached is a patch that allows manually disabling mbcache on a per-filesystem basis with a mount option.
I don't think fixing the mbcache to be more efficient (more buckets, more locks, etc) is really solving the problem which is that mbcache is adding overhead without value in these situations.

A mount option would be very ugly, but a kernel internal NO_MBCACHE flag sounds more acceptable to me.

Quoted 3 lines

Better would be to
automatically disable it if e.g. some hundreds or thousands of objects were inserted into the cache and there was < 1% cache hit rate.

This assumes that the workload won't change.

Quoted 2 lines

That would help everyone, even those people who don't know they have a problem.

People who don't know they have a problem would also be helped by making the cache not degrade so badly, right?

Even better would be to use a more appropriate inode size, but you've pointed that out in the bug already.

Thanks,
Andreas

Andreas Gruenbacher

2010-07-21 17:44 UTC #5 lore raw

PATCH 2/2 mbcache: fix shrinker function return value

In reply to Eric Sandeen

The shrinker function is supposed to return the number of cache entries after shrinking, not before shrinking. Fix that.

Based on a patch from Wang Sheng-Hui <[email protected]>.

Signed-off-by Andreas Gruenbacher <[email protected]>

---
 fs/mbcache.c |   27 ++++++++++-----------------
 1 files changed, 10 insertions(+), 17 deletions(-)

fs/mbcache.c +10 ~~-16~~

index 8a2cbd8..cf4e6cd 100644
@@ -176,22 +176,12 @@ static int
 mb_cache_shrink_fn(struct shrinker *shrink, int nr_to_scan, gfp_t gfp_mask)
 {
 	LIST_HEAD(free_list);
-	struct list_head *l, *ltmp;
+	struct mb_cache *cache;
+	struct mb_cache_entry *entry, *tmp;
 	int count = 0;
 
-	spin_lock(&mb_cache_spinlock);
-	list_for_each(l, &mb_cache_list) {
-		struct mb_cache *cache -			list_entry(l, struct mb_cache, c_cache_list);
-		mb_debug("cache %s (%d)", cache->c_name,
-			  atomic_read(&cache->c_entry_count));
-		count += atomic_read(&cache->c_entry_count);
-	}
 	mb_debug("trying to free %d entries", nr_to_scan);
-	if (nr_to_scan = 0) {
-		spin_unlock(&mb_cache_spinlock);
-		goto out;
-	}
+	spin_lock(&mb_cache_spinlock);
 	while (nr_to_scan-- && !list_empty(&mb_cache_lru_list)) {
 		struct mb_cache_entry *ce  			list_entry(mb_cache_lru_list.next,

@@ -199,12 +189,15 @@ mb_cache_shrink_fn(struct shrinker *shrink, int nr_to_scan, gfp_t gfp_mask)
 		list_move_tail(&ce->e_lru_list, &free_list);
 		__mb_cache_entry_unhash(ce);
 	}
+	list_for_each_entry(cache, &mb_cache_list, c_cache_list) {
+		mb_debug("cache %s (%d)", cache->c_name,
+			  atomic_read(&cache->c_entry_count));
+		count += atomic_read(&cache->c_entry_count);
+	}
 	spin_unlock(&mb_cache_spinlock);
-	list_for_each_safe(l, ltmp, &free_list) {
-		__mb_cache_entry_forget(list_entry(l, struct mb_cache_entry,
-						   e_lru_list), gfp_mask);
+	list_for_each_entry_safe(entry, tmp, &free_list, e_lru_list) {
+		__mb_cache_entry_forget(entry, gfp_mask);
 	}
-out:
 	return (count / 100) * sysctl_vfs_cache_pressure;
 }

Signature · 1 line

1.7.2.rc3.57.g77b5b

Andreas Gruenbacher

2010-07-21 17:57 UTC #6 lore raw

cover mbcache fixes

In reply to Eric Sandeen

Al,

here is an mbcache cleanup and then a fixed version of Shenghui's minor shrinker function fix. The patches have survived functional testing here.

This seems slightly too much for kernel-janitors, so could you please take the patches?

Thanks,
Andreas

Andreas Gruenbacher (2):
mbcache: Remove unused features
mbcache: fix shrinker function return value

 fs/ext2/xattr.c         |   12 ++--
 fs/ext3/xattr.c         |   12 ++--
 fs/ext4/xattr.c         |   12 ++--
 fs/mbcache.c            |  168 ++++++++++++++---------------------------------
 include/linux/mbcache.h |   20 ++----
 5 files changed, 70 insertions(+), 154 deletions(-)

Signature · 1 line

1.7.2.rc3.57.g77b5b

Al Viro

maintainer

2010-07-21 23:22 UTC #7 lore raw

Re: [PATCH 0/2] mbcache fixes

Quoted 8 lines · Andreas Gruenbacher

Al,
here is an mbcache cleanup and then a fixed version of Shenghui's minor shrinker function fix. The patches have survived functional testing here.
This seems slightly too much for kernel-janitors, so could you please take the patches?

Done.