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

[Matthew Wilcox <willy@infradead.org>]

On Sun, Nov 01, 2020 at 12:52:17PM +0100, Christoph Hellwig wrote:
> 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

Hm?  I have this:

    mm/filemap: Rename generic_file_buffered_read subfunctions
    
    The recent split of generic_file_buffered_read() created some very
    long function names which are hard to distinguish from each other.
    Rename as follows:
    
    generic_file_buffered_read_readpage -> filemap_read_page
    generic_file_buffered_read_pagenotuptodate -> filemap_update_page
    generic_file_buffered_read_no_cached_page -> filemap_create_page
    generic_file_buffered_read_get_pages -> filemap_get_pages

> mm/filemap: Change calling convention for buffered read functions
> 
>  - please also drop the mapping argument to the various functions while
>    you're at it

Not sure I see the point to it.  Sure, they _can_ retrieve it with
iocb->ki_filp->f_mapping, but usually we like to pass the mapping
argument to functions which do something with the mapping.

> mm/filemap: Reduce indentation in gfbr_read_page
> 
>  - still mentionds the old function name

Fixed.

> 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;
> 		}
> 	}

No strong feeling here.  I'll change it.

> Also I think the messy list of uptodate checks could now be simplified
> down to:
> 
> 	if (!PageUptodate(page)) {
> 		if (inode->i_blkbits <= PAGE_SHIFT &&

I've been wondering about this test's usefulness in the presence
of THP.  Do we want to make it 'if (inode->i_blkbits < (thp_order(page)
+ PAGE_SHIFT)'?  It doesn't make sense to leave it as it is because then
a 1kB and 4kB blocksize filesystem will behave differently.

> 		    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;
> 		}

Now that you've rearranged it like this, it's obvious that the truncated
check is in the wrong place.  We don't want to call filemap_read_page()
if the page has been truncated either.

> 		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

A later patch hoists the put_page to the caller, so I think you'll like
where it ends up.

> 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

I'll take a look at that.

Looking at all this again, I think I want to pull the IOCB checks out
of filemap_read_page() and just pass a struct file to it.  It'll make
it more clear that NOIO, NOWAIT and WAITQ can't get to calling ->readpage.

I'll send another rev tomorrow.