linux-kernel - Re: [PATCH] Improve heuristic detecting sequential reads

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: <20070410192722.9e6efc8b.akpm@linux-foundation.org>
Date:	Tue, 10 Apr 2007 19:27:22 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Jan Kara <jack@...e.cz>
Cc:	linux-kernel@...r.kernel.org, npiggin@...e.de
Subject: Re: [PATCH] Improve heuristic detecting sequential reads

On Tue, 10 Apr 2007 17:54:11 +0200 Jan Kara <jack@...e.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)

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

So why don't we just use a loff_t for this?

Anyway, the asymmetry in our handling of prev_index (sometimes called
prev_page!) and prev_offset is unpleasing.  This:

--- 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.

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.

Wanna take a look at all of this, please?
-
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/