Re: [PATCH 2/2] btrfs: remove the extra_gfp parameter from btrfs_alloc_page_array()

[Qu Wenruo <wqu@suse.com>]

在 2024/6/27 20:08, Filipe Manana 写道:
> On Thu, Jun 27, 2024 at 11:15 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>>
>>
>>
>> 在 2024/6/27 18:20, Filipe Manana 写道:
>>> On Thu, Jun 27, 2024 at 5:33 AM Qu Wenruo <wqu@suse.com> wrote:
>>>>
>>>> There is only one caller utilizing the @extra_gfp parameter,
>>>> alloc_eb_folio_array(). All the other callers do not really bother the
>>>> @extra_gfp at all.
>>>>
>>>> This patch removes the parameter from the public function, meanwhile
>>>> implement an internal version with @nofail parameter just for
>>>> alloc_eb_folio_array() to utilize.
>>>>
>>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>>> ---
>>>>    fs/btrfs/extent_io.c | 43 +++++++++++++++++++++++--------------------
>>>>    fs/btrfs/extent_io.h |  3 +--
>>>>    fs/btrfs/inode.c     |  2 +-
>>>>    fs/btrfs/raid56.c    |  6 +++---
>>>>    fs/btrfs/scrub.c     |  2 +-
>>>>    5 files changed, 29 insertions(+), 27 deletions(-)
>>>>
>>>> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
>>>> index dc416bad9ad8..64f8e7b4d553 100644
>>>> --- a/fs/btrfs/extent_io.c
>>>> +++ b/fs/btrfs/extent_io.c
>>>> @@ -695,22 +695,10 @@ int btrfs_alloc_folio_array(unsigned int nr_folios, struct folio **folio_array)
>>>>           return -ENOMEM;
>>>>    }
>>>>
>>>> -/*
>>>> - * Populate every free slot in a provided array with pages.
>>>> - *
>>>> - * @nr_pages:   number of pages to allocate
>>>> - * @page_array: the array to fill with pages; any existing non-null entries in
>>>> - *             the array will be skipped
>>>> - * @extra_gfp: the extra GFP flags for the allocation.
>>>> - *
>>>> - * Return: 0        if all pages were able to be allocated;
>>>> - *         -ENOMEM  otherwise, the partially allocated pages would be freed and
>>>> - *                  the array slots zeroed
>>>> - */
>>>> -int btrfs_alloc_page_array(unsigned int nr_pages, struct page **page_array,
>>>> -                          gfp_t extra_gfp)
>>>> +static int __btrfs_alloc_page_array(unsigned int nr_pages,
>>>> +                                   struct page **page_array, bool nofail)
>>>
>>> Please don't use functions prefixed with __, we have abandoned that
>>> practice, it's against our preferred coding style.
>>>
>>> Also instead of adding a wrapper function, why not just change the
>>> extra_gfp parameter of btrfs_alloc_page() to the "bool nofail" thing?
>>>
>>> You mentioned in the cover letter "callers have to pay for the extra
>>> parameter", but really are you worried about performance?
>>> On x86_64 the argument is passed in a register, which is super
>>> efficient, almost certainly better than the overhead of an extra
>>> function call.
>>
>> It's not about performance, but the burden on us reviewing/writing code
>> using that function.
>> As everytime we need to call that function, we have to check if we need
>> to use any special value for the extra parameter.
> 
> Well, that's the case for pretty much every other function call
> everywhere, we have to figure out what parameter values to pass.
> 
> If we start adding such wrappers around every case, we end up with
> plenty of these one line functions.
> And sooner or later someone will send a cleanup patch to remove them
> and make all callers pass the extra argument (we have a history of
> such cleanup patches).

OK, then I would just go rename the @extra_gfp parameter to @nofail as 
suggested.

Thanks,
Qu
> 
>>
>> The basic idea is, to keep the most common call to be simple (aka, less
>> parameters), and only for those special call sites to use the more
>> complex one.
>>
>> This is the only time I miss function overloading in C++.
>>
>> Thanks,
>> Qu
>>
>>>
>>> Thanks.
>>>
>>>>    {
>>>> -       const gfp_t gfp = GFP_NOFS | extra_gfp;
>>>> +       const gfp_t gfp = nofail ? (GFP_NOFS | __GFP_NOFAIL) : GFP_NOFS;
>>>>           unsigned int allocated;
>>>>
>>>>           for (allocated = 0; allocated < nr_pages;) {
>>>> @@ -728,19 +716,34 @@ int btrfs_alloc_page_array(unsigned int nr_pages, struct page **page_array,
>>>>           }
>>>>           return 0;
>>>>    }
>>>> +/*
>>>> + * Populate every free slot in a provided array with pages, using GFP_NOFS.
>>>> + *
>>>> + * @nr_pages:   number of pages to allocate
>>>> + * @page_array: the array to fill with pages; any existing non-null entries in
>>>> + *             the array will be skipped
>>>> + *
>>>> + * Return: 0        if all pages were able to be allocated;
>>>> + *         -ENOMEM  otherwise, the partially allocated pages would be freed and
>>>> + *                  the array slots zeroed
>>>> + */
>>>> +int btrfs_alloc_page_array(unsigned int nr_pages, struct page **page_array)
>>>> +{
>>>> +       return __btrfs_alloc_page_array(nr_pages, page_array, false);
>>>> +}
>>>>
>>>>    /*
>>>>     * Populate needed folios for the extent buffer.
>>>>     *
>>>>     * For now, the folios populated are always in order 0 (aka, single page).
>>>>     */
>>>> -static int alloc_eb_folio_array(struct extent_buffer *eb, gfp_t extra_gfp)
>>>> +static int alloc_eb_folio_array(struct extent_buffer *eb, bool nofail)
>>>>    {
>>>>           struct page *page_array[INLINE_EXTENT_BUFFER_PAGES] = { 0 };
>>>>           int num_pages = num_extent_pages(eb);
>>>>           int ret;
>>>>
>>>> -       ret = btrfs_alloc_page_array(num_pages, page_array, extra_gfp);
>>>> +       ret = __btrfs_alloc_page_array(num_pages, page_array, nofail);
>>>>           if (ret < 0)
>>>>                   return ret;
>>>>
>>>> @@ -2722,7 +2725,7 @@ struct extent_buffer *btrfs_clone_extent_buffer(const struct extent_buffer *src)
>>>>            */
>>>>           set_bit(EXTENT_BUFFER_UNMAPPED, &new->bflags);
>>>>
>>>> -       ret = alloc_eb_folio_array(new, 0);
>>>> +       ret = alloc_eb_folio_array(new, false);
>>>>           if (ret) {
>>>>                   btrfs_release_extent_buffer(new);
>>>>                   return NULL;
>>>> @@ -2755,7 +2758,7 @@ struct extent_buffer *__alloc_dummy_extent_buffer(struct btrfs_fs_info *fs_info,
>>>>           if (!eb)
>>>>                   return NULL;
>>>>
>>>> -       ret = alloc_eb_folio_array(eb, 0);
>>>> +       ret = alloc_eb_folio_array(eb, false);
>>>>           if (ret)
>>>>                   goto err;
>>>>
>>>> @@ -3121,7 +3124,7 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
>>>>
>>>>    reallocate:
>>>>           /* Allocate all pages first. */
>>>> -       ret = alloc_eb_folio_array(eb, __GFP_NOFAIL);
>>>> +       ret = alloc_eb_folio_array(eb, true);
>>>>           if (ret < 0) {
>>>>                   btrfs_free_subpage(prealloc);
>>>>                   goto out;
>>>> diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
>>>> index 8364dcb1ace3..0da5f1947a2b 100644
>>>> --- a/fs/btrfs/extent_io.h
>>>> +++ b/fs/btrfs/extent_io.h
>>>> @@ -363,8 +363,7 @@ int extent_invalidate_folio(struct extent_io_tree *tree,
>>>>    void btrfs_clear_buffer_dirty(struct btrfs_trans_handle *trans,
>>>>                                 struct extent_buffer *buf);
>>>>
>>>> -int btrfs_alloc_page_array(unsigned int nr_pages, struct page **page_array,
>>>> -                          gfp_t extra_gfp);
>>>> +int btrfs_alloc_page_array(unsigned int nr_pages, struct page **page_array);
>>>>    int btrfs_alloc_folio_array(unsigned int nr_folios, struct folio **folio_array);
>>>>
>>>>    #ifdef CONFIG_BTRFS_FS_RUN_SANITY_TESTS
>>>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>>>> index 92ef9b01cf5e..6dfcd27b88ac 100644
>>>> --- a/fs/btrfs/inode.c
>>>> +++ b/fs/btrfs/inode.c
>>>> @@ -9128,7 +9128,7 @@ static ssize_t btrfs_encoded_read_regular(struct kiocb *iocb,
>>>>           pages = kcalloc(nr_pages, sizeof(struct page *), GFP_NOFS);
>>>>           if (!pages)
>>>>                   return -ENOMEM;
>>>> -       ret = btrfs_alloc_page_array(nr_pages, pages, 0);
>>>> +       ret = btrfs_alloc_page_array(nr_pages, pages);
>>>>           if (ret) {
>>>>                   ret = -ENOMEM;
>>>>                   goto out;
>>>> diff --git a/fs/btrfs/raid56.c b/fs/btrfs/raid56.c
>>>> index 3858c00936e8..294bf7349f96 100644
>>>> --- a/fs/btrfs/raid56.c
>>>> +++ b/fs/btrfs/raid56.c
>>>> @@ -1051,7 +1051,7 @@ static int alloc_rbio_pages(struct btrfs_raid_bio *rbio)
>>>>    {
>>>>           int ret;
>>>>
>>>> -       ret = btrfs_alloc_page_array(rbio->nr_pages, rbio->stripe_pages, 0);
>>>> +       ret = btrfs_alloc_page_array(rbio->nr_pages, rbio->stripe_pages);
>>>>           if (ret < 0)
>>>>                   return ret;
>>>>           /* Mapping all sectors */
>>>> @@ -1066,7 +1066,7 @@ static int alloc_rbio_parity_pages(struct btrfs_raid_bio *rbio)
>>>>           int ret;
>>>>
>>>>           ret = btrfs_alloc_page_array(rbio->nr_pages - data_pages,
>>>> -                                    rbio->stripe_pages + data_pages, 0);
>>>> +                                    rbio->stripe_pages + data_pages);
>>>>           if (ret < 0)
>>>>                   return ret;
>>>>
>>>> @@ -1640,7 +1640,7 @@ static int alloc_rbio_data_pages(struct btrfs_raid_bio *rbio)
>>>>           const int data_pages = rbio->nr_data * rbio->stripe_npages;
>>>>           int ret;
>>>>
>>>> -       ret = btrfs_alloc_page_array(data_pages, rbio->stripe_pages, 0);
>>>> +       ret = btrfs_alloc_page_array(data_pages, rbio->stripe_pages);
>>>>           if (ret < 0)
>>>>                   return ret;
>>>>
>>>> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
>>>> index 4677a4f55b6a..2d819b027321 100644
>>>> --- a/fs/btrfs/scrub.c
>>>> +++ b/fs/btrfs/scrub.c
>>>> @@ -261,7 +261,7 @@ static int init_scrub_stripe(struct btrfs_fs_info *fs_info,
>>>>           atomic_set(&stripe->pending_io, 0);
>>>>           spin_lock_init(&stripe->write_error_lock);
>>>>
>>>> -       ret = btrfs_alloc_page_array(SCRUB_STRIPE_PAGES, stripe->pages, 0);
>>>> +       ret = btrfs_alloc_page_array(SCRUB_STRIPE_PAGES, stripe->pages);
>>>>           if (ret < 0)
>>>>                   goto error;
>>>>
>>>> --
>>>> 2.45.2
>>>>
>>>>
>>>