Bdev inodes are routinely shared by multiple memcgs. Recording their owner wb as foreign can flush unrelated file data when a source memcg enters dirty throttling. Record bdev device numbers separately and queue best-effort work on memcg_bdev_frn_flusher to write only their mappings. Keep the existing foreign-wb mechanism for non-bdev inodes. Bdev foreign flushes can be triggered frequently. Use a dedicated workqueue to avoid interfering with tasks on existing workqueues. If allocation fails, retain the existing foreign-wb path. Use fixed per-memcg slots, with a shared lock protecting device records and in-flight state. Tracking is best effort: record only dev_t without holding device or inode references. Skip closed devices or devices whose open_mutex is busy. Device removal and device-number reuse may race with lookup. Fixes: 97b27821b485 ("writeback, memcg: Implement foreign dirty flushing") Suggested-by: Jan Kara Signed-off-by: Julian Sun --- include/linux/memcontrol.h | 15 ++++++ mm/memcontrol.c | 106 ++++++++++++++++++++++++++++++++++--- 2 files changed, 113 insertions(+), 8 deletions(-) diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index 7d1c0ce189a8..d44d3ce51b50 100644 --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -176,6 +176,18 @@ struct memcg_cgwb_frn { struct wb_completion done; /* tracks in-flight foreign writebacks */ }; +/* + * frn_lock protects dev and inflight. + * No extra memcg reference is taken: this work is embedded in the memcg, + * and mem_cgroup_css_free() waits for it to finish before freeing the memcg. + */ +struct bdev_frn_flush_ctx { + struct work_struct work; + dev_t dev; + bool inflight; + struct mem_cgroup *memcg; +}; + /* * Bucket for arbitrarily byte-sized objects charged to a memory * cgroup. The bucket can be reparented in one piece when the cgroup @@ -279,6 +291,9 @@ struct mem_cgroup { #ifdef CONFIG_CGROUP_WRITEBACK struct wb_domain cgwb_domain; struct memcg_cgwb_frn cgwb_frn[MEMCG_CGWB_FRN_CNT]; + struct bdev_frn_flush_ctx bdev_frn[MEMCG_CGWB_FRN_CNT]; + /* Nests inside mapping->i_pages. */ + spinlock_t frn_lock; #endif #ifdef CONFIG_LRU_GEN_WALKS_MMU diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 1271d390b617..aac8c893d7d2 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -39,6 +39,7 @@ #include #include #include +#include #include #include #include @@ -104,6 +105,7 @@ static struct kmem_cache *memcg_pn_cachep; #ifdef CONFIG_CGROUP_WRITEBACK static DECLARE_WAIT_QUEUE_HEAD(memcg_cgwb_frn_waitq); +static struct workqueue_struct *memcg_bdev_frn_flusher __ro_after_init; #endif static inline bool task_is_dying(void) @@ -3845,6 +3847,41 @@ void mem_cgroup_wb_stats(struct bdi_writeback *wb, unsigned long *pfilepages, } } +/* + * Bdev inodes are commonly shared by many memcgs. Flushing their owner wb + * can write unrelated file data, so record device numbers separately and + * flush only the bdev mappings. Tracking is best effort: record only dev_t + * without holding device or inode references, so it may race with device + * removal and re-addition. + */ +static void mem_cgroup_track_foreign_bdev(struct mem_cgroup *memcg, dev_t dev) +{ + struct bdev_frn_flush_ctx *ctx; + unsigned long flags; + int i; + + /* + * __folio_mark_dirty() takes mapping->i_pages with xa_lock_irqsave() + * before reaching this helper. Use irqsave here as well so frn_lock's + * IRQ protection does not depend on that outer locking. + */ + spin_lock_irqsave(&memcg->frn_lock, flags); + for (i = 0; i < MEMCG_CGWB_FRN_CNT; i++) { + if (memcg->bdev_frn[i].dev == dev) + goto out; + } + + for (i = 0; i < MEMCG_CGWB_FRN_CNT; i++) { + ctx = &memcg->bdev_frn[i]; + if (ctx->inflight) + continue; + ctx->dev = dev; + break; + } +out: + spin_unlock_irqrestore(&memcg->frn_lock, flags); +} + /* * Foreign dirty flushing * @@ -3875,17 +3912,17 @@ void mem_cgroup_wb_stats(struct bdi_writeback *wb, unsigned long *pfilepages, * most recent foreign dirtying events and initiating remote flushes on * them when local writeback isn't enough to keep the memory clean enough. * - * The following two functions implement such mechanism. When a foreign - * page - a page whose memcg and writeback ownerships don't match - is - * dirtied, mem_cgroup_track_foreign_dirty() records the inode owning - * bdi_writeback on the page owning memcg. When balance_dirty_pages() - * decides that the memcg needs to sleep due to high dirty ratio, it calls + * For non-bdev inodes, when a foreign page - a page whose memcg and + * writeback ownerships don't match - is dirtied, + * mem_cgroup_track_foreign_dirty() records the inode owning bdi_writeback + * on the page owning memcg. When balance_dirty_pages() decides that the + * memcg needs to sleep due to high dirty ratio, it calls * mem_cgroup_flush_foreign() which queues writeback on the recorded * foreign bdi_writebacks which haven't expired. Both the numbers of * recorded bdi_writebacks and concurrent in-flight foreign writebacks are * limited to MEMCG_CGWB_FRN_CNT. * - * The mechanism only remembers IDs and doesn't hold any object references. + * These wb records only remember IDs and don't hold any object references. * As being wrong occasionally doesn't matter, updates and accesses to the * records are lockless and racy. */ @@ -3898,9 +3935,17 @@ void mem_cgroup_track_foreign_dirty_slowpath(struct folio *folio, u64 oldest_at = now; int oldest = -1; int i; + struct address_space *mapping = folio_mapping(folio); + struct inode *bdev_inode = mapping ? mapping->host : NULL; trace_track_foreign_dirty(folio, wb); + if (memcg_bdev_frn_flusher && bdev_inode && + sb_is_blkdev_sb(bdev_inode->i_sb)) { + mem_cgroup_track_foreign_bdev(memcg, bdev_inode->i_rdev); + return; + } + /* * Pick the slot to use. If there is already a slot for @wb, keep * using it. If not replace the oldest one which isn't being @@ -3941,6 +3986,25 @@ void mem_cgroup_track_foreign_dirty_slowpath(struct folio *folio, } } +static void bdev_frn_flush_work(struct work_struct *work) +{ + struct bdev_frn_flush_ctx *ctx = + container_of(work, struct bdev_frn_flush_ctx, work); + unsigned long flags; + + bdev_flush_by_dev(ctx->dev); + + /* + * The dirty tracking path takes frn_lock while holding mapping->i_pages. + * Disable local IRQs here to avoid deadlocks with I/O completion + * handlers that also take mapping->i_pages. + */ + spin_lock_irqsave(&ctx->memcg->frn_lock, flags); + ctx->inflight = false; + ctx->dev = 0; + spin_unlock_irqrestore(&ctx->memcg->frn_lock, flags); +} + /* issue foreign writeback flushes for recorded foreign dirtying events */ void mem_cgroup_flush_foreign(struct bdi_writeback *wb) { @@ -3949,6 +4013,18 @@ void mem_cgroup_flush_foreign(struct bdi_writeback *wb) u64 now = jiffies_64; int i; + for (i = 0; i < MEMCG_CGWB_FRN_CNT; i++) { + struct bdev_frn_flush_ctx *ctx = &memcg->bdev_frn[i]; + unsigned long flags; + + spin_lock_irqsave(&memcg->frn_lock, flags); + if (!ctx->inflight && ctx->dev) { + ctx->inflight = true; + queue_work(memcg_bdev_frn_flusher, &ctx->work); + } + spin_unlock_irqrestore(&memcg->frn_lock, flags); + } + for (i = 0; i < MEMCG_CGWB_FRN_CNT; i++) { struct memcg_cgwb_frn *frn = &memcg->cgwb_frn[i]; @@ -4202,9 +4278,15 @@ static struct mem_cgroup *mem_cgroup_alloc(struct mem_cgroup *parent) memcg->kmemcg_id = -1; #ifdef CONFIG_CGROUP_WRITEBACK INIT_LIST_HEAD(&memcg->cgwb_list); - for (i = 0; i < MEMCG_CGWB_FRN_CNT; i++) + for (i = 0; i < MEMCG_CGWB_FRN_CNT; i++) { + struct bdev_frn_flush_ctx *ctx = &memcg->bdev_frn[i]; + memcg->cgwb_frn[i].done = __WB_COMPLETION_INIT(&memcg_cgwb_frn_waitq); + INIT_WORK(&ctx->work, bdev_frn_flush_work); + ctx->memcg = memcg; + } + spin_lock_init(&memcg->frn_lock); #endif lru_gen_init_memcg(memcg); return memcg; @@ -4386,8 +4468,10 @@ static void mem_cgroup_css_free(struct cgroup_subsys_state *css) int __maybe_unused i; #ifdef CONFIG_CGROUP_WRITEBACK - for (i = 0; i < MEMCG_CGWB_FRN_CNT; i++) + for (i = 0; i < MEMCG_CGWB_FRN_CNT; i++) { wb_wait_for_completion(&memcg->cgwb_frn[i].done); + flush_work(&memcg->bdev_frn[i].work); + } #endif if (cgroup_subsys_on_dfl(memory_cgrp_subsys) && !cgroup_memory_nosocket) static_branch_dec(&memcg_sockets_enabled_key); @@ -5703,6 +5787,12 @@ int __init mem_cgroup_init(void) memcg_wq = alloc_workqueue("memcg", WQ_PERCPU, 0); WARN_ON(!memcg_wq); +#ifdef CONFIG_CGROUP_WRITEBACK + memcg_bdev_frn_flusher = alloc_workqueue("memcg_bdev_frn_flusher", + WQ_UNBOUND | WQ_MEM_RECLAIM, 0); + WARN_ON(!memcg_bdev_frn_flusher); +#endif + for_each_possible_cpu(cpu) { INIT_WORK(&per_cpu_ptr(&memcg_stock, cpu)->work, drain_local_memcg_stock); -- 2.39.5