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

From: 尹欣

Date: Tue Sep 08 2026 - 22:08:41 EST

> From: "Jan Kara"<jack@xxxxxxx>
> Date: Tue, Sep 8, 2026, 19:01
> Subject: Re: [PATCH v1 2/2] writeback: snapshot foreign flush pages
> To: "Xin Yin"<yinxin.x@xxxxxxxxxxxxx>
> Cc: "Alexander Viro"<viro@xxxxxxxxxxxxxxxxxx>, "Christian Brauner"<brauner@xxxxxxxxxx>, "Jan Kara"<jack@xxxxxxx>, "Tejun Heo"<tj@xxxxxxxxxx>, "Jens Axboe"<axboe@xxxxxxxxx>, <linux-fsdevel@xxxxxxxxxxxxxxx>, <linux-kernel@xxxxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxx>
>
> 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@xxxxxxxx>
> SUSE Labs, CR
>