| Message ID | 1527177774-21414-2-git-send-email-axboe@kernel.dk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 24 May 2018 10:02:52 -0600 Jens Axboe <axboe@kernel.dk> wrote: > a_ops->readpages() is only ever used for read-ahead, yet we don't > flag the IO being submitted as such. Fix that up. Any file system > that uses mpage_readpages() as it's ->readpages() implementation > will now get this right. > > Signed-off-by: Jens Axboe <axboe@kernel.dk> > --- > fs/mpage.c | 17 +++++++++-------- > 1 file changed, 9 insertions(+), 8 deletions(-) > > diff --git a/fs/mpage.c b/fs/mpage.c > index b7e7f570733a..0a5474237f5e 100644 > --- a/fs/mpage.c > +++ b/fs/mpage.c > @@ -146,7 +146,7 @@ static struct bio * > do_mpage_readpage(struct bio *bio, struct page *page, unsigned nr_pages, > sector_t *last_block_in_bio, struct buffer_head *map_bh, > unsigned long *first_logical_block, get_block_t get_block, > - gfp_t gfp) > + gfp_t gfp, bool is_readahead) That's a lot of arguments. I suspect we'll have a faster kernel if we mark this __always_inline. I think my ancient "This isn't called much at all" over mpage_readpage() remains true. Almost all callers come in via mpage_readpages(), which would benefit from the inlining. But mpage.o gets 1.5k fatter. hm.
On Thu, May 24, 2018 at 12:43:06PM -0700, Andrew Morton wrote: > On Thu, 24 May 2018 10:02:52 -0600 Jens Axboe <axboe@kernel.dk> wrote: > > do_mpage_readpage(struct bio *bio, struct page *page, unsigned nr_pages, > > sector_t *last_block_in_bio, struct buffer_head *map_bh, > > unsigned long *first_logical_block, get_block_t get_block, > > - gfp_t gfp) > > + gfp_t gfp, bool is_readahead) > > That's a lot of arguments. struct mpage_args args = { .bio = NULL, .first_logical_block = 0, .last_block_in_bio = 0, .is_readahead = true, .map_bh = { .b_state = 0; .b_size = 0; }, .get_block = get_block, .gfp = readahead_gfp_mask(mapping); }; ... do_mpage_readpages(&args, page, nr_pages - page_idx); better than inlining?
On 5/24/18 1:43 PM, Andrew Morton wrote: > On Thu, 24 May 2018 10:02:52 -0600 Jens Axboe <axboe@kernel.dk> wrote: > >> a_ops->readpages() is only ever used for read-ahead, yet we don't >> flag the IO being submitted as such. Fix that up. Any file system >> that uses mpage_readpages() as it's ->readpages() implementation >> will now get this right. >> >> Signed-off-by: Jens Axboe <axboe@kernel.dk> >> --- >> fs/mpage.c | 17 +++++++++-------- >> 1 file changed, 9 insertions(+), 8 deletions(-) >> >> diff --git a/fs/mpage.c b/fs/mpage.c >> index b7e7f570733a..0a5474237f5e 100644 >> --- a/fs/mpage.c >> +++ b/fs/mpage.c >> @@ -146,7 +146,7 @@ static struct bio * >> do_mpage_readpage(struct bio *bio, struct page *page, unsigned nr_pages, >> sector_t *last_block_in_bio, struct buffer_head *map_bh, >> unsigned long *first_logical_block, get_block_t get_block, >> - gfp_t gfp) >> + gfp_t gfp, bool is_readahead) > > That's a lot of arguments. > > I suspect we'll have a faster kernel if we mark this __always_inline. > I think my ancient "This isn't called much at all" over > mpage_readpage() remains true. Almost all callers come in via > mpage_readpages(), which would benefit from the inlining. But mpage.o > gets 1.5k fatter. hm. Was going to send out a v2, but would be nice to get some consensus on what you prefer here. I can either do the struct version, or I can keep it as-is (going from 8 to 9 arguments). For the struct version, I'd prefer to do that as a prep patch, so the functional change is clear.
On Tue, 29 May 2018 08:36:41 -0600 Jens Axboe <axboe@kernel.dk> wrote: > On 5/24/18 1:43 PM, Andrew Morton wrote: > > On Thu, 24 May 2018 10:02:52 -0600 Jens Axboe <axboe@kernel.dk> wrote: > > > >> a_ops->readpages() is only ever used for read-ahead, yet we don't > >> flag the IO being submitted as such. Fix that up. Any file system > >> that uses mpage_readpages() as it's ->readpages() implementation > >> will now get this right. > >> > >> Signed-off-by: Jens Axboe <axboe@kernel.dk> > >> --- > >> fs/mpage.c | 17 +++++++++-------- > >> 1 file changed, 9 insertions(+), 8 deletions(-) > >> > >> diff --git a/fs/mpage.c b/fs/mpage.c > >> index b7e7f570733a..0a5474237f5e 100644 > >> --- a/fs/mpage.c > >> +++ b/fs/mpage.c > >> @@ -146,7 +146,7 @@ static struct bio * > >> do_mpage_readpage(struct bio *bio, struct page *page, unsigned nr_pages, > >> sector_t *last_block_in_bio, struct buffer_head *map_bh, > >> unsigned long *first_logical_block, get_block_t get_block, > >> - gfp_t gfp) > >> + gfp_t gfp, bool is_readahead) > > > > That's a lot of arguments. > > > > I suspect we'll have a faster kernel if we mark this __always_inline. > > I think my ancient "This isn't called much at all" over > > mpage_readpage() remains true. Almost all callers come in via > > mpage_readpages(), which would benefit from the inlining. But mpage.o > > gets 1.5k fatter. hm. > > Was going to send out a v2, but would be nice to get some consensus on > what you prefer here. I can either do the struct version, or I can > keep it as-is (going from 8 to 9 arguments). For the struct version, > I'd prefer to do that as a prep patch, so the functional change is > clear. The struct thing makes the code smaller, and presumably faster, doesn't it? I suppose it saves a bit of stack as well, by letting the callee access the caller's locals rather than a copy of them. All sounds good to me?
On 5/29/18 3:59 PM, Andrew Morton wrote: > On Tue, 29 May 2018 08:36:41 -0600 Jens Axboe <axboe@kernel.dk> wrote: > >> On 5/24/18 1:43 PM, Andrew Morton wrote: >>> On Thu, 24 May 2018 10:02:52 -0600 Jens Axboe <axboe@kernel.dk> wrote: >>> >>>> a_ops->readpages() is only ever used for read-ahead, yet we don't >>>> flag the IO being submitted as such. Fix that up. Any file system >>>> that uses mpage_readpages() as it's ->readpages() implementation >>>> will now get this right. >>>> >>>> Signed-off-by: Jens Axboe <axboe@kernel.dk> >>>> --- >>>> fs/mpage.c | 17 +++++++++-------- >>>> 1 file changed, 9 insertions(+), 8 deletions(-) >>>> >>>> diff --git a/fs/mpage.c b/fs/mpage.c >>>> index b7e7f570733a..0a5474237f5e 100644 >>>> --- a/fs/mpage.c >>>> +++ b/fs/mpage.c >>>> @@ -146,7 +146,7 @@ static struct bio * >>>> do_mpage_readpage(struct bio *bio, struct page *page, unsigned nr_pages, >>>> sector_t *last_block_in_bio, struct buffer_head *map_bh, >>>> unsigned long *first_logical_block, get_block_t get_block, >>>> - gfp_t gfp) >>>> + gfp_t gfp, bool is_readahead) >>> >>> That's a lot of arguments. >>> >>> I suspect we'll have a faster kernel if we mark this __always_inline. >>> I think my ancient "This isn't called much at all" over >>> mpage_readpage() remains true. Almost all callers come in via >>> mpage_readpages(), which would benefit from the inlining. But mpage.o >>> gets 1.5k fatter. hm. >> >> Was going to send out a v2, but would be nice to get some consensus on >> what you prefer here. I can either do the struct version, or I can >> keep it as-is (going from 8 to 9 arguments). For the struct version, >> I'd prefer to do that as a prep patch, so the functional change is >> clear. > > The struct thing makes the code smaller, and presumably faster, doesn't > it? I suppose it saves a bit of stack as well, by letting the callee > access the caller's locals rather than a copy of them. All sounds good > to me? That's what I thought to, so already prepped the series. Sending it out.
diff --git a/fs/mpage.c b/fs/mpage.c index b7e7f570733a..0a5474237f5e 100644 --- a/fs/mpage.c +++ b/fs/mpage.c @@ -146,7 +146,7 @@ static struct bio * do_mpage_readpage(struct bio *bio, struct page *page, unsigned nr_pages, sector_t *last_block_in_bio, struct buffer_head *map_bh, unsigned long *first_logical_block, get_block_t get_block, - gfp_t gfp) + gfp_t gfp, bool is_readahead) { struct inode *inode = page->mapping->host; const unsigned blkbits = inode->i_blkbits; @@ -161,6 +161,7 @@ do_mpage_readpage(struct bio *bio, struct page *page, unsigned nr_pages, struct block_device *bdev = NULL; int length; int fully_mapped = 1; + int op_flags = is_readahead ? REQ_RAHEAD : 0; unsigned nblocks; unsigned relative_block; @@ -274,7 +275,7 @@ do_mpage_readpage(struct bio *bio, struct page *page, unsigned nr_pages, * This page will go to BIO. Do we need to send this BIO off first? */ if (bio && (*last_block_in_bio != blocks[0] - 1)) - bio = mpage_bio_submit(REQ_OP_READ, 0, bio); + bio = mpage_bio_submit(REQ_OP_READ, op_flags, bio); alloc_new: if (bio == NULL) { @@ -291,7 +292,7 @@ do_mpage_readpage(struct bio *bio, struct page *page, unsigned nr_pages, length = first_hole << blkbits; if (bio_add_page(bio, page, length, 0) < length) { - bio = mpage_bio_submit(REQ_OP_READ, 0, bio); + bio = mpage_bio_submit(REQ_OP_READ, op_flags, bio); goto alloc_new; } @@ -299,7 +300,7 @@ do_mpage_readpage(struct bio *bio, struct page *page, unsigned nr_pages, nblocks = map_bh->b_size >> blkbits; if ((buffer_boundary(map_bh) && relative_block == nblocks) || (first_hole != blocks_per_page)) - bio = mpage_bio_submit(REQ_OP_READ, 0, bio); + bio = mpage_bio_submit(REQ_OP_READ, op_flags, bio); else *last_block_in_bio = blocks[blocks_per_page - 1]; out: @@ -307,7 +308,7 @@ do_mpage_readpage(struct bio *bio, struct page *page, unsigned nr_pages, confused: if (bio) - bio = mpage_bio_submit(REQ_OP_READ, 0, bio); + bio = mpage_bio_submit(REQ_OP_READ, op_flags, bio); if (!PageUptodate(page)) block_read_full_page(page, get_block); else @@ -384,13 +385,13 @@ mpage_readpages(struct address_space *mapping, struct list_head *pages, nr_pages - page_idx, &last_block_in_bio, &map_bh, &first_logical_block, - get_block, gfp); + get_block, gfp, true); } put_page(page); } BUG_ON(!list_empty(pages)); if (bio) - mpage_bio_submit(REQ_OP_READ, 0, bio); + mpage_bio_submit(REQ_OP_READ, REQ_RAHEAD, bio); return 0; } EXPORT_SYMBOL(mpage_readpages); @@ -409,7 +410,7 @@ int mpage_readpage(struct page *page, get_block_t get_block) map_bh.b_state = 0; map_bh.b_size = 0; bio = do_mpage_readpage(bio, page, 1, &last_block_in_bio, - &map_bh, &first_logical_block, get_block, gfp); + &map_bh, &first_logical_block, get_block, gfp, false); if (bio) mpage_bio_submit(REQ_OP_READ, 0, bio); return 0;
a_ops->readpages() is only ever used for read-ahead, yet we don't flag the IO being submitted as such. Fix that up. Any file system that uses mpage_readpages() as it's ->readpages() implementation will now get this right. Signed-off-by: Jens Axboe <axboe@kernel.dk> --- fs/mpage.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-)