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@xxxxxxxxxxxxxx> 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.