On Fri, May 26, 2006 at 10:15:36AM -0700, Andrew Morton wrote:
> Wu Fengguang <[email protected]> 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?
Sure, me too, have been thinking about it ;-)
> > +/*
> > + * 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.
I'm now using DIV_ROUND_UP(), maybe we can settle with that.
> > + 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.
Sorry for being a bit lazy.
It is true that some situations are rather tricky, and some
if()/numbers are carefully chosen. I'll continue expanding/detailing
the documentation with future releases. Or would you prefer to add
them as small and distinct patches?
Comments for this one(also rationalized code):
/*
* Snap to EOF, if the request
* - crossed the EOF boundary;
* - is close to EOF(explained below).
*
* Imagine a file sized 18 pages, and we dicided to read-ahead the
* first 16 pages. It is highly possible that in the near future we
* will have to do another read-ahead for the remaining 2 pages,
* which is an unfavorable small I/O.
*
* So we prefer to take a bit risk to enlarge the current read-ahead,
* to eliminate possible future small I/O.
*/
if (ra->readahead_index + ra_readahead_size(ra)/4 > eof_index) {
ra->readahead_index = eof_index;
if (ra->lookahead_index > eof_index)
ra->lookahead_index = eof_index;
ra->flags |= RA_FLAG_EOF;
}
Wu
-
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/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
