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