Re: [PATCH 5/9] readahead: on-demand readahead logic

[Fengguang Wu <wfg@mail.ustc.edu.cn>]

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