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

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

[Jiaming Zhang]

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@mail.gmail.com/
Cc: stable@vger.kernel.org
Assisted-by: Claude Code:claude-opus-5
Signed-off-by: Jiaming Zhang <r772577952@gmail.com>
---
 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);
 			nilfs_copy_folio(dfolio, folio, false);
 			folio_unlock(dfolio);
 			folio_put(dfolio);
-- 
2.43.0

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

[Viacheslav Dubeyko]

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@mail.gmail.com/
> Cc: stable@vger.kernel.org
> Assisted-by: Claude Code:claude-opus-5
> Signed-off-by: Jiaming Zhang <r772577952@gmail.com>
> ---
>  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?

Thanks,
Slava.

>  			nilfs_copy_folio(dfolio, folio, false);
>  			folio_unlock(dfolio);
>  			folio_put(dfolio);

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

[Jiaming Zhang]

Viacheslav Dubeyko <slava@dubeyko.com> 于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@mail.gmail.com/
> > Cc: stable@vger.kernel.org
> > Assisted-by: Claude Code:claude-opus-5
> > Signed-off-by: Jiaming Zhang <r772577952@gmail.com>
> > ---
> >  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@mail.gmail.com/T/#t

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