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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2023/11/28 02:58, Josef Bacik wrote:
> On Thu, Nov 23, 2023 at 06:30:05AM +1030, Qu Wenruo wrote:
>>
>>
>> On 2023/11/23 00:44, Josef Bacik wrote:
>>> 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,
>>
>> The problem is, with the new way of allocating pages first, then attach
>> them to filemap, we can have a case where the extent buffer has 4 pages,
>> but only the first page is attached to filemap of btree inode.
>>
>> In that case, we still want to lock the btree inode private, but
>> page->mapping is still NULL for the remaining 3 pages.
>>
>
> In this case I think we just change the error handling at the end, as we already
> have to do something special anyways, so we now would do
>
> for (int i = 0; i < attached; i++) {
> 	unlock_page(eb->pages[i]);
> 	detach_extent_buffer_page(eb->pages[i]);
> }
>
> for (int i = 0; i < num_pages; i++) {
> 	if (!eb->pages[i])
> 		break;
> 	put_page(eb->pages[i]);
> }
>
> and then we can leave detach_extent_buffer_page on its own.  Thanks,

This sounds pretty reasonable, I'll do more tests to make sure this works.

Thanks,
Qu
>
> Josef
>