On Tue, Jan 27, 2015 at 03:51:20PM +0100, Michal Hocko wrote:
> commit 45f87de57f8fad59302fd263dd81ffa4843b5b24 upstream.
>
> Commit 2457aec63745 ("mm: non-atomically mark page accessed during page
> cache allocation where possible") has added a separate parameter for
> specifying gfp mask for radix tree allocations.
>
> Not only this is less than optimal from the API point of view because it
> is error prone, it is also buggy currently because
> grab_cache_page_write_begin is using GFP_KERNEL for radix tree and if
> fgp_flags doesn't contain FGP_NOFS (mostly controlled by fs by
> AOP_FLAG_NOFS flag) but the mapping_gfp_mask has __GFP_FS cleared then
> the radix tree allocation wouldn't obey the restriction and might
> recurse into filesystem and cause deadlocks. This is the case for most
> filesystems unfortunately because only ext4 and gfs2 are using
> AOP_FLAG_NOFS.
>
> Let's simply remove radix_gfp_mask parameter because the allocation
> context is same for both page cache and for the radix tree. Just make
> sure that the radix tree gets only the sane subset of the mask (e.g. do
> not pass __GFP_WRITE).
>
> Long term it is more preferable to convert remaining users of
> AOP_FLAG_NOFS to use mapping_gfp_mask instead and simplify this
> interface even further.
>
> Reported-by: Dave Chinner <david@xxxxxxxxxxxxx>
> Signed-off-by: Michal Hocko <mhocko@xxxxxxx>
> Signed-off-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> ---
>
> 2457aec63745 has been introduced in 3.16 but it has been backported to
> stable-3.12 (stable sha1 d618a27c7808608e376de803a4fd3940f33776c2). I
> haven't checked other long term stable trees but it is possible they
> need this patch as well.
>
Thanks, I'll queue this patch for the 3.16 kernel as well.
Cheers,
--
Luís
> include/linux/pagemap.h | 13 ++++++-------
> mm/filemap.c | 20 +++++++++-----------
> 2 files changed, 15 insertions(+), 18 deletions(-)
>
> diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
> index d57a02a9747b..bf944e86895b 100644
> --- a/include/linux/pagemap.h
> +++ b/include/linux/pagemap.h
> @@ -256,7 +256,7 @@ pgoff_t page_cache_prev_hole(struct address_space *mapping,
> #define FGP_NOWAIT 0x00000020
>
> struct page *pagecache_get_page(struct address_space *mapping, pgoff_t offset,
> - int fgp_flags, gfp_t cache_gfp_mask, gfp_t radix_gfp_mask);
> + int fgp_flags, gfp_t cache_gfp_mask);
>
> /**
> * find_get_page - find and get a page reference
> @@ -271,13 +271,13 @@ struct page *pagecache_get_page(struct address_space *mapping, pgoff_t offset,
> static inline struct page *find_get_page(struct address_space *mapping,
> pgoff_t offset)
> {
> - return pagecache_get_page(mapping, offset, 0, 0, 0);
> + return pagecache_get_page(mapping, offset, 0, 0);
> }
>
> static inline struct page *find_get_page_flags(struct address_space *mapping,
> pgoff_t offset, int fgp_flags)
> {
> - return pagecache_get_page(mapping, offset, fgp_flags, 0, 0);
> + return pagecache_get_page(mapping, offset, fgp_flags, 0);
> }
>
> /**
> @@ -297,7 +297,7 @@ static inline struct page *find_get_page_flags(struct address_space *mapping,
> static inline struct page *find_lock_page(struct address_space *mapping,
> pgoff_t offset)
> {
> - return pagecache_get_page(mapping, offset, FGP_LOCK, 0, 0);
> + return pagecache_get_page(mapping, offset, FGP_LOCK, 0);
> }
>
> /**
> @@ -324,7 +324,7 @@ static inline struct page *find_or_create_page(struct address_space *mapping,
> {
> return pagecache_get_page(mapping, offset,
> FGP_LOCK|FGP_ACCESSED|FGP_CREAT,
> - gfp_mask, gfp_mask & GFP_RECLAIM_MASK);
> + gfp_mask);
> }
>
> /**
> @@ -345,8 +345,7 @@ static inline struct page *grab_cache_page_nowait(struct address_space *mapping,
> {
> return pagecache_get_page(mapping, index,
> FGP_LOCK|FGP_CREAT|FGP_NOFS|FGP_NOWAIT,
> - mapping_gfp_mask(mapping),
> - GFP_NOFS);
> + mapping_gfp_mask(mapping));
> }
>
> struct page *find_get_entry(struct address_space *mapping, pgoff_t offset);
> diff --git a/mm/filemap.c b/mm/filemap.c
> index e94c70380deb..bd08e9bbf347 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -897,7 +897,7 @@ EXPORT_SYMBOL(find_lock_entry);
> * @mapping: the address_space to search
> * @offset: the page index
> * @fgp_flags: PCG flags
> - * @gfp_mask: gfp mask to use if a page is to be allocated
> + * @gfp_mask: gfp mask to use for the page cache data page allocation
> *
> * Looks up the page cache slot at @mapping & @offset.
> *
> @@ -916,7 +916,7 @@ EXPORT_SYMBOL(find_lock_entry);
> * If there is a page cache page, it is returned with an increased refcount.
> */
> struct page *pagecache_get_page(struct address_space *mapping, pgoff_t offset,
> - int fgp_flags, gfp_t cache_gfp_mask, gfp_t radix_gfp_mask)
> + int fgp_flags, gfp_t gfp_mask)
> {
> struct page *page;
>
> @@ -953,13 +953,11 @@ no_page:
> if (!page && (fgp_flags & FGP_CREAT)) {
> int err;
> if ((fgp_flags & FGP_WRITE) && mapping_cap_account_dirty(mapping))
> - cache_gfp_mask |= __GFP_WRITE;
> - if (fgp_flags & FGP_NOFS) {
> - cache_gfp_mask &= ~__GFP_FS;
> - radix_gfp_mask &= ~__GFP_FS;
> - }
> + gfp_mask |= __GFP_WRITE;
> + if (fgp_flags & FGP_NOFS)
> + gfp_mask &= ~__GFP_FS;
>
> - page = __page_cache_alloc(cache_gfp_mask);
> + page = __page_cache_alloc(gfp_mask);
> if (!page)
> return NULL;
>
> @@ -970,7 +968,8 @@ no_page:
> if (fgp_flags & FGP_ACCESSED)
> init_page_accessed(page);
>
> - err = add_to_page_cache_lru(page, mapping, offset, radix_gfp_mask);
> + err = add_to_page_cache_lru(page, mapping, offset,
> + gfp_mask & GFP_RECLAIM_MASK);
> if (unlikely(err)) {
> page_cache_release(page);
> page = NULL;
> @@ -2462,8 +2461,7 @@ struct page *grab_cache_page_write_begin(struct address_space *mapping,
> fgp_flags |= FGP_NOFS;
>
> page = pagecache_get_page(mapping, index, fgp_flags,
> - mapping_gfp_mask(mapping),
> - GFP_KERNEL);
> + mapping_gfp_mask(mapping));
> if (page)
> wait_for_stable_page(page);
>
> --
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe stable" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html