linux-ext4.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/5] mbcache: correctly handle 'e_referenced' bit
@ 2016-11-27  6:17 Eric Biggers
  2016-11-27  6:17 ` [PATCH 2/5] mbcache: don't BUG() if entry cache cannot be allocated Eric Biggers
                   ` (5 more replies)
  0 siblings, 6 replies; 16+ messages in thread
From: Eric Biggers @ 2016-11-27  6:17 UTC (permalink / raw)
  To: linux-ext4; +Cc: Jan Kara, Andreas Gruenbacher, Eric Biggers

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 <ebiggers@google.com>
---
 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


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 2/5] mbcache: don't BUG() if entry cache cannot be allocated
  2016-11-27  6:17 [PATCH 1/5] mbcache: correctly handle 'e_referenced' bit Eric Biggers
@ 2016-11-27  6:17 ` Eric Biggers
  2016-11-28 12:09   ` Jan Kara
  2016-12-03 20:30   ` Theodore Ts'o
  2016-11-27  6:17 ` [PATCH 3/5] mbcache: remove unnecessary module_get/module_put Eric Biggers
                   ` (4 subsequent siblings)
  5 siblings, 2 replies; 16+ messages in thread
From: Eric Biggers @ 2016-11-27  6:17 UTC (permalink / raw)
  To: linux-ext4; +Cc: Jan Kara, Andreas Gruenbacher, Eric Biggers

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 <ebiggers@google.com>
---
 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


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 3/5] mbcache: remove unnecessary module_get/module_put
  2016-11-27  6:17 [PATCH 1/5] mbcache: correctly handle 'e_referenced' bit Eric Biggers
  2016-11-27  6:17 ` [PATCH 2/5] mbcache: don't BUG() if entry cache cannot be allocated Eric Biggers
@ 2016-11-27  6:17 ` Eric Biggers
  2016-11-28 13:12   ` Jan Kara
  2016-12-03 20:41   ` Theodore Ts'o
  2016-11-27  6:18 ` [PATCH 4/5] mbcache: use consistent type for entry count Eric Biggers
                   ` (3 subsequent siblings)
  5 siblings, 2 replies; 16+ messages in thread
From: Eric Biggers @ 2016-11-27  6:17 UTC (permalink / raw)
  To: linux-ext4; +Cc: Jan Kara, Andreas Gruenbacher, Eric Biggers

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 <ebiggers@google.com>
---
 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;

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 4/5] mbcache: use consistent type for entry count
  2016-11-27  6:17 [PATCH 1/5] mbcache: correctly handle 'e_referenced' bit Eric Biggers
  2016-11-27  6:17 ` [PATCH 2/5] mbcache: don't BUG() if entry cache cannot be allocated Eric Biggers
  2016-11-27  6:17 ` [PATCH 3/5] mbcache: remove unnecessary module_get/module_put Eric Biggers
@ 2016-11-27  6:18 ` Eric Biggers
  2016-11-28 13:18   ` Jan Kara
  2016-12-03 20:54   ` Theodore Ts'o
  2016-11-27  6:18 ` [PATCH 5/5] mbcache: document that "find" functions only return reusable entries Eric Biggers
                   ` (2 subsequent siblings)
  5 siblings, 2 replies; 16+ messages in thread
From: Eric Biggers @ 2016-11-27  6:18 UTC (permalink / raw)
  To: linux-ext4; +Cc: Jan Kara, Andreas Gruenbacher, Eric Biggers

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 <ebiggers@google.com>
---
 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


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 5/5] mbcache: document that "find" functions only return reusable entries
  2016-11-27  6:17 [PATCH 1/5] mbcache: correctly handle 'e_referenced' bit Eric Biggers
                   ` (2 preceding siblings ...)
  2016-11-27  6:18 ` [PATCH 4/5] mbcache: use consistent type for entry count Eric Biggers
@ 2016-11-27  6:18 ` Eric Biggers
  2016-11-28 13:19   ` Jan Kara
  2016-12-03 20:56   ` Theodore Ts'o
  2016-11-28 10:44 ` [PATCH 1/5] mbcache: correctly handle 'e_referenced' bit Jan Kara
  2016-12-03 20:28 ` Theodore Ts'o
  5 siblings, 2 replies; 16+ messages in thread
From: Eric Biggers @ 2016-11-27  6:18 UTC (permalink / raw)
  To: linux-ext4; +Cc: Jan Kara, Andreas Gruenbacher, Eric Biggers

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 <ebiggers@google.com>
---
 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


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/5] mbcache: correctly handle 'e_referenced' bit
  2016-11-27  6:17 [PATCH 1/5] mbcache: correctly handle 'e_referenced' bit Eric Biggers
                   ` (3 preceding siblings ...)
  2016-11-27  6:18 ` [PATCH 5/5] mbcache: document that "find" functions only return reusable entries Eric Biggers
@ 2016-11-28 10:44 ` Jan Kara
  2016-11-28 17:32   ` Eric Biggers
  2016-12-03 20:28 ` Theodore Ts'o
  5 siblings, 1 reply; 16+ messages in thread
From: Jan Kara @ 2016-11-28 10:44 UTC (permalink / raw)
  To: Eric Biggers; +Cc: linux-ext4, Jan Kara, Andreas Gruenbacher

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 <ebiggers@google.com>

Bah, good spotting. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

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 <jack@suse.com>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/5] mbcache: don't BUG() if entry cache cannot be allocated
  2016-11-27  6:17 ` [PATCH 2/5] mbcache: don't BUG() if entry cache cannot be allocated Eric Biggers
@ 2016-11-28 12:09   ` Jan Kara
  2016-12-03 20:30   ` Theodore Ts'o
  1 sibling, 0 replies; 16+ messages in thread
From: Jan Kara @ 2016-11-28 12:09 UTC (permalink / raw)
  To: Eric Biggers; +Cc: linux-ext4, Jan Kara, Andreas Gruenbacher

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 <ebiggers@google.com>

Makes sense. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								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 <jack@suse.com>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 3/5] mbcache: remove unnecessary module_get/module_put
  2016-11-27  6:17 ` [PATCH 3/5] mbcache: remove unnecessary module_get/module_put Eric Biggers
@ 2016-11-28 13:12   ` Jan Kara
  2016-12-03 20:41   ` Theodore Ts'o
  1 sibling, 0 replies; 16+ messages in thread
From: Jan Kara @ 2016-11-28 13:12 UTC (permalink / raw)
  To: Eric Biggers; +Cc: linux-ext4, Jan Kara, Andreas Gruenbacher

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 <jack@suse.cz>

								Honza

> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  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 <jack@suse.com>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 4/5] mbcache: use consistent type for entry count
  2016-11-27  6:18 ` [PATCH 4/5] mbcache: use consistent type for entry count Eric Biggers
@ 2016-11-28 13:18   ` Jan Kara
  2016-12-03 20:54   ` Theodore Ts'o
  1 sibling, 0 replies; 16+ messages in thread
From: Jan Kara @ 2016-11-28 13:18 UTC (permalink / raw)
  To: Eric Biggers; +Cc: linux-ext4, Jan Kara, Andreas Gruenbacher

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 <ebiggers@google.com>

OK, why not. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								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 <jack@suse.com>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 5/5] mbcache: document that "find" functions only return reusable entries
  2016-11-27  6:18 ` [PATCH 5/5] mbcache: document that "find" functions only return reusable entries Eric Biggers
@ 2016-11-28 13:19   ` Jan Kara
  2016-12-03 20:56   ` Theodore Ts'o
  1 sibling, 0 replies; 16+ messages in thread
From: Jan Kara @ 2016-11-28 13:19 UTC (permalink / raw)
  To: Eric Biggers; +Cc: linux-ext4, Jan Kara, Andreas Gruenbacher

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 <ebiggers@google.com>

Looks good. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								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 <jack@suse.com>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/5] mbcache: correctly handle 'e_referenced' bit
  2016-11-28 10:44 ` [PATCH 1/5] mbcache: correctly handle 'e_referenced' bit Jan Kara
@ 2016-11-28 17:32   ` Eric Biggers
  0 siblings, 0 replies; 16+ messages in thread
From: Eric Biggers @ 2016-11-28 17:32 UTC (permalink / raw)
  To: Jan Kara; +Cc: linux-ext4, Andreas Gruenbacher

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 <ebiggers@google.com>
> 
> Bah, good spotting. You can add:
> 
> Reviewed-by: Jan Kara <jack@suse.cz>
> 
> BTW, how did you find out?
> 

Nothing special --- I just happened to notice while reading over the code.

Eric

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/5] mbcache: correctly handle 'e_referenced' bit
  2016-11-27  6:17 [PATCH 1/5] mbcache: correctly handle 'e_referenced' bit Eric Biggers
                   ` (4 preceding siblings ...)
  2016-11-28 10:44 ` [PATCH 1/5] mbcache: correctly handle 'e_referenced' bit Jan Kara
@ 2016-12-03 20:28 ` Theodore Ts'o
  5 siblings, 0 replies; 16+ messages in thread
From: Theodore Ts'o @ 2016-12-03 20:28 UTC (permalink / raw)
  To: Eric Biggers; +Cc: linux-ext4, Jan Kara, Andreas Gruenbacher

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 <ebiggers@google.com>

Thanks, applied.

					- Ted

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/5] mbcache: don't BUG() if entry cache cannot be allocated
  2016-11-27  6:17 ` [PATCH 2/5] mbcache: don't BUG() if entry cache cannot be allocated Eric Biggers
  2016-11-28 12:09   ` Jan Kara
@ 2016-12-03 20:30   ` Theodore Ts'o
  1 sibling, 0 replies; 16+ messages in thread
From: Theodore Ts'o @ 2016-12-03 20:30 UTC (permalink / raw)
  To: Eric Biggers; +Cc: linux-ext4, Jan Kara, Andreas Gruenbacher

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 <ebiggers@google.com>

Thanks, applied.

					- Ted

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 3/5] mbcache: remove unnecessary module_get/module_put
  2016-11-27  6:17 ` [PATCH 3/5] mbcache: remove unnecessary module_get/module_put Eric Biggers
  2016-11-28 13:12   ` Jan Kara
@ 2016-12-03 20:41   ` Theodore Ts'o
  1 sibling, 0 replies; 16+ messages in thread
From: Theodore Ts'o @ 2016-12-03 20:41 UTC (permalink / raw)
  To: Eric Biggers; +Cc: linux-ext4, Jan Kara, Andreas Gruenbacher

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 <ebiggers@google.com>

Thanks, applied.

					- Ted

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 4/5] mbcache: use consistent type for entry count
  2016-11-27  6:18 ` [PATCH 4/5] mbcache: use consistent type for entry count Eric Biggers
  2016-11-28 13:18   ` Jan Kara
@ 2016-12-03 20:54   ` Theodore Ts'o
  1 sibling, 0 replies; 16+ messages in thread
From: Theodore Ts'o @ 2016-12-03 20:54 UTC (permalink / raw)
  To: Eric Biggers; +Cc: linux-ext4, Jan Kara, Andreas Gruenbacher

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 <ebiggers@google.com>

Thanks, applied.

					- Ted

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 5/5] mbcache: document that "find" functions only return reusable entries
  2016-11-27  6:18 ` [PATCH 5/5] mbcache: document that "find" functions only return reusable entries Eric Biggers
  2016-11-28 13:19   ` Jan Kara
@ 2016-12-03 20:56   ` Theodore Ts'o
  1 sibling, 0 replies; 16+ messages in thread
From: Theodore Ts'o @ 2016-12-03 20:56 UTC (permalink / raw)
  To: Eric Biggers; +Cc: linux-ext4, Jan Kara, Andreas Gruenbacher

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 <ebiggers@google.com>

Thanks, applied.

					- Ted

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2016-12-03 20:56 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-11-27  6:17 [PATCH 1/5] mbcache: correctly handle 'e_referenced' bit Eric Biggers
2016-11-27  6:17 ` [PATCH 2/5] mbcache: don't BUG() if entry cache cannot be allocated Eric Biggers
2016-11-28 12:09   ` Jan Kara
2016-12-03 20:30   ` Theodore Ts'o
2016-11-27  6:17 ` [PATCH 3/5] mbcache: remove unnecessary module_get/module_put Eric Biggers
2016-11-28 13:12   ` Jan Kara
2016-12-03 20:41   ` Theodore Ts'o
2016-11-27  6:18 ` [PATCH 4/5] mbcache: use consistent type for entry count Eric Biggers
2016-11-28 13:18   ` Jan Kara
2016-12-03 20:54   ` Theodore Ts'o
2016-11-27  6:18 ` [PATCH 5/5] mbcache: document that "find" functions only return reusable entries Eric Biggers
2016-11-28 13:19   ` Jan Kara
2016-12-03 20:56   ` Theodore Ts'o
2016-11-28 10:44 ` [PATCH 1/5] mbcache: correctly handle 'e_referenced' bit Jan Kara
2016-11-28 17:32   ` Eric Biggers
2016-12-03 20:28 ` Theodore Ts'o

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).