lkml.org 
[lkml]   [2007]   [Apr]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] Improve heuristic detecting sequential reads
On Tue 10-04-07 19:27:22, Andrew Morton wrote:
> On Tue, 10 Apr 2007 17:54:11 +0200 Jan Kara <jack@suse.cz> wrote:
>
> > Introduce ra.offset and store in it an offset where the previous read ended. This way
> > we can detect whether reads are really sequential (and thus we should not mark the page
> > as accessed repeatedly) or whether they are random and just happen to be in the same page
> > (and the page should really be marked accessed again).
>
> (less columns, please)
Oops, sorry.

> OK. So prev_page and prev_offset are now a complexified representation of a
> loff_t, no?
Yes.

> So why don't we just use a loff_t for this?
I did not merge them because most other things in file_ra_state are in
pages and thus comparisons and assignments would require additional
shifts, which would IMHO make the whole thing less clear.

> Anyway, the asymmetry in our handling of prev_index (sometimes called
> prev_page!) and prev_offset is unpleasing. This:
prev_page is a member of the file_ra_state. Probably prev_index would be
better name there...

> --- a/mm/filemap.c~readahead-improve-heuristic-detecting-sequential-reads-tidy
> +++ a/mm/filemap.c
> @@ -933,6 +933,7 @@ page_ok:
> if (prev_index != index || offset != prev_offset)
> mark_page_accessed(page);
> prev_index = index;
> + prev_offset = ra.offset = offset;
>
> /*
> * Ok, we have the page, and it's up-to-date, so
> @@ -948,7 +949,6 @@ page_ok:
> offset += ret;
> index += offset >> PAGE_CACHE_SHIFT;
> offset &= ~PAGE_CACHE_MASK;
> - prev_offset = ra.offset = offset;
>
> page_cache_release(page);
> if (ret == nr && desc->count)
>
> improves things somewhat. But I think it would be nicer if their handling
> was unified, or at least consistent. We update ra.offset here, and we
> update ra.prev_page over there.
It's clearer but wrong, as Wu already noted :). But the assignment
prev_index = index
can be shifted somewhat lower (definitely after the comment) which makes
things slightly more readable.

> And shouldn't offset be called prev_offset? Or should prev_page be called
> page? Or index? Or prev_index? Or Marmaduke? The naming is all a mess.
I vote for prev_index and 'offset' could be prev_offset. I'll create a
tidy-up patch...

Honza
--
Jan Kara <jack@suse.cz>
SuSE CR Labs
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2007-04-11 14:11    [W:0.079 / U:0.720 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site