Re: [PATCH] btrfs: refactor alloc_extent_buffer() to allocate-then-attach method

[Josef Bacik <josef@toxicpanda.com>]

On Wed, Nov 22, 2023 at 10:05:04AM +1030, Qu Wenruo wrote:
> Currently alloc_extent_buffer() utilizes find_or_create_page() to
> allocate one page a time for an extent buffer.
> 
> This method has the following disadvantages:
> 
> - find_or_create_page() is the legacy way of allocating new pages
>   With the new folio infrastructure, find_or_create_page() is just
>   redirected to filemap_get_folio().
> 
> - Lacks the way to support higher order (order >= 1) folios
>   As we can not yet let filemap to give us a higher order folio (yet).
> 
> This patch would change the workflow by the following way:
> 
> 		Old		   |		new
> -----------------------------------+-------------------------------------
>                                    | ret = btrfs_alloc_page_array();
> for (i = 0; i < num_pages; i++) {  | for (i = 0; i < num_pages; i++) {
>     p = find_or_create_page();     |     ret = filemap_add_folio();
>     /* Attach page private */      |     /* Reuse page cache if needed */
>     /* Reused eb if needed */      |
> 				   |     /* Attach page private and
> 				   |        reuse eb if needed */
> 				   | }
> 
> By this we split the page allocation and private attaching into two
> parts, allowing future updates to each part more easily, and migrate to
> folio interfaces (especially for possible higher order folios).
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/extent_io.c | 173 +++++++++++++++++++++++++++++++------------
>  1 file changed, 126 insertions(+), 47 deletions(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 99cc16aed9d7..0ea65f248c15 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -3084,6 +3084,14 @@ static bool page_range_has_eb(struct btrfs_fs_info *fs_info, struct page *page)
>  static void detach_extent_buffer_page(struct extent_buffer *eb, struct page *page)
>  {
>  	struct btrfs_fs_info *fs_info = eb->fs_info;
> +	/*
> +	 * We can no longer using page->mapping reliably, as some extent buffer
> +	 * may not have any page mapped to btree_inode yet.
> +	 * Furthermore we have to handle dummy ebs during selftests, where
> +	 * btree_inode is not yet initialized.
> +	 */
> +	struct address_space *mapping = fs_info->btree_inode ?
> +					fs_info->btree_inode->i_mapping : NULL;

I don't understand this, this should only happen if we managed to get
PagePrivate set on the page, and in that case page->mapping is definitely
reliable.  We shouldn't be getting in here with a page that hasn't actually been
attached to the extent buffer, and if we are that needs to be fixed, we don't
need to be dealing with that case in this way.  Thanks,

Josef