Re: [PATCH v3 2/3] iomap: Don't create iomap_page objects for inline files

[Matthew Wilcox <willy@infradead.org>]

On Thu, Jul 08, 2021 at 09:27:37PM -0700, Darrick J. Wong wrote:
> On Wed, Jul 07, 2021 at 03:28:47PM +0100, Matthew Wilcox wrote:
> > On Wed, Jul 07, 2021 at 01:55:23PM +0200, Andreas Gruenbacher wrote:
> > > @@ -252,6 +253,7 @@ iomap_readpage_actor(struct inode *inode, loff_t pos, loff_t length, void *data,
> > >  	}
> > >  
> > >  	/* zero post-eof blocks as the page may be mapped */
> > > +	iop = iomap_page_create(inode, page);
> > >  	iomap_adjust_read_range(inode, iop, &pos, length, &poff, &plen);
> > >  	if (plen == 0)
> > >  		goto done;
> > 
> > I /think/ a subsequent patch would look like this:
> > 
> > +	/* No need to create an iop if the page is within an extent */
> > +	loff_t page_pos = page_offset(page);
> > +	if (pos > page_pos || pos + length < page_pos + page_size(page))
> > +		iop = iomap_page_create(inode, page);
> > 
> > but that might miss some other reasons to create an iop.
> 
> I was under the impression that for blksize<pagesize filesystems, the
> page always had to have an iop attached.  In principle I think you're
> right that we don't need one if all i_blocks_per_page blocks have the
> same uptodate state, but someone would have to perform a close reading
> of buffered-io.c to make it drop them when unnecessary and re-add them
> if it becomes necessary.  That might be more cycling through kmem_alloc
> than we like, but as I said, I have never studied this idea.

I wouldn't free them unnecessarily; that is, once we've determined that
we need an iop, we should just keep it, even once the entire page is
Uptodate (because we'll need it for write-out eventually anyway).

I haven't noticed any ill-effects from discarding iops while running
xfstests on the THP/multipage folio patches.  That will discard iops
when splitting a page (the page must be entirely uptodate at that point).