mbcache entries have an 'e_referenced' bit which users can set with
mb_cache_entry_touch() to indicate that an entry should be given another
pass through the LRU list before the shrinker can delete it. However,
mb_cache_shrink() actually would, when seeing an e_referenced entry at
the front of the list (the least-recently used end), place it right at
the front of the list again. The next iteration would then remove the
entry from the list and delete it. Consequently, e_referenced had
essentially no effect, so ext2/ext4 xattr blocks would sometimes not be
reused as often as expected.
Fix this by making the shrinker move e_referenced entries to the back of
the list rather than the front.
Signed-off-by: Eric Biggers <[email protected]>
---
fs/mbcache.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/mbcache.c b/fs/mbcache.c
index c5bd19f..31e54c2 100644
--- a/fs/mbcache.c
+++ b/fs/mbcache.c
@@ -286,7 +286,7 @@ static unsigned long mb_cache_shrink(struct mb_cache *cache,
struct mb_cache_entry, e_list);
if (entry->e_referenced) {
entry->e_referenced = 0;
- list_move_tail(&cache->c_list, &entry->e_list);
+ list_move_tail(&entry->e_list, &cache->c_list);
continue;
}
list_del_init(&entry->e_list);
--
2.8.0.rc3.226.g39d4020
mb_cache_entry_find_first() and mb_cache_entry_find_next() only return
cache entries with the 'e_reusable' bit set. This should be documented.
Signed-off-by: Eric Biggers <[email protected]>
---
fs/mbcache.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/fs/mbcache.c b/fs/mbcache.c
index bf65906..b19be42 100644
--- a/fs/mbcache.c
+++ b/fs/mbcache.c
@@ -155,12 +155,12 @@ static struct mb_cache_entry *__entry_find(struct mb_cache *cache,
}
/*
- * mb_cache_entry_find_first - find the first entry in cache with given key
+ * mb_cache_entry_find_first - find the first reusable entry with the given key
* @cache: cache where we should search
* @key: key to look for
*
- * Search in @cache for entry with key @key. Grabs reference to the first
- * entry found and returns the entry.
+ * Search in @cache for a reusable entry with key @key. Grabs reference to the
+ * first reusable entry found and returns the entry.
*/
struct mb_cache_entry *mb_cache_entry_find_first(struct mb_cache *cache,
u32 key)
@@ -170,14 +170,14 @@ struct mb_cache_entry *mb_cache_entry_find_first(struct mb_cache *cache,
EXPORT_SYMBOL(mb_cache_entry_find_first);
/*
- * mb_cache_entry_find_next - find next entry in cache with the same
+ * mb_cache_entry_find_next - find next reusable entry with the same key
* @cache: cache where we should search
* @entry: entry to start search from
*
- * Finds next entry in the hash chain which has the same key as @entry.
- * If @entry is unhashed (which can happen when deletion of entry races
- * with the search), finds the first entry in the hash chain. The function
- * drops reference to @entry and returns with a reference to the found entry.
+ * Finds next reusable entry in the hash chain which has the same key as @entry.
+ * If @entry is unhashed (which can happen when deletion of entry races with the
+ * search), finds the first reusable entry in the hash chain. The function drops
+ * reference to @entry and returns with a reference to the found entry.
*/
struct mb_cache_entry *mb_cache_entry_find_next(struct mb_cache *cache,
struct mb_cache_entry *entry)
--
2.8.0.rc3.226.g39d4020
mbcache used several different types to represent the number of entries
in the cache. For consistency within mbcache and with the shrinker API,
always use unsigned long.
This does not change behavior for current mbcache users (ext2 and ext4)
since they limit the entry count to a value which easily fits in an int.
Signed-off-by: Eric Biggers <[email protected]>
---
fs/mbcache.c | 15 +++++++--------
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/fs/mbcache.c b/fs/mbcache.c
index 07c5d7d..bf65906 100644
--- a/fs/mbcache.c
+++ b/fs/mbcache.c
@@ -29,7 +29,7 @@ struct mb_cache {
/* log2 of hash table size */
int c_bucket_bits;
/* Maximum entries in cache to avoid degrading hash too much */
- int c_max_entries;
+ unsigned long c_max_entries;
/* Protects c_list, c_entry_count */
spinlock_t c_list_lock;
struct list_head c_list;
@@ -43,7 +43,7 @@ struct mb_cache {
static struct kmem_cache *mb_entry_cache;
static unsigned long mb_cache_shrink(struct mb_cache *cache,
- unsigned int nr_to_scan);
+ unsigned long nr_to_scan);
static inline struct hlist_bl_head *mb_cache_entry_head(struct mb_cache *cache,
u32 key)
@@ -274,11 +274,11 @@ static unsigned long mb_cache_count(struct shrinker *shrink,
/* Shrink number of entries in cache */
static unsigned long mb_cache_shrink(struct mb_cache *cache,
- unsigned int nr_to_scan)
+ unsigned long nr_to_scan)
{
struct mb_cache_entry *entry;
struct hlist_bl_head *head;
- unsigned int shrunk = 0;
+ unsigned long shrunk = 0;
spin_lock(&cache->c_list_lock);
while (nr_to_scan-- && !list_empty(&cache->c_list)) {
@@ -316,10 +316,9 @@ static unsigned long mb_cache_shrink(struct mb_cache *cache,
static unsigned long mb_cache_scan(struct shrinker *shrink,
struct shrink_control *sc)
{
- int nr_to_scan = sc->nr_to_scan;
struct mb_cache *cache = container_of(shrink, struct mb_cache,
c_shrink);
- return mb_cache_shrink(cache, nr_to_scan);
+ return mb_cache_shrink(cache, sc->nr_to_scan);
}
/* We shrink 1/X of the cache when we have too many entries in it */
@@ -341,8 +340,8 @@ static void mb_cache_shrink_worker(struct work_struct *work)
struct mb_cache *mb_cache_create(int bucket_bits)
{
struct mb_cache *cache;
- int bucket_count = 1 << bucket_bits;
- int i;
+ unsigned long bucket_count = 1UL << bucket_bits;
+ unsigned long i;
cache = kzalloc(sizeof(struct mb_cache), GFP_KERNEL);
if (!cache)
--
2.8.0.rc3.226.g39d4020
When mbcache is built as a module, any modules that use it (ext2 and/or
ext4) will depend on its symbols directly, incrementing its reference
count. Therefore, there is no need to do module_get/module_put.
Also note that since the module_get/module_put were in the mbcache
module itself, executing those lines of code was already dependent on
another reference to the mbcache module being held.
Signed-off-by: Eric Biggers <[email protected]>
---
fs/mbcache.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/fs/mbcache.c b/fs/mbcache.c
index c56ab21..07c5d7d 100644
--- a/fs/mbcache.c
+++ b/fs/mbcache.c
@@ -344,9 +344,6 @@ struct mb_cache *mb_cache_create(int bucket_bits)
int bucket_count = 1 << bucket_bits;
int i;
- if (!try_module_get(THIS_MODULE))
- return NULL;
mbcache can be a module that is loaded long after startup, when someone
asks to mount an ext2 or ext4 filesystem. Therefore it should not BUG()
if kmem_cache_create() fails, but rather just fail the module load.
Signed-off-by: Eric Biggers <[email protected]>
---
fs/mbcache.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/fs/mbcache.c b/fs/mbcache.c
index 31e54c2..c56ab21 100644
--- a/fs/mbcache.c
+++ b/fs/mbcache.c
@@ -420,7 +420,8 @@ static int __init mbcache_init(void)
mb_entry_cache = kmem_cache_create("mbcache",
sizeof(struct mb_cache_entry), 0,
SLAB_RECLAIM_ACCOUNT|SLAB_MEM_SPREAD, NULL);
- BUG_ON(!mb_entry_cache);
+ if (!mb_entry_cache)
+ return -ENOMEM;
return 0;
}
--
2.8.0.rc3.226.g39d4020
On Sat 26-11-16 22:17:57, Eric Biggers wrote:
> mbcache entries have an 'e_referenced' bit which users can set with
> mb_cache_entry_touch() to indicate that an entry should be given another
> pass through the LRU list before the shrinker can delete it. However,
> mb_cache_shrink() actually would, when seeing an e_referenced entry at
> the front of the list (the least-recently used end), place it right at
> the front of the list again. The next iteration would then remove the
> entry from the list and delete it. Consequently, e_referenced had
> essentially no effect, so ext2/ext4 xattr blocks would sometimes not be
> reused as often as expected.
>
> Fix this by making the shrinker move e_referenced entries to the back of
> the list rather than the front.
>
> Signed-off-by: Eric Biggers <[email protected]>
Bah, good spotting. You can add:
Reviewed-by: Jan Kara <[email protected]>
BTW, how did you find out?
Honza
> ---
> fs/mbcache.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/mbcache.c b/fs/mbcache.c
> index c5bd19f..31e54c2 100644
> --- a/fs/mbcache.c
> +++ b/fs/mbcache.c
> @@ -286,7 +286,7 @@ static unsigned long mb_cache_shrink(struct mb_cache *cache,
> struct mb_cache_entry, e_list);
> if (entry->e_referenced) {
> entry->e_referenced = 0;
> - list_move_tail(&cache->c_list, &entry->e_list);
> + list_move_tail(&entry->e_list, &cache->c_list);
> continue;
> }
> list_del_init(&entry->e_list);
> --
> 2.8.0.rc3.226.g39d4020
>
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Sat 26-11-16 22:17:58, Eric Biggers wrote:
> mbcache can be a module that is loaded long after startup, when someone
> asks to mount an ext2 or ext4 filesystem. Therefore it should not BUG()
> if kmem_cache_create() fails, but rather just fail the module load.
>
> Signed-off-by: Eric Biggers <[email protected]>
Makes sense. You can add:
Reviewed-by: Jan Kara <[email protected]>
Honza
> ---
> fs/mbcache.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/fs/mbcache.c b/fs/mbcache.c
> index 31e54c2..c56ab21 100644
> --- a/fs/mbcache.c
> +++ b/fs/mbcache.c
> @@ -420,7 +420,8 @@ static int __init mbcache_init(void)
> mb_entry_cache = kmem_cache_create("mbcache",
> sizeof(struct mb_cache_entry), 0,
> SLAB_RECLAIM_ACCOUNT|SLAB_MEM_SPREAD, NULL);
> - BUG_ON(!mb_entry_cache);
> + if (!mb_entry_cache)
> + return -ENOMEM;
> return 0;
> }
>
> --
> 2.8.0.rc3.226.g39d4020
>
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Sat 26-11-16 22:17:59, Eric Biggers wrote:
> When mbcache is built as a module, any modules that use it (ext2 and/or
> ext4) will depend on its symbols directly, incrementing its reference
> count. Therefore, there is no need to do module_get/module_put.
>
> Also note that since the module_get/module_put were in the mbcache
> module itself, executing those lines of code was already dependent on
> another reference to the mbcache module being held.
Correct. You can add:
Reviewed-by: Jan Kara <[email protected]>
Honza
>
> Signed-off-by: Eric Biggers <[email protected]>
> ---
> fs/mbcache.c | 5 -----
> 1 file changed, 5 deletions(-)
>
> diff --git a/fs/mbcache.c b/fs/mbcache.c
> index c56ab21..07c5d7d 100644
> --- a/fs/mbcache.c
> +++ b/fs/mbcache.c
> @@ -344,9 +344,6 @@ struct mb_cache *mb_cache_create(int bucket_bits)
> int bucket_count = 1 << bucket_bits;
> int i;
>
> - if (!try_module_get(THIS_MODULE))
> - return NULL;
> -
> cache = kzalloc(sizeof(struct mb_cache), GFP_KERNEL);
> if (!cache)
> goto err_out;
> @@ -377,7 +374,6 @@ struct mb_cache *mb_cache_create(int bucket_bits)
> return cache;
>
> err_out:
> - module_put(THIS_MODULE);
> return NULL;
> }
> EXPORT_SYMBOL(mb_cache_create);
> @@ -411,7 +407,6 @@ void mb_cache_destroy(struct mb_cache *cache)
> }
> kfree(cache->c_hash);
> kfree(cache);
> - module_put(THIS_MODULE);
> }
> EXPORT_SYMBOL(mb_cache_destroy);
>
> --
> 2.8.0.rc3.226.g39d4020
>
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Sat 26-11-16 22:18:00, Eric Biggers wrote:
> mbcache used several different types to represent the number of entries
> in the cache. For consistency within mbcache and with the shrinker API,
> always use unsigned long.
>
> This does not change behavior for current mbcache users (ext2 and ext4)
> since they limit the entry count to a value which easily fits in an int.
>
> Signed-off-by: Eric Biggers <[email protected]>
OK, why not. You can add:
Reviewed-by: Jan Kara <[email protected]>
Honza
> ---
> fs/mbcache.c | 15 +++++++--------
> 1 file changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/fs/mbcache.c b/fs/mbcache.c
> index 07c5d7d..bf65906 100644
> --- a/fs/mbcache.c
> +++ b/fs/mbcache.c
> @@ -29,7 +29,7 @@ struct mb_cache {
> /* log2 of hash table size */
> int c_bucket_bits;
> /* Maximum entries in cache to avoid degrading hash too much */
> - int c_max_entries;
> + unsigned long c_max_entries;
> /* Protects c_list, c_entry_count */
> spinlock_t c_list_lock;
> struct list_head c_list;
> @@ -43,7 +43,7 @@ struct mb_cache {
> static struct kmem_cache *mb_entry_cache;
>
> static unsigned long mb_cache_shrink(struct mb_cache *cache,
> - unsigned int nr_to_scan);
> + unsigned long nr_to_scan);
>
> static inline struct hlist_bl_head *mb_cache_entry_head(struct mb_cache *cache,
> u32 key)
> @@ -274,11 +274,11 @@ static unsigned long mb_cache_count(struct shrinker *shrink,
>
> /* Shrink number of entries in cache */
> static unsigned long mb_cache_shrink(struct mb_cache *cache,
> - unsigned int nr_to_scan)
> + unsigned long nr_to_scan)
> {
> struct mb_cache_entry *entry;
> struct hlist_bl_head *head;
> - unsigned int shrunk = 0;
> + unsigned long shrunk = 0;
>
> spin_lock(&cache->c_list_lock);
> while (nr_to_scan-- && !list_empty(&cache->c_list)) {
> @@ -316,10 +316,9 @@ static unsigned long mb_cache_shrink(struct mb_cache *cache,
> static unsigned long mb_cache_scan(struct shrinker *shrink,
> struct shrink_control *sc)
> {
> - int nr_to_scan = sc->nr_to_scan;
> struct mb_cache *cache = container_of(shrink, struct mb_cache,
> c_shrink);
> - return mb_cache_shrink(cache, nr_to_scan);
> + return mb_cache_shrink(cache, sc->nr_to_scan);
> }
>
> /* We shrink 1/X of the cache when we have too many entries in it */
> @@ -341,8 +340,8 @@ static void mb_cache_shrink_worker(struct work_struct *work)
> struct mb_cache *mb_cache_create(int bucket_bits)
> {
> struct mb_cache *cache;
> - int bucket_count = 1 << bucket_bits;
> - int i;
> + unsigned long bucket_count = 1UL << bucket_bits;
> + unsigned long i;
>
> cache = kzalloc(sizeof(struct mb_cache), GFP_KERNEL);
> if (!cache)
> --
> 2.8.0.rc3.226.g39d4020
>
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Sat 26-11-16 22:18:01, Eric Biggers wrote:
> mb_cache_entry_find_first() and mb_cache_entry_find_next() only return
> cache entries with the 'e_reusable' bit set. This should be documented.
>
> Signed-off-by: Eric Biggers <[email protected]>
Looks good. You can add:
Reviewed-by: Jan Kara <[email protected]>
Honza
> ---
> fs/mbcache.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/fs/mbcache.c b/fs/mbcache.c
> index bf65906..b19be42 100644
> --- a/fs/mbcache.c
> +++ b/fs/mbcache.c
> @@ -155,12 +155,12 @@ static struct mb_cache_entry *__entry_find(struct mb_cache *cache,
> }
>
> /*
> - * mb_cache_entry_find_first - find the first entry in cache with given key
> + * mb_cache_entry_find_first - find the first reusable entry with the given key
> * @cache: cache where we should search
> * @key: key to look for
> *
> - * Search in @cache for entry with key @key. Grabs reference to the first
> - * entry found and returns the entry.
> + * Search in @cache for a reusable entry with key @key. Grabs reference to the
> + * first reusable entry found and returns the entry.
> */
> struct mb_cache_entry *mb_cache_entry_find_first(struct mb_cache *cache,
> u32 key)
> @@ -170,14 +170,14 @@ struct mb_cache_entry *mb_cache_entry_find_first(struct mb_cache *cache,
> EXPORT_SYMBOL(mb_cache_entry_find_first);
>
> /*
> - * mb_cache_entry_find_next - find next entry in cache with the same
> + * mb_cache_entry_find_next - find next reusable entry with the same key
> * @cache: cache where we should search
> * @entry: entry to start search from
> *
> - * Finds next entry in the hash chain which has the same key as @entry.
> - * If @entry is unhashed (which can happen when deletion of entry races
> - * with the search), finds the first entry in the hash chain. The function
> - * drops reference to @entry and returns with a reference to the found entry.
> + * Finds next reusable entry in the hash chain which has the same key as @entry.
> + * If @entry is unhashed (which can happen when deletion of entry races with the
> + * search), finds the first reusable entry in the hash chain. The function drops
> + * reference to @entry and returns with a reference to the found entry.
> */
> struct mb_cache_entry *mb_cache_entry_find_next(struct mb_cache *cache,
> struct mb_cache_entry *entry)
> --
> 2.8.0.rc3.226.g39d4020
>
--
Jan Kara <[email protected]>
SUSE Labs, CR
On Mon, Nov 28, 2016 at 11:44:20AM +0100, Jan Kara wrote:
> On Sat 26-11-16 22:17:57, Eric Biggers wrote:
> > mbcache entries have an 'e_referenced' bit which users can set with
> > mb_cache_entry_touch() to indicate that an entry should be given another
> > pass through the LRU list before the shrinker can delete it. However,
> > mb_cache_shrink() actually would, when seeing an e_referenced entry at
> > the front of the list (the least-recently used end), place it right at
> > the front of the list again. The next iteration would then remove the
> > entry from the list and delete it. Consequently, e_referenced had
> > essentially no effect, so ext2/ext4 xattr blocks would sometimes not be
> > reused as often as expected.
> >
> > Fix this by making the shrinker move e_referenced entries to the back of
> > the list rather than the front.
> >
> > Signed-off-by: Eric Biggers <[email protected]>
>
> Bah, good spotting. You can add:
>
> Reviewed-by: Jan Kara <[email protected]>
>
> BTW, how did you find out?
>
Nothing special --- I just happened to notice while reading over the code.
Eric
On Sat, Nov 26, 2016 at 10:17:57PM -0800, Eric Biggers wrote:
> mbcache entries have an 'e_referenced' bit which users can set with
> mb_cache_entry_touch() to indicate that an entry should be given another
> pass through the LRU list before the shrinker can delete it. However,
> mb_cache_shrink() actually would, when seeing an e_referenced entry at
> the front of the list (the least-recently used end), place it right at
> the front of the list again. The next iteration would then remove the
> entry from the list and delete it. Consequently, e_referenced had
> essentially no effect, so ext2/ext4 xattr blocks would sometimes not be
> reused as often as expected.
>
> Fix this by making the shrinker move e_referenced entries to the back of
> the list rather than the front.
>
> Signed-off-by: Eric Biggers <[email protected]>
Thanks, applied.
- Ted
On Sat, Nov 26, 2016 at 10:17:58PM -0800, Eric Biggers wrote:
> mbcache can be a module that is loaded long after startup, when someone
> asks to mount an ext2 or ext4 filesystem. Therefore it should not BUG()
> if kmem_cache_create() fails, but rather just fail the module load.
>
> Signed-off-by: Eric Biggers <[email protected]>
Thanks, applied.
- Ted
On Sat, Nov 26, 2016 at 10:17:59PM -0800, Eric Biggers wrote:
> When mbcache is built as a module, any modules that use it (ext2 and/or
> ext4) will depend on its symbols directly, incrementing its reference
> count. Therefore, there is no need to do module_get/module_put.
>
> Also note that since the module_get/module_put were in the mbcache
> module itself, executing those lines of code was already dependent on
> another reference to the mbcache module being held.
>
> Signed-off-by: Eric Biggers <[email protected]>
Thanks, applied.
- Ted
On Sat, Nov 26, 2016 at 10:18:00PM -0800, Eric Biggers wrote:
> mbcache used several different types to represent the number of entries
> in the cache. For consistency within mbcache and with the shrinker API,
> always use unsigned long.
>
> This does not change behavior for current mbcache users (ext2 and ext4)
> since they limit the entry count to a value which easily fits in an int.
>
> Signed-off-by: Eric Biggers <[email protected]>
Thanks, applied.
- Ted
On Sat, Nov 26, 2016 at 10:18:01PM -0800, Eric Biggers wrote:
> mb_cache_entry_find_first() and mb_cache_entry_find_next() only return
> cache entries with the 'e_reusable' bit set. This should be documented.
>
> Signed-off-by: Eric Biggers <[email protected]>
Thanks, applied.
- Ted