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@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 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@xxxxxxxx>
SUSE Labs, CR