Re: [PATCH] nilfs2: clear folio dirty flag when copying back from the shadow map

[Jiaming Zhang]

Viacheslav Dubeyko <slava@xxxxxxxxxxx> 于2026年9月2日周三 02:13写道：
>
> On Tue, 2026-09-01 at 21:44 +0800, Jiaming Zhang wrote:
> > While garbage collection runs, nilfs2 keeps a shadow copy of the DAT
> > metadata file's page cache so that it can roll the file back if GC
> > fails.  The rollback has two steps: nilfs_clear_dirty_pages() drops
> > the
> > dirty state of the folios in the DAT cache, then
> > nilfs_copy_back_pages()
> > overwrites them with the saved contents.  The second step warns if it
> > still finds a dirty folio, because the first step is supposed to have
> > cleared every one of them:
> >
> >       /* overwrite existing folio in the destination cache */
> >       WARN_ON(folio_test_dirty(dfolio));
> >
> > Clearing has been best-effort since commit ca76bb226bf4 ("nilfs2: do
> > not
> > force clear folio if buffer is referenced"):
> > nilfs_clear_folio_dirty()
> > leaves a folio dirty if a buffer head under it is still busy.
> > Reading
> > metadata creates such buffers.  nilfs_mdt_read_block() submits
> > read-ahead for the blocks following the one it was asked for and
> > waits
> > only for that one, so the read-ahead buffers are still locked when it
> > returns.  When the block size is smaller than the page size, several
> > metadata blocks share a folio, so a single folio can hold both a
> > dirty
> > block and a locked read-ahead buffer.  Such a folio survives the
> > clearing step, and the copy-back warns on it.
> >
> > Use __nilfs_clear_folio_dirty() to clear the dirty flag of the
> > destination folio before overwriting it, rather than making
> > nilfs_clear_folio_dirty() force-clear busy buffer heads again.
> >
> > Fixes: ca76bb226bf4 ("nilfs2: do not force clear folio if buffer is
> > referenced")
> > Closes:
> > https://lore.kernel.org/lkml/CANypQFZSYrtcshnUzOPiqatyLd-M8_OReOewQoAi_V5yY0dTtg@xxxxxxxxxxxxxx/
> > Cc: stable@xxxxxxxxxxxxxxx
> > Assisted-by: Claude Code:claude-opus-5
> > Signed-off-by: Jiaming Zhang <r772577952@xxxxxxxxx>
> > ---
> >  fs/nilfs2/page.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/nilfs2/page.c b/fs/nilfs2/page.c
> > index cf4f1c6798f5..b26d9c3bda6d 100644
> > --- a/fs/nilfs2/page.c
> > +++ b/fs/nilfs2/page.c
> > @@ -328,7 +328,8 @@ void nilfs_copy_back_pages(struct address_space
> > *dmap,
> >               dfolio = filemap_lock_folio(dmap, index);
> >               if (!IS_ERR(dfolio)) {
> >                       /* overwrite existing folio in the
> > destination cache */
> > -                     WARN_ON(folio_test_dirty(dfolio));
> > +                     if (unlikely(folio_test_dirty(dfolio)))
> > +                             __nilfs_clear_folio_dirty(dfolio);
>
> Maybe, I am missing something. But the main point of this WARN() if we
> have found that folio is dirty, then potentially we are trying to re-
> write already modified data by already obsolete state. How can we
> distinguish that folio has been survived the clearing step or it can be
> somehow modified in the background? Even if it is survived the
> clearing, then it sounds that something is wrong in another logic. What
> do you think?

I think the folio cannot be modified in the background here, because
nilfs_clean_segments() holds ns_segctor_sem for write across the
rollback. ns_segctor_sem stops the log writer, and every other path
that could dirty the DAT cache takes the same semaphore for read in
nilfs_transaction_begin().  So it should only be a folio that
nilfs_clear_dirty_pages() failed to clear, and what blocked the
clearing is an in-flight read-ahead buffer from
nilfs_mdt_read_block(), not somebody's modified data.

And you are right that this means the problem is elsewhere: the
clearing step is what needs fixing, not the WARN_ON().  I discussed
this with Ryusuke in the report thread [1], he suggested adding a bool
force argument so that the forced clearing is restored for the
rollback caller only.  I will send a v2 patch with that approach
shortly, please review it at your convenience.

[1] https://lore.kernel.org/lkml/CANypQFaUfeXU7S9N0PpJP343h1ibRxSJ27oJxhJ0Jd5ChpiDcw@xxxxxxxxxxxxxx/T/#t

>
> Thanks,
> Slava.
>
> >                       nilfs_copy_folio(dfolio, folio, false);
> >                       folio_unlock(dfolio);
> >                       folio_put(dfolio);