On Wed 30-11-11 08:37:16, Wu Fengguang wrote:
> (snip)
> > > @@ -676,6 +677,20 @@ ondemand_readahead(struct address_space
> > > }
> > >
> > > /*
> > > + * backwards reading
> > > + */
> > > + if (offset < ra->start && offset + req_size >= ra->start) {
> > > + ra->pattern = RA_PATTERN_BACKWARDS;
> > > + ra->size = get_next_ra_size(ra, max);
> > > + max = ra->start;
> > > + if (ra->size > max)
> > > + ra->size = max;
> > > + ra->async_size = 0;
> > > + ra->start -= ra->size;
> > IMHO much more obvious way to write this is:
> > ra->size = get_next_ra_size(ra, max);
> > if (ra->size > ra->start) {
> > ra->size = ra->start;
> > ra->start = 0;
> > } else
> > ra->start -= ra->size;
>
> Good idea! Here is the updated code:
>
> /*
> * backwards reading
> */
> if (offset < ra->start && offset + req_size >= ra->start) {
> ra->pattern = RA_PATTERN_BACKWARDS;
> ra->size = get_next_ra_size(ra, max);
> if (ra->size > ra->start) {
> /*
> * ra->start may be concurrently set to some huge
> * value, the min() at least avoids submitting huge IO
> * in this race condition
> */
> ra->size = min(ra->start, max);
> ra->start = 0;
> } else
> ra->start -= ra->size;
> ra->async_size = 0;
> goto readit;
> }
Looks good. You can add:
Acked-by: Jan Kara <jack@xxxxxxx>
to the patch.
Honza
--
Jan Kara <jack@xxxxxxx>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
