On Thu, May 31, 2018 at 09:45:57AM +1000, Dave Chinner wrote:
> sentence ends with a ".". :)
Ok. This was intended to point to the WARN_ON calls below, but a "."
is fine with me, too.
>
> > + WARN_ON_ONCE(pos != page_offset(page));
> > + WARN_ON_ONCE(plen != PAGE_SIZE);
> > +
> > + if (iomap->type != IOMAP_MAPPED || pos >= i_size_read(inode)) {
>
> In what situation do we get a read request completely beyond EOF?
> (comment, please!)
This is generally to cover a racing read beyond EOF. That being said
I'd have to look up if it can really happen for blocksize == pagesize.
All this becomes moot once small block size support is added, so I think
I'd rather skip the comment and research here for now.
> > + if (ctx.bio) {
> > + submit_bio(ctx.bio);
> > + WARN_ON_ONCE(!ctx.cur_page_in_bio);
> > + } else {
> > + WARN_ON_ONCE(ctx.cur_page_in_bio);
> > + unlock_page(page);
> > + }
> > + return 0;
>
> Hmmm. If we had an error from iomap_apply, shouldn't we be returning
> it here instead just throwing it away? some ->readpage callers
> appear to ignore the PageError() state on return but do expect
> errors to be returned.
Both mpage_readpage and block_read_full_page always return 0, so for
now I'd like to stay compatible to them. Might be worth a full audit
later.
> > + loff_t pos = page_offset(list_entry(pages->prev, struct page, lru));
> > + loff_t last = page_offset(list_entry(pages->next, struct page, lru));
> > + loff_t length = last - pos + PAGE_SIZE, ret = 0;
>
> Two lines, please.
I really like it that way, though..
> > +done:
> > + if (ctx.bio)
> > + submit_bio(ctx.bio);
> > + if (ctx.cur_page) {
> > + if (!ctx.cur_page_in_bio)
> > + unlock_page(ctx.cur_page);
> > + put_page(ctx.cur_page);
> > + }
> > + WARN_ON_ONCE(!ret && !list_empty(ctx.pages));
>
> What error condition is this warning about?
Not finishing all pages without an error. Which wasn't too hard to get
wrong given the arance readpages calling convention.
