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