Subject: Re: [PATCH 01/12] buffer: have alloc_page_buffers() use __GFP_NOFAIL
To: Jens Axboe <axboe@kernel.dk>, linux-kernel@vger.kernel.org,
        linux-fsdevel@vger.kernel.org
Cc: hannes@cmpxchg.org, jack@suse.cz, torvalds@linux-foundation.org
References: <1506543239-31470-1-git-send-email-axboe@kernel.dk>
 <1506543239-31470-2-git-send-email-axboe@kernel.dk>
From: Nikolay Borisov <nborisov@suse.com>
Message-ID: <183f716b-6203-6fa1-46d2-f417bfbedbfe@suse.com>
Date: Thu, 28 Sep 2017 17:08:02 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.3.0
MIME-Version: 1.0
In-Reply-To: <1506543239-31470-2-git-send-email-axboe@kernel.dk>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 5307
Lines: 151


On 27.09.2017 23:13, Jens Axboe wrote:
> Instead of adding weird retry logic in that function, utilize
> __GFP_NOFAIL to ensure that the vm takes care of handling any
> potential retries appropriately. This means we don't have to
> call free_more_memory() from here.
> 
> Signed-off-by: Jens Axboe <axboe@kernel.dk>

Reviewed-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  drivers/md/bitmap.c         |  2 +-
>  fs/buffer.c                 | 33 ++++++++++-----------------------
>  fs/ntfs/aops.c              |  2 +-
>  fs/ntfs/mft.c               |  2 +-
>  include/linux/buffer_head.h |  2 +-
>  5 files changed, 14 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/md/bitmap.c b/drivers/md/bitmap.c
> index d2121637b4ab..4d8ed74efadf 100644
> --- a/drivers/md/bitmap.c
> +++ b/drivers/md/bitmap.c
> @@ -368,7 +368,7 @@ static int read_page(struct file *file, unsigned long index,
>  	pr_debug("read bitmap file (%dB @ %llu)\n", (int)PAGE_SIZE,
>  		 (unsigned long long)index << PAGE_SHIFT);
>  
> -	bh = alloc_page_buffers(page, 1<<inode->i_blkbits, 0);
> +	bh = alloc_page_buffers(page, 1<<inode->i_blkbits, false);
>  	if (!bh) {
>  		ret = -ENOMEM;
>  		goto out;
> diff --git a/fs/buffer.c b/fs/buffer.c
> index 170df856bdb9..1234ae343aef 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -861,16 +861,19 @@ int remove_inode_buffers(struct inode *inode)
>   * which may not fail from ordinary buffer allocations.
>   */
>  struct buffer_head *alloc_page_buffers(struct page *page, unsigned long size,
> -		int retry)
> +		bool retry)
>  {
>  	struct buffer_head *bh, *head;
> +	gfp_t gfp = GFP_NOFS;
>  	long offset;
>  
> -try_again:
> +	if (retry)
> +		gfp |= __GFP_NOFAIL;
> +
>  	head = NULL;
>  	offset = PAGE_SIZE;
>  	while ((offset -= size) >= 0) {
> -		bh = alloc_buffer_head(GFP_NOFS);
> +		bh = alloc_buffer_head(gfp);
>  		if (!bh)
>  			goto no_grow;
>  
> @@ -896,23 +899,7 @@ struct buffer_head *alloc_page_buffers(struct page *page, unsigned long size,
>  		} while (head);
>  	}
>  
> -	/*
> -	 * Return failure for non-async IO requests.  Async IO requests
> -	 * are not allowed to fail, so we have to wait until buffer heads
> -	 * become available.  But we don't want tasks sleeping with 
> -	 * partially complete buffers, so all were released above.
> -	 */
> -	if (!retry)
> -		return NULL;
> -
> -	/* We're _really_ low on memory. Now we just
> -	 * wait for old buffer heads to become free due to
> -	 * finishing IO.  Since this is an async request and
> -	 * the reserve list is empty, we're sure there are 
> -	 * async buffer heads in use.
> -	 */
> -	free_more_memory();
> -	goto try_again;
> +	return NULL;
>  }
>  EXPORT_SYMBOL_GPL(alloc_page_buffers);
>  
> @@ -1021,7 +1008,7 @@ grow_dev_page(struct block_device *bdev, sector_t block,
>  	/*
>  	 * Allocate some buffers for this page
>  	 */
> -	bh = alloc_page_buffers(page, size, 0);
> +	bh = alloc_page_buffers(page, size, false);
>  	if (!bh)
>  		goto failed;
>  
> @@ -1575,7 +1562,7 @@ void create_empty_buffers(struct page *page,
>  {
>  	struct buffer_head *bh, *head, *tail;
>  
> -	head = alloc_page_buffers(page, blocksize, 1);
> +	head = alloc_page_buffers(page, blocksize, true);
>  	bh = head;
>  	do {
>  		bh->b_state |= b_state;
> @@ -2638,7 +2625,7 @@ int nobh_write_begin(struct address_space *mapping,
>  	 * Be careful: the buffer linked list is a NULL terminated one, rather
>  	 * than the circular one we're used to.
>  	 */
> -	head = alloc_page_buffers(page, blocksize, 0);
> +	head = alloc_page_buffers(page, blocksize, false);
>  	if (!head) {
>  		ret = -ENOMEM;
>  		goto out_release;
> diff --git a/fs/ntfs/aops.c b/fs/ntfs/aops.c
> index cc91856b5e2d..3a2e509c77c5 100644
> --- a/fs/ntfs/aops.c
> +++ b/fs/ntfs/aops.c
> @@ -1739,7 +1739,7 @@ void mark_ntfs_record_dirty(struct page *page, const unsigned int ofs) {
>  	spin_lock(&mapping->private_lock);
>  	if (unlikely(!page_has_buffers(page))) {
>  		spin_unlock(&mapping->private_lock);
> -		bh = head = alloc_page_buffers(page, bh_size, 1);
> +		bh = head = alloc_page_buffers(page, bh_size, true);
>  		spin_lock(&mapping->private_lock);
>  		if (likely(!page_has_buffers(page))) {
>  			struct buffer_head *tail;
> diff --git a/fs/ntfs/mft.c b/fs/ntfs/mft.c
> index b6f402194f02..ee8392aee9f6 100644
> --- a/fs/ntfs/mft.c
> +++ b/fs/ntfs/mft.c
> @@ -507,7 +507,7 @@ int ntfs_sync_mft_mirror(ntfs_volume *vol, const unsigned long mft_no,
>  	if (unlikely(!page_has_buffers(page))) {
>  		struct buffer_head *tail;
>  
> -		bh = head = alloc_page_buffers(page, blocksize, 1);
> +		bh = head = alloc_page_buffers(page, blocksize, true);
>  		do {
>  			set_buffer_uptodate(bh);
>  			tail = bh;
> diff --git a/include/linux/buffer_head.h b/include/linux/buffer_head.h
> index c8dae555eccf..ae2d25f01b98 100644
> --- a/include/linux/buffer_head.h
> +++ b/include/linux/buffer_head.h
> @@ -156,7 +156,7 @@ void set_bh_page(struct buffer_head *bh,
>  		struct page *page, unsigned long offset);
>  int try_to_free_buffers(struct page *);
>  struct buffer_head *alloc_page_buffers(struct page *page, unsigned long size,
> -		int retry);
> +		bool retry);
>  void create_empty_buffers(struct page *, unsigned long,
>  			unsigned long b_state);
>  void end_buffer_read_sync(struct buffer_head *bh, int uptodate);
>