Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1706071 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v1 2/6] fs: use on-stack-bio if backing device has BDI_CAP_SYNC capability Minchan Kim <minchan@kernel.org> - 2017-08-08 09:00 +0200
    Re: [PATCH v1 2/6] fs: use on-stack-bio if backing device has  BDI_CAP_SYNC capability Matthew Wilcox <willy@infradead.org> - 2017-08-08 15:00 +0200
      Re: [PATCH v1 2/6] fs: use on-stack-bio if backing device has  BDI_CAP_SYNC capability Matthew Wilcox <willy@infradead.org> - 2017-08-08 15:30 +0200
        Re: [PATCH v1 2/6] fs: use on-stack-bio if backing device has  BDI_CAP_SYNC capability Minchan Kim <minchan@kernel.org> - 2017-08-09 04:00 +0200
          Re: [PATCH v1 2/6] fs: use on-stack-bio if backing device has  BDI_CAP_SYNC capability Minchan Kim <minchan@kernel.org> - 2017-08-09 04:50 +0200
            Re: [PATCH v1 2/6] fs: use on-stack-bio if backing device has  BDI_CAP_SYNC capability Matthew Wilcox <willy@infradead.org> - 2017-08-10 05:10 +0200
              Re: [PATCH v1 2/6] fs: use on-stack-bio if backing device has  BDI_CAP_SYNC capability Dan Williams <dan.j.williams@intel.com> - 2017-08-10 05:10 +0200
                Re: [PATCH v1 2/6] fs: use on-stack-bio if backing device has         BDI_CAP_SYNC capability Christoph Hellwig <hch@lst.de> - 2017-08-11 12:50 +0200
                  Re: [PATCH v1 2/6] fs: use on-stack-bio if backing device has  BDI_CAP_SYNC capability Jens Axboe <axboe@kernel.dk> - 2017-08-11 16:30 +0200
              Re: [PATCH v1 2/6] fs: use on-stack-bio if backing device has  BDI_CAP_SYNC capability Minchan Kim <minchan@kernel.org> - 2017-08-10 06:10 +0200
          Re: [PATCH v1 2/6] fs: use on-stack-bio if backing device has  BDI_CAP_SYNC capability Matthew Wilcox <willy@infradead.org> - 2017-08-09 05:40 +0200
      Re: [PATCH v1 2/6] fs: use on-stack-bio if backing device has  BDI_CAP_SYNC capability Minchan Kim <minchan@kernel.org> - 2017-08-09 03:50 +0200

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

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.

Signed-off-by: Minchan Kim <minchan@kernel.org>
---
 fs/mpage.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/fs/mpage.c b/fs/mpage.c
index 2e4c41ccb5c9..eaeaef27d693 100644
--- a/fs/mpage.c
+++ b/fs/mpage.c
@@ -31,6 +31,14 @@
 #include <linux/cleancache.h>
 #include "internal.h"
 
+static void on_stack_page_end_io(struct bio *bio)
+{
+	struct page *page = bio->bi_io_vec->bv_page;
+
+	page_endio(page, op_is_write(bio_op(bio)),
+		blk_status_to_errno(bio->bi_status));
+}
+
 /*
  * I/O completion handler for multipage BIOs.
  *
@@ -278,6 +286,22 @@ do_mpage_readpage(struct bio *bio, struct page *page, unsigned nr_pages,
 alloc_new:
 	if (bio == NULL) {
 		if (first_hole == blocks_per_page) {
+			if (bdi_cap_synchronous_io(inode_to_bdi(inode))) {
+				/* on-stack-bio */
+				struct bio sbio;
+				struct bio_vec bvec;
+
+				bio_init(&sbio, &bvec, 1);
+				sbio.bi_bdev = bdev;
+				sbio.bi_iter.bi_sector =
+					blocks[0] << (blkbits - 9);
+				sbio.bi_end_io = on_stack_page_end_io;
+				bio_add_page(&sbio, page, PAGE_SIZE, 0);
+				bio_set_op_attrs(&sbio, REQ_OP_READ, 0);
+				submit_bio(&sbio);
+				goto out;
+			}
+
 			if (!bdev_read_page(bdev, blocks[0] << (blkbits - 9),
 								page))
 				goto out;
@@ -604,6 +628,25 @@ static int __mpage_writepage(struct page *page, struct writeback_control *wbc,
 alloc_new:
 	if (bio == NULL) {
 		if (first_unmapped == blocks_per_page) {
+			if (bdi_cap_synchronous_io(inode_to_bdi(inode))) {
+				/* on-stack-bio */
+				struct bio sbio;
+				struct bio_vec bvec;
+
+				bio_init(&sbio, &bvec, 1);
+				sbio.bi_bdev = bdev;
+				sbio.bi_iter.bi_sector =
+					blocks[0] << (blkbits - 9);
+				sbio.bi_end_io = on_stack_page_end_io;
+				bio_add_page(&sbio, page, PAGE_SIZE, 0);
+				bio_set_op_attrs(&sbio, REQ_OP_WRITE, op_flags);
+				WARN_ON_ONCE(PageWriteback(page));
+				set_page_writeback(page);
+				unlock_page(page);
+				submit_bio(&sbio);
+				clean_buffers(page, first_unmapped);
+			}
+
 			if (!bdev_write_page(bdev, blocks[0] << (blkbits - 9),
 								page, wbc)) {
 				clean_buffers(page, first_unmapped);
-- 
2.7.4

[toc] | [next] | [standalone]

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

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);
 	return bio;
 
 confused:

[toc] | [prev] | [next] | [standalone]

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

On Tue, Aug 08, 2017 at 05:49:59AM -0700, Matthew Wilcox wrote:
> +	struct bio sbio;
> +	struct bio_vec sbvec;

... this needs to be sbvec[nr_pages], of course.

> -		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);

... and this needs to be 'sbvec', not '&sbvec'.

[toc] | [prev] | [next] | [standalone]

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

On Tue, Aug 08, 2017 at 06:29:04AM -0700, Matthew Wilcox wrote:
> On Tue, Aug 08, 2017 at 05:49:59AM -0700, Matthew Wilcox wrote:
> > +	struct bio sbio;
> > +	struct bio_vec sbvec;
> 
> ... this needs to be sbvec[nr_pages], of course.
> 
> > -		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);
> 
> ... and this needs to be 'sbvec', not '&sbvec'.

I don't get it why we need sbvec[nr_pages].
On-stack-bio works with per-page.
May I miss something?

[toc] | [prev] | [next] | [standalone]

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

On Tue, Aug 08, 2017 at 07:31:22PM -0700, Matthew Wilcox wrote:
> On Wed, Aug 09, 2017 at 10:51:13AM +0900, Minchan Kim wrote:
> > On Tue, Aug 08, 2017 at 06:29:04AM -0700, Matthew Wilcox wrote:
> > > On Tue, Aug 08, 2017 at 05:49:59AM -0700, Matthew Wilcox wrote:
> > > > +	struct bio sbio;
> > > > +	struct bio_vec sbvec;
> > > 
> > > ... this needs to be sbvec[nr_pages], of course.
> > > 
> > > > -		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);
> > > 
> > > ... and this needs to be 'sbvec', not '&sbvec'.
> > 
> > I don't get it why we need sbvec[nr_pages].
> > On-stack-bio works with per-page.
> > May I miss something?
> 
> The way I redid it, it will work with an arbitrary number of pages.

IIUC, it would be good things with dynamic bio alloction with passing
allocated bio back and forth but on-stack bio cannot work like that.
It should be done in per-page so it is worth?

[toc] | [prev] | [next] | [standalone]

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

On Wed, Aug 09, 2017 at 11:41:50AM +0900, Minchan Kim wrote:
> On Tue, Aug 08, 2017 at 07:31:22PM -0700, Matthew Wilcox wrote:
> > On Wed, Aug 09, 2017 at 10:51:13AM +0900, Minchan Kim wrote:
> > > On Tue, Aug 08, 2017 at 06:29:04AM -0700, Matthew Wilcox wrote:
> > > > On Tue, Aug 08, 2017 at 05:49:59AM -0700, Matthew Wilcox wrote:
> > > > > +	struct bio sbio;
> > > > > +	struct bio_vec sbvec;
> > > > 
> > > > ... this needs to be sbvec[nr_pages], of course.
> > > > 
> > > > > -		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);
> > > > 
> > > > ... and this needs to be 'sbvec', not '&sbvec'.
> > > 
> > > I don't get it why we need sbvec[nr_pages].
> > > On-stack-bio works with per-page.
> > > May I miss something?
> > 
> > The way I redid it, it will work with an arbitrary number of pages.
> 
> IIUC, it would be good things with dynamic bio alloction with passing
> allocated bio back and forth but on-stack bio cannot work like that.
> It should be done in per-page so it is worth?

I'm not passing the bio back and forth between do_mpage_readpage() and
its callers.  The version I sent allows for multiple pages in a single
on-stack bio (when called from mpage_readpages()).

[toc] | [prev] | [next] | [standalone]

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

On Wed, Aug 9, 2017 at 8:04 PM, Matthew Wilcox <willy@infradead.org> wrote:
> On Wed, Aug 09, 2017 at 11:41:50AM +0900, Minchan Kim wrote:
>> On Tue, Aug 08, 2017 at 07:31:22PM -0700, Matthew Wilcox wrote:
>> > On Wed, Aug 09, 2017 at 10:51:13AM +0900, Minchan Kim wrote:
>> > > On Tue, Aug 08, 2017 at 06:29:04AM -0700, Matthew Wilcox wrote:
>> > > > On Tue, Aug 08, 2017 at 05:49:59AM -0700, Matthew Wilcox wrote:
>> > > > > +     struct bio sbio;
>> > > > > +     struct bio_vec sbvec;
>> > > >
>> > > > ... this needs to be sbvec[nr_pages], of course.
>> > > >
>> > > > > -             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);
>> > > >
>> > > > ... and this needs to be 'sbvec', not '&sbvec'.
>> > >
>> > > I don't get it why we need sbvec[nr_pages].
>> > > On-stack-bio works with per-page.
>> > > May I miss something?
>> >
>> > The way I redid it, it will work with an arbitrary number of pages.
>>
>> IIUC, it would be good things with dynamic bio alloction with passing
>> allocated bio back and forth but on-stack bio cannot work like that.
>> It should be done in per-page so it is worth?
>
> I'm not passing the bio back and forth between do_mpage_readpage() and
> its callers.  The version I sent allows for multiple pages in a single
> on-stack bio (when called from mpage_readpages()).

I like it, but do you think we should switch to sbvec[<constant>] to
preclude pathological cases where nr_pages is large?

[toc] | [prev] | [next] | [standalone]

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

On Wed, Aug 09, 2017 at 08:06:24PM -0700, Dan Williams wrote:
> I like it, but do you think we should switch to sbvec[<constant>] to
> preclude pathological cases where nr_pages is large?

Yes, please.

Then I'd like to see that the on-stack bio even matters for
mpage_readpage / mpage_writepage.  Compared to all the buffer head
overhead the bio allocation should not actually matter in practice.

[toc] | [prev] | [next] | [standalone]

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

On 08/11/2017 04:46 AM, Christoph Hellwig wrote:
> On Wed, Aug 09, 2017 at 08:06:24PM -0700, Dan Williams wrote:
>> I like it, but do you think we should switch to sbvec[<constant>] to
>> preclude pathological cases where nr_pages is large?
> 
> Yes, please.
> 
> Then I'd like to see that the on-stack bio even matters for
> mpage_readpage / mpage_writepage.  Compared to all the buffer head
> overhead the bio allocation should not actually matter in practice.

I'm skeptical for that path, too. I also wonder how far we could go
with just doing a per-cpu bio recycling facility, to reduce the cost
of having to allocate a bio. The on-stack bio parts are fine for
simple use case, where simple means that the patch just special
cases the allocation, and doesn't have to change much else.

I had a patch for bio recycling and batched freeing a year or two
ago, I'll see if I can find and resurrect it.

-- 
Jens Axboe

[toc] | [prev] | [next] | [standalone]

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

On Wed, Aug 09, 2017 at 08:04:33PM -0700, Matthew Wilcox wrote:
> On Wed, Aug 09, 2017 at 11:41:50AM +0900, Minchan Kim wrote:
> > On Tue, Aug 08, 2017 at 07:31:22PM -0700, Matthew Wilcox wrote:
> > > On Wed, Aug 09, 2017 at 10:51:13AM +0900, Minchan Kim wrote:
> > > > On Tue, Aug 08, 2017 at 06:29:04AM -0700, Matthew Wilcox wrote:
> > > > > On Tue, Aug 08, 2017 at 05:49:59AM -0700, Matthew Wilcox wrote:
> > > > > > +	struct bio sbio;
> > > > > > +	struct bio_vec sbvec;
> > > > > 
> > > > > ... this needs to be sbvec[nr_pages], of course.
> > > > > 
> > > > > > -		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);
> > > > > 
> > > > > ... and this needs to be 'sbvec', not '&sbvec'.
> > > > 
> > > > I don't get it why we need sbvec[nr_pages].
> > > > On-stack-bio works with per-page.
> > > > May I miss something?
> > > 
> > > The way I redid it, it will work with an arbitrary number of pages.
> > 
> > IIUC, it would be good things with dynamic bio alloction with passing
> > allocated bio back and forth but on-stack bio cannot work like that.
> > It should be done in per-page so it is worth?
> 
> I'm not passing the bio back and forth between do_mpage_readpage() and
> its callers.  The version I sent allows for multiple pages in a single
> on-stack bio (when called from mpage_readpages()).

I'm confused. I want to confirm your thought before respinning.
Please correct me if I miss something.

The version you sent to me used on-stack bio within do_mpage_readpage
so that's why I said sbvec[nr_pages] would be pointless because it
works with per-page base unless if we use dynamic bio allocation.

But I guess now you suggest to use on-stack bio in mpage_readpages so
single on-stack bio in mpage_readpages's stack can batch multiple pages
in bvecs of a bio.

Right?

[toc] | [prev] | [next] | [standalone]

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

On Wed, Aug 09, 2017 at 10:51:13AM +0900, Minchan Kim wrote:
> On Tue, Aug 08, 2017 at 06:29:04AM -0700, Matthew Wilcox wrote:
> > On Tue, Aug 08, 2017 at 05:49:59AM -0700, Matthew Wilcox wrote:
> > > +	struct bio sbio;
> > > +	struct bio_vec sbvec;
> > 
> > ... this needs to be sbvec[nr_pages], of course.
> > 
> > > -		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);
> > 
> > ... and this needs to be 'sbvec', not '&sbvec'.
> 
> I don't get it why we need sbvec[nr_pages].
> On-stack-bio works with per-page.
> May I miss something?

The way I redid it, it will work with an arbitrary number of pages.

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web