On Fri, Feb 22, 2019 at 09:31:29PM +0800, Qu Wenruo wrote:
> >> 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.
Yes please, I'll have another look at the updated series. So far I agree
that the unification to IS_ERR/PTR_ERR is desired. For the plain
allocator functions that are only kmalloc+initialization it's generally
ok to return NULL and let the callers do the conversion to PTR_ERR.
In case of the eb functions, there are wrappers like
alloc_dummy_extent_buffer that only forward the return value so this
would be inconsistent and needs one to remember which functions return
what. For this reason I think it's ok to convert all.
Another question is how to do it, ideally as separate patches but the
callchains are deep and half conversion is not what we want. I'll need
to see new patches first.
Also please drop all mentions of the 'never returns NULL' in the
comments, except the function that does the GFP_NOFAIL.
