From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf0-f196.google.com ([209.85.192.196]:42268 "EHLO mail-pf0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S967502AbeE2Wh4 (ORCPT ); Tue, 29 May 2018 18:37:56 -0400 Received: by mail-pf0-f196.google.com with SMTP id p14-v6so7966539pfh.9 for ; Tue, 29 May 2018 15:37:56 -0700 (PDT) Subject: Re: [PATCH 1/3] mpage: mpage_readpages() should submit IO as read-ahead From: Jens Axboe To: Andrew Morton Cc: linux-fsdevel@vger.kernel.org, viro@zeniv.linux.org.uk References: <1527177774-21414-1-git-send-email-axboe@kernel.dk> <1527177774-21414-2-git-send-email-axboe@kernel.dk> <20180524124306.1d8833f06366fcad29506182@linux-foundation.org> <8cdea5ed-c3ef-f4f3-edea-208ebc9b45ca@kernel.dk> <20180529145935.a4f6d2487eb2c444ca8cc595@linux-foundation.org> <5fd6c066-927d-0c3f-aaae-2180fef4f1e5@kernel.dk> Message-ID: <126e709d-d9e0-59ff-fd21-aa4d36fd99cc@kernel.dk> Date: Tue, 29 May 2018 16:37:52 -0600 MIME-Version: 1.0 In-Reply-To: <5fd6c066-927d-0c3f-aaae-2180fef4f1e5@kernel.dk> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On 5/29/18 4:18 PM, Jens Axboe wrote: > On 5/29/18 3:59 PM, Andrew Morton wrote: >> On Tue, 29 May 2018 08:36:41 -0600 Jens Axboe wrote: >> >>> On 5/24/18 1:43 PM, Andrew Morton wrote: >>>> On Thu, 24 May 2018 10:02:52 -0600 Jens Axboe 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 >>>>> --- >>>>> 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. We could actually kill args->gfp as well, since that's dependent on args->is_readahead anyway. Separate patch, or fold into patch #2? Incremental below. diff --git a/fs/mpage.c b/fs/mpage.c index a6344996f924..b0f9de977526 100644 --- a/fs/mpage.c +++ b/fs/mpage.c @@ -137,12 +137,11 @@ struct mpage_readpage_args { struct bio *bio; struct page *page; unsigned nr_pages; + bool is_readahead; sector_t last_block_in_bio; struct buffer_head map_bh; unsigned long first_logical_block; get_block_t *get_block; - gfp_t gfp; - bool is_readahead; }; /* @@ -171,9 +170,18 @@ static struct bio *do_mpage_readpage(struct mpage_readpage_args *args) struct block_device *bdev = NULL; int length; int fully_mapped = 1; - int op_flags = args->is_readahead ? REQ_RAHEAD : 0; + int op_flags; unsigned nblocks; unsigned relative_block; + gfp_t gfp; + + if (args->is_readahead) { + op_flags = REQ_RAHEAD; + gfp = readahead_gfp_mask(page->mapping); + } else { + op_flags = 0; + gfp = mapping_gfp_constraint(page->mapping, GFP_KERNEL); + } if (page_has_buffers(page)) goto confused; @@ -295,7 +303,7 @@ static struct bio *do_mpage_readpage(struct mpage_readpage_args *args) goto out; } args->bio = mpage_alloc(bdev, blocks[0] << (blkbits - 9), - min_t(int, args->nr_pages, BIO_MAX_PAGES), args->gfp); + min_t(int, args->nr_pages, BIO_MAX_PAGES), gfp); if (args->bio == NULL) goto confused; } @@ -376,7 +384,6 @@ mpage_readpages(struct address_space *mapping, struct list_head *pages, { struct mpage_readpage_args args = { .get_block = get_block, - .gfp = readahead_gfp_mask(mapping), .is_readahead = true, }; unsigned page_idx; @@ -388,7 +395,7 @@ mpage_readpages(struct address_space *mapping, struct list_head *pages, list_del(&page->lru); if (!add_to_page_cache_lru(page, mapping, page->index, - args.gfp)) { + readahead_gfp_mask(mapping))) { args.page = page; args.nr_pages = nr_pages - page_idx; args.bio = do_mpage_readpage(&args); @@ -411,7 +418,6 @@ int mpage_readpage(struct page *page, get_block_t get_block) .page = page, .nr_pages = 1, .get_block = get_block, - .gfp = mapping_gfp_constraint(page->mapping, GFP_KERNEL), }; args.bio = do_mpage_readpage(&args); -- Jens Axboe