Re: [PATCH v1 2/6] fs: use on-stack-bio if backing device has BDI_CAP_SYNC capability

From: Minchan Kim
Date: Tue Aug 08 2017 - 21:48:12 EST


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.