On Tue, Jan 14, 2020 at 11:16:28PM -0800, Christoph Hellwig wrote:
> On Tue, Jan 14, 2020 at 06:38:40PM -0800, Matthew Wilcox wrote:
> > static loff_t
> > +iomap_readahead_actor(struct inode *inode, loff_t pos, loff_t length,
> > void *data, struct iomap *iomap, struct iomap *srcmap)
> > {
> > struct iomap_readpage_ctx *ctx = data;
> > @@ -410,10 +381,8 @@ iomap_readpages_actor(struct inode *inode, loff_t pos, loff_t length,
> > ctx->cur_page = NULL;
> > }
> > if (!ctx->cur_page) {
> > - ctx->cur_page = iomap_next_page(inode, ctx->pages,
> > - pos, length, &done);
> > - if (!ctx->cur_page)
> > - break;
> > + ctx->cur_page = readahead_page(inode->i_mapping,
> > + pos / PAGE_SIZE);
>
> Don't we at least need a sanity check for a NULL cur_page here?
I don't think so. The caller has already put the locked page into the
page cache at that index. If the page has gone away, that's a bug, and
I don't think BUG_ON is all that much better than a NULL pointer derefence.
Indeed, readahead_page() checks PageLocked, so it can't return NULL.
> Also the readahead_page version in your previous patch seems to expect
> a byte offset, so the division above would not be required.
Oops. I had intended to make readahead_pages() look like this:
struct page *readahead_page(struct address_space *mapping, pgoff_t index)
{
struct page *page = xa_load(&mapping->i_pages, index);
VM_BUG_ON_PAGE(!PageLocked(page), page);
return page;
}
If only our tools could warn about these kinds of mistakes.
> (and should
> probably be replaced with a right shift anyway no matter where it ends
> up)
If the compiler can't tell that x / 4096 and x >> 12 are precisely the same
and choose the more efficient of the two, we have big problems.
> > +unsigned
> > +iomap_readahead(struct address_space *mapping, pgoff_t start,
> > unsigned nr_pages, const struct iomap_ops *ops)
> > {
> > struct iomap_readpage_ctx ctx = {
> > - .pages = pages,
> > .is_readahead = true,
> > };
> > - 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;
> > + loff_t pos = start * PAGE_SIZE;
> > + loff_t length = nr_pages * PAGE_SIZE;
>
> Any good reason not to pass byte offsets for start and length?
>
> > + return length / PAGE_SIZE;
>
> Same for the return value?
>
> For the file systems that would usually be a more natural interface than
> a page index and number of pages.
That seems to depend on the filesystem. iomap definitely would be happier
with loff_t, but cifs prefers pgoff_t. I should probably survey a few
more filesystems and see if there's a strong lean in one direction or
the other.
