| Message ID | 695193a165bf538f35de84334b4da2cc3544abe0.1612248395.git.cgoldswo@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] mm: fs: Invalidate BH LRU during page migration | expand |
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.
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.
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.
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.
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(+)