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

[PATCH] 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"), explaining why
we don't want to submit the metadata write bio when the fs has something
wrong.

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, all metadata write will not result any bio
  but finished immediately.

  That covers all metadata writes better.

- Mentioning functions no longer there
  It mentions the function submit_extent_folio(), but the correct
  function is submit_eb_subpage(), which can return the number of ebs
  submitted for a page.

  And later commit 5e121ae687b8 ("btrfs: use buffer xarray for extent
  buffer writeback operations") completely reworks this, and now we
  always call write_one_eb() to submit an extent buffer, which has no
  return value.

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 errors.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 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] btrfs: remove out-of-date comments in btree_writepages()

[Filipe Manana]

On Wed, Feb 11, 2026 at 3:13 AM 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"), explaining why
> we don't want to submit the metadata write bio when the fs has something
> wrong.
>
> 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, all metadata write will not result any bio

"all metadata write" -> metadata writes
"will not result any bio" -> will not result in any bio

>   but finished immediately.

but instead complete immediately.


>
>   That covers all metadata writes better.
>
> - Mentioning functions no longer there

mentioning -> mentioned

But which functions? The only function I see mentioned in the comment
is  submit_extent_folio(), which still exists.

>   It mentions the function submit_extent_folio(), but the correct
>   function is submit_eb_subpage(), which can return the number of ebs

I don't see any function named submit_eb_subpage() in current for-next.

Thanks.

>   submitted for a page.
>
>   And later commit 5e121ae687b8 ("btrfs: use buffer xarray for extent
>   buffer writeback operations") completely reworks this, and now we
>   always call write_one_eb() to submit an extent buffer, which has no
>   return value.
>
> 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 errors.
>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  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] btrfs: remove out-of-date comments in btree_writepages()

[Qu Wenruo]

在 2026/2/11 23:17, Filipe Manana 写道:
> On Wed, Feb 11, 2026 at 3:13 AM 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"), explaining why
>> we don't want to submit the metadata write bio when the fs has something
>> wrong.
>>
>> 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, all metadata write will not result any bio
> 
> "all metadata write" -> metadata writes
> "will not result any bio" -> will not result in any bio
> 
>>    but finished immediately.
> 
> but instead complete immediately.
> 
> 
>>
>>    That covers all metadata writes better.
>>
>> - Mentioning functions no longer there
> 
> mentioning -> mentioned
> 
> But which functions? The only function I see mentioned in the comment
> is  submit_extent_folio(), which still exists.

Because it's wrong from the very beginning.

The original commit is introduced in commit c9583ada8cc4 ("btrfs: avoid 
double clean up when submit_one_bio() failed").

At that commit, the function that can return >0 to indicate how many ebs 
are submitted is submit_eb_page(), not submit_extent_page().

Function submit_extent_page() at that time can only return error from 
alloc_new_bio() or 0.

So in that commit, the function name is already incorrect.

Later commit b35397d1d325 ("btrfs: convert submit_extent_page() to use a 
folio") changed the submit_extent_page() to submit_extent_folio(), but 
it doesn't matter anyway.

> 
>>    It mentions the function submit_extent_folio(), but the correct
>>    function is submit_eb_subpage(), which can return the number of ebs
> 
> I don't see any function named submit_eb_subpage() in current for-next.

Because it's removed by commit 5e121ae687b8 ("btrfs: use buffer xarray 
for extent buffer writeback operations"), as I explained in the commit 
message.

Thanks,
Qu

> 
> Thanks.
> 
>>    submitted for a page.
>>
>>    And later commit 5e121ae687b8 ("btrfs: use buffer xarray for extent
>>    buffer writeback operations") completely reworks this, and now we
>>    always call write_one_eb() to submit an extent buffer, which has no
>>    return value.
>>
>> 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 errors.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   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
>>
>>
>