[RFC] Invalidate BH LRU during page migration

[RFC] Invalidate BH LRU during page migration

[Chris Goldsworthy]

A page containing buffer_heads can be pinned if any of its constituent
buffer_heads belongs to the BH LRU cache [1]. After going through
several iterations of a patch that attempts to solve this by removing
BH entries inside of the drop_buffers() function, which in
the worst-case could be called for each migrated page, Minchan Kim
suggested that we invalidate the entire BH LRU once, just before we
start migrating pages.  Additionally, Matthew Wilcox suggested that
we invalidate the BH LRU inside of lru_add_drain_all(), so as to
benefit functions like other functions that would be impacted by
pinned pages [2].

TODO:
 - It should be possible to remove the initial setting of
   bh_migration_done = false; in migrate_prep by passing this in as a
   parameter to invalidate_bh_lru(), but we'd still need a matching
   bh_migration_done = true; call. 
 - To really benefit other callers of lru_add_drain_all() other than
   __alloc_contig_migrate_range() in the CMA allocaiton path, we'd need
  to add additional calls of bh_migration_done = false;

[1] https://elixir.bootlin.com/linux/latest/source/fs/buffer.c#L1238
[2] https://lore.kernel.org/linux-fsdevel/cover.1611642038.git.cgoldswo@codeaurora.org/


Chris Goldsworthy (1):
  [RFC] mm: fs: Invalidate BH LRU during page migration

 fs/buffer.c                 |  6 ++++++
 include/linux/buffer_head.h |  3 +++
 include/linux/migrate.h     |  2 ++
 mm/migrate.c                | 18 ++++++++++++++++++
 mm/page_alloc.c             |  3 +++
 mm/swap.c                   |  3 +++
 6 files changed, 35 insertions(+)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH] [RFC] mm: fs: Invalidate BH LRU during page migration

[Chris Goldsworthy]

Pages containing buffer_heads that are in the buffer_head LRU cache
will be pinned and thus cannot be migrated.  Correspondingly,
invalidate the BH LRU before a migration starts and stop any
buffer_head from being cached in the LRU, until migration has
finished.

Signed-off-by: Chris Goldsworthy <cgoldswo@codeaurora.org>
Cc: Minchan Kim <minchan@kernel.org>
Cc: Matthew Wilcox <willy@infradead.org>
---
 fs/buffer.c                 |  6 ++++++
 include/linux/buffer_head.h |  3 +++
 include/linux/migrate.h     |  2 ++
 mm/migrate.c                | 18 ++++++++++++++++++
 mm/page_alloc.c             |  3 +++
 mm/swap.c                   |  3 +++
 6 files changed, 35 insertions(+)

diff --git a/fs/buffer.c b/fs/buffer.c
index 96c7604..39ec4ec 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -1289,6 +1289,8 @@ static inline void check_irqs_on(void)
 #endif
 }
 
+bool bh_migration_done = true;
+
 /*
  * Install a buffer_head into this cpu's LRU.  If not already in the LRU, it is
  * inserted at the front, and the buffer_head at the back if any is evicted.
@@ -1303,6 +1305,9 @@ static void bh_lru_install(struct buffer_head *bh)
 	check_irqs_on();
 	bh_lru_lock();
 
+	if (!bh_migration_done)
+		goto out;
+
 	b = this_cpu_ptr(&bh_lrus);
 	for (i = 0; i < BH_LRU_SIZE; i++) {
 		swap(evictee, b->bhs[i]);
@@ -1313,6 +1318,7 @@ static void bh_lru_install(struct buffer_head *bh)
 	}
 
 	get_bh(bh);
+out:
 	bh_lru_unlock();
 	brelse(evictee);
 }
diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
index 6b47f94..ae4eb6d 100644
--- a/include/linux/buffer_head.h
+++ b/include/linux/buffer_head.h
@@ -193,6 +193,9 @@ void __breadahead_gfp(struct block_device *, sector_t block, unsigned int size,
 		  gfp_t gfp);
 struct buffer_head *__bread_gfp(struct block_device *,
 				sector_t block, unsigned size, gfp_t gfp);
+
+extern bool bh_migration_done;
+
 void invalidate_bh_lrus(void);
 struct buffer_head *alloc_buffer_head(gfp_t gfp_flags);
 void free_buffer_head(struct buffer_head * bh);
diff --git a/include/linux/migrate.h b/include/linux/migrate.h
index 3a38963..9e4a2dc 100644
--- a/include/linux/migrate.h
+++ b/include/linux/migrate.h
@@ -46,6 +46,7 @@ extern int isolate_movable_page(struct page *page, isolate_mode_t mode);
 extern void putback_movable_page(struct page *page);
 
 extern void migrate_prep(void);
+extern void migrate_finish(void);
 extern void migrate_prep_local(void);
 extern void migrate_page_states(struct page *newpage, struct page *page);
 extern void migrate_page_copy(struct page *newpage, struct page *page);
@@ -67,6 +68,7 @@ static inline int isolate_movable_page(struct page *page, isolate_mode_t mode)
 	{ return -EBUSY; }
 
 static inline int migrate_prep(void) { return -ENOSYS; }
+static inline int migrate_finish(void) { return -ENOSYS; }
 static inline int migrate_prep_local(void) { return -ENOSYS; }
 
 static inline void migrate_page_states(struct page *newpage, struct page *page)
diff --git a/mm/migrate.c b/mm/migrate.c
index a69da8a..08c981d 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -64,6 +64,19 @@
  */
 void migrate_prep(void)
 {
+	bh_migration_done = false;
+
+	/*
+	 * This barrier ensures that callers of bh_lru_install() between
+	 * the barrier and the call to invalidate_bh_lrus() read
+	 *  bh_migration_done() as false.
+	 */
+	/*
+	 * TODO: Remove me? lru_add_drain_all() already has an smp_mb(),
+	 * but it would be good to ensure that the barrier isn't forgotten.
+	 */
+	smp_mb();
+
 	/*
 	 * Clear the LRU lists so pages can be isolated.
 	 * Note that pages may be moved off the LRU after we have
@@ -73,6 +86,11 @@ void migrate_prep(void)
 	lru_add_drain_all();
 }
 
+void migrate_finish(void)
+{
+	bh_migration_done = true;
+}
+
 /* Do the necessary work of migrate_prep but not if it involves other CPUs */
 void migrate_prep_local(void)
 {
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 6446778..e4cb959 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -8493,6 +8493,9 @@ static int __alloc_contig_migrate_range(struct compact_control *cc,
 		ret = migrate_pages(&cc->migratepages, alloc_migration_target,
 				NULL, (unsigned long)&mtc, cc->mode, MR_CONTIG_RANGE);
 	}
+
+	migrate_finish();
+
 	if (ret < 0) {
 		putback_movable_pages(&cc->migratepages);
 		return ret;
diff --git a/mm/swap.c b/mm/swap.c
index 31b844d..97efc49 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -36,6 +36,7 @@
 #include <linux/hugetlb.h>
 #include <linux/page_idle.h>
 #include <linux/local_lock.h>
+#include <linux/buffer_head.h>
 
 #include "internal.h"
 
@@ -759,6 +760,8 @@ void lru_add_drain_all(void)
 	if (WARN_ON(!mm_percpu_wq))
 		return;
 
+	invalidate_bh_lrus();
+
 	/*
 	 * Guarantee pagevec counter stores visible by this CPU are visible to
 	 * other CPUs before loading the current drain generation.
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH] [RFC] mm: fs: Invalidate BH LRU during page migration

[Andrew Morton]

On Mon,  1 Feb 2021 22:55:47 -0800 Chris Goldsworthy <cgoldswo@codeaurora.org> wrote:

> Pages containing buffer_heads that are in the buffer_head LRU cache
> will be pinned and thus cannot be migrated.  Correspondingly,
> invalidate the BH LRU before a migration starts and stop any
> buffer_head from being cached in the LRU, until migration has
> finished.

It's 16 pages max, system-wide.  That isn't much.

Please include here a full description of why this is a problem and how
serious it is and of the user-visible impact.

> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -1289,6 +1289,8 @@ static inline void check_irqs_on(void)
>  #endif
>  }
>  
> +bool bh_migration_done = true;
> +
>  /*
>   * Install a buffer_head into this cpu's LRU.  If not already in the LRU, it is
>   * inserted at the front, and the buffer_head at the back if any is evicted.
> @@ -1303,6 +1305,9 @@ static void bh_lru_install(struct buffer_head *bh)
>  	check_irqs_on();
>  	bh_lru_lock();
>  
> +	if (!bh_migration_done)
> +		goto out;
> +
>  	b = this_cpu_ptr(&bh_lrus);
>  	for (i = 0; i < BH_LRU_SIZE; i++) {
>  		swap(evictee, b->bhs[i]);

Seems a bit hacky, but I guess it'll work.

I expect the code won't compile with CONFIG_BLOCK=n &&
CONFIG_MIGRATION=y.  Due to bh_migration_done being unimplemented.

I suggest you add an interface function (buffer_block_lrus()?) and
arrange for an empty inlined stub version when CONFIG_BLOCK=n.

>
> ..
>
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -64,6 +64,19 @@
>   */
>  void migrate_prep(void)
>  {
> +	bh_migration_done = false;
> +
> +	/*
> +	 * This barrier ensures that callers of bh_lru_install() between
> +	 * the barrier and the call to invalidate_bh_lrus() read
> +	 *  bh_migration_done() as false.
> +	 */
> +	/*
> +	 * TODO: Remove me? lru_add_drain_all() already has an smp_mb(),
> +	 * but it would be good to ensure that the barrier isn't forgotten.
> +	 */
> +	smp_mb();

This stuff can be taken care of over in buffer_block_lrus() in
fs/buffer.c.

> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -36,6 +36,7 @@
>  #include <linux/hugetlb.h>
>  #include <linux/page_idle.h>
>  #include <linux/local_lock.h>
> +#include <linux/buffer_head.h>
>  
>  #include "internal.h"
>  
> @@ -759,6 +760,8 @@ void lru_add_drain_all(void)
>  	if (WARN_ON(!mm_percpu_wq))
>  		return;
>  
> +	invalidate_bh_lrus();
> +

Add a comment explaining why we're doing this?  Mention that bn_lru
buffers can pin pages, preventing migration.

Re: [PATCH] [RFC] mm: fs: Invalidate BH LRU during page migration

[Minchan Kim]

On Mon, Feb 01, 2021 at 10:55:47PM -0800, Chris Goldsworthy wrote:
> Pages containing buffer_heads that are in the buffer_head LRU cache
> will be pinned and thus cannot be migrated.  Correspondingly,
> invalidate the BH LRU before a migration starts and stop any
> buffer_head from being cached in the LRU, until migration has
> finished.

Thanks for the work, Chris. I have a few of comments below.

> 
> Signed-off-by: Chris Goldsworthy <cgoldswo@codeaurora.org>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: Matthew Wilcox <willy@infradead.org>
> ---
>  fs/buffer.c                 |  6 ++++++
>  include/linux/buffer_head.h |  3 +++
>  include/linux/migrate.h     |  2 ++
>  mm/migrate.c                | 18 ++++++++++++++++++
>  mm/page_alloc.c             |  3 +++
>  mm/swap.c                   |  3 +++
>  6 files changed, 35 insertions(+)
> 
> diff --git a/fs/buffer.c b/fs/buffer.c
> index 96c7604..39ec4ec 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -1289,6 +1289,8 @@ static inline void check_irqs_on(void)
>  #endif
>  }
>  
> +bool bh_migration_done = true;

How about "bh_lru_disable"?

> +
>  /*
>   * Install a buffer_head into this cpu's LRU.  If not already in the LRU, it is
>   * inserted at the front, and the buffer_head at the back if any is evicted.
> @@ -1303,6 +1305,9 @@ static void bh_lru_install(struct buffer_head *bh)
>  	check_irqs_on();
>  	bh_lru_lock();
>  
> +	if (!bh_migration_done)
> +		goto out;
> +

Let's add why we want it in the description in bh_lru_install's description.

>  	b = this_cpu_ptr(&bh_lrus);
>  	for (i = 0; i < BH_LRU_SIZE; i++) {
>  		swap(evictee, b->bhs[i]);
> @@ -1313,6 +1318,7 @@ static void bh_lru_install(struct buffer_head *bh)
>  	}
>  
>  	get_bh(bh);
> +out:
>  	bh_lru_unlock();
>  	brelse(evictee);
>  }
> diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
> index 6b47f94..ae4eb6d 100644
> --- a/include/linux/buffer_head.h
> +++ b/include/linux/buffer_head.h
> @@ -193,6 +193,9 @@ void __breadahead_gfp(struct block_device *, sector_t block, unsigned int size,
>  		  gfp_t gfp);
>  struct buffer_head *__bread_gfp(struct block_device *,
>  				sector_t block, unsigned size, gfp_t gfp);
> +
> +extern bool bh_migration_done;
> +
>  void invalidate_bh_lrus(void);
>  struct buffer_head *alloc_buffer_head(gfp_t gfp_flags);
>  void free_buffer_head(struct buffer_head * bh);
> diff --git a/include/linux/migrate.h b/include/linux/migrate.h
> index 3a38963..9e4a2dc 100644
> --- a/include/linux/migrate.h
> +++ b/include/linux/migrate.h
> @@ -46,6 +46,7 @@ extern int isolate_movable_page(struct page *page, isolate_mode_t mode);
>  extern void putback_movable_page(struct page *page);
>  
>  extern void migrate_prep(void);
> +extern void migrate_finish(void);
>  extern void migrate_prep_local(void);
>  extern void migrate_page_states(struct page *newpage, struct page *page);
>  extern void migrate_page_copy(struct page *newpage, struct page *page);
> @@ -67,6 +68,7 @@ static inline int isolate_movable_page(struct page *page, isolate_mode_t mode)
>  	{ return -EBUSY; }
>  
>  static inline int migrate_prep(void) { return -ENOSYS; }
> +static inline int migrate_finish(void) { return -ENOSYS; }
>  static inline int migrate_prep_local(void) { return -ENOSYS; }
>  
>  static inline void migrate_page_states(struct page *newpage, struct page *page)
> diff --git a/mm/migrate.c b/mm/migrate.c
> index a69da8a..08c981d 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -64,6 +64,19 @@
>   */
>  void migrate_prep(void)
>  {
> +	bh_migration_done = false;
> +
> +	/*
> +	 * This barrier ensures that callers of bh_lru_install() between
> +	 * the barrier and the call to invalidate_bh_lrus() read
> +	 *  bh_migration_done() as false.
> +	 */
> +	/*
> +	 * TODO: Remove me? lru_add_drain_all() already has an smp_mb(),
> +	 * but it would be good to ensure that the barrier isn't forgotten.
> +	 */
> +	smp_mb();
> +
>  	/*
>  	 * Clear the LRU lists so pages can be isolated.
>  	 * Note that pages may be moved off the LRU after we have
> @@ -73,6 +86,11 @@ void migrate_prep(void)
>  	lru_add_drain_all();
>  }
>  
> +void migrate_finish(void)
> +{
> +	bh_migration_done = true;
> +}
> +
>  /* Do the necessary work of migrate_prep but not if it involves other CPUs */
>  void migrate_prep_local(void)
>  {
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 6446778..e4cb959 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -8493,6 +8493,9 @@ static int __alloc_contig_migrate_range(struct compact_control *cc,
>  		ret = migrate_pages(&cc->migratepages, alloc_migration_target,
>  				NULL, (unsigned long)&mtc, cc->mode, MR_CONTIG_RANGE);
>  	}
> +
> +	migrate_finish();
> +
>  	if (ret < 0) {
>  		putback_movable_pages(&cc->migratepages);
>  		return ret;
> diff --git a/mm/swap.c b/mm/swap.c
> index 31b844d..97efc49 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -36,6 +36,7 @@
>  #include <linux/hugetlb.h>
>  #include <linux/page_idle.h>
>  #include <linux/local_lock.h>
> +#include <linux/buffer_head.h>
>  
>  #include "internal.h"
>  
> @@ -759,6 +760,8 @@ void lru_add_drain_all(void)
>  	if (WARN_ON(!mm_percpu_wq))
>  		return;
>  
> +	invalidate_bh_lrus();

Instead of adding a new IPI there, how about adding need_bh_lru_drain(cpu)
in lru_add_drain_all and then calls invalidate_bh_lru in lru_add_drain_cpu?
Not a strong but looks like more harmonized with existing LRU draining
code.

Thanks for the work.

Re: [PATCH] [RFC] mm: fs: Invalidate BH LRU during page migration

[Matthew Wilcox]

On Mon, Feb 01, 2021 at 10:55:47PM -0800, Chris Goldsworthy wrote:
> @@ -1289,6 +1289,8 @@ static inline void check_irqs_on(void)
>  #endif
>  }
>  
> +bool bh_migration_done = true;

What protects this global variable?  Or is there some subtle reason it
doesn't need protection, in which case, please put that in a comment.