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

[尹欣]

> From: "Jan Kara"<jack@xxxxxxx>
> Date:  Fri, Sep 11, 2026, 19:09
> Subject:  Re: [PATCH v1 2/2] writeback: snapshot foreign flush pages
> To: "尹欣"<yinxin.x@xxxxxxxxxxxxx>
> Cc: "Jan Kara"<jack@xxxxxxx>, "Alexander Viro"<viro@xxxxxxxxxxxxxxxxxx>, "Christian Brauner"<brauner@xxxxxxxxxx>, "Tejun Heo"<tj@xxxxxxxxxx>, "Jens Axboe"<axboe@xxxxxxxxx>, <linux-fsdevel@xxxxxxxxxxxxxxx>, <linux-kernel@xxxxxxxxxxxxxxx>
> On Wed 09-09-26 10:08:08, 尹欣 wrote:
> > > 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?
> > 
> > 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@xxxxxxxxxxxxxxx
> 
> > 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@xxxxxxxx>
> SUSE Labs, CR
>