Re: [PATCH] btrfs: change the set_page_extent_mapped() call into an ASSERT()

[Qu Wenruo <wqu@suse.com>]

On 2021/7/27 上午9:39, Qu Wenruo wrote:
> Btrfs uses set_page_extent_mapped() to properly setup a page.
> 
> That function would set PagePrivate, and populate needed structure for
> subpage.
> The timing of calling set_page_extent_mapped() happens before reading a
> page or dirtying a page.
> Thus when we got a page to write back, if it doesn't have PagePrivate,
> it is a big problem in code logic.
> 
> Calling set_page_extent_mapped() for such page would just mask the
> problem.
> Furthermore, for subpage case, we call subpage error helper to clear the
> page error bit before calling set_page_extent_mapped().
> If we really got a page without Private bit, it can call kernel NULL
> pointer dereference.
> 
> So change the set_page_extent_mapped() call to an ASSERT(), and move the
> check before any page status update call.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Please discard the patch.

Although I haven't hit any problem testing the patch, it's still 
possible that we have a special page that would need cow fixup.

Such page will be:

- Dirty

- Not Private
   Thus no page->private, this could still cause problem for subpage case
   though

- No EXTENT_DELALLOC flags set for any range inside the page
   Thus writepage_delalloc() will not find a delalloc range inside the
   page.

Such page will be caught by btrfs_writepage_cow_fixup(), but it will 
trigger the ASSERT() added by this patch.

Thanks,
Qu

> ---
>   fs/btrfs/extent_io.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 62f0ed2de2b9..219add264acf 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -4099,6 +4099,12 @@ static int __extent_writepage(struct page *page, struct writeback_control *wbc,
>   
>   	WARN_ON(!PageLocked(page));
>   
> +	/*
> +	 * All dirty page to be written should have PagePrivate set by
> +	 * set_page_extent_mapped() when creating the page.
> +	 */
> +	ASSERT(PagePrivate(page));
> +
>   	btrfs_page_clear_error(btrfs_sb(inode->i_sb), page,
>   			       page_offset(page), PAGE_SIZE);
>   
> @@ -4115,12 +4121,6 @@ static int __extent_writepage(struct page *page, struct writeback_control *wbc,
>   		flush_dcache_page(page);
>   	}
>   
> -	ret = set_page_extent_mapped(page);
> -	if (ret < 0) {
> -		SetPageError(page);
> -		goto done;
> -	}
> -
>   	if (!epd->extent_locked) {
>   		ret = writepage_delalloc(BTRFS_I(inode), page, wbc,
>   					 &nr_written);
>