Re: [PATCH 1/2] btrfs: clear block dirty if submit_one_sector() failed

[Leo Martins <loemra.dev@gmail.com>]

On Tue, 29 Jul 2025 19:01:45 +0930 Qu Wenruo <wqu@suse.com> wrote:

> [BUG]
> If submit_one_sector() failed, the block will be kept dirty, but with
> their corresponding range finished in the ordered extent.
> 
> This means if later a writeback happens again, we can hit the following
> problems:
> 
> - ASSERT(block_start != EXTENT_MAP_HOLE) in submit_one_sector()
>   If the original extent map is a hole, then we can hit this case, as
>   the new ordered extent failed, we will drop the new extent map and
>   re-read one from the disk.
> 
> - DEBUG_WARN() in btrfs_writepage_cow_fixup()
>   This is because we no longer have an ordered extent for those dirty
>   blocks. The original for them is already finished with error.
> 
> [CAUSE]
> The function submit_one_sector() is not following the regular error
> handling of writeback.
> The common practice is to clear the folio dirty, start and finish the
> writeback for the block.
> 
> This is normally done by extent_clear_unlock_delalloc() with
> PAGE_START_WRITEBACK | PAGE_END_WRITEBACK flags during
> run_delalloc_range().
> 
> So if we keep those failed blocks dirty, they will stay in the page
> cache and wait for the next writeback.
> 
> And since the original ordered extent is already finished and removed,
> depending on the original extent map, we either hit the ASSERT() inside
> submit_one_sector(), or hit the DEBUG_WARN() in
> btrfs_writepage_cow_fixup().
> 
> [FIX]
> Follow the regular error handling to clear the dirty flag for the block,
> start and finish writeback for that block instead.

Might be a dumb question, but if the page failed to finish writeback why
are we allowed to clear the dirty flag? Wouldn't the page still have dirty
data?

> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/extent_io.c | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index c11c93bcc7f6..f6765ddab4a7 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -1548,7 +1548,7 @@ static noinline_for_stack int writepage_delalloc(struct btrfs_inode *inode,
>  
>  /*
>   * Return 0 if we have submitted or queued the sector for submission.
> - * Return <0 for critical errors.
> + * Return <0 for critical errors, and the sector will have its dirty flag cleared.
>   *
>   * Caller should make sure filepos < i_size and handle filepos >= i_size case.
>   */
> @@ -1571,8 +1571,19 @@ static int submit_one_sector(struct btrfs_inode *inode,
>  	ASSERT(filepos < i_size);
>  
>  	em = btrfs_get_extent(inode, NULL, filepos, sectorsize);
> -	if (IS_ERR(em))
> -		return PTR_ERR(em);
> +	if (IS_ERR(em)) {
> +		int ret = PTR_ERR(em);
> +
> +		/*
> +		 * When submission failed, we should still clear the folio dirty.
> +		 * Or the folio will be written back again but without any
> +		 * ordered extent.
> +		 */
> +		btrfs_folio_clear_dirty(fs_info, folio, filepos, sectorsize);
> +		btrfs_folio_set_writeback(fs_info, folio, filepos, sectorsize);
> +		btrfs_folio_clear_writeback(fs_info, folio, filepos, sectorsize);
> +		return ret;
> +	}
>  
>  	extent_offset = filepos - em->start;
>  	em_end = btrfs_extent_map_end(em);
> @@ -1702,8 +1713,9 @@ static noinline_for_stack int extent_writepage_io(struct btrfs_inode *inode,
>  	 * Here we set writeback and clear for the range. If the full folio
>  	 * is no longer dirty then we clear the PAGECACHE_TAG_DIRTY tag.
>  	 *
> -	 * If we hit any error, the corresponding sector will still be dirty
> -	 * thus no need to clear PAGECACHE_TAG_DIRTY.
> +	 * If we hit any error, the corresponding sector will still have its
> +	 * dirty flag cleared and finish writeback just like below, thus
> +	 * no need to handle error case either.
>  	 */
>  	if (!submitted_io && !error) {
>  		btrfs_folio_set_writeback(fs_info, folio, start, len);
> -- 
> 2.50.1

Sent using hkml (https://github.com/sjp38/hackermail)