GC patch for eCos port

GC patch for eCos port

[Mark Hamilton]

[-- Attachment #1: Type: text/plain, Size: 2341 bytes --]

There looks to be a bug with the eCos port on how garbage collection is
done. I sent a notice about this bug before but I didn't get a resolution.
Another fellow came across the same bug and verified my fix. The bug seems
specific to eCos because of how jffs2_gc_fetch_page was ported but the
proposed fix is applied to gc.c. Since the gc.c is a JFFS2 core file, I
guess this is the appropriate mailing list for posting the patch. The patch
is attached.

Here is the problem.
ffs2_gc_fetch_page reads 4K of data into a static buffer. The static buffer
is hidden in the jffs2_gc_fetch_page function. The problem is when the
writebuf pointer is calculated. The offset is used again to reference into
the pg_ptr. You can image when start is equal to 4K that writebuf will
extend beyond the end of the pg_ptr valid memory. Offset is set to start
just before the while loop.

I made a comment below with what I think the fix should be.
Am I missing something?

            pg_ptr = jffs2_gc_fetch_page(c, f, start, &pg);
            if (IS_ERR(pg_ptr)) {
                printk(KERN_WARNING "read_cache_page() returned error:
%ld\n",
                PTR_ERR(pg_ptr));
                return PTR_ERR(pg_ptr);
            }
            offset = start;
            while(offset < orig_end) {
                        uint32_t datalen;
                        uint32_t cdatalen;
                        char comprtype = JFFS2_COMPR_NONE;
                        ret = jffs2_reserve_space_gc(c, sizeof(ri) +
JFFS2_MIN_DATA_LEN, &phys_ofs, &alloclen);
                        if (ret) {
                            printk(KERN_WARNING "jffs2_reserve_space_gc of
%zd bytes for
                                       garbage_collect_dnode failed: %d\n",
                                         sizeof(ri)+ JFFS2_MIN_DATA_LEN,
ret);
                                    break;
                        }
                        cdatalen = min_t(uint32_t, alloclen - sizeof(ri),
end - offset);
                        datalen = end - offset;
                        // This looks to be wrong.
                        writebuf = pg_ptr + (offset & PAGE_CACHE_SIZE -1));
                        // I think it should be.
                        writebuf = pg_ptr + ((offset -start) &
(PAGE_CACHE_SIZE -1));

The patch uses a define(__ECOS) to fix this problem.

[-- Attachment #2: gc.patch --]
[-- Type: application/octet-stream, Size: 766 bytes --]

Index: gc.c
===================================================================
RCS file: /home/cvs/ecos_artisan/packages/fs/jffs2/current/src/gc.c,v
retrieving revision 1.5
diff -u -F^f -r1.5 gc.c
--- gc.c	22 Jul 2004 21:35:32 -0000	1.5
+++ gc.c	6 Oct 2004 15:09:18 -0000
@@ -1188,7 +1188,15 @@
 		cdatalen = min_t(uint32_t, alloclen - sizeof(ri), end - offset);
 		datalen = end - offset;
 
+#if defined(__ECOS)
+    /* 
+       For the eCos port, jffs2_gc_fetch_page above reads 4K into a static 
+       buffer. 
+    */
+		writebuf = pg_ptr + ((offset-start) & (PAGE_CACHE_SIZE -1));
+#else
 		writebuf = pg_ptr + (offset & (PAGE_CACHE_SIZE -1));
+#endif
 
 		comprtype = jffs2_compress(c, f, writebuf, &comprbuf, &datalen, &cdatalen);
 

Re: [JFFS2] GC patch for eCos port

[Estelle HAMMACHE]

Hi Mark,

isn't it cleaner (for compatibility with Linux) to modify 
jffs2_gc_fetch_page so that it reads a 
PAGE_CACHE_SIZE-aligned buffer ?
BTW I'd like to make the whole page caching thing optional for
a specific use (application needs to write structures to a 
file atomically & structure size is _not_ a power of 2 so it
may overlap a page boundary)... I think eCos doesn't care
about page caches ?

Estelle

 

Mark Hamilton wrote:
> 
> There looks to be a bug with the eCos port on how garbage collection is
> done. I sent a notice about this bug before but I didn't get a resolution.
> Another fellow came across the same bug and verified my fix. The bug seems
> specific to eCos because of how jffs2_gc_fetch_page was ported but the
> proposed fix is applied to gc.c. Since the gc.c is a JFFS2 core file, I
> guess this is the appropriate mailing list for posting the patch. The patch
> is attached.
> 
> Here is the problem.
> ffs2_gc_fetch_page reads 4K of data into a static buffer. The static buffer
> is hidden in the jffs2_gc_fetch_page function. The problem is when the
> writebuf pointer is calculated. The offset is used again to reference into
> the pg_ptr. You can image when start is equal to 4K that writebuf will
> extend beyond the end of the pg_ptr valid memory. Offset is set to start
> just before the while loop.
> 
> I made a comment below with what I think the fix should be.
> Am I missing something?
> 
>             pg_ptr = jffs2_gc_fetch_page(c, f, start, &pg);
>             if (IS_ERR(pg_ptr)) {
>                 printk(KERN_WARNING "read_cache_page() returned error:
> %ld\n",
>                 PTR_ERR(pg_ptr));
>                 return PTR_ERR(pg_ptr);
>             }
>             offset = start;
>             while(offset < orig_end) {
>                         uint32_t datalen;
>                         uint32_t cdatalen;
>                         char comprtype = JFFS2_COMPR_NONE;
>                         ret = jffs2_reserve_space_gc(c, sizeof(ri) +
> JFFS2_MIN_DATA_LEN, &phys_ofs, &alloclen);
>                         if (ret) {
>                             printk(KERN_WARNING "jffs2_reserve_space_gc of
> %zd bytes for
>                                        garbage_collect_dnode failed: %d\n",
>                                          sizeof(ri)+ JFFS2_MIN_DATA_LEN,
> ret);
>                                     break;
>                         }
>                         cdatalen = min_t(uint32_t, alloclen - sizeof(ri),
> end - offset);
>                         datalen = end - offset;
>                         // This looks to be wrong.
>                         writebuf = pg_ptr + (offset & PAGE_CACHE_SIZE -1));
>                         // I think it should be.
>                         writebuf = pg_ptr + ((offset -start) &
> (PAGE_CACHE_SIZE -1));
> 
> The patch uses a define(__ECOS) to fix this problem.

Re: [JFFS2] GC patch for eCos port

[Estelle HAMMACHE]

Estelle Hammache wrote:
> 
> Hi Mark,
> 
> isn't it cleaner (for compatibility with Linux) to modify
> jffs2_gc_fetch_page so that it reads a
> PAGE_CACHE_SIZE-aligned buffer ?

Ok so this was discussed in the eCos mailing list, but I meant that
jffs2_gc_fetch_page should call jffs2_read_inode with the reading
offset set to the page boundary: (offset & ~(PAGE_CACHE_SIZE-1)).
I believe this is the expected behavior for jffs2_gc_fetch_page.

Estelle

Re: [JFFS2] GC patch for eCos port

[Mark Hamilton]

Estelle,
  Sorry for not getting back to you sooner on this. I agree that ideally I
would prefer to see a fix done in the eCos port of JFFS2. I'll try your
suggestion and rerun my tests to make sure that something isn't overlooked.
Hopefully, I can resubmit a patch to the eCos group.
-Mark
----- Original Message ----- 
From: "Estelle HAMMACHE" <estelle.hammache@st.com>
To: "Mark Hamilton" <mhamilton@alliantnetworks.com>;
<linux-mtd@lists.infradead.org>
Sent: Monday, October 11, 2004 4:18 AM
Subject: Re: [JFFS2] GC patch for eCos port


> Estelle Hammache wrote:
> >
> > Hi Mark,
> >
> > isn't it cleaner (for compatibility with Linux) to modify
> > jffs2_gc_fetch_page so that it reads a
> > PAGE_CACHE_SIZE-aligned buffer ?
>
> Ok so this was discussed in the eCos mailing list, but I meant that
> jffs2_gc_fetch_page should call jffs2_read_inode with the reading
> offset set to the page boundary: (offset & ~(PAGE_CACHE_SIZE-1)).
> I believe this is the expected behavior for jffs2_gc_fetch_page.
>
> Estelle
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [JFFS2] GC patch for eCos port

[David Woodhouse]

On Mon, 2004-10-11 at 14:18 +0200, Estelle HAMMACHE wrote:
> was discussed in the eCos mailing list, but I meant that
> jffs2_gc_fetch_page should call jffs2_read_inode with the reading
> offset set to the page boundary: (offset & ~(PAGE_CACHE_SIZE-1)).
> I believe this is the expected behavior for jffs2_gc_fetch_page.

Yes, that is the expected behaviour and looks like the correct fix. I've
just committed the corresponding change to the eCos version of
jffs2_gc_fetch_page(). 

Sorry for the delay in responding.

The eCos port could definitely do with some love -- I did a lot of work
to remove all the Linuxisms from it, and provide abstractions like
jffs2_gc_fetch_page(), and it would be nice if someone could make the
eCos bits no longer look like it's trying to emulate the Linux VFS, but
actually behave as a proper eCos file system.

-- 
dwmw2

RE: [JFFS2] GC patch for eCos port

[Per Hedblom]

On Thursday 11 November 2004 12:28 David Woodhouse Wrote
>Yes, that is the expected behaviour and looks like the correct fix. I've
>just committed the corresponding change to the eCos version of
>jffs2_gc_fetch_page().

Will this code handle files larger than PAGE_SIZE?
I believe the patch for ecos should look like:
{
        /* FIXME: This works only with one file system mounted at a time */
        int ret;

        ret = jffs2_read_inode_range(c, f, gc_buffer, offset,
PAGE_CACHE_SIZE);
        if (ret)
                return ERR_PTR(ret);

        return gc_buffer - ( offset & ( PAGE_CACHE_SIZE - 1 ) );
}

if you want to do it in fs-ecos.c. (This works in our test case but I think
I prefer the original patch suggested by Mark for readbility.)



>The eCos port could definitely do with some love 

I agree, it is hard to believe that anyone can successfully use the jffs2
file system as a general file system in eCos.

Some experience with jffs2 for eCos so far: 
* it requires a lot of software maintenance to keep it updated and tested
for the a specific usage in eCos.
 
* it uses a lot of memory and memory management - a eCos system without
memory management needs fixed memory pools to run stable for a long time. 
We use a lot of small files and if someone is considering jffs2 for a new
design I would recommend reserving a ram space for jffs2 that is in the
range of the flash area in use.

* it doesn't do gc as a thread in eCos. This means that it almost always
want to do gc at the time when you need to write data.


Tanks,
Per Hedblom

RE: [JFFS2] GC patch for eCos port

[David Woodhouse]

On Thu, 2004-11-11 at 14:19 +0100, Per Hedblom wrote:
> On Thursday 11 November 2004 12:28 David Woodhouse Wrote
> >Yes, that is the expected behaviour and looks like the correct fix. I've
> >just committed the corresponding change to the eCos version of
> >jffs2_gc_fetch_page().
> 
> Will this code handle files larger than PAGE_SIZE?

Yes. The problem was that the core code expects you to return a _page_
from the file. That's a page in size, and aligned to a page in the file.
So if it passes you an offset of 0x1234, it doesn't actually want byte
0x1234 to be the first in the returned buffer -- it just wants the page
which contains by 0x1234. Which starts at 0x1000.

Your way does something similar -- adjusting the returned pointer so
that the part which the core code actually looks at is the same. But I
really don't like returning a pointer which is outside the real buffer.

> Some experience with jffs2 for eCos so far: 
> * it requires a lot of software maintenance to keep it updated and tested
> for the a specific usage in eCos.

Can you elaborate? I've seen very few patches submitted, and from that
I've inferred that we've been doing a good job of keeping the eCos port
working. 
 
> * it uses a lot of memory and memory management - a eCos system without
> memory management needs fixed memory pools to run stable for a long time. 
> We use a lot of small files and if someone is considering jffs2 for a new
> design I would recommend reserving a ram space for jffs2 that is in the
> range of the flash area in use.

Yeah. That's sort of inherent to the design, unfortunately. We're toying
with ways to improve it, but it's hard.

> * it doesn't do gc as a thread in eCos. This means that it almost always
> want to do gc at the time when you need to write data.

People have reported this a few times. Each time I replied that it was
easy for them to make it work if they care, and if they know how to do
this stuff in eCos. Nobody did, so I even threw in the skeleton
gcthread.c for eCos to give people a head-start.... and still nobody's
implemented it.

-- 
dwmw2

RE: [JFFS2] GC patch for eCos port

[Per Hedblom]

> -----Original Message-----
> From: David Woodhouse [mailto:dwmw2@infradead.org]
> Sent: Thursday, November 11, 2004 2:59 PM

Thank you for your explanation. We will rerun our tests.

> Can you elaborate? I've seen very few patches submitted, and from that
> I've inferred that we've been doing a good job of keeping the eCos port
> working.
 
That's true, but I am afraid that jffs2 has not been stressed and tested
enough in eCos. We have spent a lot of time without finding serious bugs
like this and I don't expect it to be stable yet.

> 
> > * it uses a lot of memory and memory management - a eCos system without
> > memory management needs fixed memory pools to run stable for a long
> time.
> > We use a lot of small files and if someone is considering jffs2 for a
> new
> > design I would recommend reserving a ram space for jffs2 that is in the
> > range of the flash area in use.
> 
> Yeah. That's sort of inherent to the design, unfortunately. We're toying
> with ways to improve it, but it's hard.
> 
Just wanted to give everybody an honest hint on memory usage and that eCos
users should use the pool implementation and I think that an updated ecos
implementation should include a fully pool supported memory management. We
currently runs the system with dynamic allocation to find out how may blocks
of different types that we need and then use this data to initialize a
static pool. 

> > * it doesn't do gc as a thread in eCos. This means that it almost always
> > want to do gc at the time when you need to write data.
> 
> People have reported this a few times. Each time I replied that it was
> easy for them to make it work if they care, and if they know how to do
> this stuff in eCos. Nobody did, so I even threw in the skeleton
> gcthread.c for eCos to give people a head-start.... and still nobody's
> implemented it.
> 

I have seen the skeleton but think I need some more guidance about how to
protect file operations and the gc operations from each other (what kind of
locking is needed?) before I give it a try.

Tanks,
Per Hedblom

RE: [JFFS2] GC patch for eCos port

[David Woodhouse]

On Thu, 2004-11-11 at 16:50 +0100, Per Hedblom wrote:
> > Can you elaborate? I've seen very few patches submitted, and from that
> > I've inferred that we've been doing a good job of keeping the eCos port
> > working.
>  
> That's true, but I am afraid that jffs2 has not been stressed and tested
> enough in eCos. We have spent a lot of time without finding serious bugs
> like this and I don't expect it to be stable yet.

True. But please do keep reporting them. It shouldn't be hard to keep
the eCos port working, assuming the core code is OK. The glue layer to
plug it into the operating system code isn't the hard part.

> Just wanted to give everybody an honest hint on memory usage and that eCos
> users should use the pool implementation and I think that an updated ecos
> implementation should include a fully pool supported memory management. 

Yeah. We already have something like that in the Linux version, with the
slab caches for different objects. It's still dynamic though, because we
can allocate more slab pages at runtime; it's not a fixed pool.

> I have seen the skeleton but think I need some more guidance about how to
> protect file operations and the gc operations from each other (what kind of
> locking is needed?) before I give it a try.

No locking is required for that purpose. Look at fs/jffs2/background.c
in my CVS tree -- the Linux implementation of the background thread. The
only locking there is to synchronise starting and killing the thread.

-- 
dwmw2