Re: [PATCH v2 06/18] btrfs: extent_io: make attach_extent_buffer_page() to handle subpage case

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

On 2020/12/19 上午8:24, Qu Wenruo wrote:
> 
> 
> On 2020/12/18 下午11:41, Josef Bacik wrote:
>> On 12/17/20 7:44 PM, Qu Wenruo wrote:
>>>
>>>
>>> On 2020/12/18 上午12:00, Josef Bacik wrote:
>>>> On 12/10/20 1:38 AM, Qu Wenruo wrote:
>>>>> For subpage case, we need to allocate new memory for each metadata 
>>>>> page.
>>>>>
>>>>> So we need to:
>>>>> - Allow attach_extent_buffer_page() to return int
>>>>>    To indicate allocation failure
>>>>>
>>>>> - Prealloc page->private for alloc_extent_buffer()
>>>>>    We don't want to call memory allocation with spinlock hold, so
>>>>>    do preallocation before we acquire the spin lock.
>>>>>
>>>>> - Handle subpage and regular case differently in
>>>>>    attach_extent_buffer_page()
>>>>>    For regular case, just do the usual thing.
>>>>>    For subpage case, allocate new memory and update the tree_block
>>>>>    bitmap.
>>>>>
>>>>>    The bitmap update will be handled by new subpage specific helper,
>>>>>    btrfs_subpage_set_tree_block().
>>>>>
>>>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>>>> ---
>>>>>   fs/btrfs/extent_io.c | 69 
>>>>> +++++++++++++++++++++++++++++++++++---------
>>>>>   fs/btrfs/subpage.h   | 44 ++++++++++++++++++++++++++++
>>>>>   2 files changed, 99 insertions(+), 14 deletions(-)
>>>>>
>>>>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
>>>>> index 6350c2687c7e..51dd7ec3c2b3 100644
>>>>> --- a/fs/btrfs/extent_io.c
>>>>> +++ b/fs/btrfs/extent_io.c
>>>>> @@ -24,6 +24,7 @@
>>>>>   #include "rcu-string.h"
>>>>>   #include "backref.h"
>>>>>   #include "disk-io.h"
>>>>> +#include "subpage.h"
>>>>>   static struct kmem_cache *extent_state_cache;
>>>>>   static struct kmem_cache *extent_buffer_cache;
>>>>> @@ -3142,22 +3143,41 @@ static int submit_extent_page(unsigned int 
>>>>> opf,
>>>>>       return ret;
>>>>>   }
>>>>> -static void attach_extent_buffer_page(struct extent_buffer *eb,
>>>>> +static int attach_extent_buffer_page(struct extent_buffer *eb,
>>>>>                         struct page *page)
>>>>>   {
>>>>> -    /*
>>>>> -     * If the page is mapped to btree inode, we should hold the 
>>>>> private
>>>>> -     * lock to prevent race.
>>>>> -     * For cloned or dummy extent buffers, their pages are not 
>>>>> mapped and
>>>>> -     * will not race with any other ebs.
>>>>> -     */
>>>>> -    if (page->mapping)
>>>>> -        lockdep_assert_held(&page->mapping->private_lock);
>>>>> +    struct btrfs_fs_info *fs_info = eb->fs_info;
>>>>> +    int ret;
>>>>> -    if (!PagePrivate(page))
>>>>> -        attach_page_private(page, eb);
>>>>> -    else
>>>>> -        WARN_ON(page->private != (unsigned long)eb);
>>>>> +    if (fs_info->sectorsize == PAGE_SIZE) {
>>>>> +        /*
>>>>> +         * If the page is mapped to btree inode, we should hold the
>>>>> +         * private lock to prevent race.
>>>>> +         * For cloned or dummy extent buffers, their pages are not
>>>>> +         * mapped and will not race with any other ebs.
>>>>> +         */
>>>>> +        if (page->mapping)
>>>>> +            lockdep_assert_held(&page->mapping->private_lock);
>>>>> +
>>>>> +        if (!PagePrivate(page))
>>>>> +            attach_page_private(page, eb);
>>>>> +        else
>>>>> +            WARN_ON(page->private != (unsigned long)eb);
>>>>> +        return 0;
>>>>> +    }
>>>>> +
>>>>> +    /* Already mapped, just update the existing range */
>>>>> +    if (PagePrivate(page))
>>>>> +        goto update_bitmap;
>>>>> +
>>>>> +    /* Do new allocation to attach subpage */
>>>>> +    ret = btrfs_attach_subpage(fs_info, page);
>>>>> +    if (ret < 0)
>>>>> +        return ret;
>>>>> +
>>>>> +update_bitmap:
>>>>> +    btrfs_subpage_set_tree_block(fs_info, page, eb->start, eb->len);
>>>>> +    return 0;
>>>>>   }
>>>>>   void set_page_extent_mapped(struct page *page)
>>>>> @@ -5067,12 +5087,19 @@ struct extent_buffer 
>>>>> *btrfs_clone_extent_buffer(const struct extent_buffer *src)
>>>>>           return NULL;
>>>>>       for (i = 0; i < num_pages; i++) {
>>>>> +        int ret;
>>>>> +
>>>>>           p = alloc_page(GFP_NOFS);
>>>>>           if (!p) {
>>>>>               btrfs_release_extent_buffer(new);
>>>>>               return NULL;
>>>>>           }
>>>>> -        attach_extent_buffer_page(new, p);
>>>>> +        ret = attach_extent_buffer_page(new, p);
>>>>> +        if (ret < 0) {
>>>>> +            put_page(p);
>>>>> +            btrfs_release_extent_buffer(new);
>>>>> +            return NULL;
>>>>> +        }
>>>>>           WARN_ON(PageDirty(p));
>>>>>           SetPageUptodate(p);
>>>>>           new->pages[i] = p;
>>>>> @@ -5321,6 +5348,18 @@ struct extent_buffer 
>>>>> *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
>>>>>               goto free_eb;
>>>>>           }
>>>>> +        /*
>>>>> +         * Preallocate page->private for subpage case, so that
>>>>> +         * we won't allocate memory with private_lock hold.
>>>>> +         */
>>>>> +        ret = btrfs_attach_subpage(fs_info, p);
>>>>> +        if (ret < 0) {
>>>>> +            unlock_page(p);
>>>>> +            put_page(p);
>>>>> +            exists = ERR_PTR(-ENOMEM);
>>>>> +            goto free_eb;
>>>>> +        }
>>>>> +
>>>>
>>>> This is broken, if we race with another thread adding an extent 
>>>> buffer for this same range we'll overwrite the page private with the 
>>>> new thing, losing any of the work that was done previously.  Thanks,
>>>
>>> Firstly the page is locked, so there should be only one to grab the 
>>> page.
>>>
>>> Secondly, btrfs_attach_subpage() would just exit if it detects the 
>>> page is already private.
>>>
>>> So there shouldn't be a race.
>>>
>> Task1                        Task2
>> alloc_extent_buffer(4096)            alloc_extent_buffer(4096)
>>    find_extent_buffer, nothing              find_extent_buffer, nothing
>>      find_or_create_page(1)
>>                              find_or_create_page(1)
>>                                waits on page lock
>>        btrfs_attach_subpage()
>>    radix_tree_insert()
>>    unlock pages
>>                                exit find_or_create_page()
>>                              btrfs_attach_subpage(), BAD

To be more clear, in above case, btrfs_attach_subpage() would find page 
is already private, thus exit without doing anything (no extra attaching 
nor bitmap update).

Thus no btrfs_subpage info is overwritten.

>>
>> there's definitely a race, again this is why the code does the check 
>> to see if there's a private attached to the EB already.  Thanks,

That's exactly btrfs_attach_subpage() is doing.

Anyway, all the hassle is needed just to avoid memory allocation inside 
the spinlock.

Personally speaking I don't see any better solution than pre-allocating 
right now.

Thanks,
Qu

> 
> btrfs_attach_subpage() is already doing the private check.
> 
> Thanks,
> Qu
> 
>>
>> Josef