Re: jffs2 mutex problem

["Artem B. Bityutskiy" <dedekind@yandex.ru>]

Keijiro Yano wrote:
> Hello,
> 
> I find out the problem related to the mutex lock in JFFS2.
> I always use Linux 2.6.10, but Linux 2.6.14 also has a same
> problem, I think.
> 
> jffs2_create() calls jffs2_new_inode() that allocates new
> "jffs2_inode_info" with jffs2_alloc_inode().
> Generally, in this case, jffs2_i_init_once(), which is the
> constructor for jffs2_inode_cachep, will be called and
> initialize the mutex with lock state by the below code.
> 
> static void jffs2_i_init_once(void * foo, kmem_cache_t * cachep, 
> unsigned long flags)
> {
> struct jffs2_inode_info *ei = (struct jffs2_inode_info *) foo;
> 
> if ((flags & (SLAB_CTOR_VERIFY|SLAB_CTOR_CONSTRUCTOR)) ==
>    SLAB_CTOR_CONSTRUCTOR) {
> init_MUTEX_LOCKED(&ei->sem);
> 
> But, the constructor is called by kmem_cache_alloc()
> only when no active objects are left in a cache.
> 
> Therefore, sometimes jffs2_i_init_once() will not be
> called after jffs2_alloc_inode() is done.
> 
> So, jffs2_do_create(), which is called by jffs2_create(), will
> do "up(&f->sem)" in unlocking state.
Hmm, really. IMO it is a bug to initialize the mutex as locked in the 
constructor while the mutex is left unlocked when it is being freed.

> I try to fix this problem with the following patch. Please
> review it and/or give me your comments.
> 
> --- fs/jffs2/os-linux.h 10 Feb 2005 10:15:41 -0000 1.1.1.1
> +++ fs/jffs2/os-linux.h 9 Nov 2005 08:36:13 -0000
> @@ -93,8 +93,8 @@
>  f->usercompr = 0;
> #else
>  memset(f, 0, sizeof(*f));
> - init_MUTEX_LOCKED(&f->sem);
> #endif
> + init_MUTEX_LOCKED(&f->sem);
> }
> 
> #define jffs2_is_readonly(c) (OFNI_BS_2SFFJ(c)->s_flags & MS_RDONLY)
> 
> --- fs/jffs2/super.c 10 Feb 2005 10:15:41 -0000 1.1.1.1
> +++ fs/jffs2/super.c 9 Nov 2005 08:36:13 -0000
> @@ -51,7 +51,7 @@
> 
>  if ((flags & (SLAB_CTOR_VERIFY|SLAB_CTOR_CONSTRUCTOR)) ==
>      SLAB_CTOR_CONSTRUCTOR) {
> - init_MUTEX_LOCKED(&ei->sem);
>  inode_init_once(&ei->vfs_inode);
>  }
> }

Well, the patch is not against the latest MTD CVS snapshot which is bad.

The right solution, IMO, would be:

1. use init_MUTEX() in the slab constructor for jffs2_inode_info() objects.
2. add explicit down(&f->sem) everywhere after jffs2_new_inode().

This is right because the "constructed" state of the inode is when the 
mutex is initialized and unlocked. FYI, here is a good article where you 
can read about the idea of constructors in the slab cache: 
http://srl.cs.jhu.edu/courses/600.418/SlabAllocator.pdf


-- 
Best Regards,
Artem B. Bityutskiy,
St.-Petersburg, Russia.