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.
}
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(-)
>>
>
signature.asc
Description: OpenPGP digital signature
