fix return value for mb_cache_shrink_fn when nr_to_scan > 0

Hi,

The comment for struct shrinker in include/linux/mm.h says "shrink...It should return the number of objects which remain in the cache."
Please notice the word "remain".

In fs/mbcache.h, mb_cache_shrink_fn is used as the shrink function:
static struct shrinker mb_cache_shrinker = {
.shrink = mb_cache_shrink_fn,
.seeks = DEFAULT_SEEKS,
};
In mb_cache_shrink_fn, the return value for nr_to_scan > 0 is the number of mb_cache_entry before shrink operation. It may because the memory usage for mbcache is low, so the effect is not so obvious. I think we'd better fix the return value issue.

Following patch is against 2.6.35-rc5. Please check it.

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

index ec88ff3..412e7cc 100644
@@ -228,6 +228,16 @@ mb_cache_shrink_fn(int nr_to_scan, gfp_t gfp_mask)
 		__mb_cache_entry_forget(list_entry(l, struct mb_cache_entry,
 						   e_lru_list), gfp_mask);
 	}
+	spin_lock(&mb_cache_spinlock);
+	count = 0;
+	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);
+	}
+	spin_unlock(&mb_cache_spinlock);
 out:
 	return (count / 100) * sysctl_vfs_cache_pressure;
 }

1.7.1.1





-- 
Thanks and Regards,
shenghui

you are right that it's not returning the remaining entries, but I think we can do this more simply; there isn't any reason to calculate it twice How about just moving the accounting to the end, since "count" isn't actually used when freeing, anyway.... something like this?

index ec88ff3..3af79de 100644
@@ -203,19 +203,11 @@ mb_cache_shrink_fn(int nr_to_scan, gfp_t gfp_mask)
 	struct list_head *l, *ltmp;
 	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);
+	if (nr_to_scan = 0)
 		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,

@@ -229,6 +221,17 @@ mb_cache_shrink_fn(int nr_to_scan, gfp_t gfp_mask)
 						   e_lru_list), gfp_mask);
 	}
 out:
+	/* Count remaining entries */
+	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);
+	}
+	spin_unlock(&mb_cache_spinlock);
+
 	return (count / 100) * sysctl_vfs_cache_pressure;
 }
 

ao? 2010-7-18 14:01, Christoph Hellwig a??e??:

I regenerated the patch. Please check it.

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

index ec88ff3..5697d9e 100644
@@ -201,21 +201,13 @@ 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);
+	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,

@@ -229,6 +221,14 @@ mb_cache_shrink_fn(int nr_to_scan, gfp_t gfp_mask)
 						   e_lru_list), gfp_mask);
 	}
 out:
+	spin_lock(&mb_cache_spinlock);
+	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;
 }
 

1.7.1.1



-- 
Thanks and Regards,
shenghui

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

The logic for calculating how many objects to free is still wrong:
mb_cache_shrink_fn returns the number of entries scaled by sysctl_vfs_cache_pressure / 100. It should also scale nr_to_scan by the inverse of that. The sysctl_vfs_cache_pressure = 0 case (never scale) may require special attention.

See dcache_shrinker() in fs/dcache.c.

Thanks,
Andreas

It would make sense to change this throughout the whole file.

Thanks,
Andreas

2010/7/20 Andreas Gruenbacher <[email protected]>:

Sorry, I haven't found any special attention on
sysctl_vfs_cache_pressure = 0 case or scale
nr_to_scan in fs/dcache.c

900static int shrink_dcache_memory(int nr, gfp_t gfp_mask) 901{
902 if (nr) {
903 if (!(gfp_mask & __GFP_FS))
904 return -1;
905 prune_dcache(nr);
906 }
907 return (dentry_stat.nr_unused / 100) * sysctl_vfs_cache_pressure;
908}

Thanks and Best Regards,
shenghui

2010/7/20 shenghui <[email protected]>:

And for sysctl_vfs_cache_pressure = 0 case, it's enough to return 0 to indicate no cache entries left.

Thanks and Best Regards,
shenghui

I don't think that's right:

vfs_cache_pressure
------------------

Controls the tendency of the kernel to reclaim the memory which is used for caching of directory and inode objects.

At the default value of vfs_cache_pressure0 the kernel will attempt to reclaim dentries and inodes at a "fair" rate with respect to pagecache and swapcache reclaim. Decreasing vfs_cache_pressure causes the kernel to prefer to retain dentry and inode caches. When vfs_cache_pressure=0, the kernel will never reclaim dentries and inodes due to memory pressure and this can easily lead to out-of-memory conditions. Increasing vfs_cache_pressure beyond 100 causes the kernel to prefer to reclaim dentries and inodes.

0 means "never reclaim," it doesn't mean "never scale."

As for nr_to_scan, after the first call, the shrinker has a scaled version of the total count, so the requested nr_to_scan on the next call is already scaled based on that.

I think the logic in the mbcache shrinker is fine.

-Eric

Indeed yes, I got confused, sorry.

Sheng-Hui, the mb_cache_lru_list list is now accessed without holding mb_cache_spinlock.

Thanks,
Andreas