Re: [PATCH 15/33] readahead: state based method - routines

[Andrew Morton <akpm@osdl.org>]

Wu Fengguang <wfg@mail.ustc.edu.cn> wrote:
>
> Define some helpers on struct file_ra_state.
> 
> +/*
> + * The 64bit cache_hits stores three accumulated values and a counter value.
> + * MSB                                                                   LSB
> + * 3333333333333333 : 2222222222222222 : 1111111111111111 : 0000000000000000
> + */
> +static int ra_cache_hit(struct file_ra_state *ra, int nr)
> +{
> +	return (ra->cache_hits >> (nr * 16)) & 0xFFFF;
> +}

So...   why not use four u16s?

> +/*
> + * Submit IO for the read-ahead request in file_ra_state.
> + */
> +static int ra_dispatch(struct file_ra_state *ra,
> +			struct address_space *mapping, struct file *filp)
> +{
> +	enum ra_class ra_class = ra_class_new(ra);
> +	unsigned long ra_size = ra_readahead_size(ra);
> +	unsigned long la_size = ra_lookahead_size(ra);
> +	pgoff_t eof_index = PAGES_BYTE(i_size_read(mapping->host)) + 1;

Sigh.  I guess one gets used to that PAGES_BYTE thing after a while.  If
you're not familiar with it, it obfuscates things.

<hunts around for its definition>

So in fact it's converting a loff_t to a pgoff_t.  Why not call it
convert_loff_t_to_pgoff_t()?  ;)

Something better, anyway.  Something lower-case and an inline-not-a-macro, too.

> +	int actual;
> +
> +	if (unlikely(ra->ra_index >= eof_index))
> +		return 0;
> +
> +	/* Snap to EOF. */
> +	if (ra->readahead_index + ra_size / 2 > eof_index) {

You've had a bit of a think and you've arrived at a design decision
surrounding the arithmetic in here.  It's very very hard to look at this line
of code and to work out why you decided to implement it in this fashion. 
The only way to make such code comprehensible (and hence maintainable) is
to fully comment such things.