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.
> }
>
> 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(-)
>>>
>>
>
