[PATCH v1 0/2] writeback: bound foreign dirty flushing

[PATCH v1 0/2] writeback: bound foreign dirty flushing

[Xin Yin]

Hi,

We have seen production stalls where syncfs() and other writeback waiters
are blocked behind long-running WB_REASON_FOREIGN_FLUSH work on the same
bdi_writeback.  The foreign work is best-effort writeback for memcg
foreign dirtying, but on busy multi-device systems it can be much larger
or longer-lived than the dirty pages that are actually relevant to the
selected target wb.

There are two parts to the problem.  First, the foreign flush budget is
currently sized from the target memcg's global dirty counter, while the
queued work writes only one target wb.  This can over-size a single-bdi
flush when the target memcg has dirty pages on other devices, or under-size
it when dirty pages on the target wb are charged to other memcgs.

Second, the work can keep extending itself under sustained dirtying.  Even
though foreign flushes are best-effort WB_SYNC_NONE work, a large budget
plus continued dirtying can keep the target wb busy long enough to delay
later writeback work, including syncfs() on the same backing device.

This series keeps foreign flushing aligned with its best-effort reclaim
role:

  1. size the work from the selected wb's reclaimable pages, with the
     existing 25% headroom;
  2. make each foreign flush operate on a bounded snapshot of dirty
     inodes/pages, while preserving WB_SYNC_NONE semantics and the finite
     work budget.

Later dirtying remains the responsibility of later foreign flushes, kupdate,
or background writeback.  The changes are intended to bound one
foreign-flush work item without turning it into a data-integrity operation.

Xin Yin (2):
  writeback: size foreign flushes by target wb dirty pages
  writeback: snapshot foreign flush pages

 fs/fs-writeback.c         | 74 +++++++++++++++++++++++++++------------
 include/linux/writeback.h |  1 +
 2 files changed, 52 insertions(+), 23 deletions(-)


base-commit: 46b513250491a7bfc97d98791dbe6a10bcc8129d
-- 
2.20.1

[PATCH v1 1/2] writeback: size foreign flushes by target wb dirty pages

[Xin Yin]

Foreign dirty tracking records the bdi and wb memcg IDs of recently
dirtied foreign inodes. When the source memcg needs foreign writeback,
mem_cgroup_flush_foreign() queues WB_REASON_FOREIGN_FLUSH to the
recorded target bdi_writeback.

cgroup_writeback_by_id() currently sizes this best-effort work from the
target memcg's NR_FILE_DIRTY counter. The work is scoped to one target
wb, so a memcg-wide dirty count can over-size a single-bdi flush when
the target memcg has dirty pages on other devices. It can also under-size
the flush when the target wb has dirty pages charged to other memcgs.

Over-sizing keeps the target wb busy longer and can delay later
writeback work. Under-sizing can finish before enough pages are written
back for the source memcg.

Use the target wb's WB_RECLAIMABLE counter and keep the existing 25%
headroom. This keeps the budget aligned with the writeback object being
queued.

Fixes: 97b27821b485 ("writeback, memcg: Implement foreign dirty flushing")
Signed-off-by: Xin Yin <yinxin.x@bytedance.com>
---
 fs/fs-writeback.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index 7c75ed7e8979..7c2340a5dead 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -1131,7 +1131,7 @@ int cgroup_writeback_by_id(u64 bdi_id, int memcg_id,
 	struct cgroup_subsys_state *memcg_css;
 	struct bdi_writeback *wb;
 	struct wb_writeback_work *work;
-	unsigned long dirty;
+	long dirty;
 	int ret;
 
 	/* lookup bdi and memcg */
@@ -1160,16 +1160,13 @@ int cgroup_writeback_by_id(u64 bdi_id, int memcg_id,
 	}
 
 	/*
-	 * The caller is attempting to write out most of
-	 * the currently dirty pages.  Let's take the current dirty page
-	 * count and inflate it by 25% which should be large enough to
-	 * flush out most dirty pages while avoiding getting livelocked by
-	 * concurrent dirtiers.
-	 *
-	 * BTW the memcg stats are flushed periodically and this is best-effort
-	 * estimation, so some potential error is ok.
+	 * The caller is attempting to write out most of the target wb's
+	 * currently dirty pages.  Size the work from the wb's reclaimable pages
+	 * and inflate the count by 25%, which should be large enough to flush
+	 * out most dirty pages while avoiding getting livelocked by concurrent
+	 * dirtiers.
 	 */
-	dirty = memcg_page_state(mem_cgroup_from_css(memcg_css), NR_FILE_DIRTY);
+	dirty = wb_stat_sum(wb, WB_RECLAIMABLE);
 	dirty = dirty * 10 / 8;
 
 	/* issue the writeback work */
-- 
2.20.1

[PATCH v1 2/2] writeback: snapshot foreign flush pages

[Xin Yin]

WB_REASON_FOREIGN_FLUSH is best-effort writeback for foreign dirtying. It
should write pages that are already visible to the target wb, but should
not keep extending the same work with pages dirtied while the work is
running.

Make foreign flushes use tagged writeback while preserving WB_SYNC_NONE
semantics. Use the remaining work budget as the per-inode chunk, so a
tagged pass is not truncated to the normal periodic chunk and a large
inode still cannot exceed the finite work budget.

Queue b_dirty only once for each foreign-flush work. Later passes only
retry b_more_io inodes skipped due to I_SYNC, and the work exits once
b_io and b_more_io are drained. If the budget is consumed, redirty the
inode instead of requeueing it to b_more_io so the same work cannot loop
on a continuously dirtied inode.

Do not refresh dirtied_when for foreign flushes; later dirtying should be
handled by later foreign, kupdate, or background writeback.

Fixes: 97b27821b485 ("writeback, memcg: Implement foreign dirty flushing")
Signed-off-by: Xin Yin <yinxin.x@bytedance.com>
---
 fs/fs-writeback.c         | 57 ++++++++++++++++++++++++++++++---------
 include/linux/writeback.h |  1 +
 2 files changed, 45 insertions(+), 13 deletions(-)

diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index 7c2340a5dead..6c5d1c85b6f8 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -40,6 +40,7 @@ struct wb_writeback_work {
 	struct super_block *sb;
 	enum writeback_sync_modes sync_mode;
 	unsigned int tagged_writepages:1;
+	unsigned int for_foreign_flush:1;
 	unsigned int for_kupdate:1;
 	unsigned int range_cyclic:1;
 	unsigned int for_background:1;
@@ -1174,6 +1175,12 @@ int cgroup_writeback_by_id(u64 bdi_id, int memcg_id,
 	if (work) {
 		work->nr_pages = dirty;
 		work->sync_mode = WB_SYNC_NONE;
+		/*
+		 * Foreign flushes should write a snapshot of dirty pages without
+		 * chasing concurrent dirtiers, but still honor the finite target
+		 * wb budget calculated above.
+		 */
+		work->for_foreign_flush = 1;
 		work->range_cyclic = 1;
 		work->reason = reason;
 		work->done = done;
@@ -1553,18 +1560,21 @@ static int move_expired_inodes(struct list_head *delaying_queue,
  *                                           +--> dequeue for IO
  */
 static void queue_io(struct bdi_writeback *wb, struct wb_writeback_work *work,
-		     unsigned long dirtied_before)
+		     unsigned long dirtied_before, bool queue_dirty)
 {
-	int moved;
+	int moved = 0;
 	unsigned long time_expire_jif = dirtied_before;
 
 	assert_spin_locked(&wb->list_lock);
 	list_splice_init(&wb->b_more_io, &wb->b_io);
-	moved = move_expired_inodes(&wb->b_dirty, &wb->b_io, dirtied_before);
-	if (!work->for_sync)
-		time_expire_jif = jiffies - dirtytime_expire_interval * HZ;
-	moved += move_expired_inodes(&wb->b_dirty_time, &wb->b_io,
-				     time_expire_jif);
+	if (queue_dirty) {
+		moved = move_expired_inodes(&wb->b_dirty, &wb->b_io,
+					    dirtied_before);
+		if (!work->for_sync)
+			time_expire_jif = jiffies - dirtytime_expire_interval * HZ;
+		moved += move_expired_inodes(&wb->b_dirty_time, &wb->b_io,
+					     time_expire_jif);
+	}
 	if (moved)
 		wb_io_lists_populated(wb);
 	trace_writeback_queue_io(wb, work, dirtied_before, moved);
@@ -1651,11 +1661,13 @@ static void requeue_inode(struct inode *inode, struct bdi_writeback *wb,
 
 	/*
 	 * Sync livelock prevention. Each inode is tagged and synced in one
-	 * shot. If still dirty, it will be redirty_tail()'ed below.  Update
-	 * the dirty time to prevent enqueue and sync it again.
+	 * shot for WB_SYNC_ALL or unbudgeted tagged writeback. If still dirty,
+	 * it will be redirty_tail()'ed below. Update the dirty time to prevent
+	 * enqueue and sync it again.
 	 */
 	if ((inode_state_read(inode) & I_DIRTY) &&
-	    (wbc->sync_mode == WB_SYNC_ALL || wbc->tagged_writepages))
+	    (wbc->sync_mode == WB_SYNC_ALL ||
+	     (wbc->tagged_writepages && !wbc->for_foreign_flush)))
 		inode->dirtied_when = jiffies;
 
 	if (wbc->pages_skipped) {
@@ -1678,6 +1690,7 @@ static void requeue_inode(struct inode *inode, struct bdi_writeback *wb,
 		 * sometimes bales out without doing anything.
 		 */
 		if (wbc->nr_to_write <= 0 &&
+		    !wbc->for_foreign_flush &&
 		    !inode_dirtied_after(inode, dirtied_before)) {
 			/* Slice used up. Queue for next turn. */
 			requeue_io(inode, wb);
@@ -1918,6 +1931,8 @@ static long writeback_chunk_size(struct super_block *sb,
 	 *                   (quickly) tag currently dirty pages
 	 *                   (maybe slowly) sync all tagged pages
 	 */
+	if (work->for_foreign_flush)
+		return work->nr_pages;
 	if (work->sync_mode == WB_SYNC_ALL || work->tagged_writepages)
 		return LONG_MAX;
 
@@ -1943,7 +1958,9 @@ static long writeback_sb_inodes(struct super_block *sb,
 {
 	struct writeback_control wbc = {
 		.sync_mode		= work->sync_mode,
-		.tagged_writepages	= work->tagged_writepages,
+		.tagged_writepages	= work->tagged_writepages ||
+					  work->for_foreign_flush,
+		.for_foreign_flush	= work->for_foreign_flush,
 		.for_kupdate		= work->for_kupdate,
 		.for_background		= work->for_background,
 		.for_sync		= work->for_sync,
@@ -2141,7 +2158,7 @@ static long writeback_inodes_wb(struct bdi_writeback *wb, long nr_pages,
 	blk_start_plug(&plug);
 	spin_lock(&wb->list_lock);
 	if (list_empty(&wb->b_io))
-		queue_io(wb, &work, jiffies);
+		queue_io(wb, &work, jiffies, true);
 	__writeback_inodes_wb(wb, &work);
 	spin_unlock(&wb->list_lock);
 	blk_finish_plug(&plug);
@@ -2202,6 +2219,13 @@ static long wb_writeback(struct bdi_writeback *wb,
 
 		spin_lock(&wb->list_lock);
 
+		if (queued && work->for_foreign_flush &&
+		    list_empty(&wb->b_io) &&
+		    list_empty(&wb->b_more_io)) {
+			spin_unlock(&wb->list_lock);
+			break;
+		}
+
 		trace_writeback_start(wb, work);
 		if (list_empty(&wb->b_io)) {
 			/*
@@ -2217,7 +2241,14 @@ static long wb_writeback(struct bdi_writeback *wb,
 			} else if (work->for_background)
 				dirtied_before = jiffies;
 
-			queue_io(wb, work, dirtied_before);
+			/*
+			 * After the initial queue_io() pass, a foreign flush may
+			 * still have I_SYNC-skipped inodes on b_more_io.  Move
+			 * those back to b_io without selecting another batch from
+			 * b_dirty.
+			 */
+			queue_io(wb, work, dirtied_before,
+				 !work->for_foreign_flush || !queued);
 			queued = true;
 		}
 		if (work->sb)
diff --git a/include/linux/writeback.h b/include/linux/writeback.h
index e530112c4b3a..096f0963d104 100644
--- a/include/linux/writeback.h
+++ b/include/linux/writeback.h
@@ -59,6 +59,7 @@ struct writeback_control {
 	unsigned for_kupdate:1;		/* A kupdate writeback */
 	unsigned for_background:1;	/* A background writeback */
 	unsigned tagged_writepages:1;	/* tag-and-write to avoid livelock */
+	unsigned for_foreign_flush:1;	/* foreign dirty flushing */
 	unsigned range_cyclic:1;	/* range_start is cyclic */
 	unsigned for_sync:1;		/* sync(2) WB_SYNC_ALL writeback */
 	unsigned unpinned_netfs_wb:1;	/* Cleared I_PINNING_NETFS_WB */
-- 
2.20.1

Re: [PATCH v1 1/2] writeback: size foreign flushes by target wb dirty pages

[Jan Kara]

On Tue 08-09-26 11:23:51, Xin Yin wrote:
> Foreign dirty tracking records the bdi and wb memcg IDs of recently
> dirtied foreign inodes. When the source memcg needs foreign writeback,
> mem_cgroup_flush_foreign() queues WB_REASON_FOREIGN_FLUSH to the
> recorded target bdi_writeback.
> 
> cgroup_writeback_by_id() currently sizes this best-effort work from the
> target memcg's NR_FILE_DIRTY counter. The work is scoped to one target
> wb, so a memcg-wide dirty count can over-size a single-bdi flush when
> the target memcg has dirty pages on other devices. It can also under-size
> the flush when the target wb has dirty pages charged to other memcgs.
> 
> Over-sizing keeps the target wb busy longer and can delay later
> writeback work. Under-sizing can finish before enough pages are written
> back for the source memcg.
> 
> Use the target wb's WB_RECLAIMABLE counter and keep the existing 25%
> headroom. This keeps the budget aligned with the writeback object being
> queued.
> 
> Fixes: 97b27821b485 ("writeback, memcg: Implement foreign dirty flushing")
> Signed-off-by: Xin Yin <yinxin.x@bytedance.com>

Yeah, this looks like an obvious win. Feel free to add:

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

								Honza

> ---
>  fs/fs-writeback.c | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> index 7c75ed7e8979..7c2340a5dead 100644
> --- a/fs/fs-writeback.c
> +++ b/fs/fs-writeback.c
> @@ -1131,7 +1131,7 @@ int cgroup_writeback_by_id(u64 bdi_id, int memcg_id,
>  	struct cgroup_subsys_state *memcg_css;
>  	struct bdi_writeback *wb;
>  	struct wb_writeback_work *work;
> -	unsigned long dirty;
> +	long dirty;
>  	int ret;
>  
>  	/* lookup bdi and memcg */
> @@ -1160,16 +1160,13 @@ int cgroup_writeback_by_id(u64 bdi_id, int memcg_id,
>  	}
>  
>  	/*
> -	 * The caller is attempting to write out most of
> -	 * the currently dirty pages.  Let's take the current dirty page
> -	 * count and inflate it by 25% which should be large enough to
> -	 * flush out most dirty pages while avoiding getting livelocked by
> -	 * concurrent dirtiers.
> -	 *
> -	 * BTW the memcg stats are flushed periodically and this is best-effort
> -	 * estimation, so some potential error is ok.
> +	 * The caller is attempting to write out most of the target wb's
> +	 * currently dirty pages.  Size the work from the wb's reclaimable pages
> +	 * and inflate the count by 25%, which should be large enough to flush
> +	 * out most dirty pages while avoiding getting livelocked by concurrent
> +	 * dirtiers.
>  	 */
> -	dirty = memcg_page_state(mem_cgroup_from_css(memcg_css), NR_FILE_DIRTY);
> +	dirty = wb_stat_sum(wb, WB_RECLAIMABLE);
>  	dirty = dirty * 10 / 8;
>  
>  	/* issue the writeback work */
> -- 
> 2.20.1
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v1 2/2] writeback: snapshot foreign flush pages

[Jan Kara]

On Tue 08-09-26 11:23:52, Xin Yin wrote:
> WB_REASON_FOREIGN_FLUSH is best-effort writeback for foreign dirtying. It
> should write pages that are already visible to the target wb, but should
> not keep extending the same work with pages dirtied while the work is
> running.
> 
> Make foreign flushes use tagged writeback while preserving WB_SYNC_NONE
> semantics. Use the remaining work budget as the per-inode chunk, so a
> tagged pass is not truncated to the normal periodic chunk and a large
> inode still cannot exceed the finite work budget.
> 
> Queue b_dirty only once for each foreign-flush work. Later passes only
> retry b_more_io inodes skipped due to I_SYNC, and the work exits once
> b_io and b_more_io are drained. If the budget is consumed, redirty the
> inode instead of requeueing it to b_more_io so the same work cannot loop
> on a continuously dirtied inode.
> 
> Do not refresh dirtied_when for foreign flushes; later dirtying should be
> handled by later foreign, kupdate, or background writeback.
> 
> Fixes: 97b27821b485 ("writeback, memcg: Implement foreign dirty flushing")
> Signed-off-by: Xin Yin <yinxin.x@bytedance.com>

I really dislike how your implementation ties the two writeback deadlock
avoidance schemes (nr_to_write for WB_SYNC_NONE writeback vs folio tagging
+ inode dirty timestamp for WB_SYNC_ALL writeback) together. But before we
get to the implementation details let's discuss the purpose a bit:

In principle I agree with you that the primary purpose of
cgroup_writeback_by_id() is that it wants to flush inodes that have folios
that are accounted to our memcg. It does this by a rather rough approach of
flushing the whole wb. If this races with other dirtying, it might happen
that we won't actually flush the foreign folios we want. Am I getting your
concern right? Are you able to actually observe such situation in practice?

								Honza

> ---
>  fs/fs-writeback.c         | 57 ++++++++++++++++++++++++++++++---------
>  include/linux/writeback.h |  1 +
>  2 files changed, 45 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> index 7c2340a5dead..6c5d1c85b6f8 100644
> --- a/fs/fs-writeback.c
> +++ b/fs/fs-writeback.c
> @@ -40,6 +40,7 @@ struct wb_writeback_work {
>  	struct super_block *sb;
>  	enum writeback_sync_modes sync_mode;
>  	unsigned int tagged_writepages:1;
> +	unsigned int for_foreign_flush:1;
>  	unsigned int for_kupdate:1;
>  	unsigned int range_cyclic:1;
>  	unsigned int for_background:1;
> @@ -1174,6 +1175,12 @@ int cgroup_writeback_by_id(u64 bdi_id, int memcg_id,
>  	if (work) {
>  		work->nr_pages = dirty;
>  		work->sync_mode = WB_SYNC_NONE;
> +		/*
> +		 * Foreign flushes should write a snapshot of dirty pages without
> +		 * chasing concurrent dirtiers, but still honor the finite target
> +		 * wb budget calculated above.
> +		 */
> +		work->for_foreign_flush = 1;
>  		work->range_cyclic = 1;
>  		work->reason = reason;
>  		work->done = done;
> @@ -1553,18 +1560,21 @@ static int move_expired_inodes(struct list_head *delaying_queue,
>   *                                           +--> dequeue for IO
>   */
>  static void queue_io(struct bdi_writeback *wb, struct wb_writeback_work *work,
> -		     unsigned long dirtied_before)
> +		     unsigned long dirtied_before, bool queue_dirty)
>  {
> -	int moved;
> +	int moved = 0;
>  	unsigned long time_expire_jif = dirtied_before;
>  
>  	assert_spin_locked(&wb->list_lock);
>  	list_splice_init(&wb->b_more_io, &wb->b_io);
> -	moved = move_expired_inodes(&wb->b_dirty, &wb->b_io, dirtied_before);
> -	if (!work->for_sync)
> -		time_expire_jif = jiffies - dirtytime_expire_interval * HZ;
> -	moved += move_expired_inodes(&wb->b_dirty_time, &wb->b_io,
> -				     time_expire_jif);
> +	if (queue_dirty) {
> +		moved = move_expired_inodes(&wb->b_dirty, &wb->b_io,
> +					    dirtied_before);
> +		if (!work->for_sync)
> +			time_expire_jif = jiffies - dirtytime_expire_interval * HZ;
> +		moved += move_expired_inodes(&wb->b_dirty_time, &wb->b_io,
> +					     time_expire_jif);
> +	}
>  	if (moved)
>  		wb_io_lists_populated(wb);
>  	trace_writeback_queue_io(wb, work, dirtied_before, moved);
> @@ -1651,11 +1661,13 @@ static void requeue_inode(struct inode *inode, struct bdi_writeback *wb,
>  
>  	/*
>  	 * Sync livelock prevention. Each inode is tagged and synced in one
> -	 * shot. If still dirty, it will be redirty_tail()'ed below.  Update
> -	 * the dirty time to prevent enqueue and sync it again.
> +	 * shot for WB_SYNC_ALL or unbudgeted tagged writeback. If still dirty,
> +	 * it will be redirty_tail()'ed below. Update the dirty time to prevent
> +	 * enqueue and sync it again.
>  	 */
>  	if ((inode_state_read(inode) & I_DIRTY) &&
> -	    (wbc->sync_mode == WB_SYNC_ALL || wbc->tagged_writepages))
> +	    (wbc->sync_mode == WB_SYNC_ALL ||
> +	     (wbc->tagged_writepages && !wbc->for_foreign_flush)))
>  		inode->dirtied_when = jiffies;
>  
>  	if (wbc->pages_skipped) {
> @@ -1678,6 +1690,7 @@ static void requeue_inode(struct inode *inode, struct bdi_writeback *wb,
>  		 * sometimes bales out without doing anything.
>  		 */
>  		if (wbc->nr_to_write <= 0 &&
> +		    !wbc->for_foreign_flush &&
>  		    !inode_dirtied_after(inode, dirtied_before)) {
>  			/* Slice used up. Queue for next turn. */
>  			requeue_io(inode, wb);
> @@ -1918,6 +1931,8 @@ static long writeback_chunk_size(struct super_block *sb,
>  	 *                   (quickly) tag currently dirty pages
>  	 *                   (maybe slowly) sync all tagged pages
>  	 */
> +	if (work->for_foreign_flush)
> +		return work->nr_pages;
>  	if (work->sync_mode == WB_SYNC_ALL || work->tagged_writepages)
>  		return LONG_MAX;
>  
> @@ -1943,7 +1958,9 @@ static long writeback_sb_inodes(struct super_block *sb,
>  {
>  	struct writeback_control wbc = {
>  		.sync_mode		= work->sync_mode,
> -		.tagged_writepages	= work->tagged_writepages,
> +		.tagged_writepages	= work->tagged_writepages ||
> +					  work->for_foreign_flush,
> +		.for_foreign_flush	= work->for_foreign_flush,
>  		.for_kupdate		= work->for_kupdate,
>  		.for_background		= work->for_background,
>  		.for_sync		= work->for_sync,
> @@ -2141,7 +2158,7 @@ static long writeback_inodes_wb(struct bdi_writeback *wb, long nr_pages,
>  	blk_start_plug(&plug);
>  	spin_lock(&wb->list_lock);
>  	if (list_empty(&wb->b_io))
> -		queue_io(wb, &work, jiffies);
> +		queue_io(wb, &work, jiffies, true);
>  	__writeback_inodes_wb(wb, &work);
>  	spin_unlock(&wb->list_lock);
>  	blk_finish_plug(&plug);
> @@ -2202,6 +2219,13 @@ static long wb_writeback(struct bdi_writeback *wb,
>  
>  		spin_lock(&wb->list_lock);
>  
> +		if (queued && work->for_foreign_flush &&
> +		    list_empty(&wb->b_io) &&
> +		    list_empty(&wb->b_more_io)) {
> +			spin_unlock(&wb->list_lock);
> +			break;
> +		}
> +
>  		trace_writeback_start(wb, work);
>  		if (list_empty(&wb->b_io)) {
>  			/*
> @@ -2217,7 +2241,14 @@ static long wb_writeback(struct bdi_writeback *wb,
>  			} else if (work->for_background)
>  				dirtied_before = jiffies;
>  
> -			queue_io(wb, work, dirtied_before);
> +			/*
> +			 * After the initial queue_io() pass, a foreign flush may
> +			 * still have I_SYNC-skipped inodes on b_more_io.  Move
> +			 * those back to b_io without selecting another batch from
> +			 * b_dirty.
> +			 */
> +			queue_io(wb, work, dirtied_before,
> +				 !work->for_foreign_flush || !queued);
>  			queued = true;
>  		}
>  		if (work->sb)
> diff --git a/include/linux/writeback.h b/include/linux/writeback.h
> index e530112c4b3a..096f0963d104 100644
> --- a/include/linux/writeback.h
> +++ b/include/linux/writeback.h
> @@ -59,6 +59,7 @@ struct writeback_control {
>  	unsigned for_kupdate:1;		/* A kupdate writeback */
>  	unsigned for_background:1;	/* A background writeback */
>  	unsigned tagged_writepages:1;	/* tag-and-write to avoid livelock */
> +	unsigned for_foreign_flush:1;	/* foreign dirty flushing */
>  	unsigned range_cyclic:1;	/* range_start is cyclic */
>  	unsigned for_sync:1;		/* sync(2) WB_SYNC_ALL writeback */
>  	unsigned unpinned_netfs_wb:1;	/* Cleared I_PINNING_NETFS_WB */
> -- 
> 2.20.1
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v1 1/2] writeback: size foreign flushes by target wb dirty pages

[Tejun Heo]

On Tue, Sep 08, 2026 at 11:23:51AM +0800, Xin Yin wrote:
> Foreign dirty tracking records the bdi and wb memcg IDs of recently
> dirtied foreign inodes. When the source memcg needs foreign writeback,
> mem_cgroup_flush_foreign() queues WB_REASON_FOREIGN_FLUSH to the
> recorded target bdi_writeback.
> 
> cgroup_writeback_by_id() currently sizes this best-effort work from the
> target memcg's NR_FILE_DIRTY counter. The work is scoped to one target
> wb, so a memcg-wide dirty count can over-size a single-bdi flush when
> the target memcg has dirty pages on other devices. It can also under-size
> the flush when the target wb has dirty pages charged to other memcgs.
> 
> Over-sizing keeps the target wb busy longer and can delay later
> writeback work. Under-sizing can finish before enough pages are written
> back for the source memcg.
> 
> Use the target wb's WB_RECLAIMABLE counter and keep the existing 25%
> headroom. This keeps the budget aligned with the writeback object being
> queued.
> 
> Fixes: 97b27821b485 ("writeback, memcg: Implement foreign dirty flushing")
> Signed-off-by: Xin Yin <yinxin.x@bytedance.com>

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

Thanks.

-- 
tejun

Re: [PATCH v1 2/2] writeback: snapshot foreign flush pages

[尹欣]

> From: "Jan Kara"<jack@suse.cz>
> Date:  Tue, Sep 8, 2026, 19:01
> Subject:  Re: [PATCH v1 2/2] writeback: snapshot foreign flush pages
> To: "Xin Yin"<yinxin.x@bytedance.com>
> Cc: "Alexander Viro"<viro@zeniv.linux.org.uk>, "Christian Brauner"<brauner@kernel.org>, "Jan Kara"<jack@suse.cz>, "Tejun Heo"<tj@kernel.org>, "Jens Axboe"<axboe@kernel.dk>, <linux-fsdevel@vger.kernel.org>, <linux-kernel@vger.kernel.org>
> On Tue 08-09-26 11:23:52, Xin Yin wrote:
> > WB_REASON_FOREIGN_FLUSH is best-effort writeback for foreign dirtying. It
> > should write pages that are already visible to the target wb, but should
> > not keep extending the same work with pages dirtied while the work is
> > running.
> > 
> > Make foreign flushes use tagged writeback while preserving WB_SYNC_NONE
> > semantics. Use the remaining work budget as the per-inode chunk, so a
> > tagged pass is not truncated to the normal periodic chunk and a large
> > inode still cannot exceed the finite work budget.
> > 
> > Queue b_dirty only once for each foreign-flush work. Later passes only
> > retry b_more_io inodes skipped due to I_SYNC, and the work exits once
> > b_io and b_more_io are drained. If the budget is consumed, redirty the
> > inode instead of requeueing it to b_more_io so the same work cannot loop
> > on a continuously dirtied inode.
> > 
> > Do not refresh dirtied_when for foreign flushes; later dirtying should be
> > handled by later foreign, kupdate, or background writeback.
> > 
> > Fixes: 97b27821b485 ("writeback, memcg: Implement foreign dirty flushing")
> > Signed-off-by: Xin Yin <yinxin.x@bytedance.com>
> 
> I really dislike how your implementation ties the two writeback deadlock
> avoidance schemes (nr_to_write for WB_SYNC_NONE writeback vs folio tagging
> + inode dirty timestamp for WB_SYNC_ALL writeback) together. But before we
> get to the implementation details let's discuss the purpose a bit:
> 
> In principle I agree with you that the primary purpose of
> cgroup_writeback_by_id() is that it wants to flush inodes that have folios
> that are accounted to our memcg. It does this by a rather rough approach of
> flushing the whole wb. If this races with other dirtying, it might happen
> that we won't actually flush the foreign folios we want. Am I getting your
> concern right? Are you able to actually observe such situation in practice?
Hi Jan,

Yes, that's the concern. Concurrent dirtying could consume the work's budget without sufficiently flushing the foreign folios charged to the memcg we're trying to help.

We haven't confirmed that specific situation in production. What we've observed is long-running foreign work with many subsequent works queued behind it. We haven't traced the original source memcg's folios closely enough to show that they were left dirty because newly dirtied pages consumed the budget. So that part is still a concern, not an observed failure.

The idea behind combining the two approaches was to get the budget right for the target wb, while keeping the flush from chasing concurrent dirtying. The first is about not holding up later work on the same wb, including syncfs. There's also a fairness concern: an old inode can keep getting newly dirtied pages written while later-dirtied inodes have to wait for another work. The second is about making the flush more useful to the source memcg, although tagging alone doesn't guarantee that its folios get flushed.

I see your concern about mixing the two mechanisms, though. Would you suggest keeping the budget-only approach for now, or is there a simpler way to address the concurrent-dirtying concern?

Thanks
Xin Yin
> 
>                                                                 Honza
> 
> > ---
> >  fs/fs-writeback.c         | 57 ++++++++++++++++++++++++++++++---------
> >  include/linux/writeback.h |  1 +
> >  2 files changed, 45 insertions(+), 13 deletions(-)
> > 
> > diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> > index 7c2340a5dead..6c5d1c85b6f8 100644
> > --- a/fs/fs-writeback.c
> > +++ b/fs/fs-writeback.c
> > @@ -40,6 +40,7 @@ struct wb_writeback_work {
> >          struct super_block *sb;
> >          enum writeback_sync_modes sync_mode;
> >          unsigned int tagged_writepages:1;
> > +        unsigned int for_foreign_flush:1;
> >          unsigned int for_kupdate:1;
> >          unsigned int range_cyclic:1;
> >          unsigned int for_background:1;
> > @@ -1174,6 +1175,12 @@ int cgroup_writeback_by_id(u64 bdi_id, int memcg_id,
> >          if (work) {
> >                  work->nr_pages = dirty;
> >                  work->sync_mode = WB_SYNC_NONE;
> > +                /*
> > +                 * Foreign flushes should write a snapshot of dirty pages without
> > +                 * chasing concurrent dirtiers, but still honor the finite target
> > +                 * wb budget calculated above.
> > +                 */
> > +                work->for_foreign_flush = 1;
> >                  work->range_cyclic = 1;
> >                  work->reason = reason;
> >                  work->done = done;
> > @@ -1553,18 +1560,21 @@ static int move_expired_inodes(struct list_head *delaying_queue,
> >   *                                           +--> dequeue for IO
> >   */
> >  static void queue_io(struct bdi_writeback *wb, struct wb_writeback_work *work,
> > -                     unsigned long dirtied_before)
> > +                     unsigned long dirtied_before, bool queue_dirty)
> >  {
> > -        int moved;
> > +        int moved = 0;
> >          unsigned long time_expire_jif = dirtied_before;
> >  
> >          assert_spin_locked(&wb->list_lock);
> >          list_splice_init(&wb->b_more_io, &wb->b_io);
> > -        moved = move_expired_inodes(&wb->b_dirty, &wb->b_io, dirtied_before);
> > -        if (!work->for_sync)
> > -                time_expire_jif = jiffies - dirtytime_expire_interval * HZ;
> > -        moved += move_expired_inodes(&wb->b_dirty_time, &wb->b_io,
> > -                                     time_expire_jif);
> > +        if (queue_dirty) {
> > +                moved = move_expired_inodes(&wb->b_dirty, &wb->b_io,
> > +                                            dirtied_before);
> > +                if (!work->for_sync)
> > +                        time_expire_jif = jiffies - dirtytime_expire_interval * HZ;
> > +                moved += move_expired_inodes(&wb->b_dirty_time, &wb->b_io,
> > +                                             time_expire_jif);
> > +        }
> >          if (moved)
> >                  wb_io_lists_populated(wb);
> >          trace_writeback_queue_io(wb, work, dirtied_before, moved);
> > @@ -1651,11 +1661,13 @@ static void requeue_inode(struct inode *inode, struct bdi_writeback *wb,
> >  
> >          /*
> >           * Sync livelock prevention. Each inode is tagged and synced in one
> > -         * shot. If still dirty, it will be redirty_tail()'ed below.  Update
> > -         * the dirty time to prevent enqueue and sync it again.
> > +         * shot for WB_SYNC_ALL or unbudgeted tagged writeback. If still dirty,
> > +         * it will be redirty_tail()'ed below. Update the dirty time to prevent
> > +         * enqueue and sync it again.
> >           */
> >          if ((inode_state_read(inode) & I_DIRTY) &&
> > -            (wbc->sync_mode == WB_SYNC_ALL || wbc->tagged_writepages))
> > +            (wbc->sync_mode == WB_SYNC_ALL ||
> > +             (wbc->tagged_writepages && !wbc->for_foreign_flush)))
> >                  inode->dirtied_when = jiffies;
> >  
> >          if (wbc->pages_skipped) {
> > @@ -1678,6 +1690,7 @@ static void requeue_inode(struct inode *inode, struct bdi_writeback *wb,
> >                   * sometimes bales out without doing anything.
> >                   */
> >                  if (wbc->nr_to_write <= 0 &&
> > +                    !wbc->for_foreign_flush &&
> >                      !inode_dirtied_after(inode, dirtied_before)) {
> >                          /* Slice used up. Queue for next turn. */
> >                          requeue_io(inode, wb);
> > @@ -1918,6 +1931,8 @@ static long writeback_chunk_size(struct super_block *sb,
> >           *                   (quickly) tag currently dirty pages
> >           *                   (maybe slowly) sync all tagged pages
> >           */
> > +        if (work->for_foreign_flush)
> > +                return work->nr_pages;
> >          if (work->sync_mode == WB_SYNC_ALL || work->tagged_writepages)
> >                  return LONG_MAX;
> >  
> > @@ -1943,7 +1958,9 @@ static long writeback_sb_inodes(struct super_block *sb,
> >  {
> >          struct writeback_control wbc = {
> >                  .sync_mode                = work->sync_mode,
> > -                .tagged_writepages        = work->tagged_writepages,
> > +                .tagged_writepages        = work->tagged_writepages ||
> > +                                          work->for_foreign_flush,
> > +                .for_foreign_flush        = work->for_foreign_flush,
> >                  .for_kupdate                = work->for_kupdate,
> >                  .for_background                = work->for_background,
> >                  .for_sync                = work->for_sync,
> > @@ -2141,7 +2158,7 @@ static long writeback_inodes_wb(struct bdi_writeback *wb, long nr_pages,
> >          blk_start_plug(&plug);
> >          spin_lock(&wb->list_lock);
> >          if (list_empty(&wb->b_io))
> > -                queue_io(wb, &work, jiffies);
> > +                queue_io(wb, &work, jiffies, true);
> >          __writeback_inodes_wb(wb, &work);
> >          spin_unlock(&wb->list_lock);
> >          blk_finish_plug(&plug);
> > @@ -2202,6 +2219,13 @@ static long wb_writeback(struct bdi_writeback *wb,
> >  
> >                  spin_lock(&wb->list_lock);
> >  
> > +                if (queued && work->for_foreign_flush &&
> > +                    list_empty(&wb->b_io) &&
> > +                    list_empty(&wb->b_more_io)) {
> > +                        spin_unlock(&wb->list_lock);
> > +                        break;
> > +                }
> > +
> >                  trace_writeback_start(wb, work);
> >                  if (list_empty(&wb->b_io)) {
> >                          /*
> > @@ -2217,7 +2241,14 @@ static long wb_writeback(struct bdi_writeback *wb,
> >                          } else if (work->for_background)
> >                                  dirtied_before = jiffies;
> >  
> > -                        queue_io(wb, work, dirtied_before);
> > +                        /*
> > +                         * After the initial queue_io() pass, a foreign flush may
> > +                         * still have I_SYNC-skipped inodes on b_more_io.  Move
> > +                         * those back to b_io without selecting another batch from
> > +                         * b_dirty.
> > +                         */
> > +                        queue_io(wb, work, dirtied_before,
> > +                                 !work->for_foreign_flush || !queued);
> >                          queued = true;
> >                  }
> >                  if (work->sb)
> > diff --git a/include/linux/writeback.h b/include/linux/writeback.h
> > index e530112c4b3a..096f0963d104 100644
> > --- a/include/linux/writeback.h
> > +++ b/include/linux/writeback.h
> > @@ -59,6 +59,7 @@ struct writeback_control {
> >          unsigned for_kupdate:1;                /* A kupdate writeback */
> >          unsigned for_background:1;        /* A background writeback */
> >          unsigned tagged_writepages:1;        /* tag-and-write to avoid livelock */
> > +        unsigned for_foreign_flush:1;        /* foreign dirty flushing */
> >          unsigned range_cyclic:1;        /* range_start is cyclic */
> >          unsigned for_sync:1;                /* sync(2) WB_SYNC_ALL writeback */
> >          unsigned unpinned_netfs_wb:1;        /* Cleared I_PINNING_NETFS_WB */
> > -- 
> > 2.20.1
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR
> 

Re: [PATCH v1 2/2] writeback: snapshot foreign flush pages

[Jan Kara]

On Wed 09-09-26 10:08:08, 尹欣 wrote:
> > From: "Jan Kara"<jack@suse.cz>
> > Date:  Tue, Sep 8, 2026, 19:01
> > Subject:  Re: [PATCH v1 2/2] writeback: snapshot foreign flush pages
> > To: "Xin Yin"<yinxin.x@bytedance.com>
> > Cc: "Alexander Viro"<viro@zeniv.linux.org.uk>, "Christian Brauner"<brauner@kernel.org>, "Jan Kara"<jack@suse.cz>, "Tejun Heo"<tj@kernel.org>, "Jens Axboe"<axboe@kernel.dk>, <linux-fsdevel@vger.kernel.org>, <linux-kernel@vger.kernel.org>
> > On Tue 08-09-26 11:23:52, Xin Yin wrote:
> > > WB_REASON_FOREIGN_FLUSH is best-effort writeback for foreign dirtying. It
> > > should write pages that are already visible to the target wb, but should
> > > not keep extending the same work with pages dirtied while the work is
> > > running.
> > > 
> > > Make foreign flushes use tagged writeback while preserving WB_SYNC_NONE
> > > semantics. Use the remaining work budget as the per-inode chunk, so a
> > > tagged pass is not truncated to the normal periodic chunk and a large
> > > inode still cannot exceed the finite work budget.
> > > 
> > > Queue b_dirty only once for each foreign-flush work. Later passes only
> > > retry b_more_io inodes skipped due to I_SYNC, and the work exits once
> > > b_io and b_more_io are drained. If the budget is consumed, redirty the
> > > inode instead of requeueing it to b_more_io so the same work cannot loop
> > > on a continuously dirtied inode.
> > > 
> > > Do not refresh dirtied_when for foreign flushes; later dirtying should be
> > > handled by later foreign, kupdate, or background writeback.
> > > 
> > > Fixes: 97b27821b485 ("writeback, memcg: Implement foreign dirty flushing")
> > > Signed-off-by: Xin Yin <yinxin.x@bytedance.com>
> > 
> > I really dislike how your implementation ties the two writeback deadlock
> > avoidance schemes (nr_to_write for WB_SYNC_NONE writeback vs folio tagging
> > + inode dirty timestamp for WB_SYNC_ALL writeback) together. But before we
> > get to the implementation details let's discuss the purpose a bit:
> > 
> > In principle I agree with you that the primary purpose of
> > cgroup_writeback_by_id() is that it wants to flush inodes that have folios
> > that are accounted to our memcg. It does this by a rather rough approach of
> > flushing the whole wb. If this races with other dirtying, it might happen
> > that we won't actually flush the foreign folios we want. Am I getting your
> > concern right? Are you able to actually observe such situation in practice?
> 
> Yes, that's the concern. Concurrent dirtying could consume the work's
> budget without sufficiently flushing the foreign folios charged to the
> memcg we're trying to help.
> 
> We haven't confirmed that specific situation in production. What we've
> observed is long-running foreign work with many subsequent works queued
> behind it. We haven't traced the original source memcg's folios closely
> enough to show that they were left dirty because newly dirtied pages
> consumed the budget. So that part is still a concern, not an observed
> failure.

OK. So I fully agree that foreign dirty tracking and flushing is very
rough. The basic premise of that infrastructure is that foreign dirtying
should be very rare and amount of foreign dirty pages very low and thus
largely irrelevant for the dirty throttling. The flushing is there mostly
as a last resort solution to have a chance of *some* kind of forward
progress in corner cases where the amount of foreign dirty pages is
substantial.

As Tejun wrote [1], bdev inodes are kind of breaking these assumptions.
Still number of dirty pages there should be low compared to data pages but
for some workloads that isn't necessarily the case.

[1] https://lore.kernel.org/all/aqGmmw3rv_2f5Jk4@slm.duckdns.org

> The idea behind combining the two approaches was to get the budget right
> for the target wb, while keeping the flush from chasing concurrent
> dirtying. The first is about not holding up later work on the same wb,
> including syncfs. There's also a fairness concern: an old inode can keep
> getting newly dirtied pages written while later-dirtied inodes have to
> wait for another work. The second is about making the flush more useful
> to the source memcg, although tagging alone doesn't guarantee that its
> folios get flushed.

If you look at the logic in writeback_sb_inodes(), it actually goes to
great lengths to avoid the fairness issues you describe above. We only
write a limited amount of pages from each inode (see
writeback_chunk_size()), then we switch to another one, cycling through all
inodes queued to b_io list in this way. Inodes on b_io list get fixed when
the writeback work starts (see queue_io()) so new inodes cannot appear
there. This is not bulletproof but it avoids the practical livelocking
cases. So generally I don't expect your systems have practical issues with
foreign flushes not really flushing what they should.

> I see your concern about mixing the two mechanisms, though. Would you
> suggest keeping the budget-only approach for now, or is there a simpler
> way to address the concurrent-dirtying concern?

As I wrote above, at this point I'm not convinced concurrent dirtying is
your practical problem. If you can show foreign flushes in your real
workloads often don't end up flushing what they should, then we can discuss
how to improve the writeback behavior (or foreign flush behavior) to avoid
that.

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

Re: [PATCH v1 2/2] writeback: snapshot foreign flush pages

[尹欣]

> From: "Jan Kara"<jack@suse.cz>
> Date:  Fri, Sep 11, 2026, 19:09
> Subject:  Re: [PATCH v1 2/2] writeback: snapshot foreign flush pages
> To: "尹欣"<yinxin.x@bytedance.com>
> Cc: "Jan Kara"<jack@suse.cz>, "Alexander Viro"<viro@zeniv.linux.org.uk>, "Christian Brauner"<brauner@kernel.org>, "Tejun Heo"<tj@kernel.org>, "Jens Axboe"<axboe@kernel.dk>, <linux-fsdevel@vger.kernel.org>, <linux-kernel@vger.kernel.org>
> On Wed 09-09-26 10:08:08, 尹欣 wrote:
> > > From: "Jan Kara"<jack@suse.cz>
> > > Date:  Tue, Sep 8, 2026, 19:01
> > > Subject:  Re: [PATCH v1 2/2] writeback: snapshot foreign flush pages
> > > To: "Xin Yin"<yinxin.x@bytedance.com>
> > > Cc: "Alexander Viro"<viro@zeniv.linux.org.uk>, "Christian Brauner"<brauner@kernel.org>, "Jan Kara"<jack@suse.cz>, "Tejun Heo"<tj@kernel.org>, "Jens Axboe"<axboe@kernel.dk>, <linux-fsdevel@vger.kernel.org>, <linux-kernel@vger.kernel.org>
> > > On Tue 08-09-26 11:23:52, Xin Yin wrote:
> > > > WB_REASON_FOREIGN_FLUSH is best-effort writeback for foreign dirtying. It
> > > > should write pages that are already visible to the target wb, but should
> > > > not keep extending the same work with pages dirtied while the work is
> > > > running.
> > > > 
> > > > Make foreign flushes use tagged writeback while preserving WB_SYNC_NONE
> > > > semantics. Use the remaining work budget as the per-inode chunk, so a
> > > > tagged pass is not truncated to the normal periodic chunk and a large
> > > > inode still cannot exceed the finite work budget.
> > > > 
> > > > Queue b_dirty only once for each foreign-flush work. Later passes only
> > > > retry b_more_io inodes skipped due to I_SYNC, and the work exits once
> > > > b_io and b_more_io are drained. If the budget is consumed, redirty the
> > > > inode instead of requeueing it to b_more_io so the same work cannot loop
> > > > on a continuously dirtied inode.
> > > > 
> > > > Do not refresh dirtied_when for foreign flushes; later dirtying should be
> > > > handled by later foreign, kupdate, or background writeback.
> > > > 
> > > > Fixes: 97b27821b485 ("writeback, memcg: Implement foreign dirty flushing")
> > > > Signed-off-by: Xin Yin <yinxin.x@bytedance.com>
> > > 
> > > I really dislike how your implementation ties the two writeback deadlock
> > > avoidance schemes (nr_to_write for WB_SYNC_NONE writeback vs folio tagging
> > > + inode dirty timestamp for WB_SYNC_ALL writeback) together. But before we
> > > get to the implementation details let's discuss the purpose a bit:
> > > 
> > > In principle I agree with you that the primary purpose of
> > > cgroup_writeback_by_id() is that it wants to flush inodes that have folios
> > > that are accounted to our memcg. It does this by a rather rough approach of
> > > flushing the whole wb. If this races with other dirtying, it might happen
> > > that we won't actually flush the foreign folios we want. Am I getting your
> > > concern right? Are you able to actually observe such situation in practice?
> > 
> > Yes, that's the concern. Concurrent dirtying could consume the work's
> > budget without sufficiently flushing the foreign folios charged to the
> > memcg we're trying to help.
> > 
> > We haven't confirmed that specific situation in production. What we've
> > observed is long-running foreign work with many subsequent works queued
> > behind it. We haven't traced the original source memcg's folios closely
> > enough to show that they were left dirty because newly dirtied pages
> > consumed the budget. So that part is still a concern, not an observed
> > failure.
> 
> OK. So I fully agree that foreign dirty tracking and flushing is very
> rough. The basic premise of that infrastructure is that foreign dirtying
> should be very rare and amount of foreign dirty pages very low and thus
> largely irrelevant for the dirty throttling. The flushing is there mostly
> as a last resort solution to have a chance of *some* kind of forward
> progress in corner cases where the amount of foreign dirty pages is
> substantial.
> 
> As Tejun wrote [1], bdev inodes are kind of breaking these assumptions.
> Still number of dirty pages there should be low compared to data pages but
> for some workloads that isn't necessarily the case.
> 
> [1] https://lore.kernel.org/all/aqGmmw3rv_2f5Jk4@slm.duckdns.org
> 
> > The idea behind combining the two approaches was to get the budget right
> > for the target wb, while keeping the flush from chasing concurrent
> > dirtying. The first is about not holding up later work on the same wb,
> > including syncfs. There's also a fairness concern: an old inode can keep
> > getting newly dirtied pages written while later-dirtied inodes have to
> > wait for another work. The second is about making the flush more useful
> > to the source memcg, although tagging alone doesn't guarantee that its
> > folios get flushed.
> 
> If you look at the logic in writeback_sb_inodes(), it actually goes to
> great lengths to avoid the fairness issues you describe above. We only
> write a limited amount of pages from each inode (see
> writeback_chunk_size()), then we switch to another one, cycling through all
> inodes queued to b_io list in this way. Inodes on b_io list get fixed when
> the writeback work starts (see queue_io()) so new inodes cannot appear
> there. This is not bulletproof but it avoids the practical livelocking
> cases. So generally I don't expect your systems have practical issues with
> foreign flushes not really flushing what they should.

I may not have explained the fairness concern clearly. The scenario involves multiple foreign-flush work items targeting the same wb. W1 can run for a long time while its eligible inodes are continuously dirtied. Inodes dirtied after W1's `dirtied_before` cutoff are excluded from its scan, while subsequent works that could flush them remain queued behind W1. Our concern is therefore head-of-line blocking between work items, rather than fairness among inodes within a single work. And this is still only a concern, not something we have observed in production.

> 
> > I see your concern about mixing the two mechanisms, though. Would you
> > suggest keeping the budget-only approach for now, or is there a simpler
> > way to address the concurrent-dirtying concern?
> 
> As I wrote above, at this point I'm not convinced concurrent dirtying is
> your practical problem. If you can show foreign flushes in your real
> workloads often don't end up flushing what they should, then we can discuss
> how to improve the writeback behavior (or foreign flush behavior) to avoid
> that.
That makes sense. We can drop patch2 for now and move forward with patch1 alone. Once we have concrete production evidence and a clearer understanding of the problem, we can revisit whether further changes are needed.

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