Date: Wed, 9 Aug 2017 10:48:04 +0900
From: Minchan Kim <minchan@kernel.org>
To: Matthew Wilcox <willy@infradead.org>
Cc: Andrew Morton <akpm@linux-foundation.org>,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        Ross Zwisler <ross.zwisler@linux.intel.com>,
        "karam . lee" <karam.lee@lge.com>, seungho1.park@lge.com,
        Christoph Hellwig <hch@lst.de>,
        Dan Williams <dan.j.williams@intel.com>,
        Dave Chinner <david@fromorbit.com>, jack@suse.cz,
        Jens Axboe <axboe@kernel.dk>, Vishal Verma <vishal.l.verma@intel.com>,
        linux-nvdimm@lists.01.org, kernel-team <kernel-team@lge.com>
Subject: Re: [PATCH v1 2/6] fs: use on-stack-bio if backing device has
 BDI_CAP_SYNC capability
Message-ID: <20170809014804.GA32338@bbox>
References: <1502175024-28338-1-git-send-email-minchan@kernel.org>
 <1502175024-28338-3-git-send-email-minchan@kernel.org>
 <20170808124959.GB31390@bombadil.infradead.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20170808124959.GB31390@bombadil.infradead.org>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2026
Lines: 59

Hi Matthew,

On Tue, Aug 08, 2017 at 05:49:59AM -0700, Matthew Wilcox wrote:
> On Tue, Aug 08, 2017 at 03:50:20PM +0900, Minchan Kim wrote:
> > There is no need to use dynamic bio allocation for BDI_CAP_SYNC
> > devices. They can with on-stack-bio without concern about waiting
> > bio allocation from mempool under heavy memory pressure.
> 
> This seems ... more complex than necessary?  Why not simply do this:
> 
> diff --git a/fs/mpage.c b/fs/mpage.c
> index baff8f820c29..6db6bf5131ed 100644
> --- a/fs/mpage.c
> +++ b/fs/mpage.c
> @@ -157,6 +157,8 @@ do_mpage_readpage(struct bio *bio, struct page *page, unsigned nr_pages,
>  	unsigned page_block;
>  	unsigned first_hole = blocks_per_page;
>  	struct block_device *bdev = NULL;
> +	struct bio sbio;
> +	struct bio_vec sbvec;
>  	int length;
>  	int fully_mapped = 1;
>  	unsigned nblocks;
> @@ -281,10 +283,17 @@ do_mpage_readpage(struct bio *bio, struct page *page, unsigned nr_pages,
>  								page))
>  				goto out;
>  		}
> -		bio = mpage_alloc(bdev, blocks[0] << (blkbits - 9),
> +		if (bdi_cap_synchronous_io(inode_to_bdi(inode))) {
> +			bio = &sbio;
> +			bio_init(bio, &sbvec, nr_pages);
> +			sbio.bi_bdev = bdev;
> +			sbio.bi_iter.bi_sector = blocks[0] << (blkbits - 9);
> +		} else {
> +			bio = mpage_alloc(bdev, blocks[0] << (blkbits - 9),
>  				min_t(int, nr_pages, BIO_MAX_PAGES), gfp);
> -		if (bio == NULL)
> -			goto confused;
> +			if (bio == NULL)
> +				goto confused;
> +		}
>  	}
>  
>  	length = first_hole << blkbits;
> @@ -301,6 +310,8 @@ do_mpage_readpage(struct bio *bio, struct page *page, unsigned nr_pages,
>  	else
>  		*last_block_in_bio = blocks[blocks_per_page - 1];
>  out:
> +	if (bio == &sbio)
> +		bio = mpage_bio_submit(REQ_OP_READ, 0, bio);

Looks nicer but one nitpick:

For reusing mpage_bio_submit, we need to call bio_get for on-stack-bio which
doesn't make sense to me but if you think it's more readable and ok with
overhead with two unnecessary atomic instructions(bio_get/put), I will do it
in next spin.

Thanks.