From: Matthew Wilcox <willy@infradead.org>
To: Christoph Hellwig <hch@lst.de>
Cc: linux-fsdevel@vger.kernel.org, linux-mm@kvack.org,
kent.overstreet@gmail.com
Subject: Re: [PATCH 11/17] mm/filemap: Add filemap_range_uptodate
Date: Tue, 3 Nov 2020 15:18:47 +0000 [thread overview]
Message-ID: <20201103151847.GZ27442@casper.infradead.org> (raw)
In-Reply-To: <20201103074944.GK8389@lst.de>
On Tue, Nov 03, 2020 at 08:49:44AM +0100, Christoph Hellwig wrote:
> On Mon, Nov 02, 2020 at 06:43:06PM +0000, Matthew Wilcox (Oracle) wrote:
> > Move the complicated condition and the calculations out of
> > filemap_update_page() into its own function.
>
> The logic looks good, but the flow inside of filemap_update_page looks
> odd. The patch below relative to this commit shows how I'd do it:
I have a simplification in mind that gets rid of the awkward 'first'
parameter. In filemap_get_pages(), do:
if ((iocb->ki_flags & IOCB_WAITQ) && (pagevec_count(pvec) > 1))
iocb->ki_flags |= IOCB_NOWAIT;
before calling filemap_update_page(). That matches what Kent did in
filemap_read():
if ((iocb->ki_flags & IOCB_WAITQ) && already_read)
iocb->ki_flags |= IOCB_NOWAIT;
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 81b569d818a3f8..304180c022d38a 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -2266,40 +2266,39 @@ static int filemap_update_page(struct kiocb *iocb,
> if (!trylock_page(page)) {
> if (iocb->ki_flags & (IOCB_NOWAIT | IOCB_NOIO))
> goto error;
> - if (iocb->ki_flags & IOCB_WAITQ) {
> - if (!first)
> - goto error;
> - error = __lock_page_async(page, iocb->ki_waitq);
> - if (error)
> - goto error;
> - } else {
> + if (!(iocb->ki_flags & IOCB_WAITQ)) {
> put_and_wait_on_page_locked(page, TASK_KILLABLE);
> return AOP_TRUNCATED_PAGE;
> }
> + if (!first)
> + goto error;
> + error = __lock_page_async(page, iocb->ki_waitq);
> + if (error)
> + goto error;
> }
I see where you're going there. OK.
>
> + error = AOP_TRUNCATED_PAGE;
> if (!page->mapping)
> - goto truncated;
> - if (filemap_range_uptodate(iocb, mapping, iter, page)) {
> - unlock_page(page);
> - return 0;
> - }
> + goto error_unlock_page;
>
> - if (iocb->ki_flags & (IOCB_NOIO | IOCB_NOWAIT | IOCB_WAITQ)) {
> - unlock_page(page);
> + if (!filemap_range_uptodate(iocb, mapping, iter, page)) {
> error = -EAGAIN;
> - } else {
> + if (iocb->ki_flags & (IOCB_NOIO | IOCB_NOWAIT | IOCB_WAITQ))
> + goto error_unlock_page;
> error = filemap_read_page(iocb->ki_filp, mapping, page);
> - if (!error)
> - return 0;
> + if (error)
> + goto error;
> + return 0; /* filemap_read_page unlocks the page */
> }
> +
> + unlock_page(page);
> + return 0;
> +
> +error_unlock_page:
> + unlock_page(page);
> error:
> put_page(page);
> return error;
> -truncated:
> - unlock_page(page);
> - put_page(page);
> - return AOP_TRUNCATED_PAGE;
> }
There's something niggling at me about this change ... I'll play with
it a bit.
next prev parent reply other threads:[~2020-11-03 15:18 UTC|newest]
Thread overview: 65+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-02 18:42 [PATCH 00/17] Refactor generic_file_buffered_read Matthew Wilcox (Oracle)
2020-11-02 18:42 ` [PATCH 01/17] mm/filemap: Rename generic_file_buffered_read subfunctions Matthew Wilcox (Oracle)
2020-11-02 18:53 ` Kent Overstreet
2020-11-03 7:27 ` Christoph Hellwig
2020-11-03 14:52 ` Matthew Wilcox
2020-11-03 15:02 ` Amy Parker
2020-11-02 18:42 ` [PATCH 02/17] mm/filemap: Use THPs in generic_file_buffered_read Matthew Wilcox (Oracle)
2020-11-02 18:55 ` Kent Overstreet
2020-11-03 7:28 ` Christoph Hellwig
2020-11-02 18:42 ` [PATCH 03/17] mm/filemap: Pass a sleep state to put_and_wait_on_page_locked Matthew Wilcox (Oracle)
2020-11-02 18:56 ` Kent Overstreet
2020-11-03 7:28 ` Christoph Hellwig
2020-11-02 18:42 ` [PATCH 04/17] mm/filemap: Support readpage splitting a page Matthew Wilcox (Oracle)
2020-11-02 19:00 ` Kent Overstreet
2020-11-02 19:03 ` Matthew Wilcox
2020-11-02 19:12 ` Kent Overstreet
2020-11-03 7:31 ` Christoph Hellwig
2020-11-02 18:43 ` [PATCH 05/17] mm/filemap: Inline __wait_on_page_locked_async into caller Matthew Wilcox (Oracle)
2020-11-02 19:00 ` Kent Overstreet
2020-11-03 7:31 ` Christoph Hellwig
2020-11-02 18:43 ` [PATCH 06/17] mm/filemap: Don't call ->readpage if IOCB_WAITQ is set Matthew Wilcox (Oracle)
2020-11-02 19:17 ` Kent Overstreet
2020-11-03 7:31 ` Christoph Hellwig
2020-11-05 7:22 ` Nikolay Borisov
2020-11-02 18:43 ` [PATCH 07/17] mm/filemap: Change filemap_read_page calling conventions Matthew Wilcox (Oracle)
2020-11-02 19:37 ` Kent Overstreet
2020-11-03 7:34 ` Christoph Hellwig
2020-11-03 15:11 ` Matthew Wilcox
2020-11-02 18:43 ` [PATCH 08/17] mm/filemap: Change filemap_create_page arguments Matthew Wilcox (Oracle)
2020-11-02 19:38 ` Kent Overstreet
2020-11-03 7:35 ` Christoph Hellwig
2020-11-02 18:43 ` [PATCH 09/17] mm/filemap: Convert filemap_update_page to return an errno Matthew Wilcox (Oracle)
2020-11-02 19:39 ` Kent Overstreet
2020-11-03 7:36 ` Christoph Hellwig
2020-11-02 18:43 ` [PATCH 10/17] mm/filemap: Move the iocb checks into filemap_update_page Matthew Wilcox (Oracle)
2020-11-02 19:45 ` Kent Overstreet
2020-11-03 7:41 ` Christoph Hellwig
2020-11-02 18:43 ` [PATCH 11/17] mm/filemap: Add filemap_range_uptodate Matthew Wilcox (Oracle)
2020-11-02 19:50 ` Kent Overstreet
2020-11-02 20:09 ` Matthew Wilcox
2020-11-03 7:49 ` Christoph Hellwig
2020-11-03 15:18 ` Matthew Wilcox [this message]
2020-11-03 15:31 ` Christoph Hellwig
2020-11-03 15:42 ` Matthew Wilcox
2020-11-02 18:43 ` [PATCH 12/17] mm/filemap: Split filemap_readahead out of filemap_get_pages Matthew Wilcox (Oracle)
2020-11-02 19:52 ` Kent Overstreet
2020-11-02 18:43 ` [PATCH 13/17] mm/filemap: Remove parameters from filemap_update_page() Matthew Wilcox (Oracle)
2020-11-02 19:52 ` Kent Overstreet
2020-11-03 7:50 ` Christoph Hellwig
2020-11-02 18:43 ` [PATCH 14/17] mm/filemap: Restructure filemap_get_pages Matthew Wilcox (Oracle)
2020-11-02 20:05 ` Kent Overstreet
2020-11-03 7:57 ` Christoph Hellwig
2020-11-03 14:46 ` Matthew Wilcox
2020-11-03 15:29 ` Christoph Hellwig
2020-11-02 18:43 ` [PATCH 15/17] mm/filemap: Don't relock the page after calling readpage Matthew Wilcox (Oracle)
2020-11-02 20:06 ` Kent Overstreet
2020-11-03 8:00 ` Christoph Hellwig
2020-11-03 15:24 ` Matthew Wilcox
2020-11-03 17:13 ` Christoph Hellwig
2020-11-03 18:55 ` Matthew Wilcox
2020-11-02 18:43 ` [PATCH 16/17] mm/filemap: rename generic_file_buffered_read to filemap_read Matthew Wilcox (Oracle)
2020-11-02 20:06 ` Kent Overstreet
2020-11-02 18:43 ` [PATCH 17/17] mm: simplify generic_file_read_iter Matthew Wilcox (Oracle)
2020-11-02 20:07 ` Kent Overstreet
2020-11-02 20:14 ` [PATCH 00/17] Refactor generic_file_buffered_read Kent Overstreet
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201103151847.GZ27442@casper.infradead.org \
--to=willy@infradead.org \
--cc=hch@lst.de \
--cc=kent.overstreet@gmail.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-mm@kvack.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).