[PATCH v2] nilfs2: force clear dirty state when restoring from the shadow map

Jiaming Zhang posted 1 patch 3 weeks, 2 days ago
fs/nilfs2/inode.c |  2 +-
fs/nilfs2/mdt.c   |  6 +++---
fs/nilfs2/page.c  | 50 +++++++++++++++++++++++++++--------------------
fs/nilfs2/page.h  |  4 ++--
4 files changed, 35 insertions(+), 27 deletions(-)
[PATCH v2] nilfs2: force clear dirty state when restoring from the shadow map
Posted by Jiaming Zhang 3 weeks, 2 days ago
When garbage collection fails, nilfs2 rolls the DAT metadata file's page
cache back to a shadow copy taken before GC started.
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 and warns if one is still dirty:

	/* overwrite existing folio in the destination cache */
	WARN_ON(folio_test_dirty(dfolio));

nilfs_clear_dirty_pages() used to clear the dirty state unconditionally,
which is safe here because the rollback runs with the log writer
stopped, so nothing else can dirty the cache while it runs.  The same
helper is also used when writeback finds dirty folios after the
filesystem has degraded to read-only, where it does run concurrently
with the log writer, so commit ca76bb226bf4 ("nilfs2: do not force clear
folio if buffer is referenced") made nilfs_clear_folio_dirty() skip a
folio if any of its buffer heads is busy.  The rollback caller shares
that helper, so its clearing step can now return with a folio still
dirty.

A DAT folio can hold a busy buffer head without anyone modifying the
folio: nilfs_mdt_read_block() submits read-ahead for the blocks
following the one it was asked for and waits only for the first, so the
read-ahead buffers are still locked when it returns.  With a block size
smaller than the page size, a locked read-ahead buffer can share a folio
with a block that GC dirtied and keep the whole folio dirty past the
clearing step.

Add a force flag to nilfs_clear_dirty_pages() and
nilfs_clear_folio_dirty() that skips the busy buffer check, and set it
in the two calls from nilfs_mdt_restore_from_shadow_map().  The
read-only fallback callers keep passing false, so that commit still
protects them and the WARN_ON() is left alone.

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
Suggested-by: Ryusuke Konishi <konishi.ryusuke@gmail.com>
Assisted-by: Claude Code:claude-opus-5
Signed-off-by: Jiaming Zhang <r772577952@gmail.com>
---
Changes in v2:
- Keep the WARN_ON() in nilfs_copy_back_pages() untouched.
- Add a bool force argument to nilfs_clear_dirty_pages() and
  nilfs_clear_folio_dirty(), and set it to true in the two calls from
  nilfs_mdt_restore_from_shadow_map().

v1: https://lore.kernel.org/lkml/20260901134430.1292467-1-r772577952@gmail.com/

 fs/nilfs2/inode.c |  2 +-
 fs/nilfs2/mdt.c   |  6 +++---
 fs/nilfs2/page.c  | 50 +++++++++++++++++++++++++++--------------------
 fs/nilfs2/page.h  |  4 ++--
 4 files changed, 35 insertions(+), 27 deletions(-)

diff --git a/fs/nilfs2/inode.c b/fs/nilfs2/inode.c
index 34e6096069ad..64437aed8390 100644
--- a/fs/nilfs2/inode.c
+++ b/fs/nilfs2/inode.c
@@ -163,7 +163,7 @@ static int nilfs_writepages(struct address_space *mapping,
 	int err = 0;
 
 	if (sb_rdonly(inode->i_sb)) {
-		nilfs_clear_dirty_pages(mapping);
+		nilfs_clear_dirty_pages(mapping, false);
 		return -EROFS;
 	}
 
diff --git a/fs/nilfs2/mdt.c b/fs/nilfs2/mdt.c
index 2a435349fd21..b50c88b65183 100644
--- a/fs/nilfs2/mdt.c
+++ b/fs/nilfs2/mdt.c
@@ -405,7 +405,7 @@ static int nilfs_mdt_write_folio(struct folio *folio,
 		 * have dirty folios that try to be flushed in background.
 		 * So, here we simply discard this dirty folio.
 		 */
-		nilfs_clear_folio_dirty(folio);
+		nilfs_clear_folio_dirty(folio, false);
 		folio_unlock(folio);
 		return -EROFS;
 	}
@@ -648,10 +648,10 @@ void nilfs_mdt_restore_from_shadow_map(struct inode *inode)
 	if (mi->mi_palloc_cache)
 		nilfs_palloc_clear_cache(inode);
 
-	nilfs_clear_dirty_pages(inode->i_mapping);
+	nilfs_clear_dirty_pages(inode->i_mapping, true);
 	nilfs_copy_back_pages(inode->i_mapping, shadow->inode->i_mapping);
 
-	nilfs_clear_dirty_pages(ii->i_assoc_inode->i_mapping);
+	nilfs_clear_dirty_pages(ii->i_assoc_inode->i_mapping, true);
 	nilfs_copy_back_pages(ii->i_assoc_inode->i_mapping,
 			      NILFS_I(shadow->inode)->i_assoc_inode->i_mapping);
 
diff --git a/fs/nilfs2/page.c b/fs/nilfs2/page.c
index cf4f1c6798f5..857926da9f21 100644
--- a/fs/nilfs2/page.c
+++ b/fs/nilfs2/page.c
@@ -369,8 +369,9 @@ void nilfs_copy_back_pages(struct address_space *dmap,
 /**
  * nilfs_clear_dirty_pages - discard dirty pages in address space
  * @mapping: address space with dirty pages for discarding
+ * @force: whether to clear the dirty state regardless of busy buffer heads
  */
-void nilfs_clear_dirty_pages(struct address_space *mapping)
+void nilfs_clear_dirty_pages(struct address_space *mapping, bool force)
 {
 	struct folio_batch fbatch;
 	unsigned int i;
@@ -391,7 +392,7 @@ void nilfs_clear_dirty_pages(struct address_space *mapping)
 			 * was acquired.  Skip processing in that case.
 			 */
 			if (likely(folio->mapping == mapping))
-				nilfs_clear_folio_dirty(folio);
+				nilfs_clear_folio_dirty(folio, force);
 
 			folio_unlock(folio);
 		}
@@ -403,13 +404,16 @@ void nilfs_clear_dirty_pages(struct address_space *mapping)
 /**
  * nilfs_clear_folio_dirty - discard dirty folio
  * @folio: dirty folio that will be discarded
+ * @force: whether to clear the states regardless of busy buffer heads
  *
  * nilfs_clear_folio_dirty() clears working states including dirty state for
- * the folio and its buffers.  If the folio has buffers, clear only if it is
- * confirmed that none of the buffer heads are busy (none have valid
- * references and none are locked).
+ * the folio and its buffers.  If the folio has buffers and force is false,
+ * clear only if it is confirmed that none of the buffer heads are busy (none
+ * have valid references and none are locked).  If force is true, the states
+ * are cleared unconditionally, the caller should guarantee that the folio is
+ * not being modified concurrently.
  */
-void nilfs_clear_folio_dirty(struct folio *folio)
+void nilfs_clear_folio_dirty(struct folio *folio, bool force)
 {
 	struct buffer_head *bh, *head;
 
@@ -422,24 +426,28 @@ void nilfs_clear_folio_dirty(struct folio *folio)
 			 BIT(BH_Async_Write) | BIT(BH_NILFS_Volatile) |
 			 BIT(BH_NILFS_Checked) | BIT(BH_NILFS_Redirected) |
 			 BIT(BH_Delay));
-		bool busy, invalidated = false;
+
+		if (!force) {
+			bool busy, invalidated = false;
 
 recheck_buffers:
-		busy = false;
-		bh = head;
-		do {
-			if (atomic_read(&bh->b_count) | buffer_locked(bh)) {
-				busy = true;
-				break;
+			busy = false;
+			bh = head;
+			do {
+				if (atomic_read(&bh->b_count) |
+				    buffer_locked(bh)) {
+					busy = true;
+					break;
+				}
+			} while (bh = bh->b_this_page, bh != head);
+
+			if (busy) {
+				if (invalidated)
+					return;
+				invalidate_bh_lrus();
+				invalidated = true;
+				goto recheck_buffers;
 			}
-		} while (bh = bh->b_this_page, bh != head);
-
-		if (busy) {
-			if (invalidated)
-				return;
-			invalidate_bh_lrus();
-			invalidated = true;
-			goto recheck_buffers;
 		}
 
 		bh = head;
diff --git a/fs/nilfs2/page.h b/fs/nilfs2/page.h
index 136cd1c143c9..c3ba3468af5c 100644
--- a/fs/nilfs2/page.h
+++ b/fs/nilfs2/page.h
@@ -41,8 +41,8 @@ void nilfs_folio_bug(struct folio *);
 
 int nilfs_copy_dirty_pages(struct address_space *, struct address_space *);
 void nilfs_copy_back_pages(struct address_space *, struct address_space *);
-void nilfs_clear_folio_dirty(struct folio *folio);
-void nilfs_clear_dirty_pages(struct address_space *mapping);
+void nilfs_clear_folio_dirty(struct folio *folio, bool force);
+void nilfs_clear_dirty_pages(struct address_space *mapping, bool force);
 unsigned int nilfs_page_count_clean_buffers(struct folio *folio,
 		unsigned int from, unsigned int to);
 unsigned long nilfs_find_uncommitted_extent(struct inode *inode,
-- 
2.43.0
Re: [PATCH v2] nilfs2: force clear dirty state when restoring from the shadow map
Posted by Ryusuke Konishi 1 week, 2 days ago
On Wed, Sep 2, 2026 at 8:21 PM Jiaming Zhang wrote:
>
> When garbage collection fails, nilfs2 rolls the DAT metadata file's page
> cache back to a shadow copy taken before GC started.
> 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 and warns if one is still dirty:
>
>         /* overwrite existing folio in the destination cache */
>         WARN_ON(folio_test_dirty(dfolio));
>
> nilfs_clear_dirty_pages() used to clear the dirty state unconditionally,
> which is safe here because the rollback runs with the log writer
> stopped, so nothing else can dirty the cache while it runs.  The same
> helper is also used when writeback finds dirty folios after the
> filesystem has degraded to read-only, where it does run concurrently
> with the log writer, so commit ca76bb226bf4 ("nilfs2: do not force clear
> folio if buffer is referenced") made nilfs_clear_folio_dirty() skip a
> folio if any of its buffer heads is busy.  The rollback caller shares
> that helper, so its clearing step can now return with a folio still
> dirty.
>
> A DAT folio can hold a busy buffer head without anyone modifying the
> folio: nilfs_mdt_read_block() submits read-ahead for the blocks
> following the one it was asked for and waits only for the first, so the
> read-ahead buffers are still locked when it returns.  With a block size
> smaller than the page size, a locked read-ahead buffer can share a folio
> with a block that GC dirtied and keep the whole folio dirty past the
> clearing step.
>
> Add a force flag to nilfs_clear_dirty_pages() and
> nilfs_clear_folio_dirty() that skips the busy buffer check, and set it
> in the two calls from nilfs_mdt_restore_from_shadow_map().  The
> read-only fallback callers keep passing false, so that commit still
> protects them and the WARN_ON() is left alone.
>
> 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
> Suggested-by: Ryusuke Konishi <konishi.ryusuke@gmail.com>
> Assisted-by: Claude Code:claude-opus-5
> Signed-off-by: Jiaming Zhang <r772577952@gmail.com>
> ---
> Changes in v2:
> - Keep the WARN_ON() in nilfs_copy_back_pages() untouched.
> - Add a bool force argument to nilfs_clear_dirty_pages() and
>   nilfs_clear_folio_dirty(), and set it to true in the two calls from
>   nilfs_mdt_restore_from_shadow_map().
>
> v1: https://lore.kernel.org/lkml/20260901134430.1292467-1-r772577952@gmail.com/

Acked-by: Ryusuke Konishi <konishi.ryusuke@gmail.com>

Viacheslav, could you please apply this directly to your tree?

Both the patch contents and the test results were exactly as expected.

Thanks,
Ryusuke Konishi

>
>  fs/nilfs2/inode.c |  2 +-
>  fs/nilfs2/mdt.c   |  6 +++---
>  fs/nilfs2/page.c  | 50 +++++++++++++++++++++++++++--------------------
>  fs/nilfs2/page.h  |  4 ++--
>  4 files changed, 35 insertions(+), 27 deletions(-)
>
> diff --git a/fs/nilfs2/inode.c b/fs/nilfs2/inode.c
> index 34e6096069ad..64437aed8390 100644
> --- a/fs/nilfs2/inode.c
> +++ b/fs/nilfs2/inode.c
> @@ -163,7 +163,7 @@ static int nilfs_writepages(struct address_space *mapping,
>         int err = 0;
>
>         if (sb_rdonly(inode->i_sb)) {
> -               nilfs_clear_dirty_pages(mapping);
> +               nilfs_clear_dirty_pages(mapping, false);
>                 return -EROFS;
>         }
>
> diff --git a/fs/nilfs2/mdt.c b/fs/nilfs2/mdt.c
> index 2a435349fd21..b50c88b65183 100644
> --- a/fs/nilfs2/mdt.c
> +++ b/fs/nilfs2/mdt.c
> @@ -405,7 +405,7 @@ static int nilfs_mdt_write_folio(struct folio *folio,
>                  * have dirty folios that try to be flushed in background.
>                  * So, here we simply discard this dirty folio.
>                  */
> -               nilfs_clear_folio_dirty(folio);
> +               nilfs_clear_folio_dirty(folio, false);
>                 folio_unlock(folio);
>                 return -EROFS;
>         }
> @@ -648,10 +648,10 @@ void nilfs_mdt_restore_from_shadow_map(struct inode *inode)
>         if (mi->mi_palloc_cache)
>                 nilfs_palloc_clear_cache(inode);
>
> -       nilfs_clear_dirty_pages(inode->i_mapping);
> +       nilfs_clear_dirty_pages(inode->i_mapping, true);
>         nilfs_copy_back_pages(inode->i_mapping, shadow->inode->i_mapping);
>
> -       nilfs_clear_dirty_pages(ii->i_assoc_inode->i_mapping);
> +       nilfs_clear_dirty_pages(ii->i_assoc_inode->i_mapping, true);
>         nilfs_copy_back_pages(ii->i_assoc_inode->i_mapping,
>                               NILFS_I(shadow->inode)->i_assoc_inode->i_mapping);
>
> diff --git a/fs/nilfs2/page.c b/fs/nilfs2/page.c
> index cf4f1c6798f5..857926da9f21 100644
> --- a/fs/nilfs2/page.c
> +++ b/fs/nilfs2/page.c
> @@ -369,8 +369,9 @@ void nilfs_copy_back_pages(struct address_space *dmap,
>  /**
>   * nilfs_clear_dirty_pages - discard dirty pages in address space
>   * @mapping: address space with dirty pages for discarding
> + * @force: whether to clear the dirty state regardless of busy buffer heads
>   */
> -void nilfs_clear_dirty_pages(struct address_space *mapping)
> +void nilfs_clear_dirty_pages(struct address_space *mapping, bool force)
>  {
>         struct folio_batch fbatch;
>         unsigned int i;
> @@ -391,7 +392,7 @@ void nilfs_clear_dirty_pages(struct address_space *mapping)
>                          * was acquired.  Skip processing in that case.
>                          */
>                         if (likely(folio->mapping == mapping))
> -                               nilfs_clear_folio_dirty(folio);
> +                               nilfs_clear_folio_dirty(folio, force);
>
>                         folio_unlock(folio);
>                 }
> @@ -403,13 +404,16 @@ void nilfs_clear_dirty_pages(struct address_space *mapping)
>  /**
>   * nilfs_clear_folio_dirty - discard dirty folio
>   * @folio: dirty folio that will be discarded
> + * @force: whether to clear the states regardless of busy buffer heads
>   *
>   * nilfs_clear_folio_dirty() clears working states including dirty state for
> - * the folio and its buffers.  If the folio has buffers, clear only if it is
> - * confirmed that none of the buffer heads are busy (none have valid
> - * references and none are locked).
> + * the folio and its buffers.  If the folio has buffers and force is false,
> + * clear only if it is confirmed that none of the buffer heads are busy (none
> + * have valid references and none are locked).  If force is true, the states
> + * are cleared unconditionally, the caller should guarantee that the folio is
> + * not being modified concurrently.
>   */
> -void nilfs_clear_folio_dirty(struct folio *folio)
> +void nilfs_clear_folio_dirty(struct folio *folio, bool force)
>  {
>         struct buffer_head *bh, *head;
>
> @@ -422,24 +426,28 @@ void nilfs_clear_folio_dirty(struct folio *folio)
>                          BIT(BH_Async_Write) | BIT(BH_NILFS_Volatile) |
>                          BIT(BH_NILFS_Checked) | BIT(BH_NILFS_Redirected) |
>                          BIT(BH_Delay));
> -               bool busy, invalidated = false;
> +
> +               if (!force) {
> +                       bool busy, invalidated = false;
>
>  recheck_buffers:
> -               busy = false;
> -               bh = head;
> -               do {
> -                       if (atomic_read(&bh->b_count) | buffer_locked(bh)) {
> -                               busy = true;
> -                               break;
> +                       busy = false;
> +                       bh = head;
> +                       do {
> +                               if (atomic_read(&bh->b_count) |
> +                                   buffer_locked(bh)) {
> +                                       busy = true;
> +                                       break;
> +                               }
> +                       } while (bh = bh->b_this_page, bh != head);
> +
> +                       if (busy) {
> +                               if (invalidated)
> +                                       return;
> +                               invalidate_bh_lrus();
> +                               invalidated = true;
> +                               goto recheck_buffers;
>                         }
> -               } while (bh = bh->b_this_page, bh != head);
> -
> -               if (busy) {
> -                       if (invalidated)
> -                               return;
> -                       invalidate_bh_lrus();
> -                       invalidated = true;
> -                       goto recheck_buffers;
>                 }
>
>                 bh = head;
> diff --git a/fs/nilfs2/page.h b/fs/nilfs2/page.h
> index 136cd1c143c9..c3ba3468af5c 100644
> --- a/fs/nilfs2/page.h
> +++ b/fs/nilfs2/page.h
> @@ -41,8 +41,8 @@ void nilfs_folio_bug(struct folio *);
>
>  int nilfs_copy_dirty_pages(struct address_space *, struct address_space *);
>  void nilfs_copy_back_pages(struct address_space *, struct address_space *);
> -void nilfs_clear_folio_dirty(struct folio *folio);
> -void nilfs_clear_dirty_pages(struct address_space *mapping);
> +void nilfs_clear_folio_dirty(struct folio *folio, bool force);
> +void nilfs_clear_dirty_pages(struct address_space *mapping, bool force);
>  unsigned int nilfs_page_count_clean_buffers(struct folio *folio,
>                 unsigned int from, unsigned int to);
>  unsigned long nilfs_find_uncommitted_extent(struct inode *inode,
> --
> 2.43.0
>
Re: [PATCH v2] nilfs2: force clear dirty state when restoring from the shadow map
Posted by Viacheslav Dubeyko 1 week, 1 day ago
On Thu, 2026-09-17 at 14:54 +0900, Ryusuke Konishi wrote:
> On Wed, Sep 2, 2026 at 8:21 PM Jiaming Zhang wrote:
> > 
> > When garbage collection fails, nilfs2 rolls the DAT metadata file's
> > page
> > cache back to a shadow copy taken before GC started.
> > 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 and warns if one is still dirty:
> > 
> >         /* overwrite existing folio in the destination cache */
> >         WARN_ON(folio_test_dirty(dfolio));
> > 
> > nilfs_clear_dirty_pages() used to clear the dirty state
> > unconditionally,
> > which is safe here because the rollback runs with the log writer
> > stopped, so nothing else can dirty the cache while it runs.  The
> > same
> > helper is also used when writeback finds dirty folios after the
> > filesystem has degraded to read-only, where it does run
> > concurrently
> > with the log writer, so commit ca76bb226bf4 ("nilfs2: do not force
> > clear
> > folio if buffer is referenced") made nilfs_clear_folio_dirty() skip
> > a
> > folio if any of its buffer heads is busy.  The rollback caller
> > shares
> > that helper, so its clearing step can now return with a folio still
> > dirty.
> > 
> > A DAT folio can hold a busy buffer head without anyone modifying
> > the
> > folio: nilfs_mdt_read_block() submits read-ahead for the blocks
> > following the one it was asked for and waits only for the first, so
> > the
> > read-ahead buffers are still locked when it returns.  With a block
> > size
> > smaller than the page size, a locked read-ahead buffer can share a
> > folio
> > with a block that GC dirtied and keep the whole folio dirty past
> > the
> > clearing step.
> > 
> > Add a force flag to nilfs_clear_dirty_pages() and
> > nilfs_clear_folio_dirty() that skips the busy buffer check, and set
> > it
> > in the two calls from nilfs_mdt_restore_from_shadow_map().  The
> > read-only fallback callers keep passing false, so that commit still
> > protects them and the WARN_ON() is left alone.
> > 
> > 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
> > Suggested-by: Ryusuke Konishi <konishi.ryusuke@gmail.com>
> > Assisted-by: Claude Code:claude-opus-5
> > Signed-off-by: Jiaming Zhang <r772577952@gmail.com>
> > ---
> > Changes in v2:
> > - Keep the WARN_ON() in nilfs_copy_back_pages() untouched.
> > - Add a bool force argument to nilfs_clear_dirty_pages() and
> >   nilfs_clear_folio_dirty(), and set it to true in the two calls
> > from
> >   nilfs_mdt_restore_from_shadow_map().
> > 
> > v1:
> > https://lore.kernel.org/lkml/20260901134430.1292467-1-r772577952@gmail.com/
> 
> Acked-by: Ryusuke Konishi <konishi.ryusuke@gmail.com>
> 
> Viacheslav, could you please apply this directly to your tree?
> 
> Both the patch contents and the test results were exactly as
> expected.
> 
> Thanks,
> Ryusuke Konishi
> 
> > 
> >  fs/nilfs2/inode.c |  2 +-
> >  fs/nilfs2/mdt.c   |  6 +++---
> >  fs/nilfs2/page.c  | 50 +++++++++++++++++++++++++++----------------
> > ----
> >  fs/nilfs2/page.h  |  4 ++--
> >  4 files changed, 35 insertions(+), 27 deletions(-)
> > 
> > diff --git a/fs/nilfs2/inode.c b/fs/nilfs2/inode.c
> > index 34e6096069ad..64437aed8390 100644
> > --- a/fs/nilfs2/inode.c
> > +++ b/fs/nilfs2/inode.c
> > @@ -163,7 +163,7 @@ static int nilfs_writepages(struct
> > address_space *mapping,
> >         int err = 0;
> > 
> >         if (sb_rdonly(inode->i_sb)) {
> > -               nilfs_clear_dirty_pages(mapping);
> > +               nilfs_clear_dirty_pages(mapping, false);
> >                 return -EROFS;
> >         }
> > 
> > diff --git a/fs/nilfs2/mdt.c b/fs/nilfs2/mdt.c
> > index 2a435349fd21..b50c88b65183 100644
> > --- a/fs/nilfs2/mdt.c
> > +++ b/fs/nilfs2/mdt.c
> > @@ -405,7 +405,7 @@ static int nilfs_mdt_write_folio(struct folio
> > *folio,
> >                  * have dirty folios that try to be flushed in
> > background.
> >                  * So, here we simply discard this dirty folio.
> >                  */
> > -               nilfs_clear_folio_dirty(folio);
> > +               nilfs_clear_folio_dirty(folio, false);
> >                 folio_unlock(folio);
> >                 return -EROFS;
> >         }
> > @@ -648,10 +648,10 @@ void nilfs_mdt_restore_from_shadow_map(struct
> > inode *inode)
> >         if (mi->mi_palloc_cache)
> >                 nilfs_palloc_clear_cache(inode);
> > 
> > -       nilfs_clear_dirty_pages(inode->i_mapping);
> > +       nilfs_clear_dirty_pages(inode->i_mapping, true);
> >         nilfs_copy_back_pages(inode->i_mapping, shadow->inode-
> > >i_mapping);
> > 
> > -       nilfs_clear_dirty_pages(ii->i_assoc_inode->i_mapping);
> > +       nilfs_clear_dirty_pages(ii->i_assoc_inode->i_mapping,
> > true);
> >         nilfs_copy_back_pages(ii->i_assoc_inode->i_mapping,
> >                               NILFS_I(shadow->inode)-
> > >i_assoc_inode->i_mapping);
> > 
> > diff --git a/fs/nilfs2/page.c b/fs/nilfs2/page.c
> > index cf4f1c6798f5..857926da9f21 100644
> > --- a/fs/nilfs2/page.c
> > +++ b/fs/nilfs2/page.c
> > @@ -369,8 +369,9 @@ void nilfs_copy_back_pages(struct address_space
> > *dmap,
> >  /**
> >   * nilfs_clear_dirty_pages - discard dirty pages in address space
> >   * @mapping: address space with dirty pages for discarding
> > + * @force: whether to clear the dirty state regardless of busy
> > buffer heads
> >   */
> > -void nilfs_clear_dirty_pages(struct address_space *mapping)
> > +void nilfs_clear_dirty_pages(struct address_space *mapping, bool
> > force)
> >  {
> >         struct folio_batch fbatch;
> >         unsigned int i;
> > @@ -391,7 +392,7 @@ void nilfs_clear_dirty_pages(struct
> > address_space *mapping)
> >                          * was acquired.  Skip processing in that
> > case.
> >                          */
> >                         if (likely(folio->mapping == mapping))
> > -                               nilfs_clear_folio_dirty(folio);
> > +                               nilfs_clear_folio_dirty(folio,
> > force);
> > 
> >                         folio_unlock(folio);
> >                 }
> > @@ -403,13 +404,16 @@ void nilfs_clear_dirty_pages(struct
> > address_space *mapping)
> >  /**
> >   * nilfs_clear_folio_dirty - discard dirty folio
> >   * @folio: dirty folio that will be discarded
> > + * @force: whether to clear the states regardless of busy buffer
> > heads
> >   *
> >   * nilfs_clear_folio_dirty() clears working states including dirty
> > state for
> > - * the folio and its buffers.  If the folio has buffers, clear
> > only if it is
> > - * confirmed that none of the buffer heads are busy (none have
> > valid
> > - * references and none are locked).
> > + * the folio and its buffers.  If the folio has buffers and force
> > is false,
> > + * clear only if it is confirmed that none of the buffer heads are
> > busy (none
> > + * have valid references and none are locked).  If force is true,
> > the states
> > + * are cleared unconditionally, the caller should guarantee that
> > the folio is
> > + * not being modified concurrently.
> >   */
> > -void nilfs_clear_folio_dirty(struct folio *folio)
> > +void nilfs_clear_folio_dirty(struct folio *folio, bool force)
> >  {
> >         struct buffer_head *bh, *head;
> > 
> > @@ -422,24 +426,28 @@ void nilfs_clear_folio_dirty(struct folio
> > *folio)
> >                          BIT(BH_Async_Write) |
> > BIT(BH_NILFS_Volatile) |
> >                          BIT(BH_NILFS_Checked) |
> > BIT(BH_NILFS_Redirected) |
> >                          BIT(BH_Delay));
> > -               bool busy, invalidated = false;
> > +
> > +               if (!force) {
> > +                       bool busy, invalidated = false;
> > 
> >  recheck_buffers:
> > -               busy = false;
> > -               bh = head;
> > -               do {
> > -                       if (atomic_read(&bh->b_count) |
> > buffer_locked(bh)) {
> > -                               busy = true;
> > -                               break;
> > +                       busy = false;
> > +                       bh = head;
> > +                       do {
> > +                               if (atomic_read(&bh->b_count) |
> > +                                   buffer_locked(bh)) {
> > +                                       busy = true;
> > +                                       break;
> > +                               }
> > +                       } while (bh = bh->b_this_page, bh != head);
> > +
> > +                       if (busy) {
> > +                               if (invalidated)
> > +                                       return;
> > +                               invalidate_bh_lrus();
> > +                               invalidated = true;
> > +                               goto recheck_buffers;
> >                         }
> > -               } while (bh = bh->b_this_page, bh != head);
> > -
> > -               if (busy) {
> > -                       if (invalidated)
> > -                               return;
> > -                       invalidate_bh_lrus();
> > -                       invalidated = true;
> > -                       goto recheck_buffers;
> >                 }
> > 
> >                 bh = head;
> > diff --git a/fs/nilfs2/page.h b/fs/nilfs2/page.h
> > index 136cd1c143c9..c3ba3468af5c 100644
> > --- a/fs/nilfs2/page.h
> > +++ b/fs/nilfs2/page.h
> > @@ -41,8 +41,8 @@ void nilfs_folio_bug(struct folio *);
> > 
> >  int nilfs_copy_dirty_pages(struct address_space *, struct
> > address_space *);
> >  void nilfs_copy_back_pages(struct address_space *, struct
> > address_space *);
> > -void nilfs_clear_folio_dirty(struct folio *folio);
> > -void nilfs_clear_dirty_pages(struct address_space *mapping);
> > +void nilfs_clear_folio_dirty(struct folio *folio, bool force);
> > +void nilfs_clear_dirty_pages(struct address_space *mapping, bool
> > force);
> >  unsigned int nilfs_page_count_clean_buffers(struct folio *folio,
> >                 unsigned int from, unsigned int to);
> >  unsigned long nilfs_find_uncommitted_extent(struct inode *inode,
> > --
> > 2.43.0
> > 

Applied.

Thanks,
Slava.
Re: [PATCH v2] nilfs2: force clear dirty state when restoring from the shadow map
Posted by Jiaming Zhang 1 week, 5 days ago
Jiaming Zhang <r772577952@gmail.com> 于2026年9月2日周三 19:21写道:
>
> When garbage collection fails, nilfs2 rolls the DAT metadata file's page
> cache back to a shadow copy taken before GC started.
> 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 and warns if one is still dirty:
>
>         /* overwrite existing folio in the destination cache */
>         WARN_ON(folio_test_dirty(dfolio));
>
> nilfs_clear_dirty_pages() used to clear the dirty state unconditionally,
> which is safe here because the rollback runs with the log writer
> stopped, so nothing else can dirty the cache while it runs.  The same
> helper is also used when writeback finds dirty folios after the
> filesystem has degraded to read-only, where it does run concurrently
> with the log writer, so commit ca76bb226bf4 ("nilfs2: do not force clear
> folio if buffer is referenced") made nilfs_clear_folio_dirty() skip a
> folio if any of its buffer heads is busy.  The rollback caller shares
> that helper, so its clearing step can now return with a folio still
> dirty.
>
> A DAT folio can hold a busy buffer head without anyone modifying the
> folio: nilfs_mdt_read_block() submits read-ahead for the blocks
> following the one it was asked for and waits only for the first, so the
> read-ahead buffers are still locked when it returns.  With a block size
> smaller than the page size, a locked read-ahead buffer can share a folio
> with a block that GC dirtied and keep the whole folio dirty past the
> clearing step.
>
> Add a force flag to nilfs_clear_dirty_pages() and
> nilfs_clear_folio_dirty() that skips the busy buffer check, and set it
> in the two calls from nilfs_mdt_restore_from_shadow_map().  The
> read-only fallback callers keep passing false, so that commit still
> protects them and the WARN_ON() is left alone.
>
> 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
> Suggested-by: Ryusuke Konishi <konishi.ryusuke@gmail.com>
> Assisted-by: Claude Code:claude-opus-5
> Signed-off-by: Jiaming Zhang <r772577952@gmail.com>
> ---
> Changes in v2:
> - Keep the WARN_ON() in nilfs_copy_back_pages() untouched.
> - Add a bool force argument to nilfs_clear_dirty_pages() and
>   nilfs_clear_folio_dirty(), and set it to true in the two calls from
>   nilfs_mdt_restore_from_shadow_map().
>
> v1: https://lore.kernel.org/lkml/20260901134430.1292467-1-r772577952@gmail.com/
>
>  fs/nilfs2/inode.c |  2 +-
>  fs/nilfs2/mdt.c   |  6 +++---
>  fs/nilfs2/page.c  | 50 +++++++++++++++++++++++++++--------------------
>  fs/nilfs2/page.h  |  4 ++--
>  4 files changed, 35 insertions(+), 27 deletions(-)
>
> diff --git a/fs/nilfs2/inode.c b/fs/nilfs2/inode.c
> index 34e6096069ad..64437aed8390 100644
> --- a/fs/nilfs2/inode.c
> +++ b/fs/nilfs2/inode.c
> @@ -163,7 +163,7 @@ static int nilfs_writepages(struct address_space *mapping,
>         int err = 0;
>
>         if (sb_rdonly(inode->i_sb)) {
> -               nilfs_clear_dirty_pages(mapping);
> +               nilfs_clear_dirty_pages(mapping, false);
>                 return -EROFS;
>         }
>
> diff --git a/fs/nilfs2/mdt.c b/fs/nilfs2/mdt.c
> index 2a435349fd21..b50c88b65183 100644
> --- a/fs/nilfs2/mdt.c
> +++ b/fs/nilfs2/mdt.c
> @@ -405,7 +405,7 @@ static int nilfs_mdt_write_folio(struct folio *folio,
>                  * have dirty folios that try to be flushed in background.
>                  * So, here we simply discard this dirty folio.
>                  */
> -               nilfs_clear_folio_dirty(folio);
> +               nilfs_clear_folio_dirty(folio, false);
>                 folio_unlock(folio);
>                 return -EROFS;
>         }
> @@ -648,10 +648,10 @@ void nilfs_mdt_restore_from_shadow_map(struct inode *inode)
>         if (mi->mi_palloc_cache)
>                 nilfs_palloc_clear_cache(inode);
>
> -       nilfs_clear_dirty_pages(inode->i_mapping);
> +       nilfs_clear_dirty_pages(inode->i_mapping, true);
>         nilfs_copy_back_pages(inode->i_mapping, shadow->inode->i_mapping);
>
> -       nilfs_clear_dirty_pages(ii->i_assoc_inode->i_mapping);
> +       nilfs_clear_dirty_pages(ii->i_assoc_inode->i_mapping, true);
>         nilfs_copy_back_pages(ii->i_assoc_inode->i_mapping,
>                               NILFS_I(shadow->inode)->i_assoc_inode->i_mapping);
>
> diff --git a/fs/nilfs2/page.c b/fs/nilfs2/page.c
> index cf4f1c6798f5..857926da9f21 100644
> --- a/fs/nilfs2/page.c
> +++ b/fs/nilfs2/page.c
> @@ -369,8 +369,9 @@ void nilfs_copy_back_pages(struct address_space *dmap,
>  /**
>   * nilfs_clear_dirty_pages - discard dirty pages in address space
>   * @mapping: address space with dirty pages for discarding
> + * @force: whether to clear the dirty state regardless of busy buffer heads
>   */
> -void nilfs_clear_dirty_pages(struct address_space *mapping)
> +void nilfs_clear_dirty_pages(struct address_space *mapping, bool force)
>  {
>         struct folio_batch fbatch;
>         unsigned int i;
> @@ -391,7 +392,7 @@ void nilfs_clear_dirty_pages(struct address_space *mapping)
>                          * was acquired.  Skip processing in that case.
>                          */
>                         if (likely(folio->mapping == mapping))
> -                               nilfs_clear_folio_dirty(folio);
> +                               nilfs_clear_folio_dirty(folio, force);
>
>                         folio_unlock(folio);
>                 }
> @@ -403,13 +404,16 @@ void nilfs_clear_dirty_pages(struct address_space *mapping)
>  /**
>   * nilfs_clear_folio_dirty - discard dirty folio
>   * @folio: dirty folio that will be discarded
> + * @force: whether to clear the states regardless of busy buffer heads
>   *
>   * nilfs_clear_folio_dirty() clears working states including dirty state for
> - * the folio and its buffers.  If the folio has buffers, clear only if it is
> - * confirmed that none of the buffer heads are busy (none have valid
> - * references and none are locked).
> + * the folio and its buffers.  If the folio has buffers and force is false,
> + * clear only if it is confirmed that none of the buffer heads are busy (none
> + * have valid references and none are locked).  If force is true, the states
> + * are cleared unconditionally, the caller should guarantee that the folio is
> + * not being modified concurrently.
>   */
> -void nilfs_clear_folio_dirty(struct folio *folio)
> +void nilfs_clear_folio_dirty(struct folio *folio, bool force)
>  {
>         struct buffer_head *bh, *head;
>
> @@ -422,24 +426,28 @@ void nilfs_clear_folio_dirty(struct folio *folio)
>                          BIT(BH_Async_Write) | BIT(BH_NILFS_Volatile) |
>                          BIT(BH_NILFS_Checked) | BIT(BH_NILFS_Redirected) |
>                          BIT(BH_Delay));
> -               bool busy, invalidated = false;
> +
> +               if (!force) {
> +                       bool busy, invalidated = false;
>
>  recheck_buffers:
> -               busy = false;
> -               bh = head;
> -               do {
> -                       if (atomic_read(&bh->b_count) | buffer_locked(bh)) {
> -                               busy = true;
> -                               break;
> +                       busy = false;
> +                       bh = head;
> +                       do {
> +                               if (atomic_read(&bh->b_count) |
> +                                   buffer_locked(bh)) {
> +                                       busy = true;
> +                                       break;
> +                               }
> +                       } while (bh = bh->b_this_page, bh != head);
> +
> +                       if (busy) {
> +                               if (invalidated)
> +                                       return;
> +                               invalidate_bh_lrus();
> +                               invalidated = true;
> +                               goto recheck_buffers;
>                         }
> -               } while (bh = bh->b_this_page, bh != head);
> -
> -               if (busy) {
> -                       if (invalidated)
> -                               return;
> -                       invalidate_bh_lrus();
> -                       invalidated = true;
> -                       goto recheck_buffers;
>                 }
>
>                 bh = head;
> diff --git a/fs/nilfs2/page.h b/fs/nilfs2/page.h
> index 136cd1c143c9..c3ba3468af5c 100644
> --- a/fs/nilfs2/page.h
> +++ b/fs/nilfs2/page.h
> @@ -41,8 +41,8 @@ void nilfs_folio_bug(struct folio *);
>
>  int nilfs_copy_dirty_pages(struct address_space *, struct address_space *);
>  void nilfs_copy_back_pages(struct address_space *, struct address_space *);
> -void nilfs_clear_folio_dirty(struct folio *folio);
> -void nilfs_clear_dirty_pages(struct address_space *mapping);
> +void nilfs_clear_folio_dirty(struct folio *folio, bool force);
> +void nilfs_clear_dirty_pages(struct address_space *mapping, bool force);
>  unsigned int nilfs_page_count_clean_buffers(struct folio *folio,
>                 unsigned int from, unsigned int to);
>  unsigned long nilfs_find_uncommitted_extent(struct inode *inode,
> --
> 2.43.0
>

Friendly ping, please tell me if any further changes is needed.
Re: [PATCH v2] nilfs2: force clear dirty state when restoring from the shadow map
Posted by Viacheslav Dubeyko 1 week, 4 days ago
On Mon, 2026-09-14 at 14:33 +0800, Jiaming Zhang wrote:
> Jiaming Zhang <r772577952@gmail.com> 于2026年9月2日周三 19:21写道:
> > 
> > When garbage collection fails, nilfs2 rolls the DAT metadata file's
> > page
> > cache back to a shadow copy taken before GC started.
> > 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 and warns if one is still dirty:
> > 
> >         /* overwrite existing folio in the destination cache */
> >         WARN_ON(folio_test_dirty(dfolio));
> > 
> > nilfs_clear_dirty_pages() used to clear the dirty state
> > unconditionally,
> > which is safe here because the rollback runs with the log writer
> > stopped, so nothing else can dirty the cache while it runs.  The
> > same
> > helper is also used when writeback finds dirty folios after the
> > filesystem has degraded to read-only, where it does run
> > concurrently
> > with the log writer, so commit ca76bb226bf4 ("nilfs2: do not force
> > clear
> > folio if buffer is referenced") made nilfs_clear_folio_dirty() skip
> > a
> > folio if any of its buffer heads is busy.  The rollback caller
> > shares
> > that helper, so its clearing step can now return with a folio still
> > dirty.
> > 
> > A DAT folio can hold a busy buffer head without anyone modifying
> > the
> > folio: nilfs_mdt_read_block() submits read-ahead for the blocks
> > following the one it was asked for and waits only for the first, so
> > the
> > read-ahead buffers are still locked when it returns.  With a block
> > size
> > smaller than the page size, a locked read-ahead buffer can share a
> > folio
> > with a block that GC dirtied and keep the whole folio dirty past
> > the
> > clearing step.
> > 
> > Add a force flag to nilfs_clear_dirty_pages() and
> > nilfs_clear_folio_dirty() that skips the busy buffer check, and set
> > it
> > in the two calls from nilfs_mdt_restore_from_shadow_map().  The
> > read-only fallback callers keep passing false, so that commit still
> > protects them and the WARN_ON() is left alone.
> > 
> > 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
> > Suggested-by: Ryusuke Konishi <konishi.ryusuke@gmail.com>
> > Assisted-by: Claude Code:claude-opus-5
> > Signed-off-by: Jiaming Zhang <r772577952@gmail.com>
> > ---
> > Changes in v2:
> > - Keep the WARN_ON() in nilfs_copy_back_pages() untouched.
> > - Add a bool force argument to nilfs_clear_dirty_pages() and
> >   nilfs_clear_folio_dirty(), and set it to true in the two calls
> > from
> >   nilfs_mdt_restore_from_shadow_map().
> > 
> > v1:
> > https://lore.kernel.org/lkml/20260901134430.1292467-1-r772577952@gmail.com/
> > 
> >  fs/nilfs2/inode.c |  2 +-
> >  fs/nilfs2/mdt.c   |  6 +++---
> >  fs/nilfs2/page.c  | 50 +++++++++++++++++++++++++++----------------
> > ----
> >  fs/nilfs2/page.h  |  4 ++--
> >  4 files changed, 35 insertions(+), 27 deletions(-)
> > 
> > diff --git a/fs/nilfs2/inode.c b/fs/nilfs2/inode.c
> > index 34e6096069ad..64437aed8390 100644
> > --- a/fs/nilfs2/inode.c
> > +++ b/fs/nilfs2/inode.c
> > @@ -163,7 +163,7 @@ static int nilfs_writepages(struct
> > address_space *mapping,
> >         int err = 0;
> > 
> >         if (sb_rdonly(inode->i_sb)) {
> > -               nilfs_clear_dirty_pages(mapping);
> > +               nilfs_clear_dirty_pages(mapping, false);
> >                 return -EROFS;
> >         }
> > 
> > diff --git a/fs/nilfs2/mdt.c b/fs/nilfs2/mdt.c
> > index 2a435349fd21..b50c88b65183 100644
> > --- a/fs/nilfs2/mdt.c
> > +++ b/fs/nilfs2/mdt.c
> > @@ -405,7 +405,7 @@ static int nilfs_mdt_write_folio(struct folio
> > *folio,
> >                  * have dirty folios that try to be flushed in
> > background.
> >                  * So, here we simply discard this dirty folio.
> >                  */
> > -               nilfs_clear_folio_dirty(folio);
> > +               nilfs_clear_folio_dirty(folio, false);
> >                 folio_unlock(folio);
> >                 return -EROFS;
> >         }
> > @@ -648,10 +648,10 @@ void nilfs_mdt_restore_from_shadow_map(struct
> > inode *inode)
> >         if (mi->mi_palloc_cache)
> >                 nilfs_palloc_clear_cache(inode);
> > 
> > -       nilfs_clear_dirty_pages(inode->i_mapping);
> > +       nilfs_clear_dirty_pages(inode->i_mapping, true);
> >         nilfs_copy_back_pages(inode->i_mapping, shadow->inode-
> > >i_mapping);
> > 
> > -       nilfs_clear_dirty_pages(ii->i_assoc_inode->i_mapping);
> > +       nilfs_clear_dirty_pages(ii->i_assoc_inode->i_mapping,
> > true);
> >         nilfs_copy_back_pages(ii->i_assoc_inode->i_mapping,
> >                               NILFS_I(shadow->inode)-
> > >i_assoc_inode->i_mapping);
> > 
> > diff --git a/fs/nilfs2/page.c b/fs/nilfs2/page.c
> > index cf4f1c6798f5..857926da9f21 100644
> > --- a/fs/nilfs2/page.c
> > +++ b/fs/nilfs2/page.c
> > @@ -369,8 +369,9 @@ void nilfs_copy_back_pages(struct address_space
> > *dmap,
> >  /**
> >   * nilfs_clear_dirty_pages - discard dirty pages in address space
> >   * @mapping: address space with dirty pages for discarding
> > + * @force: whether to clear the dirty state regardless of busy
> > buffer heads
> >   */
> > -void nilfs_clear_dirty_pages(struct address_space *mapping)
> > +void nilfs_clear_dirty_pages(struct address_space *mapping, bool
> > force)
> >  {
> >         struct folio_batch fbatch;
> >         unsigned int i;
> > @@ -391,7 +392,7 @@ void nilfs_clear_dirty_pages(struct
> > address_space *mapping)
> >                          * was acquired.  Skip processing in that
> > case.
> >                          */
> >                         if (likely(folio->mapping == mapping))
> > -                               nilfs_clear_folio_dirty(folio);
> > +                               nilfs_clear_folio_dirty(folio,
> > force);
> > 
> >                         folio_unlock(folio);
> >                 }
> > @@ -403,13 +404,16 @@ void nilfs_clear_dirty_pages(struct
> > address_space *mapping)
> >  /**
> >   * nilfs_clear_folio_dirty - discard dirty folio
> >   * @folio: dirty folio that will be discarded
> > + * @force: whether to clear the states regardless of busy buffer
> > heads
> >   *
> >   * nilfs_clear_folio_dirty() clears working states including dirty
> > state for
> > - * the folio and its buffers.  If the folio has buffers, clear
> > only if it is
> > - * confirmed that none of the buffer heads are busy (none have
> > valid
> > - * references and none are locked).
> > + * the folio and its buffers.  If the folio has buffers and force
> > is false,
> > + * clear only if it is confirmed that none of the buffer heads are
> > busy (none
> > + * have valid references and none are locked).  If force is true,
> > the states
> > + * are cleared unconditionally, the caller should guarantee that
> > the folio is
> > + * not being modified concurrently.
> >   */
> > -void nilfs_clear_folio_dirty(struct folio *folio)
> > +void nilfs_clear_folio_dirty(struct folio *folio, bool force)
> >  {
> >         struct buffer_head *bh, *head;
> > 
> > @@ -422,24 +426,28 @@ void nilfs_clear_folio_dirty(struct folio
> > *folio)
> >                          BIT(BH_Async_Write) |
> > BIT(BH_NILFS_Volatile) |
> >                          BIT(BH_NILFS_Checked) |
> > BIT(BH_NILFS_Redirected) |
> >                          BIT(BH_Delay));
> > -               bool busy, invalidated = false;
> > +
> > +               if (!force) {
> > +                       bool busy, invalidated = false;
> > 
> >  recheck_buffers:
> > -               busy = false;
> > -               bh = head;
> > -               do {
> > -                       if (atomic_read(&bh->b_count) |
> > buffer_locked(bh)) {
> > -                               busy = true;
> > -                               break;
> > +                       busy = false;
> > +                       bh = head;
> > +                       do {
> > +                               if (atomic_read(&bh->b_count) |
> > +                                   buffer_locked(bh)) {
> > +                                       busy = true;
> > +                                       break;
> > +                               }
> > +                       } while (bh = bh->b_this_page, bh != head);
> > +
> > +                       if (busy) {
> > +                               if (invalidated)
> > +                                       return;
> > +                               invalidate_bh_lrus();
> > +                               invalidated = true;
> > +                               goto recheck_buffers;
> >                         }
> > -               } while (bh = bh->b_this_page, bh != head);
> > -
> > -               if (busy) {
> > -                       if (invalidated)
> > -                               return;
> > -                       invalidate_bh_lrus();
> > -                       invalidated = true;
> > -                       goto recheck_buffers;
> >                 }
> > 
> >                 bh = head;
> > diff --git a/fs/nilfs2/page.h b/fs/nilfs2/page.h
> > index 136cd1c143c9..c3ba3468af5c 100644
> > --- a/fs/nilfs2/page.h
> > +++ b/fs/nilfs2/page.h
> > @@ -41,8 +41,8 @@ void nilfs_folio_bug(struct folio *);
> > 
> >  int nilfs_copy_dirty_pages(struct address_space *, struct
> > address_space *);
> >  void nilfs_copy_back_pages(struct address_space *, struct
> > address_space *);
> > -void nilfs_clear_folio_dirty(struct folio *folio);
> > -void nilfs_clear_dirty_pages(struct address_space *mapping);
> > +void nilfs_clear_folio_dirty(struct folio *folio, bool force);
> > +void nilfs_clear_dirty_pages(struct address_space *mapping, bool
> > force);
> >  unsigned int nilfs_page_count_clean_buffers(struct folio *folio,
> >                 unsigned int from, unsigned int to);
> >  unsigned long nilfs_find_uncommitted_extent(struct inode *inode,
> > --
> > 2.43.0
> > 
> 
> Friendly ping, please tell me if any further changes is needed.

As far as I know, Ryusuke is traveling right now. Please, expect some
delay with the review. I don't see any issues with the patch.

Thanks,
Slava.
Re: [PATCH v2] nilfs2: force clear dirty state when restoring from the shadow map
Posted by Ryusuke Konishi 1 week, 2 days ago
On Tue, Sep 15, 2026 at 6:12 AM Viacheslav Dubeyko wrote:
>
> On Mon, 2026-09-14 at 14:33 +0800, Jiaming Zhang wrote:
> > Jiaming Zhang <r772577952@gmail.com> 于2026年9月2日周三 19:21写道:
> > >
> > > When garbage collection fails, nilfs2 rolls the DAT metadata file's
> > > page
> > > cache back to a shadow copy taken before GC started.
> > > 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 and warns if one is still dirty:
> > >
> > >         /* overwrite existing folio in the destination cache */
> > >         WARN_ON(folio_test_dirty(dfolio));
> > >
> > > nilfs_clear_dirty_pages() used to clear the dirty state
> > > unconditionally,
> > > which is safe here because the rollback runs with the log writer
> > > stopped, so nothing else can dirty the cache while it runs.  The
> > > same
> > > helper is also used when writeback finds dirty folios after the
> > > filesystem has degraded to read-only, where it does run
> > > concurrently
> > > with the log writer, so commit ca76bb226bf4 ("nilfs2: do not force
> > > clear
> > > folio if buffer is referenced") made nilfs_clear_folio_dirty() skip
> > > a
> > > folio if any of its buffer heads is busy.  The rollback caller
> > > shares
> > > that helper, so its clearing step can now return with a folio still
> > > dirty.
> > >
> > > A DAT folio can hold a busy buffer head without anyone modifying
> > > the
> > > folio: nilfs_mdt_read_block() submits read-ahead for the blocks
> > > following the one it was asked for and waits only for the first, so
> > > the
> > > read-ahead buffers are still locked when it returns.  With a block
> > > size
> > > smaller than the page size, a locked read-ahead buffer can share a
> > > folio
> > > with a block that GC dirtied and keep the whole folio dirty past
> > > the
> > > clearing step.
> > >
> > > Add a force flag to nilfs_clear_dirty_pages() and
> > > nilfs_clear_folio_dirty() that skips the busy buffer check, and set
> > > it
> > > in the two calls from nilfs_mdt_restore_from_shadow_map().  The
> > > read-only fallback callers keep passing false, so that commit still
> > > protects them and the WARN_ON() is left alone.
> > >
> > > 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
> > > Suggested-by: Ryusuke Konishi <konishi.ryusuke@gmail.com>
> > > Assisted-by: Claude Code:claude-opus-5
> > > Signed-off-by: Jiaming Zhang <r772577952@gmail.com>
> > > ---
> > > Changes in v2:
> > > - Keep the WARN_ON() in nilfs_copy_back_pages() untouched.
> > > - Add a bool force argument to nilfs_clear_dirty_pages() and
> > >   nilfs_clear_folio_dirty(), and set it to true in the two calls
> > > from
> > >   nilfs_mdt_restore_from_shadow_map().
> > >
> > > v1:
> > > https://lore.kernel.org/lkml/20260901134430.1292467-1-r772577952@gmail.com/
> > >
> > >  fs/nilfs2/inode.c |  2 +-
> > >  fs/nilfs2/mdt.c   |  6 +++---
> > >  fs/nilfs2/page.c  | 50 +++++++++++++++++++++++++++----------------
> > > ----
> > >  fs/nilfs2/page.h  |  4 ++--
> > >  4 files changed, 35 insertions(+), 27 deletions(-)
> > >
> > > diff --git a/fs/nilfs2/inode.c b/fs/nilfs2/inode.c
> > > index 34e6096069ad..64437aed8390 100644
> > > --- a/fs/nilfs2/inode.c
> > > +++ b/fs/nilfs2/inode.c
> > > @@ -163,7 +163,7 @@ static int nilfs_writepages(struct
> > > address_space *mapping,
> > >         int err = 0;
> > >
> > >         if (sb_rdonly(inode->i_sb)) {
> > > -               nilfs_clear_dirty_pages(mapping);
> > > +               nilfs_clear_dirty_pages(mapping, false);
> > >                 return -EROFS;
> > >         }
> > >
> > > diff --git a/fs/nilfs2/mdt.c b/fs/nilfs2/mdt.c
> > > index 2a435349fd21..b50c88b65183 100644
> > > --- a/fs/nilfs2/mdt.c
> > > +++ b/fs/nilfs2/mdt.c
> > > @@ -405,7 +405,7 @@ static int nilfs_mdt_write_folio(struct folio
> > > *folio,
> > >                  * have dirty folios that try to be flushed in
> > > background.
> > >                  * So, here we simply discard this dirty folio.
> > >                  */
> > > -               nilfs_clear_folio_dirty(folio);
> > > +               nilfs_clear_folio_dirty(folio, false);
> > >                 folio_unlock(folio);
> > >                 return -EROFS;
> > >         }
> > > @@ -648,10 +648,10 @@ void nilfs_mdt_restore_from_shadow_map(struct
> > > inode *inode)
> > >         if (mi->mi_palloc_cache)
> > >                 nilfs_palloc_clear_cache(inode);
> > >
> > > -       nilfs_clear_dirty_pages(inode->i_mapping);
> > > +       nilfs_clear_dirty_pages(inode->i_mapping, true);
> > >         nilfs_copy_back_pages(inode->i_mapping, shadow->inode-
> > > >i_mapping);
> > >
> > > -       nilfs_clear_dirty_pages(ii->i_assoc_inode->i_mapping);
> > > +       nilfs_clear_dirty_pages(ii->i_assoc_inode->i_mapping,
> > > true);
> > >         nilfs_copy_back_pages(ii->i_assoc_inode->i_mapping,
> > >                               NILFS_I(shadow->inode)-
> > > >i_assoc_inode->i_mapping);
> > >
> > > diff --git a/fs/nilfs2/page.c b/fs/nilfs2/page.c
> > > index cf4f1c6798f5..857926da9f21 100644
> > > --- a/fs/nilfs2/page.c
> > > +++ b/fs/nilfs2/page.c
> > > @@ -369,8 +369,9 @@ void nilfs_copy_back_pages(struct address_space
> > > *dmap,
> > >  /**
> > >   * nilfs_clear_dirty_pages - discard dirty pages in address space
> > >   * @mapping: address space with dirty pages for discarding
> > > + * @force: whether to clear the dirty state regardless of busy
> > > buffer heads
> > >   */
> > > -void nilfs_clear_dirty_pages(struct address_space *mapping)
> > > +void nilfs_clear_dirty_pages(struct address_space *mapping, bool
> > > force)
> > >  {
> > >         struct folio_batch fbatch;
> > >         unsigned int i;
> > > @@ -391,7 +392,7 @@ void nilfs_clear_dirty_pages(struct
> > > address_space *mapping)
> > >                          * was acquired.  Skip processing in that
> > > case.
> > >                          */
> > >                         if (likely(folio->mapping == mapping))
> > > -                               nilfs_clear_folio_dirty(folio);
> > > +                               nilfs_clear_folio_dirty(folio,
> > > force);
> > >
> > >                         folio_unlock(folio);
> > >                 }
> > > @@ -403,13 +404,16 @@ void nilfs_clear_dirty_pages(struct
> > > address_space *mapping)
> > >  /**
> > >   * nilfs_clear_folio_dirty - discard dirty folio
> > >   * @folio: dirty folio that will be discarded
> > > + * @force: whether to clear the states regardless of busy buffer
> > > heads
> > >   *
> > >   * nilfs_clear_folio_dirty() clears working states including dirty
> > > state for
> > > - * the folio and its buffers.  If the folio has buffers, clear
> > > only if it is
> > > - * confirmed that none of the buffer heads are busy (none have
> > > valid
> > > - * references and none are locked).
> > > + * the folio and its buffers.  If the folio has buffers and force
> > > is false,
> > > + * clear only if it is confirmed that none of the buffer heads are
> > > busy (none
> > > + * have valid references and none are locked).  If force is true,
> > > the states
> > > + * are cleared unconditionally, the caller should guarantee that
> > > the folio is
> > > + * not being modified concurrently.
> > >   */
> > > -void nilfs_clear_folio_dirty(struct folio *folio)
> > > +void nilfs_clear_folio_dirty(struct folio *folio, bool force)
> > >  {
> > >         struct buffer_head *bh, *head;
> > >
> > > @@ -422,24 +426,28 @@ void nilfs_clear_folio_dirty(struct folio
> > > *folio)
> > >                          BIT(BH_Async_Write) |
> > > BIT(BH_NILFS_Volatile) |
> > >                          BIT(BH_NILFS_Checked) |
> > > BIT(BH_NILFS_Redirected) |
> > >                          BIT(BH_Delay));
> > > -               bool busy, invalidated = false;
> > > +
> > > +               if (!force) {
> > > +                       bool busy, invalidated = false;
> > >
> > >  recheck_buffers:
> > > -               busy = false;
> > > -               bh = head;
> > > -               do {
> > > -                       if (atomic_read(&bh->b_count) |
> > > buffer_locked(bh)) {
> > > -                               busy = true;
> > > -                               break;
> > > +                       busy = false;
> > > +                       bh = head;
> > > +                       do {
> > > +                               if (atomic_read(&bh->b_count) |
> > > +                                   buffer_locked(bh)) {
> > > +                                       busy = true;
> > > +                                       break;
> > > +                               }
> > > +                       } while (bh = bh->b_this_page, bh != head);
> > > +
> > > +                       if (busy) {
> > > +                               if (invalidated)
> > > +                                       return;
> > > +                               invalidate_bh_lrus();
> > > +                               invalidated = true;
> > > +                               goto recheck_buffers;
> > >                         }
> > > -               } while (bh = bh->b_this_page, bh != head);
> > > -
> > > -               if (busy) {
> > > -                       if (invalidated)
> > > -                               return;
> > > -                       invalidate_bh_lrus();
> > > -                       invalidated = true;
> > > -                       goto recheck_buffers;
> > >                 }
> > >
> > >                 bh = head;
> > > diff --git a/fs/nilfs2/page.h b/fs/nilfs2/page.h
> > > index 136cd1c143c9..c3ba3468af5c 100644
> > > --- a/fs/nilfs2/page.h
> > > +++ b/fs/nilfs2/page.h
> > > @@ -41,8 +41,8 @@ void nilfs_folio_bug(struct folio *);
> > >
> > >  int nilfs_copy_dirty_pages(struct address_space *, struct
> > > address_space *);
> > >  void nilfs_copy_back_pages(struct address_space *, struct
> > > address_space *);
> > > -void nilfs_clear_folio_dirty(struct folio *folio);
> > > -void nilfs_clear_dirty_pages(struct address_space *mapping);
> > > +void nilfs_clear_folio_dirty(struct folio *folio, bool force);
> > > +void nilfs_clear_dirty_pages(struct address_space *mapping, bool
> > > force);
> > >  unsigned int nilfs_page_count_clean_buffers(struct folio *folio,
> > >                 unsigned int from, unsigned int to);
> > >  unsigned long nilfs_find_uncommitted_extent(struct inode *inode,
> > > --
> > > 2.43.0
> > >
> >
> > Friendly ping, please tell me if any further changes is needed.
>
> As far as I know, Ryusuke is traveling right now. Please, expect some
> delay with the review. I don't see any issues with the patch.
>
> Thanks,
> Slava.

Jiaming, my apologies.

For some reason, recent emails addressed to me weren't showing up in
my inbox. (I just realized the v2 patch had been posted.)

I will review it, so please bear with me for a moment.

Thanks,
Ryusuke Konishi