Re: [PATCH 01/10] fs: buffer - Use memalloc_nofs API to allocate pages

From: Michal Hocko <mhocko@kernel.org>
To: Goldwyn Rodrigues <rgoldwyn@suse.de>
Cc: linux-fsdevel@vger.kernel.org, Goldwyn Rodrigues <rgoldwyn@suse.com>
Subject: Re: [PATCH 01/10] fs: buffer - Use memalloc_nofs API to allocate pages
Date: Mon, 19 Feb 2018 15:24:57 +0100	[thread overview]
Message-ID: <20180219142457.GO21134@dhcp22.suse.cz> (raw)
In-Reply-To: <20180219140230.5077-2-rgoldwyn@suse.de>

On Mon 19-02-18 08:02:21, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 

I am pretty sure we really need a changelog here.

> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
>  fs/buffer.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/buffer.c b/fs/buffer.c
> index 9a73924db22f..1787b29f3fb3 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -45,6 +45,7 @@
>  #include <linux/mpage.h>
>  #include <linux/bit_spinlock.h>
>  #include <linux/pagevec.h>
> +#include <linux/sched/mm.h>
>  #include <trace/events/block.h>
>  
>  static int fsync_buffers_list(spinlock_t *lock, struct list_head *list);
> @@ -836,7 +837,8 @@ struct buffer_head *alloc_page_buffers(struct page *page, unsigned long size,
>  		bool retry)
>  {
>  	struct buffer_head *bh, *head;
> -	gfp_t gfp = GFP_NOFS;
> +	gfp_t gfp = GFP_KERNEL;
> +	unsigned int nofs_flags;
>  	long offset;
>  
>  	if (retry)
> @@ -844,6 +846,7 @@ struct buffer_head *alloc_page_buffers(struct page *page, unsigned long size,
>  
>  	head = NULL;
>  	offset = PAGE_SIZE;
> +	nofs_flags = memalloc_nofs_save();

OK, so what is the actual scope here. What kind of context is recursion
problematic? This is a helper function and as such it is the caller who
knows the fs context. So this is quite opposite to what how the scope
API should be used. It is not about pure GFP_NOFS replacement. We really
want to document those scopes.

>  	while ((offset -= size) >= 0) {
>  		bh = alloc_buffer_head(gfp);
>  		if (!bh)
> @@ -858,11 +861,13 @@ struct buffer_head *alloc_page_buffers(struct page *page, unsigned long size,
>  		/* Link the buffer to its page */
>  		set_bh_page(bh, page, offset);
>  	}
> +	memalloc_nofs_restore(nofs_flags);
>  	return head;
>  /*
>   * In case anything failed, we just free everything we got.
>   */
>  no_grow:
> +	memalloc_nofs_restore(nofs_flags);
>  	if (head) {
>  		do {
>  			bh = head;
> -- 
> 2.16.1

-- 
Michal Hocko
SUSE Labs