Re: [PATCH 02/12] buffer: grow_dev_page() should use __GFP_NOFAIL for all cases

[Jan Kara]

On Tue 03-10-17 08:36:16, Jens Axboe wrote:
> On 10/03/2017 06:25 AM, Jan Kara wrote:
> > On Tue 03-10-17 14:10:49, Jan Kara wrote:
> >> On Wed 27-09-17 14:13:49, Jens Axboe wrote:
> >>> We currently it it for find_or_create_page(), which means that it
> >>> cannot fail. Ensure we also pass in 'retry == true' to
> >>> alloc_page_buffers(), which also ensure that it cannot fail.
> >>>
> >>> After this, there are no failure cases in grow_dev_page() that
> >>> occur because of a failed memory allocation.
> >>>
> >>> Signed-off-by: Jens Axboe <axboe@xxxxxxxxx>
> >>
> >> Makes sense. You can add:
> >>
> >> Reviewed-by: Jan Kara <jack@xxxxxxx>
> > 
> > I forgot one question though:
> > 
> >>>  	page = find_or_create_page(inode->i_mapping, index, gfp_mask);
> >>> -	if (!page)
> >>> -		return ret;
> > 
> > Are we sure find_or_create_page() cannot fail for other reasons than
> > ENOMEM? Currently it seems to be the case AFAICT but it isn't obvious to me
> > that is guaranteed in future as well...
> 
> It looks up a page, or allocates and adds one. If we tell the allocator
> that we cannot tolerate failure, then I don't see what else would be
> able to make it fail. That function is such an integral part of
> lookup/create for the page cache.

Well, there memcg stuff in there, radix tree handling etc. So it is not
only an allocation that could fail. But all take care of not failing if
GFP_NOFAIL is set so I guess there are good chances for this to hold even
in the future.

								Honza
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR