mb_cache_shrink() frees unexpected caches
Subsystems (2)
PATCH 1/1 mb_cache_shrink() frees unexpected caches
mb_cache_shrink() tries to free all sort of mbcache in the lru list.
All user of mb_cache_shrink() are ext2/ext3 xattr.
fs/mbcache.c renamed from fs/mbcache.c.orig +1 -1
@@ -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); }
Hello,
Quoted 9 lines · 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 <[email protected]>
fs/mbcache.c mb_cache_shrink(struct mb_cache *cache, · line 335
Quoted 8 lines
fs/mbcache.c renamed from fs/mbcache.c.orig +1
-1@@ -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.
Quoted 16 lines
fs/mbcache.c mb_cache_shrink(struct mb_cache *cache, · line 335
Quoted 12 lines
fs/mbcache.c renamed from fs/mbcache.c.orig +1
-1@@ -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_devicefrom different file systems? Theblock_deviceis sufficient to identify the file system, and hence its cache entries.
Why is mb_cache_shrink() declared as:
voidmb_cache_shrink(struct mb_cache *cache, struct block_device *bdev);
The variable cache was never used.
Quoted 16 lines · Akinobu Mita
Quoted 7 lines
Quoted 7 lines
fs/mbcache.c renamed from fs/mbcache.c.orig +0
-1@@ -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) {Quoted 8 lines
fs/mbcache.c mb_cache_shrink(struct mb_cache *cache, · line 335
Quoted 4 lines
fs/mbcache.c renamed from fs/mbcache.c.orig +1
-0+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_devicefrom different file systems? Theblock_deviceis sufficient to identify the file system, and hence its cache entries.
Quoted 6 lines
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.
fs/ext2/xattr.c +1 -1
@@ -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); }
fs/ext3/xattr.c +1 -1
@@ -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); }/*
fs/mbcache.c +1 -2
@@ -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;
include/linux/mbcache.h +1 -1
@@ -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 */
Quoted 2 lines · 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.
Quoted 7 lines · 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.
Quoted 7 lines · Akinobu Mita
Quoted 2 lines · 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:
fs/reiserfs/xattr.c +0 -1
@@ -41,3 +41,2 @@#include <linux/reiserfs_acl.h> -#include <linux/mbcache.h> #include <asm/uaccess.h>
Quoted 5 lines
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,
Signature · 2 lines
Andreas Gruenbacher <[email protected]> SUSE Labs, SUSE LINUX PRODUCTS GMBH
Quoted 4 lines · 2005-07-16 (Sat) 03:44 +0200 Andreas Gruenbacher wrote:
Removing the cache parameter from
mb_cache_shrinkwould break when more than onemb_cacheis 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 onemb_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.