[PATCH v2] btrfs: remove out-of-date comments in btree_writepages()

[PATCH v2] btrfs: remove out-of-date comments in btree_writepages()

[Qu Wenruo]

There is a lengthy comment introduced in commit b3ff8f1d380e ("btrfs:
Don't submit any btree write bio if the fs has errors") and commit
c9583ada8cc4 ("btrfs: avoid double clean up when submit_one_bio()
failed"), explaining two things:

- Why we don't want to submit metadata write if the fs has errors

- Why we re-set @ret to 0 if it's positive

However it's no longer uptodate by the following reasons:

- We have better checks nowadays

  Commit 2618849f31e7 ("btrfs: ensure no dirty metadata is written back
  for an fs with errors") has introduced better checks, that if the
  fs is in an error state, metadata writes will not result in any bio
  but instead complete immediately.

  That covers all metadata writes better.

- Mentioned incorrect function name

  The commit c9583ada8cc4 ("btrfs: avoid double clean up when
  submit_one_bio() failed") introduced this ret > 0 handling, but at that
  time the function name submit_extent_page() is already incorrect.

  It was submit_eb_page() that could return >0 at that time,
  and submit_extent_page() could only return 0 or <0 for errors, never >0.

  Later commit b35397d1d325 ("btrfs: convert submit_extent_page() to use
  a folio") changed "submit_extent_page()" to "submit_extent_folio()" in
  the comment, but it doesn't make any difference since the function name
  is wrong from day 1.

  Finally commit 5e121ae687b8 ("btrfs: use buffer xarray for extent
  buffer writeback operations") completely reworked how metadata
  writeback works, and removed submit_eb_page(), leaving only the wrong
  function name in the comment.

  Furthermore the function submit_extent_folio() still exists in the
  latest code base, but is never utilized for metadata writeback, causing
  more confusion.

Just remove the lengthy comment, and replace the "if (ret > 0)" check
with an ASSERT(), since only btrfs_check_meta_write_pointer() can modify
@ret and it returns 0 or <0 for errors.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
Changelog:
v2:
- Fix grammar errors
- Add a more comprehensive history for the bad "submit_extent_folio()"
  mention
  It's incorrect from day 1.
---
 fs/btrfs/extent_io.c | 34 ++++------------------------------
 1 file changed, 4 insertions(+), 30 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 3df399dc8856..9999c3f4afa4 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -2386,38 +2386,12 @@ int btree_writepages(struct address_space *mapping, struct writeback_control *wb
 		index = 0;
 		goto retry;
 	}
+
 	/*
-	 * If something went wrong, don't allow any metadata write bio to be
-	 * submitted.
-	 *
-	 * This would prevent use-after-free if we had dirty pages not
-	 * cleaned up, which can still happen by fuzzed images.
-	 *
-	 * - Bad extent tree
-	 *   Allowing existing tree block to be allocated for other trees.
-	 *
-	 * - Log tree operations
-	 *   Exiting tree blocks get allocated to log tree, bumps its
-	 *   generation, then get cleaned in tree re-balance.
-	 *   Such tree block will not be written back, since it's clean,
-	 *   thus no WRITTEN flag set.
-	 *   And after log writes back, this tree block is not traced by
-	 *   any dirty extent_io_tree.
-	 *
-	 * - Offending tree block gets re-dirtied from its original owner
-	 *   Since it has bumped generation, no WRITTEN flag, it can be
-	 *   reused without COWing. This tree block will not be traced
-	 *   by btrfs_transaction::dirty_pages.
-	 *
-	 *   Now such dirty tree block will not be cleaned by any dirty
-	 *   extent io tree. Thus we don't want to submit such wild eb
-	 *   if the fs already has error.
-	 *
-	 * We can get ret > 0 from submit_extent_folio() indicating how many ebs
-	 * were submitted. Reset it to 0 to avoid false alerts for the caller.
+	 * Only btrfs_check_meta_write_pointer() can update @ret,
+	 * and it only returns 0 or errors.
 	 */
-	if (ret > 0)
-		ret = 0;
+	ASSERT(ret <= 0);
 	if (!ret && BTRFS_FS_ERROR(fs_info))
 		ret = -EROFS;
 
-- 
2.52.0

Re: [PATCH v2] btrfs: remove out-of-date comments in btree_writepages()

[Filipe Manana]

On Wed, Feb 11, 2026 at 9:14 PM Qu Wenruo <wqu@suse.com> wrote:
>
> There is a lengthy comment introduced in commit b3ff8f1d380e ("btrfs:
> Don't submit any btree write bio if the fs has errors") and commit
> c9583ada8cc4 ("btrfs: avoid double clean up when submit_one_bio()
> failed"), explaining two things:
>
> - Why we don't want to submit metadata write if the fs has errors
>
> - Why we re-set @ret to 0 if it's positive
>
> However it's no longer uptodate by the following reasons:
>
> - We have better checks nowadays
>
>   Commit 2618849f31e7 ("btrfs: ensure no dirty metadata is written back
>   for an fs with errors") has introduced better checks, that if the
>   fs is in an error state, metadata writes will not result in any bio
>   but instead complete immediately.
>
>   That covers all metadata writes better.
>
> - Mentioned incorrect function name
>
>   The commit c9583ada8cc4 ("btrfs: avoid double clean up when
>   submit_one_bio() failed") introduced this ret > 0 handling, but at that
>   time the function name submit_extent_page() is already incorrect.

is already -> was already

Otherwise,

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Thanks.

>
>   It was submit_eb_page() that could return >0 at that time,
>   and submit_extent_page() could only return 0 or <0 for errors, never >0.
>
>   Later commit b35397d1d325 ("btrfs: convert submit_extent_page() to use
>   a folio") changed "submit_extent_page()" to "submit_extent_folio()" in
>   the comment, but it doesn't make any difference since the function name
>   is wrong from day 1.
>
>   Finally commit 5e121ae687b8 ("btrfs: use buffer xarray for extent
>   buffer writeback operations") completely reworked how metadata
>   writeback works, and removed submit_eb_page(), leaving only the wrong
>   function name in the comment.
>
>   Furthermore the function submit_extent_folio() still exists in the
>   latest code base, but is never utilized for metadata writeback, causing
>   more confusion.
>
> Just remove the lengthy comment, and replace the "if (ret > 0)" check
> with an ASSERT(), since only btrfs_check_meta_write_pointer() can modify
> @ret and it returns 0 or <0 for errors.
>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
> Changelog:
> v2:
> - Fix grammar errors
> - Add a more comprehensive history for the bad "submit_extent_folio()"
>   mention
>   It's incorrect from day 1.
> ---
>  fs/btrfs/extent_io.c | 34 ++++------------------------------
>  1 file changed, 4 insertions(+), 30 deletions(-)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 3df399dc8856..9999c3f4afa4 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -2386,38 +2386,12 @@ int btree_writepages(struct address_space *mapping, struct writeback_control *wb
>                 index = 0;
>                 goto retry;
>         }
> +
>         /*
> -        * If something went wrong, don't allow any metadata write bio to be
> -        * submitted.
> -        *
> -        * This would prevent use-after-free if we had dirty pages not
> -        * cleaned up, which can still happen by fuzzed images.
> -        *
> -        * - Bad extent tree
> -        *   Allowing existing tree block to be allocated for other trees.
> -        *
> -        * - Log tree operations
> -        *   Exiting tree blocks get allocated to log tree, bumps its
> -        *   generation, then get cleaned in tree re-balance.
> -        *   Such tree block will not be written back, since it's clean,
> -        *   thus no WRITTEN flag set.
> -        *   And after log writes back, this tree block is not traced by
> -        *   any dirty extent_io_tree.
> -        *
> -        * - Offending tree block gets re-dirtied from its original owner
> -        *   Since it has bumped generation, no WRITTEN flag, it can be
> -        *   reused without COWing. This tree block will not be traced
> -        *   by btrfs_transaction::dirty_pages.
> -        *
> -        *   Now such dirty tree block will not be cleaned by any dirty
> -        *   extent io tree. Thus we don't want to submit such wild eb
> -        *   if the fs already has error.
> -        *
> -        * We can get ret > 0 from submit_extent_folio() indicating how many ebs
> -        * were submitted. Reset it to 0 to avoid false alerts for the caller.
> +        * Only btrfs_check_meta_write_pointer() can update @ret,
> +        * and it only returns 0 or errors.
>          */
> -       if (ret > 0)
> -               ret = 0;
> +       ASSERT(ret <= 0);
>         if (!ret && BTRFS_FS_ERROR(fs_info))
>                 ret = -EROFS;
>
> --
> 2.52.0
>
>