On Tue, Jun 19, 2018 at 12:52:11PM -0400, Brian Foster wrote:
> > + /*
> > + * Move the caller beyond our range so that it keeps making progress.
> > + * For that we have to include any leading non-uptodate ranges, but
>
> Do you mean "leading uptodate ranges" here? E.g., pos is pushed forward
> past those ranges we don't have to read, so (pos - orig_pos) reflects
> the initial uptodate range while plen reflects the length we have to
> read..?
Yes.
> > +
> > + do {
>
> Kind of a nit, but this catches my eye and manages to confuse me every
> time I look at it. A comment along the lines of:
>
> /*
> * Pass in the block aligned start/end so we get back block
> * aligned/adjusted poff/plen and can compare with unaligned
> * from/to below.
> */
>
> ... would be nice here, IMO.
Fine with me.
> > + iomap_adjust_read_range(inode, iop, &block_start,
> > + block_end - block_start, &poff, &plen);
> > + if (plen == 0)
> > + break;
> > +
> > + if ((from > poff && from < poff + plen) ||
> > + (to > poff && to < poff + plen)) {
> > + status = iomap_read_page_sync(inode, block_start, page,
> > + poff, plen, from, to, iomap);
>
> After taking another look at the buffer head path, it does look like we
> have slightly different behavior here. IIUC, the former reads only the
> !uptodate blocks that fall along the from/to boundaries. Here, if say
> from = 1, to = PAGE_SIZE and the page is fully !uptodate, it looks like
> we'd read the entire page worth of blocks (assuming contiguous 512b
> blocks, for example). Intentional? Doesn't seem like a big deal, but
> could be worth a followup fix.
It wasn't actuall intentional, but I actually think it is the right thing
in then end, as it means we'll often do a single read instead of two
separate ones.
