Re: [RFC PATCH] mm, memcg: use consistent gfp flags during readahead

[Vladimir Davydov]

On Tue, Jun 07, 2016 at 02:12:36PM +0200, Michal Hocko wrote:
> From: Michal Hocko <mhocko@xxxxxxxx>
> 
> Vladimir has noticed that we might declare memcg oom even during
> readahead because read_pages only uses GFP_KERNEL (with mapping_gfp
> restriction) while __do_page_cache_readahead uses
> page_cache_alloc_readahead which adds __GFP_NORETRY to prevent from
> OOMs. This gfp mask discrepancy is really unfortunate and easily
> fixable. Drop page_cache_alloc_readahead() which only has one user
> and outsource the gfp_mask logic into readahead_gfp_mask and propagate
> this mask from __do_page_cache_readahead down to read_pages.
> 
> This alone would have only very limited impact as most filesystems
> are implementing ->readpages and the common implementation
> mpage_readpages does GFP_KERNEL (with mapping_gfp restriction) again.
> We can tell it to use readahead_gfp_mask instead as this function is
> called only during readahead as well. The same applies to
> read_cache_pages.
> 
> ext4 has its own ext4_mpage_readpages but the path which has pages !=
> NULL can use the same gfp mask.
> Btrfs, cifs, f2fs and orangefs are doing a very similar pattern to
> mpage_readpages so the same can be applied to them as well.
> 
> Signed-off-by: Michal Hocko <mhocko@xxxxxxxx>
> ---
> 
> Hi,
> an alternative solution for ->readpages part would be add the gfp mask
> as a new argument. This would be a larger change and I am not even sure
> it would be so much better. An explicit usage of the readahead gfp mask
> sounds like easier to track. If there is a general agreement this is a
> proper way to go I can rework the patch to do so, of course.
> 
> Does this make sense?
...
> diff --git a/fs/ext4/readpage.c b/fs/ext4/readpage.c
> index dc54a4b60eba..c75b66a64982 100644
> --- a/fs/ext4/readpage.c
> +++ b/fs/ext4/readpage.c
> @@ -166,7 +166,7 @@ int ext4_mpage_readpages(struct address_space *mapping,
>  			page = list_entry(pages->prev, struct page, lru);
>  			list_del(&page->lru);
>  			if (add_to_page_cache_lru(page, mapping, page->index,
> -				  mapping_gfp_constraint(mapping, GFP_KERNEL)))
> +				  readahead_gfp_mask(mapping)))
>  				goto next_page;
>  		}
>  

ext4 (at least) might issue other allocations in ->readpages, e.g.
bio_alloc with GFP_KERNEL.

I wonder if it would be better to set GFP_NOFS context on task_struct in
read_pages() and handle it in alloc_pages. You've been planning doing
something like this anyway, haven't you?