Re: [PATCH 1/2] iomap: support tail packing inline read

[Gao Xiang <hsiangkao@linux.alibaba.com>]

On Fri, Jul 16, 2021 at 03:44:04PM +0100, Matthew Wilcox wrote:
> On Fri, Jul 16, 2021 at 09:56:23PM +0800, Gao Xiang wrote:
> > Hi Matthew,
> > 
> > On Fri, Jul 16, 2021 at 02:02:29PM +0100, Matthew Wilcox wrote:
> > > On Fri, Jul 16, 2021 at 01:07:23PM +0800, Gao Xiang wrote:
> > > > This tries to add tail packing inline read to iomap. Different from
> > > > the previous approach, it only marks the block range uptodate in the
> > > > page it covers.
> > > 
> > > Why?  This path is called under two circumstances: readahead and readpage.
> > > In both cases, we're trying to bring the entire page uptodate.  The inline
> > > extent is always the tail of the file, so we may as well zero the part of
> > > the page past the end of file and mark the entire page uptodate instead
> > > and leaving the end of the page !uptodate.
> > > 
> > > I see the case where, eg, we have the first 2048 bytes of the file
> > > out-of-inode and then 20 bytes in the inode.  So we'll create the iop
> > > for the head of the file, but then we may as well finish the entire
> > > PAGE_SIZE chunk as part of this iteration rather than update 2048-3071
> > > as being uptodate and leave the 3072-4095 block for a future iteration.
> > 
> > Thanks for your comments. Hmm... If I understand the words above correctly,
> > what I'd like to do is to cover the inline extents (blocks) only
> > reported by iomap_begin() rather than handling other (maybe)
> > logical-not-strictly-relevant areas such as post-EOF (even pages
> > will be finally entirely uptodated), I think such zeroed area should
> > be handled by from the point of view of the extent itself
> > 
> >          if (iomap_block_needs_zeroing(inode, iomap, pos)) {
> >                  zero_user(page, poff, plen);
> >                  iomap_set_range_uptodate(page, poff, plen);
> >                  goto done;
> >          }
> 
> That does work.  But we already mapped the page to write to it, and
> we already have to zero to the end of the block.  Why not zero to
> the end of the page?  It saves an iteration around the loop, it saves
> a mapping of the page, and it saves a call to flush_dcache_page().

I completely understand your concern, and that's also (sort of) why I
left iomap_read_inline_page() to make the old !pos behavior as before.

Anyway, I could update Christoph's patch to behave like what you
suggested. Will do later since I'm now taking some rest...

> 
> > The benefits I can think out are 1) it makes the logic understand
> > easier and no special cases just for tail-packing handling 2) it can
> > be then used for any inline extent cases (I mean e.g. in the middle of
> > the file) rather than just tail-packing inline blocks although currently
> > there is a BUG_ON to prevent this but it's easier to extend even further.
> > 3) it can be used as a part for later partial page uptodate logic in
> > order to match the legacy buffer_head logic (I remember something if my
> > memory is not broken about this...)
> 
> Hopefully the legacy buffer_head logic will go away soon.

Hmmm.. I partially agree on this (I agree buffer_head is a legacy stuff
but...), considering some big PAGE_SIZE like 64kb or bigger, partial
uptodate can save I/O for random file read pattern in general (not mmap
read, yes, also considering readahead, but I received some regression
due to I/O amplification like this when I was at the previous * 2 company).

Thanks,
Gao Xiang