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/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
> 
> 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,

btrfs_attach_subpage() is already doing the private check.

Thanks,
Qu

> 
> Josef