Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 2 Nov 2021 12:28:06 +0000
From:   Matthew Wilcox <willy@infradead.org>
To:     Christoph Hellwig <hch@infradead.org>
Cc:     "Darrick J. Wong" <djwong@kernel.org>, linux-xfs@vger.kernel.org,
        linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-block@vger.kernel.org, Jens Axboe <axboe@kernel.dk>
Subject: Re: [PATCH 13/21] iomap: Convert readahead and readpage to use a
 folio
Message-ID: <YYEu1qj3yxF968HR@casper.infradead.org>
References: <20211101203929.954622-1-willy@infradead.org>
 <20211101203929.954622-14-willy@infradead.org>
 <YYDmz8olTe/Qr2ch@infradead.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YYDmz8olTe/Qr2ch@infradead.org>
Precedence: bulk

On Tue, Nov 02, 2021 at 12:20:47AM -0700, Christoph Hellwig wrote:
> On Mon, Nov 01, 2021 at 08:39:21PM +0000, Matthew Wilcox (Oracle) wrote:
> >  	for (done = 0; done < length; done += ret) {
> > -		if (ctx->cur_page && offset_in_page(iter->pos + done) == 0) {
> > -			if (!ctx->cur_page_in_bio)
> > -				unlock_page(ctx->cur_page);
> > -			put_page(ctx->cur_page);
> > -			ctx->cur_page = NULL;
> > +		if (ctx->cur_folio &&
> > +		    offset_in_folio(ctx->cur_folio, iter->pos + done) == 0) {
> > +			if (!ctx->cur_folio_in_bio)
> > +				folio_unlock(ctx->cur_folio);
> > +			ctx->cur_folio = NULL;
> 
> Where did the put_page here disappear to?

I'll put that explanation in the changelog:

Handle folios of arbitrary size instead of working in PAGE_SIZE units.
readahead_folio() puts the page for you, so this is not quite a mechanical
change.

---

The reason for making that change is that I messed up when introducing the
readahead() operation.  I followed the refcounting rule of ->readpages()
instead of the rule of ->readpage().  For a successful readahead, we have
two more atomic operations than necessary.  I want to fix that, and
this seems like a good opportunity to do it.  Once all filesystems are
converted to call readahead_folio(), we can remove the extra get_page()
and put_page().

I did put an explanation of that in commit 9bf70167e3c6, but it's not
reasonable to expect reviewers to remember that when reviewing changes
to their filesystem's readahead, so I'll be sure to mention it in any
future conversion's changelogs.

    mm/filemap: Add readahead_folio()

    The pointers stored in the page cache are folios, by definition.
    This change comes with a behaviour change -- callers of readahead_folio()
    are no longer required to put the page reference themselves.  This matches
    how readpage works, rather than matching how readpages used to work.