Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote:
> > I've been looking at it a long time :-), I'll look more
> > tomorrow... do you see anything obvious?
>
> Yes; Dave's sample code doesn't consume the pages from the readahead
> iterator, so the core code thinks you didn't consume them and unlocks
> / puts the pages for you. That goes wrong, because you did actually
> consume them. Glad I added the assertions now!
Yeah... The cleanup function that I posted potentially happens asynchronously
from the ->readahead function, so the ractl consumption has to be done
elsewhere and may already have happened. In the case of the code I posted
from, it's actually done in the netfs lib that's in the works:
void netfs_readahead(...)
{
...
/* Drop the refs on the pages here rather than in the cache or
* filesystem. The locks will be dropped in netfs_rreq_unlock().
*/
while ((page = readahead_page(ractl)))
put_page(page);
...
}
> We should probably add something like:
>
> static inline void readahead_consume(struct readahead_control *ractl,
> unsigned int nr)
> {
> ractl->_nr_pages -= nr;
> ractl->_index += nr;
> }
>
> to indicate that you consumed the pages other than by calling
> readahead_page() or readahead_page_batch(). Or maybe Dave can
> wrap iov_iter_xarray() in a readahead_iter() macro or something
> that takes care of adjusting index & nr_pages for you.
I'm not sure either is useful for my case since iov_iter_xarray() for me isn't
being used anywhere that there's an ractl and I still have to drop the page
refs.
However, in Mike's orangefs_readahead_cleanup(), he could replace:
rcu_read_lock();
xas_for_each(&xas, page, last) {
page_endio(page, false, 0);
put_page(page);
}
rcu_read_unlock();
with:
while ((page = readahead_page(ractl))) {
page_endio(page, false, 0);
put_page(page);
}
maybe?
David
