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).
