Re: [PATCH 04/13] mm: handle readahead in generic_file_buffered_read_pagenotuptodate

[Christoph Hellwig <hch@lst.de>]

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