linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Christoph Hellwig <hch@lst.de>
To: Matthew Wilcox <willy@infradead.org>
Cc: Christoph Hellwig <hch@lst.de>,
	Andrew Morton <akpm@linux-foundation.org>,
	Kent Overstreet <kent.overstreet@gmail.com>,
	linux-mm@kvack.org, linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH 04/13] mm: handle readahead in generic_file_buffered_read_pagenotuptodate
Date: Sun, 1 Nov 2020 12:52:17 +0100	[thread overview]
Message-ID: <20201101115217.GA27488@lst.de> (raw)
In-Reply-To: <20201101110406.GV27442@casper.infradead.org>

On Sun, Nov 01, 2020 at 11:04:06AM +0000, Matthew Wilcox wrote:
> > I'll stop for now.
> 
> http://git.infradead.org/users/willy/pagecache.git/shortlog/refs/heads/next
> 
> is what i currently have.  Haven't pulled in everything from you; certainly not renaming generic_file_buffered_read to filemap_read(), which is awesome.
> i'm about 500 seconds into an xfstests run.

A bunch of comments from a very quick look:

mm/filemap: Rename generic_file_buffered_read subfunctions

 - the commit log still mentions the old names

mm/filemap: Change calling convention for buffered read functions

 - please also drop the mapping argument to the various functions while
   you're at it

mm/filemap: Reduce indentation in gfbr_read_page

 - still mentionds the old function name

mm/filemap: Support readpage splitting a page

 - nitpick, I find this a little hard to read:

+	} else if (!trylock_page(page)) {
+		put_and_wait_on_page_locked(page, TASK_KILLABLE);
+		return NULL;
 	}

and would write this a little more coarse as:

	} else {
		if (!trylock_page(page)) {
			put_and_wait_on_page_locked(page, TASK_KILLABLE);
			return NULL;
		}
	}

Also I think the messy list of uptodate checks could now be simplified
down to:

	if (!PageUptodate(page)) {
		if (inode->i_blkbits <= PAGE_SHIFT &&
		    mapping->a_ops->is_partially_uptodate &&
		    !iov_iter_is_pipe(iter)) {
			if (!page->mapping)
				goto truncated;
			if (mapping->a_ops->is_partially_uptodate(page,
					pos & (thp_size(page) - 1), count))
				goto uptodate;
		}

		return filemap_read_page(iocb, mapping, page);
	}

mm/filemap: Convert filemap_read_page to return an errno:

 - using a goto label for the put_page + return error case like in my
   patch would be cleaner I think

mm/filemap: Restructure filemap_get_pages

 - I have to say I still like my little helper here for
   the two mapping_get_read_thps calls plus page_cache_sync_readahead





  reply	other threads:[~2020-11-01 11:52 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-31  8:59 clean up the generic pagecache read helpers Christoph Hellwig
2020-10-31  8:59 ` [PATCH 01/13] mm: simplify generic_file_buffered_read_readpage Christoph Hellwig
2020-10-31  8:59 ` [PATCH 02/13] mm: simplify generic_file_buffered_read_pagenotuptodate Christoph Hellwig
2020-10-31  8:59 ` [PATCH 03/13] mm: lift the nowait checks into generic_file_buffered_read_pagenotuptodate Christoph Hellwig
2020-10-31  8:59 ` [PATCH 04/13] mm: handle readahead in generic_file_buffered_read_pagenotuptodate Christoph Hellwig
2020-10-31 17:06   ` Matthew Wilcox
2020-11-01 10:31     ` Christoph Hellwig
2020-11-01 10:49       ` Matthew Wilcox
2020-11-01 10:51         ` Christoph Hellwig
2020-11-01 10:51           ` Christoph Hellwig
2020-11-01 11:04             ` Matthew Wilcox
2020-11-01 11:52               ` Christoph Hellwig [this message]
2020-11-01 14:55                 ` Matthew Wilcox
2020-11-02  8:18                   ` Christoph Hellwig
2020-10-31  8:59 ` [PATCH 05/13] mm: simplify generic_file_buffered_read_no_cached_page Christoph Hellwig
2020-10-31 16:20   ` Matthew Wilcox
2020-10-31 16:28   ` Matthew Wilcox
2020-11-01 10:29     ` Christoph Hellwig
2020-10-31  8:59 ` [PATCH 06/13] mm: factor out a filemap_find_get_pages helper Christoph Hellwig
2020-10-31  8:59 ` [PATCH 07/13] mm: refactor generic_file_buffered_read_get_pages Christoph Hellwig
2020-11-01 11:18   ` Matthew Wilcox
2020-10-31  8:59 ` [PATCH 08/13] mm: move putting the page on error out of filemap_readpage Christoph Hellwig
2020-10-31  9:00 ` [PATCH 09/13] mm: move putting the page on error out of filemap_make_page_uptodate Christoph Hellwig
2020-10-31  9:00 ` [PATCH 10/13] mm: open code readahead in filemap_new_page Christoph Hellwig
2020-11-01 11:20   ` Matthew Wilcox
2020-10-31  9:00 ` [PATCH 11/13] mm: streamline the partially uptodate checks in filemap_make_page_uptodate Christoph Hellwig
2020-11-01 11:23   ` Matthew Wilcox
2020-10-31  9:00 ` [PATCH 12/13] mm: rename generic_file_buffered_read to filemap_read Christoph Hellwig
2020-10-31  9:00 ` [PATCH 13/13] mm: simplify generic_file_read_iter Christoph Hellwig
2020-10-31 15:42 ` clean up the generic pagecache read helpers Matthew Wilcox

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=20201101115217.GA27488@lst.de \
    --to=hch@lst.de \
    --cc=akpm@linux-foundation.org \
    --cc=kent.overstreet@gmail.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=willy@infradead.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).