Re: [PATCH] avoid unnnecessary mtd read when read can be satisfied by write buffer

Re: [PATCH] avoid unnnecessary mtd read when read can be satisfied by write buffer

[Kevin Vigor]

ghannon@cspi.com wrote:

>However when programming a flash device and then reading back
>to verify that it was programmed correctly,  I think this write
>cache would defeat the purpose.
>
>  
>
This problem already exists with the wbuf code since a call to 
jffs2_flash_read will return the write buffer contents regardless of the 
actual contents of the media (unless you call  jffs2_flush_wbuf_pad() 
between the write and the read).

So the semantics could remain the same in that an explicit flush would 
be necessary in order to validate the data on flash.

[PATCH] avoid unnnecessary mtd read when read can be satisfied by write buffer

[Kevin Vigor]

The following small patch modifies wbuf.c::jffs2_flash_read to avoid a 
call to the mtd read layer in the case where all data returned by the 
read would be replaced by data from the write buffer anyway. This 
condition occurs relatively infrequently, but the cost of the test is 
low and the benefit of avoiding the mtd read is high.

Signed-off-by: Kevin Vigor <kevin@realmsys.com>


diff --git a/fs/jffs2/wbuf.c b/fs/jffs2/wbuf.c
index a7f153f..01ef266 100644
--- a/fs/jffs2/wbuf.c
+++ b/fs/jffs2/wbuf.c
@@ -907,6 +907,20 @@ int jffs2_flash_read(struct jffs2_sb_inf

    /* Read flash */
    down_read(&c->wbuf_sem);
+
+    /* Check if the read can be completely satisfied from the write buffer;
+     * if so, we can avoid the mtd call entirely.
+     */
+    if (c->wbuf_pagesize &&
+        (SECTOR_ADDR(ofs) == SECTOR_ADDR(c->wbuf_ofs)) &&
+        (ofs >= c->wbuf_ofs) &&
+        (ofs + len) <= (c->wbuf_ofs + c->wbuf_len)) {
+        memcpy(buf, c->wbuf + (ofs - c->wbuf_ofs), len);
+        *retlen = len;
+        ret = 0;
+        goto exit;
+    }
+
    ret = c->mtd->read(c->mtd, ofs, len, retlen, buf);

    if ( (ret == -EBADMSG || ret == -EUCLEAN) && (*retlen == len) ) {

Re: [PATCH] avoid unnnecessary mtd read when read can be satisfied by write buffer

[Jörn Engel]

On Wed, 31 May 2006 15:50:28 -0600, Kevin Vigor wrote:
> 
> The following small patch modifies wbuf.c::jffs2_flash_read to avoid a 
> call to the mtd read layer in the case where all data returned by the 
> read would be replaced by data from the write buffer anyway. This 
> condition occurs relatively infrequently, but the cost of the test is 
> low and the benefit of avoiding the mtd read is high.

Not sure whether this micro optimization is worth the code.  A real
solution would be to have a cache for the full device.  Possibly read
the device through pagecache somehow (writing obviously should be
write-through, not write-back).  That would have a significantly
higher hit rate than your patch.

Jörn

-- 
Don't worry about people stealing your ideas. If your ideas are any good,
you'll have to ram them down people's throats.
-- Howard Aiken quoted by Ken Iverson quoted by Jim Horning quoted by
   Raph Levien, 1979

Re: [PATCH] avoid unnnecessary mtd read when read can be satisfied by write buffer

[David Woodhouse]

On Thursday 01 June 2006 15:32, Jörn Engel wrote:
> Not sure whether this micro optimization is worth the code.  A real
> solution would be to have a cache for the full device.  Possibly read
> the device through pagecache somehow (writing obviously should be
> write-through, not write-back).  That would have a significantly
> higher hit rate than your patch.

We haven't written to the device yet, at this point -- this is just the case 
where the data in question exist _only_ in the wbuf, but we're actually 
reading from the flash first _anyway_, with the current code. 

I don't think that caching the compressed nodes from the flash is going to be 
particularly useful -- we already have the page cache for _uncompressed_ 
data. For other things we might want to cache, such as dirents and symlinks, 
we'll do _much_ better to store that in memory for ourselves rather than just 
caching whole pages of the backing store for such sparse content.

-- 
dwmw2

Re: [PATCH] avoid unnnecessary mtd read when read can be satisfied by write buffer

[Jörn Engel]

On Thu, 1 June 2006 15:39:34 +0100, David Woodhouse wrote:
> On Thursday 01 June 2006 15:32, Jörn Engel wrote:
> > Not sure whether this micro optimization is worth the code.  A real
> > solution would be to have a cache for the full device.  Possibly read
> > the device through pagecache somehow (writing obviously should be
> > write-through, not write-back).  That would have a significantly
> > higher hit rate than your patch.
> 
> We haven't written to the device yet, at this point -- this is just the case 
> where the data in question exist _only_ in the wbuf, but we're actually 
> reading from the flash first _anyway_, with the current code. 

Good point.  Ignore my objections then.

> I don't think that caching the compressed nodes from the flash is going to be 
> particularly useful -- we already have the page cache for _uncompressed_ 
> data. For other things we might want to cache, such as dirents and symlinks, 
> we'll do _much_ better to store that in memory for ourselves rather than just 
> caching whole pages of the backing store for such sparse content.

My observations have been a bit different.  I've had to deal with a
semi-broken simulator, where flash accesses were unbearably slow.
Booting took about an hour.  The simulator has been fixed since, but
the broken state made some details fairly visible. :)

One of the observations was that a lot of flash is being read multiple
times.  Reads on NAND happen in page granularity, so first we have to
read pretty much everything during scan, later again when accessing
the actual files.  There is some room for savings, as RAM is faster
than flash even in the real world.

Jörn

-- 
A quarrel is quickly settled when deserted by one party; there is
no battle unless there be two.
-- Seneca

Re: [PATCH] avoid unnnecessary mtd read when read can be satisfied by write buffer

[David Woodhouse]

On Thu, 2006-06-01 at 16:49 +0200, Jörn Engel wrote:
> One of the observations was that a lot of flash is being read multiple
> times.  Reads on NAND happen in page granularity, so first we have to
> read pretty much everything during scan, later again when accessing
> the actual files.  There is some room for savings, as RAM is faster
> than flash even in the real world. 

My point is that for any given problem, raw caching of flash is not the
answer -- something more cunning is going to be better.

The specific cases of dirents and symlinks I already talked about. Any
other time we keep reading the same bit of flash over and over again is
also a problem. The one you mention is fixed by using eraseblock
summary.

I need to finish what I was doing yesterday, which halves the amount of
space taken by summaries. We should probably also remove the dependency
on EXPERIMENTAL.

-- 
dwmw2

Re: [PATCH] avoid unnnecessary mtd read when read can be satisfied by write buffer

[Jörn Engel]

On Thu, 1 June 2006 16:05:07 +0100, David Woodhouse wrote:
> 
> My point is that for any given problem, raw caching of flash is not the
> answer -- something more cunning is going to be better.

Not necessarily.  JFFS2, all metadata is not just cached, but cannot
even get evicted from cache.  Logfs*) currently doesn't cache metadata
at all, which is fairly bad.  But there is no requirement to pin the
metadata into memory, so it can have a dynamic cache with a shrinker.
And now the caching should be based on a (device, offset) tupel.
Nothing cunning about it, except to decide whether any given offset
contains metadata or file data, which is already cached elsewhere.

*) No code released yet, yadda yadda

Jörn

-- 
The wise man seeks everything in himself; the ignorant man tries to get
everything from somebody else.
-- unknown