[PATCH] mb_cache_shrink() frees unexpected caches

[PATCH] mb_cache_shrink() frees unexpected caches

[Akinobu Mita]

mb_cache_shrink() tries to free all sort of mbcache in the lru list.

All user of mb_cache_shrink() are ext2/ext3 xattr.

Signed-off-by: Akinobu Mita <amgta@yacht.ocn.ne.jp>

--- 2.6-rc/fs/mbcache.c.orig	2005-07-14 20:40:34.000000000 +0900
+++ 2.6-rc/fs/mbcache.c	2005-07-14 20:43:42.000000000 +0900
@@ -329,7 +329,7 @@ mb_cache_shrink(struct mb_cache *cache, 
 	list_for_each_safe(l, ltmp, &mb_cache_lru_list) {
 		struct mb_cache_entry *ce =
 			list_entry(l, struct mb_cache_entry, e_lru_list);
-		if (ce->e_bdev == bdev) {
+		if (ce->e_cache == cache && ce->e_bdev == bdev) {
 			list_move_tail(&ce->e_lru_list, &free_list);
 			__mb_cache_entry_unhash(ce);
 		}

Re: [PATCH] mb_cache_shrink() frees unexpected caches

[Andreas Gruenbacher]

Hello,

On Thursday 14 July 2005 15:07, Akinobu Mita wrote:
> mb_cache_shrink() tries to free all sort of mbcache in the lru list.
>
> All user of mb_cache_shrink() are ext2/ext3 xattr.
>
> Signed-off-by: Akinobu Mita <amgta@yacht.ocn.ne.jp>
>
> --- 2.6-rc/fs/mbcache.c.orig	2005-07-14 20:40:34.000000000 +0900
> +++ 2.6-rc/fs/mbcache.c	2005-07-14 20:43:42.000000000 +0900
> @@ -329,7 +329,7 @@ mb_cache_shrink(struct mb_cache *cache,
>  	list_for_each_safe(l, ltmp, &mb_cache_lru_list) {
>  		struct mb_cache_entry *ce =
>  			list_entry(l, struct mb_cache_entry, e_lru_list);
> -		if (ce->e_bdev == bdev) {
> +		if (ce->e_cache == cache && ce->e_bdev == bdev) {
>  			list_move_tail(&ce->e_lru_list, &free_list);
>  			__mb_cache_entry_unhash(ce);
>  		}

this patch looks bogus to me. How could the cache contain entries for the same 
block_device from different file systems? The block_device is sufficient to 
identify the file system, and hence its cache entries.

Also, the additional check would only tell ext2 from ext3; the caches are not 
per-filesystem.

-- Andreas.

Re: [PATCH] mb_cache_shrink() frees unexpected caches

[Akinobu Mita]

> > --- 2.6-rc/fs/mbcache.c.orig	2005-07-14 20:40:34.000000000 +0900
> > +++ 2.6-rc/fs/mbcache.c	2005-07-14 20:43:42.000000000 +0900
> > @@ -329,7 +329,7 @@ mb_cache_shrink(struct mb_cache *cache,
> >  	list_for_each_safe(l, ltmp, &mb_cache_lru_list) {
> >  		struct mb_cache_entry *ce =
> >  			list_entry(l, struct mb_cache_entry, e_lru_list);
> > -		if (ce->e_bdev == bdev) {
> > +		if (ce->e_cache == cache && ce->e_bdev == bdev) {
> >  			list_move_tail(&ce->e_lru_list, &free_list);
> >  			__mb_cache_entry_unhash(ce);
> >  		}
> 
> this patch looks bogus to me. How could the cache contain entries for the same 
> block_device from different file systems? The block_device is sufficient to 
> identify the file system, and hence its cache entries.

Why is mb_cache_shrink() declared as:

void
mb_cache_shrink(struct mb_cache *cache, struct block_device *bdev);

The variable cache was never used.

Re: [PATCH] mb_cache_shrink() frees unexpected caches

[Andreas Gruenbacher]

On Friday 15 July 2005 15:41, Akinobu Mita wrote:
> > > --- 2.6-rc/fs/mbcache.c.orig	2005-07-14 20:40:34.000000000 +0900
> > > +++ 2.6-rc/fs/mbcache.c	2005-07-14 20:43:42.000000000 +0900
> > > @@ -329,7 +329,7 @@ mb_cache_shrink(struct mb_cache *cache,
> > >  	list_for_each_safe(l, ltmp, &mb_cache_lru_list) {
> > >  		struct mb_cache_entry *ce =
> > >  			list_entry(l, struct mb_cache_entry, e_lru_list);
> > > -		if (ce->e_bdev == bdev) {
> > > +		if (ce->e_cache == cache && ce->e_bdev == bdev) {
> > >  			list_move_tail(&ce->e_lru_list, &free_list);
> > >  			__mb_cache_entry_unhash(ce);
> > >  		}
> >
> > this patch looks bogus to me. How could the cache contain entries for the
> > same block_device from different file systems? The block_device is
> > sufficient to identify the file system, and hence its cache entries.
>
> Why is mb_cache_shrink() declared as:
>
> void
> mb_cache_shrink(struct mb_cache *cache, struct block_device *bdev);
>
> The variable cache was never used.

The cache parameter could indeed be removed. Not that it would matter much...

Thanks,
Andreas.

------------------------------- 8< -------------------------------
Remove unused mb_cache_shrink parameter

The cache parameter to mb_cache_shrink isn't used. We may as well
remove it.

Signed-off-by: Andreas Gruenbacher <agruen@suse.de>

Index: linux-2.6.13-rc1/fs/ext2/xattr.c
===================================================================
--- linux-2.6.13-rc1.orig/fs/ext2/xattr.c
+++ linux-2.6.13-rc1/fs/ext2/xattr.c
@@ -823,7 +823,7 @@ cleanup:
 void
 ext2_xattr_put_super(struct super_block *sb)
 {
-	mb_cache_shrink(ext2_xattr_cache, sb->s_bdev);
+	mb_cache_shrink(sb->s_bdev);
 }
 
 
Index: linux-2.6.13-rc1/fs/ext3/xattr.c
===================================================================
--- linux-2.6.13-rc1.orig/fs/ext3/xattr.c
+++ linux-2.6.13-rc1/fs/ext3/xattr.c
@@ -1106,7 +1106,7 @@ cleanup:
 void
 ext3_xattr_put_super(struct super_block *sb)
 {
-	mb_cache_shrink(ext3_xattr_cache, sb->s_bdev);
+	mb_cache_shrink(sb->s_bdev);
 }
 
 /*
Index: linux-2.6.13-rc1/fs/mbcache.c
===================================================================
--- linux-2.6.13-rc1.orig/fs/mbcache.c
+++ linux-2.6.13-rc1/fs/mbcache.c
@@ -316,11 +316,10 @@ fail:
  * currently in use cannot be freed, and thus remain in the cache. All others
  * are freed.
  *
- * @cache: which cache to shrink
  * @bdev: which device's cache entries to shrink
  */
 void
-mb_cache_shrink(struct mb_cache *cache, struct block_device *bdev)
+mb_cache_shrink(struct block_device *bdev)
 {
 	LIST_HEAD(free_list);
 	struct list_head *l, *ltmp;
Index: linux-2.6.13-rc1/include/linux/mbcache.h
===================================================================
--- linux-2.6.13-rc1.orig/include/linux/mbcache.h
+++ linux-2.6.13-rc1/include/linux/mbcache.h
@@ -29,7 +29,7 @@ struct mb_cache_op {
 
 struct mb_cache * mb_cache_create(const char *, struct mb_cache_op *, size_t,
 				  int, int);
-void mb_cache_shrink(struct mb_cache *, struct block_device *);
+void mb_cache_shrink(struct block_device *);
 void mb_cache_destroy(struct mb_cache *);
 
 /* Functions on cache entries */

Re: [PATCH] mb_cache_shrink() frees unexpected caches

[Akinobu Mita]

2005-07-15 (Fri) 16:36 +0200  Andreas Gruenbacher wrote:
> The cache parameter could indeed be removed. Not that it would matter much...
> 

Currently, mbcache is used only for xattr on ext2/ext3 and reiserfs.
In other words, only one type of mbcache is used per-filesystem.
So any problems don't happen without the patch I sent.

But, for example when someone use mbcache as another purpose on ext3,
The crash will be caused by using mb_cache_shrink().

Therefore, I think your patch should not be committed until
mbcache will be limited to use only type per-filesystem.

Re: [PATCH] mb_cache_shrink() frees unexpected caches

[Akinobu Mita]

2005-07-16 (Sat) 00:07 +0900 Akinobu Mita wrote:
> Currently, mbcache is used only for xattr on ext2/ext3 and reiserfs.
> In other words, only one type of mbcache is used per-filesystem.
> So any problems don't happen without the patch I sent.
> 
> But, for example when someone use mbcache as another purpose on ext3,
> The crash will be caused by using mb_cache_shrink().
> 

The crash doesn't happen..
But, If more than two type of mbcache exist per-filesystem,
mb_cache_shrink() is not effective.

Re: [PATCH] mb_cache_shrink() frees unexpected caches

[Andreas Gruenbacher]

On Friday 15 July 2005 17:07, Akinobu Mita wrote:
> 2005-07-15 (Fri) 16:36 +0200  Andreas Gruenbacher wrote:
> > The cache parameter could indeed be removed. Not that it would matter
> > much...
>
> Currently, mbcache is used only for xattr on ext2/ext3 and reiserfs.
> In other words, only one type of mbcache is used per-filesystem.
> So any problems don't happen without the patch I sent.

Actually, reiserfs doesn't use the mbcache, so this can go:

Index: linux-2.6.12/fs/reiserfs/xattr.c
===================================================================
--- linux-2.6.12.orig/fs/reiserfs/xattr.c
+++ linux-2.6.12/fs/reiserfs/xattr.c
@@ -41,3 +41,2 @@
 #include <linux/reiserfs_acl.h>
-#include <linux/mbcache.h>
 #include <asm/uaccess.h>

> But, for example when someone use mbcache as another purpose on ext3,
> The crash will be caused by using mb_cache_shrink().
>
> Therefore, I think your patch should not be committed until
> mbcache will be limited to use only type per-filesystem.

Removing the cache parameter from mb_cache_shrink would break when more than 
one mb_cache is used per filesystem, correct. Leaving the parameter in and 
adding your patch is more "future proof", so I'm fine with it. Are you 
actually using more than one mb_cache, or is this theoretical?

As you say in your follow-up mail, without your patch the cache would become 
less effective, it won't crash anything, though.

Thanks,
-- 
Andreas Gruenbacher <agruen@suse.de>
SUSE Labs, SUSE LINUX PRODUCTS GMBH

Re: [PATCH] mb_cache_shrink() frees unexpected caches

[Akinobu Mita]

2005-07-16 (Sat) 03:44 +0200 Andreas Gruenbacher wrote:
> Removing the cache parameter from mb_cache_shrink would break when more than 
> one mb_cache is used per filesystem, correct. Leaving the parameter in and 
> adding your patch is more "future proof", so I'm fine with it. Are you 
> actually using more than one mb_cache, or is this theoretical?

This is just theorecal.

Now I agree with your two patches except for the name of
"mb_cache_shrink".  it should be renamed to imply that this function
shrinks all sort of mbcache on specified block device.