[PATCH v8 0/3] memcg,writeback: flush foreign bdev mappings separately

[PATCH v8 0/3] memcg,writeback: flush foreign bdev mappings separately

[Julian Sun]

Hi,

This series avoids owner-wide foreign writeback triggered by dirtying
shared bdev inodes. Instead, it tracks bdev targets separately and
schedules writeback of their mappings.

Problem
=======

Bdev inodes are commonly shared by many memcgs. Dirtying their pages
can cause the owner wb to be tracked as foreign, triggering writeback
of the entire wb rather than just the bdev mapping.

In production, almost all the foreign dirtying we traced came from bdev
inodes. Across our observations, a bdev mapping had as little as about
10 MiB of dirty pages, yet foreign flushes targeted the entire owner
wb. On one machine, we observed 599 such works queued on one wb, with
an average request budget of about 43 GiB per work.

Measured impact
===============

The synthetic test used a VM with 4 vCPUs, 8 GiB RAM, ext4 and cgroup
v2, with device write bandwidth capped at 200 MiB/s. Background fio
repeatedly overwrote a 256 MiB file using buffered I/O, while
foreground fio performed sequential direct writes. Four memcgs
generated metadata and buffered writes to trigger foreign flushes.

We measured completed block writes to the background fio file,
including final sync, and foreground fio bandwidth, latency and
completion time.

Across three runs per kernel, mean background file write I/O decreased
by 72.7%, foreground bandwidth increased by 23.0%, and completion time
decreased by 18.8%.

Metric                         Baseline    Patched     Change
Background file write I/O (GiB)    6.154      1.680     -72.7%
Foreground bandwidth (MiB/s)      150.29     184.81     +23.0%
Foreground completion time (s)   109.203     88.652     -18.8%
Foreground mean latency (ms)      212.84     172.74     -18.8%

How it helps
============

Frequent writeback of the file overwritten by background fio reduces
the opportunity to coalesce buffered writes in the page cache.
Several overwrites of the same page can otherwise result in a single
write of its latest contents. Flushing between updates instead writes
the same page repeatedly, generating more device I/O for the same
application writes.

This additional I/O makes the device reach its performance limit
more easily and competes with foreground I/O for device bandwidth.

The benefit is expected to be most pronounced when these conditions
occur together on the same device:

1. Limited device bandwidth, as with an HDD, makes the additional
   writeback I/O more likely to saturate the device.
2. Multiple cgroups perform buffered overwrites. Frequent foreign
   writeback reduces write coalescing and increases device I/O.
3. Concurrent direct I/O bypasses the page cache and competes directly
   with writeback I/O for device bandwidth.

Approach
========

Following Jan's suggestion, this series records foreign bdev targets
separately and flushes their mappings instead of their owner wbs. This
preserves a way for dirty throttling to initiate bdev writeback while
avoiding owner-wide writeback triggered by these records. The existing
foreign-wb mechanism remains unchanged for other inodes.

Tracking uses oldest-first replacement and expiry after
dirty_expire_interval. Further dirtying during an ongoing flush can
trigger another flush after it finishes.

Tracking is best effort: each memcg keeps a bounded set of device
numbers without persistent device or inode references. Recording runs
under mapping->i_pages with IRQs disabled, while dropping a device
reference can sleep. A memcg that never enters dirty throttling could
also retain an unused record and pin the device object indefinitely.
Recording only dev_t avoids these lifetime constraints. Device removal
and device-number reuse may cause an unintended best-effort flush,
which is an accepted trade-off.

Writeback submission may block for a long time, with up to four work
items outstanding per memcg. A dedicated unbound workqueue provides
a separate max_active budget so these flushes do not exhaust active
slots on a shared workqueue and delay unrelated work. If the workqueue
cannot be allocated at boot, bdev inodes continue to use the existing
foreign-wb path.

This primarily affects filesystems that keep metadata in the bdev page
cache, such as ext4 and other buffer_head users, rather than the usual
metadata writeback paths of XFS and Btrfs. This also covers buffered
writes to raw block devices. The decision still does not account for
how much of the memcg's dirty memory belongs to the bdev: even a single
dirty bitmap block can trigger writeback of the entire bdev mapping
when the memcg enters dirty throttling.

Changes since v7:
- Update comments and commit messages as suggested by Tejun Heo.

Changes since v6:
- Drop open_mutex and the openers check from bdev_flush_by_dev(), and
  add a !CONFIG_BLOCK stub.
- Follow the existing foreign-wb tracking policy: use timestamps for
  oldest-first replacement and expiry, and allow dirtying during an
  in-flight flush to re-arm the record.
- Replace frn_lock with an atomic in-flight flag.
- Drop WQ_MEM_RECLAIM and explain the separate max_active budget.
- Expand the dev_t lifetime rationale and filesystem scope, update
  the foreign-dirty overview, and drop the Fixes tag.

Changes since v5:
- Fix repeated overwriting of slot 0 in patch 2 and add comments.

Changes since v4:
- Add before-and-after performance comparisons to the cover letter.

Changes since v3:
- Flush foreign-dirtied bdev inode mappings separately, as suggested
  by Jan.

Thanks.

Julian Sun (3):
  block: introduce bdev_flush_by_dev()
  memcg,writeback: flush foreign bdev mappings separately from owner wbs
  writeback: record bdev targets in foreign writeback tracepoints

 block/bdev.c                     |  18 ++++++
 include/linux/blkdev.h           |   4 ++
 include/linux/memcontrol.h       |  12 ++++
 include/trace/events/writeback.h |  22 ++++---
 mm/memcontrol.c                  | 123 +++++++++++++++++++++++++++++++++++----
 5 files changed, 160 insertions(+), 19 deletions(-)

-- 
2.39.5

[PATCH v8 1/3] block: introduce bdev_flush_by_dev()

[Julian Sun]

Add bdev_flush_by_dev() to submit page-cache writeback for a device
identified by dev_t without requiring callers to hold a persistent
device reference.

A subsequent patch will use this helper to flush foreign bdev mappings.

Signed-off-by: Julian Sun <sunjunchao@bytedance.com>
---
 block/bdev.c           | 18 ++++++++++++++++++
 include/linux/blkdev.h |  4 ++++
 2 files changed, 22 insertions(+)

diff --git a/block/bdev.c b/block/bdev.c
index cd8323083740..c7c192af6262 100644
--- a/block/bdev.c
+++ b/block/bdev.c
@@ -264,6 +264,24 @@ int sync_blockdev_nowait(struct block_device *bdev)
 }
 EXPORT_SYMBOL_GPL(sync_blockdev_nowait);
 
+/**
+ * bdev_flush_by_dev - attempt writeback of a block device's page cache
+ * @dev: target block device number
+ *
+ * May sleep while submitting writeback; does not wait for I/O completion.
+ */
+void bdev_flush_by_dev(dev_t dev)
+{
+	struct block_device *bdev;
+
+	bdev = blkdev_get_no_open(dev, false);
+	if (!bdev)
+		return;
+
+	sync_blockdev_nowait(bdev);
+	blkdev_put_no_open(bdev);
+}
+
 /*
  * Write out and wait upon all the dirty data associated with a block
  * device via its mapping.  Does not take the superblock lock.
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 4f7905c3412b..27afe3ff6fb5 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1710,6 +1710,7 @@ void invalidate_bdev(struct block_device *bdev);
 int sync_blockdev(struct block_device *bdev);
 int sync_blockdev_range(struct block_device *bdev, loff_t lstart, loff_t lend);
 int sync_blockdev_nowait(struct block_device *bdev);
+void bdev_flush_by_dev(dev_t dev);
 void sync_bdevs(bool wait);
 void bdev_statx(const struct path *path, struct kstat *stat, u32 request_mask);
 void printk_all_partitions(void);
@@ -1726,6 +1727,9 @@ static inline int sync_blockdev_nowait(struct block_device *bdev)
 {
 	return 0;
 }
+static inline void bdev_flush_by_dev(dev_t dev)
+{
+}
 static inline void sync_bdevs(bool wait)
 {
 }
-- 
2.39.5

[PATCH v8 2/3] memcg,writeback: flush foreign bdev mappings separately from owner wbs

[Julian Sun]

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 work on
memcg_bdev_frn_flusher to write only their mappings. Keep the existing
foreign-wb mechanism for non-bdev inodes.

Use fixed per-memcg slots. Tracking uses oldest-first replacement and
expiry after dirty_expire_interval. Further dirtying during an ongoing
flush can trigger another flush after it finishes.

Tracking is best effort: record only dev_t without holding device or
inode references. Records are updated under the mapping->i_pages lock
with IRQs disabled, but dropping a bdev reference can sleep. Also, since
foreign flushes are triggered by dirty throttling, a memcg that never
throttles could retain an unused record and pin the device object
indefinitely. Recording only dev_t avoids these lifetime constraints,
but device removal and device-number reuse may race with lookup. Such
races are expected under the best-effort semantics.

Bdev writeback submission can block for a long time on congested devices,
with up to four work items outstanding per memcg. Use a dedicated unbound
workqueue to give these flushes a separate max_active budget, so they do
not exhaust a shared workqueue's active slots and delay unrelated work.
If the workqueue cannot be allocated at boot, bdev inodes continue to use
the existing foreign-wb path.

This primarily affects filesystems that keep metadata in the bdev page
cache, such as ext4 and other buffer_head users, rather than the usual
metadata writeback paths of XFS and Btrfs. This also covers buffered writes
to raw block devices. The decision still does not account for how much of
the memcg's dirty memory belongs to the bdev. Even a single dirty bitmap
block can trigger writeback of the entire bdev mapping when the memcg
enters dirty throttling.

Suggested-by: Jan Kara <jack@suse.cz>
Signed-off-by: Julian Sun <sunjunchao@bytedance.com>
---
 include/linux/memcontrol.h |  12 ++++
 mm/memcontrol.c            | 117 ++++++++++++++++++++++++++++++++++---
 2 files changed, 120 insertions(+), 9 deletions(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 7d1c0ce189a8..c2699a1b82a2 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -176,6 +176,17 @@ struct memcg_cgwb_frn {
 	struct wb_completion done;	/* tracks in-flight foreign writebacks */
 };
 
+/*
+ * 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 memcg_bdev_frn {
+	struct work_struct work;
+	dev_t dev;			/* dev_t of the foreign bdev inode */
+	u64 at;				/* last recorded dirtying time in jiffies */
+	atomic_t inflight;		/* flush work is queued or running */
+};
+
 /*
  * Bucket for arbitrarily byte-sized objects charged to a memory
  * cgroup. The bucket can be reparented in one piece when the cgroup
@@ -279,6 +290,7 @@ struct mem_cgroup {
 #ifdef CONFIG_CGROUP_WRITEBACK
 	struct wb_domain cgwb_domain;
 	struct memcg_cgwb_frn cgwb_frn[MEMCG_CGWB_FRN_CNT];
+	struct memcg_bdev_frn bdev_frn[MEMCG_CGWB_FRN_CNT];
 #endif
 
 #ifdef CONFIG_LRU_GEN_WALKS_MMU
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 856a7d07586c..6042c694d56c 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -39,6 +39,7 @@
 #include <linux/smp.h>
 #include <linux/page-flags.h>
 #include <linux/backing-dev.h>
+#include <linux/blkdev.h>
 #include <linux/bit_spinlock.h>
 #include <linux/rcupdate.h>
 #include <linux/limits.h>
@@ -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_wq __ro_after_init;
 #endif
 
 static inline bool task_is_dying(void)
@@ -3875,20 +3877,72 @@ 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()
+ * 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.
- * As being wrong occasionally doesn't matter, updates and accesses to the
- * records are lockless and racy.
+ * Bdev inodes are commonly shared by many memcgs. Flushing their owner wb
+ * can write unrelated file data, so each memcg tracks foreign bdevs
+ * separately in MEMCG_CGWB_FRN_CNT slots keyed by dev_t. These records
+ * use the same expiry policy, but trigger writeback of only the bdev
+ * mappings.
+ *
+ * Both kinds of 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.
  */
+
+static void mem_cgroup_track_foreign_bdev(struct mem_cgroup *memcg, dev_t dev)
+{
+	struct memcg_bdev_frn *frn;
+	int i;
+	int oldest = -1;
+	u64 now = get_jiffies_64();
+	u64 oldest_at = now;
+
+	/*
+	 * Pick the slot to use.  If there is already a slot for @dev, keep
+	 * using it.  If not replace the oldest one which isn't being
+	 * written out.
+	 */
+	for (i = 0; i < MEMCG_CGWB_FRN_CNT; i++) {
+		frn = &memcg->bdev_frn[i];
+		if (frn->dev == dev)
+			break;
+		if (atomic_read(&frn->inflight))
+			continue;
+		if (time_before64(frn->at, oldest_at)) {
+			oldest = i;
+			oldest_at = frn->at;
+		}
+	}
+
+	if (i < MEMCG_CGWB_FRN_CNT) {
+		/*
+		 * Re-using an existing one.  Update timestamp lazily to
+		 * avoid making the cacheline hot.  We want them to be
+		 * reasonably up-to-date and significantly shorter than
+		 * dirty_expire_interval as that's what expires the record.
+		 * Use the shorter of 1s and dirty_expire_interval / 8.
+		 */
+		unsigned long update_intv =
+			min_t(unsigned long, HZ,
+			      msecs_to_jiffies(dirty_expire_interval * 10) / 8);
+
+		if (time_before64(frn->at, now - update_intv))
+			frn->at = now;
+	} else if (oldest >= 0) {
+		/* replace the oldest free one */
+		memcg->bdev_frn[oldest].dev = dev;
+		memcg->bdev_frn[oldest].at = now;
+	}
+}
+
 void mem_cgroup_track_foreign_dirty_slowpath(struct folio *folio,
 					     struct bdi_writeback *wb)
 {
@@ -3898,6 +3952,14 @@ 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 *inode = mapping->host;
+
+	if (memcg_bdev_frn_wq && sb_is_blkdev_sb(inode->i_sb)) {
+		trace_track_foreign_dirty(folio, wb);
+		mem_cgroup_track_foreign_bdev(memcg, inode->i_rdev);
+		return;
+	}
 
 	trace_track_foreign_dirty(folio, wb);
 
@@ -3941,6 +4003,16 @@ void mem_cgroup_track_foreign_dirty_slowpath(struct folio *folio,
 	}
 }
 
+static void bdev_frn_flush_work(struct work_struct *work)
+{
+	struct memcg_bdev_frn *frn =
+		container_of(work, struct memcg_bdev_frn, work);
+
+	bdev_flush_by_dev(frn->dev);
+
+	atomic_set(&frn->inflight, 0);
+}
+
 /* issue foreign writeback flushes for recorded foreign dirtying events */
 void mem_cgroup_flush_foreign(struct bdi_writeback *wb)
 {
@@ -3949,6 +4021,21 @@ 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 memcg_bdev_frn *frn = &memcg->bdev_frn[i];
+
+		/* Keep the workqueue availability check explicit. */
+		if (memcg_bdev_frn_wq && time_after64(frn->at, now - intv) &&
+		    atomic_cmpxchg(&frn->inflight, 0, 1) == 0) {
+			/*
+			 * Clear now so dirtying during writeback can refresh
+			 * the timestamp for a later flush.
+			 */
+			frn->at = 0;
+			queue_work(memcg_bdev_frn_wq, &frn->work);
+		}
+	}
+
 	for (i = 0; i < MEMCG_CGWB_FRN_CNT; i++) {
 		struct memcg_cgwb_frn *frn = &memcg->cgwb_frn[i];
 
@@ -4202,9 +4289,13 @@ 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 memcg_bdev_frn *frn = &memcg->bdev_frn[i];
+
 		memcg->cgwb_frn[i].done =
 			__WB_COMPLETION_INIT(&memcg_cgwb_frn_waitq);
+		INIT_WORK(&frn->work, bdev_frn_flush_work);
+	}
 #endif
 	lru_gen_init_memcg(memcg);
 	return memcg;
@@ -4386,8 +4477,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 +5796,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_wq = alloc_workqueue("memcg_bdev_frn_flusher",
+					    WQ_UNBOUND, 0);
+	WARN_ON(!memcg_bdev_frn_wq);
+#endif
+
 	for_each_possible_cpu(cpu) {
 		INIT_WORK(&per_cpu_ptr(&memcg_stock, cpu)->work,
 			  drain_local_memcg_stock);
-- 
2.39.5

[PATCH v8 3/3] writeback: record bdev targets in foreign writeback tracepoints

[Julian Sun]

Add dev to track_foreign_dirty and flush_foreign so traces can
distinguish foreign-wb tracking from the bdev-only path. Append the
field to each event and print it as major:minor; zero denotes the
normal foreign-wb path.

Signed-off-by: Julian Sun <sunjunchao@bytedance.com>
---
 include/trace/events/writeback.h | 22 ++++++++++++++--------
 mm/memcontrol.c                  |  8 +++++---
 2 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/include/trace/events/writeback.h b/include/trace/events/writeback.h
index 13ee076ccd16..ffd1b8df4231 100644
--- a/include/trace/events/writeback.h
+++ b/include/trace/events/writeback.h
@@ -273,9 +273,9 @@ TRACE_EVENT(inode_switch_wbs,
 
 TRACE_EVENT(track_foreign_dirty,
 
-	TP_PROTO(struct folio *folio, struct bdi_writeback *wb),
+	TP_PROTO(struct folio *folio, struct bdi_writeback *wb, dev_t dev),
 
-	TP_ARGS(folio, wb),
+	TP_ARGS(folio, wb, dev),
 
 	TP_STRUCT__entry(
 		__array(char,		name, 32)
@@ -284,6 +284,7 @@ TRACE_EVENT(track_foreign_dirty,
 		__field(u64,		cgroup_ino)
 		__field(u64,		page_cgroup_ino)
 		__field(unsigned int,	memcg_id)
+		__field(dev_t,		dev)
 	),
 
 	TP_fast_assign(
@@ -295,34 +296,37 @@ TRACE_EVENT(track_foreign_dirty,
 		__entry->ino		= inode ? inode->i_ino : 0;
 		__entry->memcg_id	= wb->memcg_css->id;
 		__entry->cgroup_ino	= __trace_wb_assign_cgroup(wb);
+		__entry->dev		= dev;
 
 		rcu_read_lock();
 		__entry->page_cgroup_ino = cgroup_ino(folio_memcg(folio)->css.cgroup);
 		rcu_read_unlock();
 	),
 
-	TP_printk("bdi %s[%llu]: ino=%llu memcg_id=%u cgroup_ino=%llu page_cgroup_ino=%llu",
+	TP_printk("bdi %s[%llu]: ino=%llu memcg_id=%u cgroup_ino=%llu page_cgroup_ino=%llu dev=%u:%u",
 		__entry->name,
 		__entry->bdi_id,
 		__entry->ino,
 		__entry->memcg_id,
 		__entry->cgroup_ino,
-		__entry->page_cgroup_ino
+		__entry->page_cgroup_ino,
+		MAJOR(__entry->dev), MINOR(__entry->dev)
 	)
 );
 
 TRACE_EVENT(flush_foreign,
 
 	TP_PROTO(struct bdi_writeback *wb, unsigned int frn_bdi_id,
-		 unsigned int frn_memcg_id),
+		 unsigned int frn_memcg_id, dev_t dev),
 
-	TP_ARGS(wb, frn_bdi_id, frn_memcg_id),
+	TP_ARGS(wb, frn_bdi_id, frn_memcg_id, dev),
 
 	TP_STRUCT__entry(
 		__array(char,		name, 32)
 		__field(u64,		cgroup_ino)
 		__field(unsigned int,	frn_bdi_id)
 		__field(unsigned int,	frn_memcg_id)
+		__field(dev_t,		dev)
 	),
 
 	TP_fast_assign(
@@ -330,13 +334,15 @@ TRACE_EVENT(flush_foreign,
 		__entry->cgroup_ino	= __trace_wb_assign_cgroup(wb);
 		__entry->frn_bdi_id	= frn_bdi_id;
 		__entry->frn_memcg_id	= frn_memcg_id;
+		__entry->dev		= dev;
 	),
 
-	TP_printk("bdi %s: cgroup_ino=%llu frn_bdi_id=%u frn_memcg_id=%u",
+	TP_printk("bdi %s: cgroup_ino=%llu frn_bdi_id=%u frn_memcg_id=%u dev=%u:%u",
 		__entry->name,
 		__entry->cgroup_ino,
 		__entry->frn_bdi_id,
-		__entry->frn_memcg_id
+		__entry->frn_memcg_id,
+		MAJOR(__entry->dev), MINOR(__entry->dev)
 	)
 );
 #endif
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 6042c694d56c..534b0d537a09 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3956,12 +3956,12 @@ void mem_cgroup_track_foreign_dirty_slowpath(struct folio *folio,
 	struct inode *inode = mapping->host;
 
 	if (memcg_bdev_frn_wq && sb_is_blkdev_sb(inode->i_sb)) {
-		trace_track_foreign_dirty(folio, wb);
+		trace_track_foreign_dirty(folio, wb, inode->i_rdev);
 		mem_cgroup_track_foreign_bdev(memcg, inode->i_rdev);
 		return;
 	}
 
-	trace_track_foreign_dirty(folio, wb);
+	trace_track_foreign_dirty(folio, wb, 0);
 
 	/*
 	 * Pick the slot to use.  If there is already a slot for @wb, keep
@@ -4032,6 +4032,7 @@ void mem_cgroup_flush_foreign(struct bdi_writeback *wb)
 			 * the timestamp for a later flush.
 			 */
 			frn->at = 0;
+			trace_flush_foreign(wb, 0, 0, frn->dev);
 			queue_work(memcg_bdev_frn_wq, &frn->work);
 		}
 	}
@@ -4048,7 +4049,8 @@ void mem_cgroup_flush_foreign(struct bdi_writeback *wb)
 		if (time_after64(frn->at, now - intv) &&
 		    atomic_read(&frn->done.cnt) == 1) {
 			frn->at = 0;
-			trace_flush_foreign(wb, frn->bdi_id, frn->memcg_id);
+			trace_flush_foreign(wb, frn->bdi_id, frn->memcg_id,
+					    0);
 			cgroup_writeback_by_id(frn->bdi_id, frn->memcg_id,
 					       WB_REASON_FOREIGN_FLUSH,
 					       &frn->done);
-- 
2.39.5

Re: [PATCH v8 0/3] memcg,writeback: flush foreign bdev mappings separately

[Tejun Heo]

For the series,

 Acked-by: Tejun Heo <tj@kernel.org>

But let's wait for Jan.

Thanks.

-- 
tejun

Re: [PATCH v8 3/3] writeback: record bdev targets in foreign writeback tracepoints

[Jan Kara]

On Tue 22-09-26 20:50:15, Julian Sun wrote:
> Add dev to track_foreign_dirty and flush_foreign so traces can
> distinguish foreign-wb tracking from the bdev-only path. Append the
> field to each event and print it as major:minor; zero denotes the
> normal foreign-wb path.
> 
> Signed-off-by: Julian Sun <sunjunchao@bytedance.com>

Hum, won't it be better to introduce new tracepoints instead of kind of
abusing the existing ones and filling in zeros? At least if I were to use
these tracepoints, I'd prefer that.

								Honza

> ---
>  include/trace/events/writeback.h | 22 ++++++++++++++--------
>  mm/memcontrol.c                  |  8 +++++---
>  2 files changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/include/trace/events/writeback.h b/include/trace/events/writeback.h
> index 13ee076ccd16..ffd1b8df4231 100644
> --- a/include/trace/events/writeback.h
> +++ b/include/trace/events/writeback.h
> @@ -273,9 +273,9 @@ TRACE_EVENT(inode_switch_wbs,
>  
>  TRACE_EVENT(track_foreign_dirty,
>  
> -	TP_PROTO(struct folio *folio, struct bdi_writeback *wb),
> +	TP_PROTO(struct folio *folio, struct bdi_writeback *wb, dev_t dev),
>  
> -	TP_ARGS(folio, wb),
> +	TP_ARGS(folio, wb, dev),
>  
>  	TP_STRUCT__entry(
>  		__array(char,		name, 32)
> @@ -284,6 +284,7 @@ TRACE_EVENT(track_foreign_dirty,
>  		__field(u64,		cgroup_ino)
>  		__field(u64,		page_cgroup_ino)
>  		__field(unsigned int,	memcg_id)
> +		__field(dev_t,		dev)
>  	),
>  
>  	TP_fast_assign(
> @@ -295,34 +296,37 @@ TRACE_EVENT(track_foreign_dirty,
>  		__entry->ino		= inode ? inode->i_ino : 0;
>  		__entry->memcg_id	= wb->memcg_css->id;
>  		__entry->cgroup_ino	= __trace_wb_assign_cgroup(wb);
> +		__entry->dev		= dev;
>  
>  		rcu_read_lock();
>  		__entry->page_cgroup_ino = cgroup_ino(folio_memcg(folio)->css.cgroup);
>  		rcu_read_unlock();
>  	),
>  
> -	TP_printk("bdi %s[%llu]: ino=%llu memcg_id=%u cgroup_ino=%llu page_cgroup_ino=%llu",
> +	TP_printk("bdi %s[%llu]: ino=%llu memcg_id=%u cgroup_ino=%llu page_cgroup_ino=%llu dev=%u:%u",
>  		__entry->name,
>  		__entry->bdi_id,
>  		__entry->ino,
>  		__entry->memcg_id,
>  		__entry->cgroup_ino,
> -		__entry->page_cgroup_ino
> +		__entry->page_cgroup_ino,
> +		MAJOR(__entry->dev), MINOR(__entry->dev)
>  	)
>  );
>  
>  TRACE_EVENT(flush_foreign,
>  
>  	TP_PROTO(struct bdi_writeback *wb, unsigned int frn_bdi_id,
> -		 unsigned int frn_memcg_id),
> +		 unsigned int frn_memcg_id, dev_t dev),
>  
> -	TP_ARGS(wb, frn_bdi_id, frn_memcg_id),
> +	TP_ARGS(wb, frn_bdi_id, frn_memcg_id, dev),
>  
>  	TP_STRUCT__entry(
>  		__array(char,		name, 32)
>  		__field(u64,		cgroup_ino)
>  		__field(unsigned int,	frn_bdi_id)
>  		__field(unsigned int,	frn_memcg_id)
> +		__field(dev_t,		dev)
>  	),
>  
>  	TP_fast_assign(
> @@ -330,13 +334,15 @@ TRACE_EVENT(flush_foreign,
>  		__entry->cgroup_ino	= __trace_wb_assign_cgroup(wb);
>  		__entry->frn_bdi_id	= frn_bdi_id;
>  		__entry->frn_memcg_id	= frn_memcg_id;
> +		__entry->dev		= dev;
>  	),
>  
> -	TP_printk("bdi %s: cgroup_ino=%llu frn_bdi_id=%u frn_memcg_id=%u",
> +	TP_printk("bdi %s: cgroup_ino=%llu frn_bdi_id=%u frn_memcg_id=%u dev=%u:%u",
>  		__entry->name,
>  		__entry->cgroup_ino,
>  		__entry->frn_bdi_id,
> -		__entry->frn_memcg_id
> +		__entry->frn_memcg_id,
> +		MAJOR(__entry->dev), MINOR(__entry->dev)
>  	)
>  );
>  #endif
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 6042c694d56c..534b0d537a09 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -3956,12 +3956,12 @@ void mem_cgroup_track_foreign_dirty_slowpath(struct folio *folio,
>  	struct inode *inode = mapping->host;
>  
>  	if (memcg_bdev_frn_wq && sb_is_blkdev_sb(inode->i_sb)) {
> -		trace_track_foreign_dirty(folio, wb);
> +		trace_track_foreign_dirty(folio, wb, inode->i_rdev);
>  		mem_cgroup_track_foreign_bdev(memcg, inode->i_rdev);
>  		return;
>  	}
>  
> -	trace_track_foreign_dirty(folio, wb);
> +	trace_track_foreign_dirty(folio, wb, 0);
>  
>  	/*
>  	 * Pick the slot to use.  If there is already a slot for @wb, keep
> @@ -4032,6 +4032,7 @@ void mem_cgroup_flush_foreign(struct bdi_writeback *wb)
>  			 * the timestamp for a later flush.
>  			 */
>  			frn->at = 0;
> +			trace_flush_foreign(wb, 0, 0, frn->dev);
>  			queue_work(memcg_bdev_frn_wq, &frn->work);
>  		}
>  	}
> @@ -4048,7 +4049,8 @@ void mem_cgroup_flush_foreign(struct bdi_writeback *wb)
>  		if (time_after64(frn->at, now - intv) &&
>  		    atomic_read(&frn->done.cnt) == 1) {
>  			frn->at = 0;
> -			trace_flush_foreign(wb, frn->bdi_id, frn->memcg_id);
> +			trace_flush_foreign(wb, frn->bdi_id, frn->memcg_id,
> +					    0);
>  			cgroup_writeback_by_id(frn->bdi_id, frn->memcg_id,
>  					       WB_REASON_FOREIGN_FLUSH,
>  					       &frn->done);
> -- 
> 2.39.5
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v8 1/3] block: introduce bdev_flush_by_dev()

[Jan Kara]

On Tue 22-09-26 20:50:13, Julian Sun wrote:
> Add bdev_flush_by_dev() to submit page-cache writeback for a device
> identified by dev_t without requiring callers to hold a persistent
> device reference.
> 
> A subsequent patch will use this helper to flush foreign bdev mappings.
> 
> Signed-off-by: Julian Sun <sunjunchao@bytedance.com>

Looks good. Feel free to add:

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

								Honza

> ---
>  block/bdev.c           | 18 ++++++++++++++++++
>  include/linux/blkdev.h |  4 ++++
>  2 files changed, 22 insertions(+)
> 
> diff --git a/block/bdev.c b/block/bdev.c
> index cd8323083740..c7c192af6262 100644
> --- a/block/bdev.c
> +++ b/block/bdev.c
> @@ -264,6 +264,24 @@ int sync_blockdev_nowait(struct block_device *bdev)
>  }
>  EXPORT_SYMBOL_GPL(sync_blockdev_nowait);
>  
> +/**
> + * bdev_flush_by_dev - attempt writeback of a block device's page cache
> + * @dev: target block device number
> + *
> + * May sleep while submitting writeback; does not wait for I/O completion.
> + */
> +void bdev_flush_by_dev(dev_t dev)
> +{
> +	struct block_device *bdev;
> +
> +	bdev = blkdev_get_no_open(dev, false);
> +	if (!bdev)
> +		return;
> +
> +	sync_blockdev_nowait(bdev);
> +	blkdev_put_no_open(bdev);
> +}
> +
>  /*
>   * Write out and wait upon all the dirty data associated with a block
>   * device via its mapping.  Does not take the superblock lock.
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index 4f7905c3412b..27afe3ff6fb5 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -1710,6 +1710,7 @@ void invalidate_bdev(struct block_device *bdev);
>  int sync_blockdev(struct block_device *bdev);
>  int sync_blockdev_range(struct block_device *bdev, loff_t lstart, loff_t lend);
>  int sync_blockdev_nowait(struct block_device *bdev);
> +void bdev_flush_by_dev(dev_t dev);
>  void sync_bdevs(bool wait);
>  void bdev_statx(const struct path *path, struct kstat *stat, u32 request_mask);
>  void printk_all_partitions(void);
> @@ -1726,6 +1727,9 @@ static inline int sync_blockdev_nowait(struct block_device *bdev)
>  {
>  	return 0;
>  }
> +static inline void bdev_flush_by_dev(dev_t dev)
> +{
> +}
>  static inline void sync_bdevs(bool wait)
>  {
>  }
> -- 
> 2.39.5
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v8 2/3] memcg,writeback: flush foreign bdev mappings separately from owner wbs

[Jan Kara]

On Tue 22-09-26 20:50:14, Julian Sun wrote:
> 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 work on
> memcg_bdev_frn_flusher to write only their mappings. Keep the existing
> foreign-wb mechanism for non-bdev inodes.
> 
> Use fixed per-memcg slots. Tracking uses oldest-first replacement and
> expiry after dirty_expire_interval. Further dirtying during an ongoing
> flush can trigger another flush after it finishes.
> 
> Tracking is best effort: record only dev_t without holding device or
> inode references. Records are updated under the mapping->i_pages lock
> with IRQs disabled, but dropping a bdev reference can sleep. Also, since
> foreign flushes are triggered by dirty throttling, a memcg that never
> throttles could retain an unused record and pin the device object
> indefinitely. Recording only dev_t avoids these lifetime constraints,
> but device removal and device-number reuse may race with lookup. Such
> races are expected under the best-effort semantics.
> 
> Bdev writeback submission can block for a long time on congested devices,
> with up to four work items outstanding per memcg. Use a dedicated unbound
> workqueue to give these flushes a separate max_active budget, so they do
> not exhaust a shared workqueue's active slots and delay unrelated work.
> If the workqueue cannot be allocated at boot, bdev inodes continue to use
> the existing foreign-wb path.
> 
> This primarily affects filesystems that keep metadata in the bdev page
> cache, such as ext4 and other buffer_head users, rather than the usual
> metadata writeback paths of XFS and Btrfs. This also covers buffered writes
> to raw block devices. The decision still does not account for how much of
> the memcg's dirty memory belongs to the bdev. Even a single dirty bitmap
> block can trigger writeback of the entire bdev mapping when the memcg
> enters dirty throttling.
> 
> Suggested-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Julian Sun <sunjunchao@bytedance.com>

Mostly looks good. What I somewhat dislike about this particular
implementation is that we are going to have new IO issuers (additional
workers for bdev foreign flushes) - potentially as many as there are memcgs
using the bdev - which could increase contention for the device. But OTOH
we already have as many wb_writeback issuers as there are memcgs for each
device so it isn't something fundamental. What is unique that all these
issuers will all flush the same inode (normally inode belongs only to one
issuer) but OTOH these are bdev inodes where IO submission path is trivial
and the IO pattern is mostly random so maybe the contention isn't going
to be too severe.

So I'm somewhat wondering if the 'foreign flush work' shouldn't be a
global 'per-bdev' thing but after some thought that would get a bit complex
and it probably isn't warranted at this point. I guess the simplest way to
reduce the unnecessary contention on bdev inode would be to use
write_inode_now(bdev_inode, 0) which will just skip the inode if someone is
already writing it (I_SYNC check) which is I think what we want.

Also one more technical comment below.

> +/*
> + * 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 memcg_bdev_frn {
> +	struct work_struct work;
> +	dev_t dev;			/* dev_t of the foreign bdev inode */
> +	u64 at;				/* last recorded dirtying time in jiffies */
> +	atomic_t inflight;		/* flush work is queued or running */
> +};
> +
>  /*
>   * Bucket for arbitrarily byte-sized objects charged to a memory
>   * cgroup. The bucket can be reparented in one piece when the cgroup
> @@ -279,6 +290,7 @@ struct mem_cgroup {
>  #ifdef CONFIG_CGROUP_WRITEBACK
>  	struct wb_domain cgwb_domain;
>  	struct memcg_cgwb_frn cgwb_frn[MEMCG_CGWB_FRN_CNT];
> +	struct memcg_bdev_frn bdev_frn[MEMCG_CGWB_FRN_CNT];
>  #endif
>  
>  #ifdef CONFIG_LRU_GEN_WALKS_MMU
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 856a7d07586c..6042c694d56c 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -39,6 +39,7 @@
>  #include <linux/smp.h>
>  #include <linux/page-flags.h>
>  #include <linux/backing-dev.h>
> +#include <linux/blkdev.h>
>  #include <linux/bit_spinlock.h>
>  #include <linux/rcupdate.h>
>  #include <linux/limits.h>
> @@ -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_wq __ro_after_init;
>  #endif
>  
>  static inline bool task_is_dying(void)
> @@ -3875,20 +3877,72 @@ 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()
> + * 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.
> - * As being wrong occasionally doesn't matter, updates and accesses to the
> - * records are lockless and racy.
> + * Bdev inodes are commonly shared by many memcgs. Flushing their owner wb
> + * can write unrelated file data, so each memcg tracks foreign bdevs
> + * separately in MEMCG_CGWB_FRN_CNT slots keyed by dev_t. These records
> + * use the same expiry policy, but trigger writeback of only the bdev
> + * mappings.
> + *
> + * Both kinds of 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.
>   */
> +
> +static void mem_cgroup_track_foreign_bdev(struct mem_cgroup *memcg, dev_t dev)
> +{
> +	struct memcg_bdev_frn *frn;
> +	int i;
> +	int oldest = -1;
> +	u64 now = get_jiffies_64();
> +	u64 oldest_at = now;
> +
> +	/*
> +	 * Pick the slot to use.  If there is already a slot for @dev, keep
> +	 * using it.  If not replace the oldest one which isn't being
> +	 * written out.
> +	 */
> +	for (i = 0; i < MEMCG_CGWB_FRN_CNT; i++) {
> +		frn = &memcg->bdev_frn[i];
> +		if (frn->dev == dev)
> +			break;
> +		if (atomic_read(&frn->inflight))
> +			continue;
> +		if (time_before64(frn->at, oldest_at)) {
> +			oldest = i;
> +			oldest_at = frn->at;
> +		}
> +	}

The handling of 'inflight' looks racy. After we check, the flush can be
queued and we'll just rewrite the entry under the pending flush below
leading to odd behavior (like missing flush later in case 'at' gets
clobbered to 0 by the running flush)...

> +
> +	if (i < MEMCG_CGWB_FRN_CNT) {
> +		/*
> +		 * Re-using an existing one.  Update timestamp lazily to
> +		 * avoid making the cacheline hot.  We want them to be
> +		 * reasonably up-to-date and significantly shorter than
> +		 * dirty_expire_interval as that's what expires the record.
> +		 * Use the shorter of 1s and dirty_expire_interval / 8.
> +		 */
> +		unsigned long update_intv =
> +			min_t(unsigned long, HZ,
> +			      msecs_to_jiffies(dirty_expire_interval * 10) / 8);
> +
> +		if (time_before64(frn->at, now - update_intv))
> +			frn->at = now;
> +	} else if (oldest >= 0) {
> +		/* replace the oldest free one */
> +		memcg->bdev_frn[oldest].dev = dev;
> +		memcg->bdev_frn[oldest].at = now;

.. so I think here we need to block submission of work (retry search if
busy) and only then update the entry.

								Honza 

> +	}
> +}
> +
>  void mem_cgroup_track_foreign_dirty_slowpath(struct folio *folio,
>  					     struct bdi_writeback *wb)
>  {
> @@ -3898,6 +3952,14 @@ 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 *inode = mapping->host;
> +
> +	if (memcg_bdev_frn_wq && sb_is_blkdev_sb(inode->i_sb)) {
> +		trace_track_foreign_dirty(folio, wb);
> +		mem_cgroup_track_foreign_bdev(memcg, inode->i_rdev);
> +		return;
> +	}
>  
>  	trace_track_foreign_dirty(folio, wb);
>  
> @@ -3941,6 +4003,16 @@ void mem_cgroup_track_foreign_dirty_slowpath(struct folio *folio,
>  	}
>  }
>  
> +static void bdev_frn_flush_work(struct work_struct *work)
> +{
> +	struct memcg_bdev_frn *frn =
> +		container_of(work, struct memcg_bdev_frn, work);
> +
> +	bdev_flush_by_dev(frn->dev);
> +
> +	atomic_set(&frn->inflight, 0);
> +}
> +
>  /* issue foreign writeback flushes for recorded foreign dirtying events */
>  void mem_cgroup_flush_foreign(struct bdi_writeback *wb)
>  {
> @@ -3949,6 +4021,21 @@ 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 memcg_bdev_frn *frn = &memcg->bdev_frn[i];
> +
> +		/* Keep the workqueue availability check explicit. */
> +		if (memcg_bdev_frn_wq && time_after64(frn->at, now - intv) &&
> +		    atomic_cmpxchg(&frn->inflight, 0, 1) == 0) {
> +			/*
> +			 * Clear now so dirtying during writeback can refresh
> +			 * the timestamp for a later flush.
> +			 */
> +			frn->at = 0;
> +			queue_work(memcg_bdev_frn_wq, &frn->work);
> +		}
> +	}
> +
>  	for (i = 0; i < MEMCG_CGWB_FRN_CNT; i++) {
>  		struct memcg_cgwb_frn *frn = &memcg->cgwb_frn[i];
>  
> @@ -4202,9 +4289,13 @@ 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 memcg_bdev_frn *frn = &memcg->bdev_frn[i];
> +
>  		memcg->cgwb_frn[i].done =
>  			__WB_COMPLETION_INIT(&memcg_cgwb_frn_waitq);
> +		INIT_WORK(&frn->work, bdev_frn_flush_work);
> +	}
>  #endif
>  	lru_gen_init_memcg(memcg);
>  	return memcg;
> @@ -4386,8 +4477,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 +5796,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_wq = alloc_workqueue("memcg_bdev_frn_flusher",
> +					    WQ_UNBOUND, 0);
> +	WARN_ON(!memcg_bdev_frn_wq);
> +#endif
> +
>  	for_each_possible_cpu(cpu) {
>  		INIT_WORK(&per_cpu_ptr(&memcg_stock, cpu)->work,
>  			  drain_local_memcg_stock);
> -- 
> 2.39.5
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v8 1/3] block: introduce bdev_flush_by_dev()

[Jan Kara]

On Thu 24-09-26 12:57:46, Jan Kara wrote:
> On Tue 22-09-26 20:50:13, Julian Sun wrote:
> > Add bdev_flush_by_dev() to submit page-cache writeback for a device
> > identified by dev_t without requiring callers to hold a persistent
> > device reference.
> > 
> > A subsequent patch will use this helper to flush foreign bdev mappings.
> > 
> > Signed-off-by: Julian Sun <sunjunchao@bytedance.com>
> 
> Looks good. Feel free to add:
> 
> Reviewed-by: Jan Kara <jack@suse.cz>

After some though I think it would be better to use

	write_inode_now(BD_INODE(bdev), 0);

in bdev_flush_by_dev().

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v8 2/3] memcg,writeback: flush foreign bdev mappings separately from owner wbs

[Julian Sun]

On 9/24/26 8:02 PM, Jan Kara wrote:
> On Tue 22-09-26 20:50:14, Julian Sun wrote:
>> 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 work on
>> memcg_bdev_frn_flusher to write only their mappings. Keep the existing
>> foreign-wb mechanism for non-bdev inodes.
>>
>> Use fixed per-memcg slots. Tracking uses oldest-first replacement and
>> expiry after dirty_expire_interval. Further dirtying during an ongoing
>> flush can trigger another flush after it finishes.
>>
>> Tracking is best effort: record only dev_t without holding device or
>> inode references. Records are updated under the mapping->i_pages lock
>> with IRQs disabled, but dropping a bdev reference can sleep. Also, since
>> foreign flushes are triggered by dirty throttling, a memcg that never
>> throttles could retain an unused record and pin the device object
>> indefinitely. Recording only dev_t avoids these lifetime constraints,
>> but device removal and device-number reuse may race with lookup. Such
>> races are expected under the best-effort semantics.
>>
>> Bdev writeback submission can block for a long time on congested devices,
>> with up to four work items outstanding per memcg. Use a dedicated unbound
>> workqueue to give these flushes a separate max_active budget, so they do
>> not exhaust a shared workqueue's active slots and delay unrelated work.
>> If the workqueue cannot be allocated at boot, bdev inodes continue to use
>> the existing foreign-wb path.
>>
>> This primarily affects filesystems that keep metadata in the bdev page
>> cache, such as ext4 and other buffer_head users, rather than the usual
>> metadata writeback paths of XFS and Btrfs. This also covers buffered writes
>> to raw block devices. The decision still does not account for how much of
>> the memcg's dirty memory belongs to the bdev. Even a single dirty bitmap
>> block can trigger writeback of the entire bdev mapping when the memcg
>> enters dirty throttling.
>>
>> Suggested-by: Jan Kara <jack@suse.cz>
>> Signed-off-by: Julian Sun <sunjunchao@bytedance.com>
> 
> Mostly looks good. What I somewhat dislike about this particular
> implementation is that we are going to have new IO issuers (additional
> workers for bdev foreign flushes) - potentially as many as there are memcgs
> using the bdev - which could increase contention for the device. But OTOH
> we already have as many wb_writeback issuers as there are memcgs for each
> device so it isn't something fundamental. What is unique that all these
> issuers will all flush the same inode (normally inode belongs only to one
> issuer) but OTOH these are bdev inodes where IO submission path is trivial
> and the IO pattern is mostly random so maybe the contention isn't going
> to be too severe.
> 
> So I'm somewhat wondering if the 'foreign flush work' shouldn't be a
> global 'per-bdev' thing but after some thought that would get a bit complex
> and it probably isn't warranted at this point. I guess the simplest way to
> reduce the unnecessary contention on bdev inode would be to use
> write_inode_now(bdev_inode, 0) which will just skip the inode if someone is
> already writing it (I_SYNC check) which is I think what we want.
> 
> Also one more technical comment below.
> 
>> +/*
>> + * 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 memcg_bdev_frn {
>> +	struct work_struct work;
>> +	dev_t dev;			/* dev_t of the foreign bdev inode */
>> +	u64 at;				/* last recorded dirtying time in jiffies */
>> +	atomic_t inflight;		/* flush work is queued or running */
>> +};
>> +
>>  /*
>>   * Bucket for arbitrarily byte-sized objects charged to a memory
>>   * cgroup. The bucket can be reparented in one piece when the cgroup
>> @@ -279,6 +290,7 @@ struct mem_cgroup {
>>  #ifdef CONFIG_CGROUP_WRITEBACK
>>  	struct wb_domain cgwb_domain;
>>  	struct memcg_cgwb_frn cgwb_frn[MEMCG_CGWB_FRN_CNT];
>> +	struct memcg_bdev_frn bdev_frn[MEMCG_CGWB_FRN_CNT];
>>  #endif
>>  
>>  #ifdef CONFIG_LRU_GEN_WALKS_MMU
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index 856a7d07586c..6042c694d56c 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -39,6 +39,7 @@
>>  #include <linux/smp.h>
>>  #include <linux/page-flags.h>
>>  #include <linux/backing-dev.h>
>> +#include <linux/blkdev.h>
>>  #include <linux/bit_spinlock.h>
>>  #include <linux/rcupdate.h>
>>  #include <linux/limits.h>
>> @@ -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_wq __ro_after_init;
>>  #endif
>>  
>>  static inline bool task_is_dying(void)
>> @@ -3875,20 +3877,72 @@ 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()
>> + * 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.
>> - * As being wrong occasionally doesn't matter, updates and accesses to the
>> - * records are lockless and racy.
>> + * Bdev inodes are commonly shared by many memcgs. Flushing their owner wb
>> + * can write unrelated file data, so each memcg tracks foreign bdevs
>> + * separately in MEMCG_CGWB_FRN_CNT slots keyed by dev_t. These records
>> + * use the same expiry policy, but trigger writeback of only the bdev
>> + * mappings.
>> + *
>> + * Both kinds of 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.
>>   */
>> +
>> +static void mem_cgroup_track_foreign_bdev(struct mem_cgroup *memcg, dev_t dev)
>> +{
>> +	struct memcg_bdev_frn *frn;
>> +	int i;
>> +	int oldest = -1;
>> +	u64 now = get_jiffies_64();
>> +	u64 oldest_at = now;
>> +
>> +	/*
>> +	 * Pick the slot to use.  If there is already a slot for @dev, keep
>> +	 * using it.  If not replace the oldest one which isn't being
>> +	 * written out.
>> +	 */
>> +	for (i = 0; i < MEMCG_CGWB_FRN_CNT; i++) {
>> +		frn = &memcg->bdev_frn[i];
>> +		if (frn->dev == dev)
>> +			break;
>> +		if (atomic_read(&frn->inflight))
>> +			continue;
>> +		if (time_before64(frn->at, oldest_at)) {
>> +			oldest = i;
>> +			oldest_at = frn->at;
>> +		}
>> +	}
> 
> The handling of 'inflight' looks racy. After we check, the flush can be
> queued and we'll just rewrite the entry under the pending flush below
> leading to odd behavior (like missing flush later in case 'at' gets
> clobbered to 0 by the running flush)...
> 
>> +
>> +	if (i < MEMCG_CGWB_FRN_CNT) {
>> +		/*
>> +		 * Re-using an existing one.  Update timestamp lazily to
>> +		 * avoid making the cacheline hot.  We want them to be
>> +		 * reasonably up-to-date and significantly shorter than
>> +		 * dirty_expire_interval as that's what expires the record.
>> +		 * Use the shorter of 1s and dirty_expire_interval / 8.
>> +		 */
>> +		unsigned long update_intv =
>> +			min_t(unsigned long, HZ,
>> +			      msecs_to_jiffies(dirty_expire_interval * 10) / 8);
>> +
>> +		if (time_before64(frn->at, now - update_intv))
>> +			frn->at = now;
>> +	} else if (oldest >= 0) {
>> +		/* replace the oldest free one */
>> +		memcg->bdev_frn[oldest].dev = dev;
>> +		memcg->bdev_frn[oldest].at = now;
> 
> .. so I think here we need to block submission of work (retry search if
> busy) and only then update the entry.

Thanks for the review and suggestions, Jan. 
Your suggestions make sense, and I'll address them in the next version.
> 
> 								Honza 
> 
>> +	}
>> +}
>> +
>>  void mem_cgroup_track_foreign_dirty_slowpath(struct folio *folio,
>>  					     struct bdi_writeback *wb)
>>  {
>> @@ -3898,6 +3952,14 @@ 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 *inode = mapping->host;
>> +
>> +	if (memcg_bdev_frn_wq && sb_is_blkdev_sb(inode->i_sb)) {
>> +		trace_track_foreign_dirty(folio, wb);
>> +		mem_cgroup_track_foreign_bdev(memcg, inode->i_rdev);
>> +		return;
>> +	}
>>  
>>  	trace_track_foreign_dirty(folio, wb);
>>  
>> @@ -3941,6 +4003,16 @@ void mem_cgroup_track_foreign_dirty_slowpath(struct folio *folio,
>>  	}
>>  }
>>  
>> +static void bdev_frn_flush_work(struct work_struct *work)
>> +{
>> +	struct memcg_bdev_frn *frn =
>> +		container_of(work, struct memcg_bdev_frn, work);
>> +
>> +	bdev_flush_by_dev(frn->dev);
>> +
>> +	atomic_set(&frn->inflight, 0);
>> +}
>> +
>>  /* issue foreign writeback flushes for recorded foreign dirtying events */
>>  void mem_cgroup_flush_foreign(struct bdi_writeback *wb)
>>  {
>> @@ -3949,6 +4021,21 @@ 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 memcg_bdev_frn *frn = &memcg->bdev_frn[i];
>> +
>> +		/* Keep the workqueue availability check explicit. */
>> +		if (memcg_bdev_frn_wq && time_after64(frn->at, now - intv) &&
>> +		    atomic_cmpxchg(&frn->inflight, 0, 1) == 0) {
>> +			/*
>> +			 * Clear now so dirtying during writeback can refresh
>> +			 * the timestamp for a later flush.
>> +			 */
>> +			frn->at = 0;
>> +			queue_work(memcg_bdev_frn_wq, &frn->work);
>> +		}
>> +	}
>> +
>>  	for (i = 0; i < MEMCG_CGWB_FRN_CNT; i++) {
>>  		struct memcg_cgwb_frn *frn = &memcg->cgwb_frn[i];
>>  
>> @@ -4202,9 +4289,13 @@ 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 memcg_bdev_frn *frn = &memcg->bdev_frn[i];
>> +
>>  		memcg->cgwb_frn[i].done =
>>  			__WB_COMPLETION_INIT(&memcg_cgwb_frn_waitq);
>> +		INIT_WORK(&frn->work, bdev_frn_flush_work);
>> +	}
>>  #endif
>>  	lru_gen_init_memcg(memcg);
>>  	return memcg;
>> @@ -4386,8 +4477,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 +5796,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_wq = alloc_workqueue("memcg_bdev_frn_flusher",
>> +					    WQ_UNBOUND, 0);
>> +	WARN_ON(!memcg_bdev_frn_wq);
>> +#endif
>> +
>>  	for_each_possible_cpu(cpu) {
>>  		INIT_WORK(&per_cpu_ptr(&memcg_stock, cpu)->work,
>>  			  drain_local_memcg_stock);
>> -- 
>> 2.39.5
>>

Thanks,
-- 
Julian Sun <sunjunchao@bytedance.com>

Re: [PATCH v8 0/3] memcg,writeback: flush foreign bdev mappings separately

[Julian Sun]

On 9/22/26 8:50 PM, Julian Sun wrote:

Hi,

Do you have any further thoughts on this series?
Should I rename bdev_flush_by_dev() to bdev_writeback_by_dev()
and send the next revision?

> Hi,
> 
> This series avoids owner-wide foreign writeback triggered by dirtying
> shared bdev inodes. Instead, it tracks bdev targets separately and
> schedules writeback of their mappings.
> 
> Problem
> =======
> 
> Bdev inodes are commonly shared by many memcgs. Dirtying their pages
> can cause the owner wb to be tracked as foreign, triggering writeback
> of the entire wb rather than just the bdev mapping.
> 
> In production, almost all the foreign dirtying we traced came from bdev
> inodes. Across our observations, a bdev mapping had as little as about
> 10 MiB of dirty pages, yet foreign flushes targeted the entire owner
> wb. On one machine, we observed 599 such works queued on one wb, with
> an average request budget of about 43 GiB per work.
> 
> Measured impact
> ===============
> 
> The synthetic test used a VM with 4 vCPUs, 8 GiB RAM, ext4 and cgroup
> v2, with device write bandwidth capped at 200 MiB/s. Background fio
> repeatedly overwrote a 256 MiB file using buffered I/O, while
> foreground fio performed sequential direct writes. Four memcgs
> generated metadata and buffered writes to trigger foreign flushes.
> 
> We measured completed block writes to the background fio file,
> including final sync, and foreground fio bandwidth, latency and
> completion time.
> 
> Across three runs per kernel, mean background file write I/O decreased
> by 72.7%, foreground bandwidth increased by 23.0%, and completion time
> decreased by 18.8%.
> 
> Metric                         Baseline    Patched     Change
> Background file write I/O (GiB)    6.154      1.680     -72.7%
> Foreground bandwidth (MiB/s)      150.29     184.81     +23.0%
> Foreground completion time (s)   109.203     88.652     -18.8%
> Foreground mean latency (ms)      212.84     172.74     -18.8%
> 
> How it helps
> ============
> 
> Frequent writeback of the file overwritten by background fio reduces
> the opportunity to coalesce buffered writes in the page cache.
> Several overwrites of the same page can otherwise result in a single
> write of its latest contents. Flushing between updates instead writes
> the same page repeatedly, generating more device I/O for the same
> application writes.
> 
> This additional I/O makes the device reach its performance limit
> more easily and competes with foreground I/O for device bandwidth.
> 
> The benefit is expected to be most pronounced when these conditions
> occur together on the same device:
> 
> 1. Limited device bandwidth, as with an HDD, makes the additional
>    writeback I/O more likely to saturate the device.
> 2. Multiple cgroups perform buffered overwrites. Frequent foreign
>    writeback reduces write coalescing and increases device I/O.
> 3. Concurrent direct I/O bypasses the page cache and competes directly
>    with writeback I/O for device bandwidth.
> 
> Approach
> ========
> 
> Following Jan's suggestion, this series records foreign bdev targets
> separately and flushes their mappings instead of their owner wbs. This
> preserves a way for dirty throttling to initiate bdev writeback while
> avoiding owner-wide writeback triggered by these records. The existing
> foreign-wb mechanism remains unchanged for other inodes.
> 
> Tracking uses oldest-first replacement and expiry after
> dirty_expire_interval. Further dirtying during an ongoing flush can
> trigger another flush after it finishes.
> 
> Tracking is best effort: each memcg keeps a bounded set of device
> numbers without persistent device or inode references. Recording runs
> under mapping->i_pages with IRQs disabled, while dropping a device
> reference can sleep. A memcg that never enters dirty throttling could
> also retain an unused record and pin the device object indefinitely.
> Recording only dev_t avoids these lifetime constraints. Device removal
> and device-number reuse may cause an unintended best-effort flush,
> which is an accepted trade-off.
> 
> Writeback submission may block for a long time, with up to four work
> items outstanding per memcg. A dedicated unbound workqueue provides
> a separate max_active budget so these flushes do not exhaust active
> slots on a shared workqueue and delay unrelated work. If the workqueue
> cannot be allocated at boot, bdev inodes continue to use the existing
> foreign-wb path.
> 
> This primarily affects filesystems that keep metadata in the bdev page
> cache, such as ext4 and other buffer_head users, rather than the usual
> metadata writeback paths of XFS and Btrfs. This also covers buffered
> writes to raw block devices. The decision still does not account for
> how much of the memcg's dirty memory belongs to the bdev: even a single
> dirty bitmap block can trigger writeback of the entire bdev mapping
> when the memcg enters dirty throttling.
> 
> Changes since v7:
> - Update comments and commit messages as suggested by Tejun Heo.
> 
> Changes since v6:
> - Drop open_mutex and the openers check from bdev_flush_by_dev(), and
>   add a !CONFIG_BLOCK stub.
> - Follow the existing foreign-wb tracking policy: use timestamps for
>   oldest-first replacement and expiry, and allow dirtying during an
>   in-flight flush to re-arm the record.
> - Replace frn_lock with an atomic in-flight flag.
> - Drop WQ_MEM_RECLAIM and explain the separate max_active budget.
> - Expand the dev_t lifetime rationale and filesystem scope, update
>   the foreign-dirty overview, and drop the Fixes tag.
> 
> Changes since v5:
> - Fix repeated overwriting of slot 0 in patch 2 and add comments.
> 
> Changes since v4:
> - Add before-and-after performance comparisons to the cover letter.
> 
> Changes since v3:
> - Flush foreign-dirtied bdev inode mappings separately, as suggested
>   by Jan.
> 
> Thanks.
> 
> Julian Sun (3):
>   block: introduce bdev_flush_by_dev()
>   memcg,writeback: flush foreign bdev mappings separately from owner wbs
>   writeback: record bdev targets in foreign writeback tracepoints
> 
>  block/bdev.c                     |  18 ++++++
>  include/linux/blkdev.h           |   4 ++
>  include/linux/memcontrol.h       |  12 ++++
>  include/trace/events/writeback.h |  22 ++++---
>  mm/memcontrol.c                  | 123 +++++++++++++++++++++++++++++++++++----
>  5 files changed, 160 insertions(+), 19 deletions(-)
> 

Thanks,
-- 
Julian Sun <sunjunchao@bytedance.com>

Re: [PATCH v8 0/3] memcg,writeback: flush foreign bdev mappings separately

[Andrew Morton]

On Sun, 27 Sep 2026 19:48:26 +0800 Julian Sun <sunjunchao@bytedance.com> wrote:

> Do you have any further thoughts on this series?

I'd like to see some feedback from memcg maintainers, please.

> Should I rename bdev_flush_by_dev() to bdev_writeback_by_dev()
> and send the next revision?

Yes please.  "flush" is a pet peeve of mine.  It's used to mean either
"writeback" or "invalidate" in various places.  Or both.  Annoying!

Also, I agree with the "dont check alloc succeeded in __init code". 
Both places.  Maybe save this little cleanup for the next development
cycle.