Re: [PATCH 0/5] Unify the return value of alloc/clone_extent_buffer()

[Qu Wenruo <wqu@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 4339 bytes --]



On 2019/2/22 下午9:29, Nikolay Borisov wrote:
> 
> 
> On 22.02.19 г. 15:02 ч., Qu Wenruo wrote:
>>
>>
>> On 2019/2/22 下午8:54, Nikolay Borisov wrote:
>>>
>>>
>>> On 22.02.19 г. 12:16 ч., Qu Wenruo wrote:
>>>> This patchset can be fetched from github:
>>>> https://github.com/adam900710/linux/tree/cleanup_alloc_extent_buffer
>>>> Which is based on v5.0-rc7
>>>>
>>>> There are 5 extent buffer alloc functions in btrfs:
>>>> __alloc_extent_buffer();
>>>> alloc_extent_buffer();
>>>> __alloc_dummy_extent_buffer();
>>>> alloc_dummy_extent_buffer();
>>>> alloc_test_extent_buffer();
>>>>
>>>> However their return value is not unified for failure mode:
>>>> __alloc_extent_buffer()		Never fail
>>>> alloc_extent_buffer()		PTR_ERR()
>>>> __alloc_dummy_extent_buffer()	NULL
>>>
>>> This function can never return NULL, if __alloc_extent_buffer cannot
>>> fail then the only error this function returns is ERR_PTR(ENOMEM);
>>
>> Nope.
>>
>>         for (i = 0; i < num_pages; i++) {
>>                 eb->pages[i] = alloc_page(GFP_NOFS);
>>                 if (!eb->pages[i])
>>                         goto err; <<< Page alloc failure here
>>         }
>> ...
>> err:
>>         for (; i > 0; i--)
>>                 __free_page(eb->pages[i - 1]);
>>         __free_extent_buffer(eb);
>>         return NULL; << We got NULL.
> 
> Right, I was looking at the code AFTER having applied your patches. So I
> agree with yout. However, the ordering of your patches and the
> changelogs make it rather hard to understand. What I'd suggest regarding
> the changelogs is - forget about unification, just say what you are
> doing, which is always ensuring that an error is returned from
> __alloc_extent_buffer in one patch - this should involve both changes to
> __alloc_extent_buffer as well as it's (in)direct callers. Then you do
> the same for other function. Otherwise review is somewhat hindered.

Makes sense.

I'll reword and reorder the patches.

Thanks,
Qu

> 
>> }
>>
>> For __alloc_dummy_extent_buffer, that's the only failure case.
>>
>> And I'm interested how did you get the PTR_ERR() case?
>>
>>>
>>>> alloc_dummy_extent_buffer()	NULL
>>> Same thing applies to this function
>>
>> Nope.
>>
>>>
>>>> alloc_test_extent_buffer()	NULL
>>>
>>> Same thing for this function, if we return exists then we must have
>>> found it by find_extent_buffer hence it cannot be null. Otherwise we
>>> return eb as allocated from alloc_dummy_extent_buffer. So how can null
>>> be returned?
>>
>> And nope.
>>
>>>
>>> To me it really seems none of the function could return a NULL value, no?
>>
>> Your misunderstand of __alloc_dummy_extent_buffer() makes the call chain
>> all wrong.
>>
>> Thanks,
>> Qu
>>
>>>
>>>>
>>>> This causes some wrapper function to have 2 failure modes, like
>>>> btrfs_find_create_tree_block() can return NULL or PTR_ERR(-ENOMEM) for
>>>> its failure.
>>>>
>>>> This inconsistent behavior is making static checker and reader crazy.
>>>>
>>>> This patchset will unify the failure more of above 5 functions to
>>>> PTR_ERR().
>>>>
>>>> Qu Wenruo (5):
>>>>   btrfs: extent_io: Add comment about the return value of
>>>>     alloc_extent_buffer()
>>>>   btrfs: extent_io: Unify the return value of __alloc_extent_buffer()
>>>>     with alloc_extent_buffer()
>>>>   btrfs: extent_io: Unify the return value of
>>>>     alloc_dummy_extent_buffer() with alloc_extent_buffer()
>>>>   btrfs: extent_io: Unify the return value of alloc_test_extent_buffer()
>>>>     with alloc_extent_buffer()
>>>>   btrfs: extent_io: Unify the return value of
>>>>     btrfs_clone_extent_buffer() with alloc_extent_buffer()
>>>>
>>>>  fs/btrfs/backref.c                     |  8 ++--
>>>>  fs/btrfs/ctree.c                       | 16 ++++----
>>>>  fs/btrfs/extent_io.c                   | 56 +++++++++++++++++---------
>>>>  fs/btrfs/qgroup.c                      |  5 ++-
>>>>  fs/btrfs/tests/extent-buffer-tests.c   |  6 ++-
>>>>  fs/btrfs/tests/extent-io-tests.c       |  4 +-
>>>>  fs/btrfs/tests/free-space-tree-tests.c |  3 +-
>>>>  fs/btrfs/tests/inode-tests.c           |  6 ++-
>>>>  fs/btrfs/tests/qgroup-tests.c          |  3 +-
>>>>  9 files changed, 68 insertions(+), 39 deletions(-)
>>>>
>>>
>>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]