On Wed, Jun 13, 2007 at 11:40:33AM +1000, Rusty Russell wrote:
> On Tue, 2007-06-12 at 18:35 +0800, Fengguang Wu wrote:
> > > This seems a little like two functions crammed into one. Do you think
> > > page_cache_readahead_ondemand() should be split into
> > > "page_cache_readahead()" which doesn't take a page*, and
> > > "page_cache_check_readahead_page()" which is an inline which does the
> > > PageReadahead(page) check as well?
> >
> > page_cache_check_readahead_page(..., page) is a good idea.
> > But which part of the code should we put to page_cache_readahead()
> > that does not take a page param?
>
> OK, here's my attempt. I also made them return void, since none of the
> callers seem to mind. I didn't change the internals much: I think
> they're pretty clear and I didn't want to clash if you decided to rename
> the "ra" fields. It compiles and boots.
>
> Thoughts?
Thank you, comments follow.
> +void page_cache_sync_readahead(struct address_space *mapping,
> + struct file_ra_state *ra,
> + struct file *filp,
> + pgoff_t offset,
> + unsigned long size);
> +
> +void page_cache_async_readahead(struct address_space *mapping,
> + struct file_ra_state *ra,
> + struct file *filp,
> + struct page *pg,
> + pgoff_t offset,
> + unsigned long size);
Got your idea: it looks like a nice split.
> +/* If page has PG_readahead flag set, call async readahead logic. */
> +static inline void
> +page_cache_check_readahead_page(struct address_space *mapping,
> + struct file_ra_state *ra,
> + struct file *filp,
> + struct page *pg,
> + pgoff_t offset,
> + unsigned long size)
> +{
> + if (!PageReadahead(pg))
> + return;
> + page_cache_async_readahead(mapping, ra, filp, pg, offset, size);
> +}
This function might not be necessary?
I guess the explicit code is clear enough.
> static unsigned long
> ondemand_readahead(struct address_space *mapping,
> struct file_ra_state *ra, struct file *filp,
> - struct page *page, pgoff_t offset,
> + bool hit_lookahead_marker, pgoff_t offset,
Or use names like async/is_async for hit_lookahead_marker?
Seems that you have accepted the 'lookahead' term ;)
> unsigned long req_size)
> {
> unsigned long max; /* max readahead pages */
> @@ -379,7 +379,7 @@ ondemand_readahead(struct address_space
> * Standalone, small read.
> * Read as is, and do not pollute the readahead state.
> */
> - if (!page && !sequential) {
> + if (!hit_lookahead_marker && !sequential) {
> return __do_page_cache_readahead(mapping, filp,
> offset, req_size, 0);
> }
> @@ -400,7 +400,7 @@ ondemand_readahead(struct address_space
> * E.g. interleaved reads.
> * Not knowing its readahead pos/size, bet on the minimal possible one.
> */
> - if (page) {
> + if (hit_lookahead_marker) {
> ra_index++;
> ra_size = min(4 * ra_size, max);
> }
> +/**
> + * page_cache_async_readahead - file readahead for marked pages
> + * @mapping: address_space which holds the pagecache and I/O vectors
> + * @ra: file_ra_state which holds the readahead state
> + * @filp: passed on to ->readpage() and ->readpages()
> + * @page: the page at @offset which has the PageReadahead flag set
^PG_readahead
> + * @offset: start offset into @mapping, in PAGE_CACHE_SIZE units
> + * @req_size: hint: total size of the read which the caller is performing in
> + * PAGE_CACHE_SIZE units
^in pagecache pages?
//Christoph is changing the fixed PAGE_CACHE_SIZE to variable ones.
> + *
> + * page_cache_async_ondemand() should be called when a page is used which
> + * has the PageReadahead flag: this is a marker to suggest that the
> application
^PG_readahead
> + * has used up enough of the readahead window that we should start pulling in
> + * more pages. */
> +void
> +page_cache_async_readahead(struct address_space *mapping,
> + struct file_ra_state *ra, struct file *filp,
> + struct page *page, pgoff_t offset,
> + unsigned long req_size)
> +{
> + /* no read-ahead */
> + if (!ra->ra_pages)
> + return;
> +
> + /*
> + * Same bit is used for PG_readahead and PG_reclaim.
I like this new comment!
> + */
> + if (PageWriteback(page))
> + return;
> +
> + ClearPageReadahead(page);
Thank you,
Fengguang
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
