jffs2 lockdep warning

jffs2 lockdep warning

[David Brownell]

This little surprise happened from busybox 1.7.2 "ash" when I typed:

	$ echo $(( 2 + 2 ))

I didn't try to reproduce it.  The kernel used GIT that was current
as of sometime this afternoon.

- Dave

=======================================================
[ INFO: possible circular locking dependency detected ]
2.6.25 #316
-------------------------------------------------------
ash/378 is trying to acquire lock:
 (&c->alloc_sem){--..}, at: [<900b9ea4>] jffs2_reserve_space+0x28/0x120

but task is already holding lock:
 (&ei->sem){--..}, at: [<900c0584>] jffs2_new_inode+0x4c/0x180

which lock already depends on the new lock.


the existing dependency chain (in reverse order) is:

-> #1 (&ei->sem){--..}:
       [<90034b48>] lock_acquire+0x40/0x58
       [<90159b2a>] mutex_lock_nested+0x82/0x1bc
       [<900c0a4a>] jffs2_do_setattr+0x13a/0x618
       [<900c0f9c>] jffs2_setattr+0x18/0x1c
       [<900615d0>] notify_change+0xd8/0x100
       [<90069d30>] do_utimes+0x168/0x198
       [<90069ed2>] sys_utime+0x6a/0x8c
       [<90010136>] syscall_return+0x0/0x12

-> #0 (&c->alloc_sem){--..}:
       [<90034b48>] lock_acquire+0x40/0x58
       [<90159b2a>] mutex_lock_nested+0x82/0x1bc
       [<900b9ea4>] jffs2_reserve_space+0x28/0x120
       [<900bc90a>] jffs2_do_create+0x26/0x220
       [<900b75ba>] jffs2_create+0x6e/0xc8
       [<90058cc8>] vfs_create+0x48/0x54
       [<9005a830>] do_filp_open+0x174/0x614
       [<90052914>] do_sys_open+0x30/0x5c
       [<90052956>] sys_open+0xe/0x10
       [<90010136>] syscall_return+0x0/0x12

other info that might help us debug this:

2 locks held by ash/378:
 #0:  (&type->i_mutex_dir_key#2){--..}, at: [<9005a7a0>] do_filp_open+0xe4/0x614
 #1:  (&ei->sem){--..}, at: [<900c0584>] jffs2_new_inode+0x4c/0x180

stack backtrace:
Call trace:
 [<90013320>] dump_stack+0x18/0x20
 [<90032f36>] print_circular_bug_tail+0x4a/0x60
 [<90034308>] __lock_acquire+0x7a8/0x9b0
 [<90034b48>] lock_acquire+0x40/0x58
 [<90159b2a>] mutex_lock_nested+0x82/0x1bc
 [<900b9ea4>] jffs2_reserve_space+0x28/0x120
 [<900bc90a>] jffs2_do_create+0x26/0x220
 [<900b75ba>] jffs2_create+0x6e/0xc8
 [<90058cc8>] vfs_create+0x48/0x54
 [<9005a830>] do_filp_open+0x174/0x614
 [<90052914>] do_sys_open+0x30/0x5c
 [<90052956>] sys_open+0xe/0x10
 [<90010136>] syscall_return+0x0/0x12

Re: jffs2 lockdep warning

[Joakim Tjernlund]

On Tue, 2008-04-29 at 21:39 -0700, David Brownell wrote:
> This little surprise happened from busybox 1.7.2 "ash" when I typed:
> 
> 	$ echo $(( 2 + 2 ))
> 
> I didn't try to reproduce it.  The kernel used GIT that was current
> as of sometime this afternoon.

[Offtopic]

David, I have been trying to reach you a few times but there was
no reply. I know our mail server got on your black list for a while,
maybe it still is? Hopefully you will get this via the MTD list in
that case.

 Jocke

Re: jffs2 lockdep warning

[David Woodhouse]

On Tue, 2008-04-29 at 21:39 -0700, David Brownell wrote:
> ash/378 is trying to acquire lock:
>  (&c->alloc_sem){--..}, at: [<900b9ea4>] jffs2_reserve_space+0x28/0x120
> 
> but task is already holding lock:
>  (&ei->sem){--..}, at: [<900c0584>] jffs2_new_inode+0x4c/0x180
> 
> which lock already depends on the new lock.

Thanks for the report. It's actually a false positive, since it's a
newly-created inode -- so nothing else can be trying to take its ->sem
after holding c->alloc_sem, and the AB-BA deadlock cannot happen even
though we shouldn't _normally_ try to take c->alloc_sem while we're
holding a per-inode mutex (as documented in README.Locking).

But it's relatively simple to fix, too:

diff --git a/fs/jffs2/dir.c b/fs/jffs2/dir.c
index c63e7a9..2bba3d3 100644
--- a/fs/jffs2/dir.c
+++ b/fs/jffs2/dir.c
@@ -208,6 +208,13 @@ static int jffs2_create(struct inode *dir_i, struct dentry *dentry, int mode,
 	f = JFFS2_INODE_INFO(inode);
 	dir_f = JFFS2_INODE_INFO(dir_i);
 
+	/* jffs2_do_create() will want to lock it, _after_ reserving
+	   space and taking c-alloc_sem. If we keep it locked here,
+	   lockdep gets unhappy (although it's a false positive;
+	   nothing else will be looking at this inode yet so there's
+	   no chance of AB-BA deadlock involving its f->sem). */
+	mutex_unlock(&f->sem);
+
 	ret = jffs2_do_create(c, dir_f, f, ri,
 			      dentry->d_name.name, dentry->d_name.len);
 	if (ret)
diff --git a/fs/jffs2/write.c b/fs/jffs2/write.c
index 665fce9..87891bd 100644
--- a/fs/jffs2/write.c
+++ b/fs/jffs2/write.c
@@ -438,10 +438,10 @@ int jffs2_do_create(struct jffs2_sb_info *c, struct jffs2_inode_info *dir_f, str
 	ret = jffs2_reserve_space(c, sizeof(*ri), &alloclen, ALLOC_NORMAL,
 				JFFS2_SUMMARY_INODE_SIZE);
 	D1(printk(KERN_DEBUG "jffs2_do_create(): reserved 0x%x bytes\n", alloclen));
-	if (ret) {
-		mutex_unlock(&f->sem);
+	if (ret)
 		return ret;
-	}
+
+	mutex_lock(&f->sem);
 
 	ri->data_crc = cpu_to_je32(0);
 	ri->node_crc = cpu_to_je32(crc32(0, ri, sizeof(*ri)-8));

-- 
dwmw2