linux-kernel - Re: [PATCH 4/9] readahead: data structure and routines

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <1181619050.6237.23.camel@localhost.localdomain>
Date:	Tue, 12 Jun 2007 13:30:50 +1000
From:	Rusty Russell <rusty@...tcorp.com.au>
To:	Fengguang Wu <wfg@...l.ustc.edu.cn>
Cc:	Andrew Morton <akpm@...l.org>, linux-kernel@...r.kernel.org,
	Andi Kleen <andi@...stfloor.org>,
	Jens Axboe <jens.axboe@...cle.com>,
	Oleg Nesterov <oleg@...sign.ru>,
	Steven Pratt <slpratt@...tin.ibm.com>,
	Ram Pai <linuxram@...ibm.com>
Subject: Re: [PATCH 4/9] readahead: data structure and routines

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 majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/