[ECOS] Is JFFS2 thread-safe?

[ECOS] Is JFFS2 thread-safe?

[Vincent Catros]

Hello,

Is JFFS2 thread safe?
Some pieces of code make me think eCos implementation of JFFS2 was
designed to be thread-safe while some other make me think it's not.

Regards.

Vincent


-- 
Before posting, please read the FAQ: http://sources.redhat.com/fom/ecos
and search the list archive: http://sources.redhat.com/ml/ecos-discuss

Re: [ECOS] Is JFFS2 thread-safe?

[David Woodhouse]

On Wed, 2003-11-26 at 10:18 +0100, Vincent Catros wrote:
> Hello,
> 
> Is JFFS2 thread safe?
> Some pieces of code make me think eCos implementation of JFFS2 was
> designed to be thread-safe while some other make me think it's not.

The core JFFS2 code is thread safe, assuming the Linux semaphore and
spinlock primitives are correctly translated into eCos mutexes and
preemption blocks. The bit that I haven't paid much attention to w.r.t.
locking is the inode cache handling in fs-ecos.c.

In fact, I suspect that is actually OK because we set the
CYG_SYNCMODE_FILE_FILESYSTEM flag and hence the fileio layer ensures
that only one file system method is invoked at a time. It would be
better to do our own locking and drop that flag though, since it makes
coordinating the locking with gcthread.c a little nicer.

Your underlying flash drivers may also need to be thread-safe, if you're
allowing concurrent calls into JFFS2 -- which I think we are; open and
close are serialised but read() and write() on two separate files can
happen simultaneously AFAICT. 

JFFS2 will assume that it's perfectly OK to have one thread reading
while another is erasing or writing -- it's the responsibility of the
flash driver to make it work.

-- 
dwmw2



-- 
Before posting, please read the FAQ: http://sources.redhat.com/fom/ecos
and search the list archive: http://sources.redhat.com/ml/ecos-discuss

[ECOS] RE : [ECOS] Is JFFS2 thread-safe?

[Vincent Catros]

Hello David,

And thank you for your answer.

> -----Message d'origine-----
> De : David Woodhouse [mailto:dwmw2@infradead.org]
> Envoyé : jeudi 27 novembre 2003 01:27
> À : Vincent Catros
> Cc : ecos-discuss@sources.redhat.com
> Objet : Re: [ECOS] Is JFFS2 thread-safe?
> 
[...]
> The core JFFS2 code is thread safe, assuming the Linux semaphore and
> spinlock primitives are correctly translated into eCos mutexes and
> preemption blocks. The bit that I haven't paid much attention to
w.r.t.
> locking is the inode cache handling in fs-ecos.c.
> 
> In fact, I suspect that is actually OK because we set the
> CYG_SYNCMODE_FILE_FILESYSTEM flag and hence the fileio layer ensures
> that only one file system method is invoked at a time. It would be
> better to do our own locking and drop that flag though, since it makes
> coordinating the locking with gcthread.c a little nicer.
[...]

If I understand, JFFS2 should be thread safe, but this has never been
tested since multual access is avoided by fileio layer when using
CYG_SYNCMODE_FILE_FILESYSTEM flag?

Am I wrong?

Regards.

Vincent


--
Before posting, please read the FAQ: http://sources.redhat.com/fom/ecos
and search the list archive: http://sources.redhat.com/ml/ecos-discuss

[ECOS] Re: RE : [ECOS] Is JFFS2 thread-safe?

[David Woodhouse]

On Thu, 2003-11-27 at 10:15 +0100, Vincent Catros wrote:
> If I understand, JFFS2 should be thread safe, but this has never been
> tested since multual access is avoided by fileio layer when using
> CYG_SYNCMODE_FILE_FILESYSTEM flag?

The core code of JFFS2, which is shared by both Linux and eCos ports, is
definitely thread safe -- assuming the semaphore and spinlock operations
from Linux are mapped to the correct locking primitives under eCos. I
wonder if spin_lock() should be mapped to a preemption lock. This
locking is documented in the file README.Locking. 

The eCos-specific code of JFFS2 lacks locking on its _own_ inode cache
data structures (i.e. struct _inode, not anything used by the JFFS2
core). In particular the i_cache_{next,prev} lists and the use counts
(i_count) need protection, to deal with simultaneous jffs2_iget() and
jffs2_iput() of the same inode, etc.

That is all currently protected by the CYG_SYNCMODE_FILE_FILESYSTEM
flag, but ideally we'd be able to lose that flag and provide our own
finer-grained locking. Currently, if the garbage collect thread is
implemented (I've put in place only a skeleton), it would need to
LOCK_FS(mte) before calling into jffs2_garbage_collect_pass(). That
would prevent even _read_ activity during the GC pass, which is
suboptimal.

-- 
dwmw2



-- 
Before posting, please read the FAQ: http://sources.redhat.com/fom/ecos
and search the list archive: http://sources.redhat.com/ml/ecos-discuss

[ECOS] RE : RE : [ECOS] Is JFFS2 thread-safe?

[Vincent Catros]

Thanks David,

This is clear for me now.

Vincent


-- 
Before posting, please read the FAQ: http://sources.redhat.com/fom/ecos
and search the list archive: http://sources.redhat.com/ml/ecos-discuss

[ECOS] Re: RE : RE : [ECOS] Is JFFS2 thread-safe?

[David Woodhouse]

On Thu, 2003-11-27 at 11:03 +0100, Vincent Catros wrote:
> Thanks David,
> 
> This is clear for me now.

Good.

In answer to your earlier question about chmod/chown -- they're trivial
to implement. JFFS2 needs to keep track of modes and ownership
internally (in its struct _inode) so that it doesn't corrupt them when
modifying a file system shared with Linux. They should already be
returned by stat(), and the only barrier to allowing you to _change_
modes or ownership from your application is the lack of suitable API for
such purpose. I don't really want to abuse jffs2_setinfo().

The TODO list for JFFS2/eCos is now as follows. I'm hoping someone with
more eCos-clue will pick up some of this:

 - Fill in the skeleton gcthread.c so it actually does something.

 - Check and possibly fix locking of icache mangling in fs-ecos.c

 - Check that os-ecos.h defines 'spin_lock()' to something appropriate.

 - Fix unmount of root file system after chdir().

 - Fix atomicity of renames. Why was the unlink added before rename?

 - Further cleanup -- should the functions in dir-ecos.c take 'struct
   dirsearch' instead of various components thereof, or should each of
   those functions just be moved inside its only caller in fs-ecos.c?

 - Improve mount time by using pointer directly into flash chip instead
   of jffs2_flash_read() for the initial scan -- look at the #ifdef
   __ECOS bit in scan.c for details.

 - Reduce memory usage. There are fields marked for possible removal in
   struct _inode, and there's the __totlen field in struct
   jffs2_raw_node_ref as discussed recently.

Sort of separate, and I have a hacked-up mostly working version already:

 - Make fileio package not gratuitously include kernel header files.

 - Add up RedBoot fileio support so RedBoot can read/write JFFS2.

-- 
dwmw2



-- 
Before posting, please read the FAQ: http://sources.redhat.com/fom/ecos
and search the list archive: http://sources.redhat.com/ml/ecos-discuss

[ECOS] RE : [ECOS] Is JFFS2 thread-safe?

[Vincent Catros]

Davis,

> -----Message d'origine-----
> De : David Woodhouse [mailto:dwmw2@infradead.org]
> Envoyé : jeudi 27 novembre 2003 01:27
> À : Vincent Catros
> Cc : ecos-discuss@sources.redhat.com
> Objet : Re: [ECOS] Is JFFS2 thread-safe?
> 
[...]
> In fact, I suspect that is actually OK because we set the
> CYG_SYNCMODE_FILE_FILESYSTEM flag and hence the fileio layer ensures
> that only one file system method is invoked at a time. It would be
> better to do our own locking and drop that flag though, since it makes
> coordinating the locking with gcthread.c a little nicer.
> 
> Your underlying flash drivers may also need to be thread-safe, if
you're
> allowing concurrent calls into JFFS2 -- which I think we are; open and
> close are serialised but read() and write() on two separate files can
> happen simultaneously AFAICT.
[...]

Effectively, before any call to a file system function, the macro
LOCK_FS is called. This macro is an alias to cyg_fs_lock.
Accordig to the flag passed to that function (ie :
CYG_SYNCMODE_FILE_FILESYSTEM) file system functions are serialized that
way.

But it seems that read() and write() lead to a cyg_file_lock which call
cyg_fs_lock (see fd.cxx).

So, read() and write() are probably serialized to.

Am I wrong?

Regards.

Vincent


--
Before posting, please read the FAQ: http://sources.redhat.com/fom/ecos
and search the list archive: http://sources.redhat.com/ml/ecos-discuss

[ECOS] Re: RE : [ECOS] Is JFFS2 thread-safe?

[David Woodhouse]

On Thu, 2003-11-27 at 18:05 +0100, Vincent Catros wrote:
> But it seems that read() and write() lead to a cyg_file_lock which call
> cyg_fs_lock (see fd.cxx).
> 
> So, read() and write() are probably serialized to.

Yes, they do seem to be serialised, since we have
CYG_SYNCMODE_IO_FILESYSTEM set. That shouldn't be needed.

All we need is CYG_SYNCMODE_IO_FILE, to ensure that large writes appear
atomic to other simultaneous readers.

-- 
dwmw2


-- 
Before posting, please read the FAQ: http://sources.redhat.com/fom/ecos
and search the list archive: http://sources.redhat.com/ml/ecos-discuss

Re: [ECOS] RE : [ECOS] Is JFFS2 thread-safe?

[David Woodhouse]

On Thu, 2003-11-27 at 10:15 +0100, Vincent Catros wrote:
> If I understand, JFFS2 should be thread safe, but this has never been
> tested since multual access is avoided by fileio layer when using
> CYG_SYNCMODE_FILE_FILESYSTEM flag?

It's never been tested under eCos, although the same JFFS2-internal
locking is well-tested under Linux.

If there are bugs, they are with the mapping from Linux locking
primitives to eCos locking primitives. In particular, in the presence of
preemptive scheduling I suspect that the spin_lock() primitive should be
getting a scheduler lock. 

-- 
dwmw2


-- 
Before posting, please read the FAQ: http://sources.redhat.com/fom/ecos
and search the list archive: http://sources.redhat.com/ml/ecos-discuss

Re: [ECOS] RE : [ECOS] Is JFFS2 thread-safe?

[David Woodhouse]

On Fri, 2003-11-28 at 11:27 +0000, David Woodhouse wrote:
> +#ifdef CYGPKG_KERNEL
> +#include <cyg/kernel/kapi.h>
> +#define spin_lock(lock) cyg_scheduler_lock()
> +#define spin_unlock(lock) cyg_scheduler_unlock()
> +#define spin_lock_bh(lock) cyg_scheduler_lock()
> +#define spin_unlock_bh(lock) cyg_scheduler_unlock()

Hmmm. I forgot eCos now had SMP support, and has its own spinlocks. 

Can't say I much like the idea of putting in a #define to call a C
function which is a wrapper around a C++ class which in turn is a
wrapper round the original HAL functions... or in fact in the common
case is just a dummy counter which doesn't actually lock the scheduler
to avoid preemption... is that intentional?

-- 
dwmw2


-- 
Before posting, please read the FAQ: http://sources.redhat.com/fom/ecos
and search the list archive: http://sources.redhat.com/ml/ecos-discuss

Re: [ECOS] RE : [ECOS] Is JFFS2 thread-safe?

[Gary Thomas]

David Woodhouse said:
> On Fri, 2003-11-28 at 11:27 +0000, David Woodhouse wrote:
>> +#ifdef CYGPKG_KERNEL
>> +#include <cyg/kernel/kapi.h>
>> +#define spin_lock(lock) cyg_scheduler_lock()
>> +#define spin_unlock(lock) cyg_scheduler_unlock()
>> +#define spin_lock_bh(lock) cyg_scheduler_lock()
>> +#define spin_unlock_bh(lock) cyg_scheduler_unlock()
>
> Hmmm. I forgot eCos now had SMP support, and has its own spinlocks.
>
> Can't say I much like the idea of putting in a #define to call a C
> function which is a wrapper around a C++ class which in turn is a
> wrapper round the original HAL functions... or in fact in the common
> case is just a dummy counter which doesn't actually lock the scheduler
> to avoid preemption... is that intentional?

What do you mean?  If the scheduler lock is non-zero, then no
scheduling takes place (certainly in the single processor case)

I would think that spin_lock_bh() would be equivalent to
cyg_drv_isr_lock() which actually locks the interrupts during
a [hyper] critical section.

>
> --
> dwmw2
>
>
> --
> Before posting, please read the FAQ: http://sources.redhat.com/fom/ecos
> and search the list archive: http://sources.redhat.com/ml/ecos-discuss
>
>




-- 
Before posting, please read the FAQ: http://sources.redhat.com/fom/ecos
and search the list archive: http://sources.redhat.com/ml/ecos-discuss

Re: [ECOS] RE : [ECOS] Is JFFS2 thread-safe?

[David Woodhouse]

On Sat, 2003-11-29 at 07:54 -0700, Gary Thomas wrote:
> What do you mean?  If the scheduler lock is non-zero, then no
> scheduling takes place (certainly in the single processor case)

Er, but what causes the scheduler lock to become non-zero when I lock a
spinlock?

-- 
dwmw2


-- 
Before posting, please read the FAQ: http://sources.redhat.com/fom/ecos
and search the list archive: http://sources.redhat.com/ml/ecos-discuss

Re: [ECOS] RE : [ECOS] Is JFFS2 thread-safe?

[Nick Garnett]

David Woodhouse <dwmw2@infradead.org> writes:

> On Fri, 2003-11-28 at 11:27 +0000, David Woodhouse wrote:
> > +#ifdef CYGPKG_KERNEL
> > +#include <cyg/kernel/kapi.h>
> > +#define spin_lock(lock) cyg_scheduler_lock()
> > +#define spin_unlock(lock) cyg_scheduler_unlock()
> > +#define spin_lock_bh(lock) cyg_scheduler_lock()
> > +#define spin_unlock_bh(lock) cyg_scheduler_unlock()
> 
> Hmmm. I forgot eCos now had SMP support, and has its own spinlocks. 
> 
> Can't say I much like the idea of putting in a #define to call a C
> function which is a wrapper around a C++ class which in turn is a
> wrapper round the original HAL functions... or in fact in the common
> case is just a dummy counter which doesn't actually lock the scheduler
> to avoid preemption... is that intentional?

Well, most of that should collapse through the use of inline functions
and macros. Only the call to the C function should be real.

eCos spinlocks are different from Linux ones in any case. They are not
generally used as mutexes over shared data structures, we use actual
mutexes for that. eCos was not really designed to be SMP and the SMP
support is currently the minimum we need to get it to work. The
scheduler lock is a global all-threads, all-CPUs lock. We don't have a
per-thread preemption lock the way Linux does. The scheduler lock is
also bound up with the way that DSRs work and the whole interrupt
mechanism, so changing it transparently is probably not possible.

For these reasons, making spin_lock() et al use the scheduler lock is
probably a bad idea, it would have an effect on the whole system if
they were held for long periods of time.

-- 
Nick Garnett                    eCos Kernel Architect
http://www.ecoscentric.com      The eCos and RedBoot experts


-- 
Before posting, please read the FAQ: http://sources.redhat.com/fom/ecos
and search the list archive: http://sources.redhat.com/ml/ecos-discuss

Re: [ECOS] RE : [ECOS] Is JFFS2 thread-safe?

[David Woodhouse]

On Sat, 2003-11-29 at 15:47 +0000, Nick Garnett wrote:
> For these reasons, making spin_lock() et al use the scheduler lock is
> probably a bad idea, it would have an effect on the whole system if
> they were held for long periods of time.

OK. That was a brain fart on my part -- in Linux without preemptive
scheduling, you mustn't sleep with spinlocks held, and with preemptive
scheduling a spin_lock() prevents preemption... I was applying that same
'knowledge' and thinking that preemption would lead to deadlock as it
would under Linux. That's obviously bogus.

We use spin_lock() in JFFS2 as a short-lived lightweight mutex; we could
map it to eCos spinlocks or mutexes as you see fit -- as long as it's
mapped to _something_ in the case where preemptive scheduling happens,
I'm happy. Only in the absence of preemption is it safe in its current
'do {} while(0)' form.

Having tidied up most of the interface between core JFFS2 code and the
eCos-specific parts, the latter could really do with some TLC from
someone who knows eCos a lot better than I do.

-- 
dwmw2


-- 
Before posting, please read the FAQ: http://sources.redhat.com/fom/ecos
and search the list archive: http://sources.redhat.com/ml/ecos-discuss