Re: mmotm 2009-07-16-14-32 - lockdep whinge in ext3/quota code

[Jan Kara <jack@suse.cz>]

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

On Wed 22-07-09 09:17:49, Valdis.Kletnieks@vt.edu wrote:
> Saw this while bisecting to find another issue.
> 
> quilt top:
> memcg-add-comments-explaining-memory-barriers-checkpatch-fixes.patch
> 
> Some checking doesn't look like any of the 58 patches after that are
> relevant (only the reiser4 patch references quotas, no grep hits for lockdep
> or ext3).
> 
> About 30 seconds after boot:
> 
> =======================================================
> [ INFO: possible circular locking dependency detected ]
> 2.6.31-rc3 #2
> -------------------------------------------------------
> rm/1562 is trying to acquire lock:
>  (&sb->s_type->i_mutex_key#11/4){+.+...}, at: [<ffffffff81139fd9>] ext3_quota_write+0xb5/0x274
> 
> but task is already holding lock:
>  (&s->s_dquot.dqio_mutex){+.+...}, at: [<ffffffff811153d5>] dquot_commit+0x26/0xee
> 
> which lock already depends on the new lock.
> 
  Grumble... Commit d01730d74d2b0155da50d44555001706294014f7 didn't quite
fix the problem. At least lockdep now warns about it ;). Attached patch
should fix it (compile tested only so far).

> the existing dependency chain (in reverse order) is:
> 
> -> #1 (&s->s_dquot.dqio_mutex){+.+...}:
>        [<ffffffff81067010>] __lock_acquire+0xa1b/0xb97
>        [<ffffffff81067278>] lock_acquire+0xec/0x110
>        [<ffffffff814a04af>] __mutex_lock_common+0x5a/0x54e
>        [<ffffffff814a0a43>] mutex_lock_nested+0x32/0x37
>        [<ffffffff81115b54>] vfs_load_quota_inode+0x264/0x496
>        [<ffffffff81116094>] vfs_quota_on_path+0x4c/0x55
>        [<ffffffff81138e9d>] ext3_quota_on+0x14c/0x167
>        [<ffffffff81119e4f>] do_quotactl+0xf4/0x44c
>        [<ffffffff8111a491>] sys_quotactl+0x2ea/0x30e
>        [<ffffffff8100b2ab>] system_call_fastpath+0x16/0x1b
>        [<ffffffffffffffff>] 0xffffffffffffffff
> 
> -> #0 (&sb->s_type->i_mutex_key#11/4){+.+...}:
>        [<ffffffff81066eed>] __lock_acquire+0x8f8/0xb97
>        [<ffffffff81067278>] lock_acquire+0xec/0x110
>        [<ffffffff814a04af>] __mutex_lock_common+0x5a/0x54e
>        [<ffffffff814a0a43>] mutex_lock_nested+0x32/0x37
>        [<ffffffff81139fd9>] ext3_quota_write+0xb5/0x274
>        [<ffffffff81119bd3>] qtree_write_dquot+0xce/0x127
>        [<ffffffff81118924>] v2_write_dquot+0x27/0x29
>        [<ffffffff8111544c>] dquot_commit+0x9d/0xee
>        [<ffffffff8113a8ae>] ext3_write_dquot+0x69/0x8a
>        [<ffffffff8111713f>] dqput+0x138/0x25c
>        [<ffffffff81117993>] dquot_drop+0x6a/0x74
>        [<ffffffff8111509f>] vfs_dq_drop+0x41/0x43
>        [<ffffffff81130cba>] ext3_free_inode+0x96/0x28c
>        [<ffffffff81134cce>] ext3_delete_inode+0xbf/0xdd
>        [<ffffffff810e357d>] generic_delete_inode+0x135/0x1db
>        [<ffffffff810e363a>] generic_drop_inode+0x17/0x56
>        [<ffffffff810e252d>] iput+0x7a/0x7f
>        [<ffffffff810db906>] do_unlinkat+0x123/0x176
>        [<ffffffff810dbab8>] sys_unlinkat+0x24/0x26
>        [<ffffffff8100b2ab>] system_call_fastpath+0x16/0x1b
>        [<ffffffffffffffff>] 0xffffffffffffffff
> 
> other info that might help us debug this:
> 
> 2 locks held by rm/1562:
>  #0:  (jbd_handle){+.+...}, at: [<ffffffff811433c9>] journal_start+0x10a/0x137
>  #1:  (&s->s_dquot.dqio_mutex){+.+...}, at: [<ffffffff811153d5>] dquot_commit+0x26/0xee
> 
> stack backtrace:
> Pid: 1562, comm: rm Not tainted 2.6.31-rc3 #2
> Call Trace:
>  [<ffffffff81066290>] print_circular_bug_tail+0x71/0x7c
>  [<ffffffff81066eed>] __lock_acquire+0x8f8/0xb97
>  [<ffffffff81139fd9>] ? ext3_quota_write+0xb5/0x274
>  [<ffffffff81067278>] lock_acquire+0xec/0x110
>  [<ffffffff81139fd9>] ? ext3_quota_write+0xb5/0x274
>  [<ffffffff814a04af>] __mutex_lock_common+0x5a/0x54e
>  [<ffffffff81139fd9>] ? ext3_quota_write+0xb5/0x274
>  [<ffffffff810665e4>] ? check_irq_usage+0xad/0xbe
>  [<ffffffff81139fd9>] ? ext3_quota_write+0xb5/0x274
>  [<ffffffff810670e2>] ? __lock_acquire+0xaed/0xb97
>  [<ffffffff814a0a43>] mutex_lock_nested+0x32/0x37
>  [<ffffffff81139fd9>] ext3_quota_write+0xb5/0x274
>  [<ffffffff81119bd3>] qtree_write_dquot+0xce/0x127
>  [<ffffffff81034c90>] ? get_parent_ip+0x11/0x42
>  [<ffffffff81118924>] v2_write_dquot+0x27/0x29
>  [<ffffffff8111544c>] dquot_commit+0x9d/0xee
>  [<ffffffff8113a8ae>] ext3_write_dquot+0x69/0x8a
>  [<ffffffff8111713f>] dqput+0x138/0x25c
>  [<ffffffff81117993>] dquot_drop+0x6a/0x74
>  [<ffffffff8111509f>] vfs_dq_drop+0x41/0x43
>  [<ffffffff81130cba>] ext3_free_inode+0x96/0x28c
>  [<ffffffff81131d63>] ? ext3_mark_inode_dirty+0x48/0x53
>  [<ffffffff81134cce>] ext3_delete_inode+0xbf/0xdd
>  [<ffffffff81134c0f>] ? ext3_delete_inode+0x0/0xdd
>  [<ffffffff810e357d>] generic_delete_inode+0x135/0x1db
>  [<ffffffff810e363a>] generic_drop_inode+0x17/0x56
>  [<ffffffff810e252d>] iput+0x7a/0x7f
>  [<ffffffff810db906>] do_unlinkat+0x123/0x176
>  [<ffffffff8107d440>] ? audit_syscall_entry+0x170/0x19c
>  [<ffffffff810dbab8>] sys_unlinkat+0x24/0x26
>  [<ffffffff8100b2ab>] system_call_fastpath+0x16/0x1b

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

[-- Attachment #2: 0001-quota-Silence-lockdep-on-quota_on.patch --]
[-- Type: text/x-patch, Size: 2436 bytes --]

>From df60fe9a9d554070e6135087e154bd5aad2cc1b5 Mon Sep 17 00:00:00 2001
From: Jan Kara <jack@suse.cz>
Date: Wed, 22 Jul 2009 18:12:17 +0200
Subject: [PATCH] quota: Silence lockdep on quota_on

Commit d01730d74d2b0155da50d44555001706294014f7 didn't completely fix
the problem since we still take dqio_mutex and i_mutex in the wrong
order. Move taking of i_mutex further down (luckily it's needed only
for updating inode flags) below where dqio_mutex is taken.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/quota/dquot.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
index 70f36c0..38f7bd5 100644
--- a/fs/quota/dquot.c
+++ b/fs/quota/dquot.c
@@ -2043,7 +2043,6 @@ static int vfs_load_quota_inode(struct inode *inode, int type, int format_id,
 		invalidate_bdev(sb->s_bdev);
 	}
 	mutex_lock(&dqopt->dqonoff_mutex);
-	mutex_lock_nested(&inode->i_mutex, I_MUTEX_QUOTA);
 	if (sb_has_quota_loaded(sb, type)) {
 		error = -EBUSY;
 		goto out_lock;
@@ -2054,9 +2053,11 @@ static int vfs_load_quota_inode(struct inode *inode, int type, int format_id,
 		 * possible) Also nobody should write to the file - we use
 		 * special IO operations which ignore the immutable bit. */
 		down_write(&dqopt->dqptr_sem);
+		mutex_lock_nested(&inode->i_mutex, I_MUTEX_QUOTA);
 		oldflags = inode->i_flags & (S_NOATIME | S_IMMUTABLE |
 					     S_NOQUOTA);
 		inode->i_flags |= S_NOQUOTA | S_NOATIME | S_IMMUTABLE;
+		mutex_unlock(&inode->i_mutex);
 		up_write(&dqopt->dqptr_sem);
 		sb->dq_op->drop(inode);
 	}
@@ -2080,7 +2081,6 @@ static int vfs_load_quota_inode(struct inode *inode, int type, int format_id,
 		goto out_file_init;
 	}
 	mutex_unlock(&dqopt->dqio_mutex);
-	mutex_unlock(&inode->i_mutex);
 	spin_lock(&dq_state_lock);
 	dqopt->flags |= dquot_state_flag(flags, type);
 	spin_unlock(&dq_state_lock);
@@ -2096,13 +2096,14 @@ out_file_init:
 out_lock:
 	if (oldflags != -1) {
 		down_write(&dqopt->dqptr_sem);
+		mutex_lock_nested(&inode->i_mutex, I_MUTEX_QUOTA);
 		/* Set the flags back (in the case of accidental quotaon()
 		 * on a wrong file we don't want to mess up the flags) */
 		inode->i_flags &= ~(S_NOATIME | S_NOQUOTA | S_IMMUTABLE);
 		inode->i_flags |= oldflags;
+		mutex_unlock(&inode->i_mutex);
 		up_write(&dqopt->dqptr_sem);
 	}
-	mutex_unlock(&inode->i_mutex);
 	mutex_unlock(&dqopt->dqonoff_mutex);
 out_fmt:
 	put_quota_format(fmt);
-- 
1.6.0.2