On Thu, 2007-05-17 at 06:47 +0800, Fengguang Wu wrote:
> /*
> * Track a single file's readahead state
> + *
> + * ================#============|==================#==================|
> + * ^ ^ ^ ^
> + * file_ra_state.la_index .ra_index .lookahead_index .readahead_index
> */
> struct file_ra_state {
> unsigned long start; /* Current window */
> @@ -711,6 +715,12 @@ struct file_ra_state {
> unsigned long prev_index; /* Cache last read() position */
> unsigned long ahead_start; /* Ahead window */
> unsigned long ahead_size;
> +
> + pgoff_t la_index; /* enqueue time */
> + pgoff_t ra_index; /* begin offset */
> + pgoff_t lookahead_index; /* time to do next readahead */
> + pgoff_t readahead_index; /* end offset */
> +
Hi Fengguang,
I found these variables a little confusing. la_index is the last offset
passed to ondemand_readahead, so perhaps "last_request_start" is a
better name? The comment "enqueue time" seems strange, too.
ra_index seems ok, although "readahead_start" might be better. Perhaps
readahead_index should be expressed as readahead_size, which is how it
seems to be used. Perhaps "lookahead_index" is best expressed as a
buffer at the end of the readahead zone (readahead_min?).
ie:
pgoff_t last_request_start; /* start of req which triggered
readahead */
pgoff_t readahead_start; /* Where readahead started */
pgoff_t readahead_size; /* PAGE_CACHE_SIZE units of readahead */
pgoff_t readahead_min; /* readahead_size left before we recalc
*/
This gets rid of many of the accessors, I think, and avoids introducing
a new term to understand (lookahead).
> +/*
> + * Where is the old read-ahead and look-ahead?
> + */
> +static inline void ra_set_index(struct file_ra_state *ra,
> + pgoff_t la_index, pgoff_t ra_index)
> +{
> + ra->la_index = la_index;
> + ra->ra_index = ra_index;
> +}
> +
> +/*
> + * Where is the new read-ahead and look-ahead?
> + */
> +static inline void ra_set_size(struct file_ra_state *ra,
> + unsigned long ra_size, unsigned long la_size)
> +{
> + ra->readahead_index = ra->ra_index + ra_size;
> + ra->lookahead_index = ra->ra_index + ra_size - la_size;
> +}
These are only called in one place, so I think it's clearer to do this
there directly. But I see you exported ra_submit, too, even though it's
only used in the same file. Are there plans for other users?
Thanks,
Rusty.
-
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/
