* Re: ext4/quota lockdep complaint: false positive?
2009-12-26 19:09 ext4/quota lockdep complaint: false positive? Theodore Ts'o
@ 2010-01-05 0:15 ` Jan Kara
2010-01-06 20:42 ` Jan Kara
1 sibling, 0 replies; 3+ messages in thread
From: Jan Kara @ 2010-01-05 0:15 UTC (permalink / raw)
To: Theodore Ts'o; +Cc: Jan Kara, linux-ext4
Hi,
On Sat 26-12-09 14:09:32, Theodore Ts'o wrote:
> While running the xfsqa regression test suite, lockdep triggered the
> following complaint, as shown below.
>
> I *think* it is a false positive, since the issue involves chown
> triggering a write to the quota file; and during the write, we are
> extending the quota file, which means grabbing i_data_sem while we have
> already grabbed the inode mutex for the quota file.
>
> The potential circular locking dependency that lockdep is complaining
> occurs only when we are mounting the filesystem, and reading from the
> quota file; and at that point we wouldn't be writing other files and
> needing to update quota file as aresult. So I don't think this is a
> problem in practice, but (1) i'd like a second opinion, and (2) I'm not
> sure what's the best way to make lockdep happy.
I think it's false positive as well. There are only two places which
really have the dqptr_sem -> i_mutex/4 dependency. One happens during
quotaon and one during quotaoff and both at a time when there are no dquot
structures active.
Another reason why the deadlock cannot really happen is that when we
are holding i_data_sem of quota file we do not ever acquire dqptr_sem
(because of IS_NOQUOTA check in quota functions) - lockdep established the
dependency before the file was marked as quota file...
So the question is how to silence the lockdep warning in a clean way. One
way would be to introduce a separate locking subclass for i_data_sem of
quota files but IMHO that would look ugly in the code - we'd have to call
down_write/read_nested if IS_NOQUOTA is true for the inode when acquiring
i_data_sem. Another solution (although mostly accidental) would be to break
the dqptr_sem->i_mutex/4 dependency happening during quotaon/quotaoff. I'll
have a look at it tomorrow and see how hard that would be.
Honza
> =======================================================
> [ INFO: possible circular locking dependency detected ]
> 2.6.32-06811-ge47eb9c #226
> -------------------------------------------------------
> chown/9834 is trying to acquire lock:
> (&ei->i_data_sem){++++..}, at: [<c0238af2>] ext4_get_blocks+0x37/0x2d4
>
> but task is already holding lock:
> (&sb->s_type->i_mutex_key#12/4){+.+...}, at: [<c0245618>] ext4_quota_write+0xb9/0x263
>
> which lock already depends on the new lock.
>
>
> the existing dependency chain (in reverse order) is:
>
> -> #2 (&sb->s_type->i_mutex_key#12/4){+.+...}:
> [<c017e0d2>] __lock_acquire+0xa16/0xb75
> [<c017e2c4>] lock_acquire+0x93/0xb1
> [<c062d75b>] __mutex_lock_common+0x32/0x314
> [<c062daea>] mutex_lock_nested+0x35/0x3d
> [<c021be29>] vfs_load_quota_inode+0x172/0x40a
> [<c021c365>] vfs_quota_on_path+0x44/0x4d
> [<c02451e5>] ext4_quota_on+0x122/0x171
> [<c0220588>] do_quotactl+0xdb/0x3da
> [<c0220b04>] sys_quotactl+0x27d/0x29d
> [<c062f06d>] syscall_call+0x7/0xb
>
> -> #1 (&s->s_dquot.dqptr_sem){++++..}:
> [<c017e0d2>] __lock_acquire+0xa16/0xb75
> [<c017e2c4>] lock_acquire+0x93/0xb1
> [<c062dd61>] down_read+0x39/0x76
> [<c021ca5e>] dquot_release_reserved_space+0x2d/0x91
> [<c023524d>] vfs_dq_release_reservation_block+0x49/0x55
> [<c0238d03>] ext4_get_blocks+0x248/0x2d4
> [<c0238ea8>] mpage_da_map_blocks+0x86/0x347
> [<c0239417>] ext4_da_writepages+0x2ae/0x452
> [<c01c461c>] do_writepages+0x1c/0x29
> [<c0200f22>] writeback_single_inode+0xd9/0x293
> [<c0201f80>] write_inode_now+0x80/0xc8
> [<c021bdbc>] vfs_load_quota_inode+0x105/0x40a
> [<c021c365>] vfs_quota_on_path+0x44/0x4d
> [<c02451e5>] ext4_quota_on+0x122/0x171
> [<c0220588>] do_quotactl+0xdb/0x3da
> [<c0220b04>] sys_quotactl+0x27d/0x29d
> [<c062f06d>] syscall_call+0x7/0xb
>
> -> #0 (&ei->i_data_sem){++++..}:
> [<c017dfd4>] __lock_acquire+0x918/0xb75
> [<c017e2c4>] lock_acquire+0x93/0xb1
> [<c062dd61>] down_read+0x39/0x76
> [<c0238af2>] ext4_get_blocks+0x37/0x2d4
> [<c0239885>] ext4_getblk+0x56/0x141
> [<c0239988>] ext4_bread+0x18/0x59
> [<c024564c>] ext4_quota_write+0xed/0x263
> [<c021f425>] write_blk+0x33/0x3b
> [<c021ffc6>] do_insert_tree+0x1eb/0x2bd
> [<c0220033>] do_insert_tree+0x258/0x2bd
> [<c0220033>] do_insert_tree+0x258/0x2bd
> [<c0220033>] do_insert_tree+0x258/0x2bd
> [<c02200f7>] qtree_write_dquot+0x5f/0x118
> [<c021ecb8>] v2_write_dquot+0x25/0x27
> [<c021ba9a>] dquot_acquire+0x89/0xde
> [<c0246b40>] ext4_acquire_dquot+0x64/0x7e
> [<c021dbbb>] dqget+0x29d/0x2d9
> [<c021e07e>] dquot_transfer+0x89/0x508
> [<c021bc9c>] vfs_dq_transfer+0x64/0x7f
> [<c0236d46>] ext4_setattr+0xa4/0x2c0
> [<c01fac16>] notify_change+0x164/0x2aa
> [<c01e8825>] chown_common+0x68/0x7a
> [<c01e88eb>] sys_fchownat+0x57/0x72
> [<c062f06d>] syscall_call+0x7/0xb
>
> other info that might help us debug this:
>
> 5 locks held by chown/9834:
> #0: (&sb->s_type->i_mutex_key#12){+.+.+.}, at: [<c01e881a>] chown_common+0x5d/0x7a
> #1: (jbd2_handle){+.+...}, at: [<c025d7c8>] start_this_handle+0x42d/0x478
> #2: (&dquot->dq_lock){+.+...}, at: [<c021ba36>] dquot_acquire+0x25/0xde
> #3: (&s->s_dquot.dqio_mutex){+.+...}, at: [<c021ba46>] dquot_acquire+0x35/0xde
> #4: (&sb->s_type->i_mutex_key#12/4){+.+...}, at: [<c0245618>] ext4_quota_write+0xb9/0x263
>
> stack backtrace:
> Pid: 9834, comm: chown Not tainted 2.6.32-06811-ge47eb9c #226
> Call Trace:
> [<c062c548>] ? printk+0x14/0x16
> [<c017d38e>] print_circular_bug+0x8a/0x96
> [<c017dfd4>] __lock_acquire+0x918/0xb75
> [<c017e2c4>] lock_acquire+0x93/0xb1
> [<c0238af2>] ? ext4_get_blocks+0x37/0x2d4
> [<c062dd61>] down_read+0x39/0x76
> [<c0238af2>] ? ext4_get_blocks+0x37/0x2d4
> [<c0238af2>] ext4_get_blocks+0x37/0x2d4
> [<c017cb93>] ? mark_lock+0x1e/0x1d9
> [<c0239885>] ext4_getblk+0x56/0x141
> [<c062da33>] ? __mutex_lock_common+0x30a/0x314
> [<c0239988>] ext4_bread+0x18/0x59
> [<c062daea>] ? mutex_lock_nested+0x35/0x3d
> [<c024564c>] ext4_quota_write+0xed/0x263
> [<c021f425>] write_blk+0x33/0x3b
> [<c021ffc6>] do_insert_tree+0x1eb/0x2bd
> [<c0220033>] do_insert_tree+0x258/0x2bd
> [<c0220033>] do_insert_tree+0x258/0x2bd
> [<c0220033>] do_insert_tree+0x258/0x2bd
> [<c02200f7>] qtree_write_dquot+0x5f/0x118
> [<c021f752>] ? qtree_read_dquot+0x62/0x1bb
> [<c021ecb8>] v2_write_dquot+0x25/0x27
> [<c021ba9a>] dquot_acquire+0x89/0xde
> [<c0246b40>] ext4_acquire_dquot+0x64/0x7e
> [<c021dbbb>] dqget+0x29d/0x2d9
> [<c021e07e>] dquot_transfer+0x89/0x508
> [<c062e8e3>] ? _spin_unlock+0x22/0x25
> [<c021d318>] ? dqput+0x1de/0x1f3
> [<c021bc9c>] vfs_dq_transfer+0x64/0x7f
> [<c0236d46>] ext4_setattr+0xa4/0x2c0
> [<c01fac16>] notify_change+0x164/0x2aa
> [<c01e8825>] chown_common+0x68/0x7a
> [<c01e88eb>] sys_fchownat+0x57/0x72
> [<c062f06d>] syscall_call+0x7/0xb
>
--
Jan Kara <jack@suse.cz>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: ext4/quota lockdep complaint: false positive?
2009-12-26 19:09 ext4/quota lockdep complaint: false positive? Theodore Ts'o
2010-01-05 0:15 ` Jan Kara
@ 2010-01-06 20:42 ` Jan Kara
1 sibling, 0 replies; 3+ messages in thread
From: Jan Kara @ 2010-01-06 20:42 UTC (permalink / raw)
To: Theodore Ts'o; +Cc: Jan Kara, linux-ext4
Hi Ted,
On Sat 26-12-09 14:09:32, Theodore Ts'o wrote:
>
> While running the xfsqa regression test suite, lockdep triggered the
> following complaint, as shown below.
>
> I *think* it is a false positive, since the issue involves chown
> triggering a write to the quota file; and during the write, we are
> extending the quota file, which means grabbing i_data_sem while we have
> already grabbed the inode mutex for the quota file.
>
> The potential circular locking dependency that lockdep is complaining
> occurs only when we are mounting the filesystem, and reading from the
> quota file; and at that point we wouldn't be writing other files and
> needing to update quota file as aresult. So I don't think this is a
> problem in practice, but (1) i'd like a second opinion, and (2) I'm not
> sure what's the best way to make lockdep happy.
>
> Jan, what do you think?
The patch below should silence the lockdep warning.
Honza
--
Jan Kara <jack@suse.cz>
SUSE Labs, CR
---
>From 815e4d46ad7e56d41096116f5a6927903658551f Mon Sep 17 00:00:00 2001
From: Jan Kara <jack@suse.cz>
Date: Wed, 6 Jan 2010 17:20:35 +0100
Subject: [PATCH] quota: Cleanup S_NOQUOTA handling
Cleanup handling of S_NOQUOTA inode flag and document it a bit. The flag
does not have to be set under dqptr_sem. Only functions modifying inode's
dquot pointers have to check the flag under dqptr_sem before going forward
with the modification. This way we are sure that we cannot add new dquot
pointers to the inode which is just becoming a quota file.
The good thing about this cleanup is that there are no more places in quota
code which enforce i_mutex vs. dqptr_sem lock ordering (in particular that
dqptr_sem -> i_mutex of quota file). This should silence some (false) lockdep
warnings with ext4 + quota and generally make life of some filesystems easier.
Signed-off-by: Jan Kara <jack@suse.cz>
---
fs/quota/dquot.c | 47 +++++++++++------------------------------------
1 files changed, 11 insertions(+), 36 deletions(-)
diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
index dea86ab..b4693f2 100644
--- a/fs/quota/dquot.c
+++ b/fs/quota/dquot.c
@@ -100,9 +100,13 @@
*
* Any operation working on dquots via inode pointers must hold dqptr_sem. If
* operation is just reading pointers from inode (or not using them at all) the
- * read lock is enough. If pointers are altered function must hold write lock
- * (these locking rules also apply for S_NOQUOTA flag in the inode - note that
- * for altering the flag i_mutex is also needed).
+ * read lock is enough. If pointers are altered function must hold write lock.
+ * Special care needs to be taken about S_NOQUOTA inode flag (marking that
+ * inode is a quota file). Functions adding pointers from inode to dquots have
+ * to check this flag under dqptr_sem and then (if S_NOQUOTA is not set) they
+ * have to do all pointer modifications before dropping dqptr_sem. This makes
+ * sure they cannot race with quotaon which first sets S_NOQUOTA flag and
+ * then drops all pointers to dquots from an inode.
*
* Each dquot has its dq_lock mutex. Locked dquots might not be referenced
* from inodes (dquot_alloc_space() and such don't check the dq_lock).
@@ -1275,7 +1279,6 @@ int dquot_initialize(struct inode *inode, int type)
}
down_write(&sb_dqopt(sb)->dqptr_sem);
- /* Having dqptr_sem we know NOQUOTA flags can't be altered... */
if (IS_NOQUOTA(inode))
goto out_err;
for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
@@ -1428,11 +1431,6 @@ int __dquot_alloc_space(struct inode *inode, qsize_t number,
}
down_read(&sb_dqopt(inode->i_sb)->dqptr_sem);
- if (IS_NOQUOTA(inode)) {
- inode_incr_space(inode, number, reserve);
- goto out_unlock;
- }
-
for (cnt = 0; cnt < MAXQUOTAS; cnt++)
warntype[cnt] = QUOTA_NL_NOWARN;
@@ -1463,7 +1461,6 @@ int __dquot_alloc_space(struct inode *inode, qsize_t number,
mark_all_dquot_dirty(inode->i_dquot);
out_flush_warn:
flush_warnings(inode->i_dquot, warntype);
-out_unlock:
up_read(&sb_dqopt(inode->i_sb)->dqptr_sem);
out:
return ret;
@@ -1496,10 +1493,6 @@ int dquot_alloc_inode(const struct inode *inode, qsize_t number)
for (cnt = 0; cnt < MAXQUOTAS; cnt++)
warntype[cnt] = QUOTA_NL_NOWARN;
down_read(&sb_dqopt(inode->i_sb)->dqptr_sem);
- if (IS_NOQUOTA(inode)) {
- up_read(&sb_dqopt(inode->i_sb)->dqptr_sem);
- return QUOTA_OK;
- }
spin_lock(&dq_data_lock);
for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
if (!inode->i_dquot[cnt])
@@ -1536,12 +1529,6 @@ int dquot_claim_space(struct inode *inode, qsize_t number)
}
down_read(&sb_dqopt(inode->i_sb)->dqptr_sem);
- if (IS_NOQUOTA(inode)) {
- up_read(&sb_dqopt(inode->i_sb)->dqptr_sem);
- inode_claim_rsv_space(inode, number);
- goto out;
- }
-
spin_lock(&dq_data_lock);
/* Claim reserved quotas to allocated quotas */
for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
@@ -1570,17 +1557,11 @@ int __dquot_free_space(struct inode *inode, qsize_t number, int reserve)
/* First test before acquiring mutex - solves deadlocks when we
* re-enter the quota code and are already holding the mutex */
if (IS_NOQUOTA(inode)) {
-out_sub:
inode_decr_space(inode, number, reserve);
return QUOTA_OK;
}
down_read(&sb_dqopt(inode->i_sb)->dqptr_sem);
- /* Now recheck reliably when holding dqptr_sem */
- if (IS_NOQUOTA(inode)) {
- up_read(&sb_dqopt(inode->i_sb)->dqptr_sem);
- goto out_sub;
- }
spin_lock(&dq_data_lock);
for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
if (!inode->i_dquot[cnt])
@@ -1633,11 +1614,6 @@ int dquot_free_inode(const struct inode *inode, qsize_t number)
return QUOTA_OK;
down_read(&sb_dqopt(inode->i_sb)->dqptr_sem);
- /* Now recheck reliably when holding dqptr_sem */
- if (IS_NOQUOTA(inode)) {
- up_read(&sb_dqopt(inode->i_sb)->dqptr_sem);
- return QUOTA_OK;
- }
spin_lock(&dq_data_lock);
for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
if (!inode->i_dquot[cnt])
@@ -1689,7 +1665,6 @@ int dquot_transfer(struct inode *inode, struct iattr *iattr)
GRPQUOTA);
down_write(&sb_dqopt(inode->i_sb)->dqptr_sem);
- /* Now recheck reliably when holding dqptr_sem */
if (IS_NOQUOTA(inode)) { /* File without quota accounting? */
up_write(&sb_dqopt(inode->i_sb)->dqptr_sem);
goto put_all;
@@ -2007,13 +1982,15 @@ static int vfs_load_quota_inode(struct inode *inode, int type, int format_id,
/* We don't want quota and atime on quota files (deadlocks
* 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);
+ /*
+ * When S_NOQUOTA is set, remove dquot references as no more
+ * references can be added
+ */
sb->dq_op->drop(inode);
}
@@ -2050,14 +2027,12 @@ out_file_init:
iput(inode);
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(&dqopt->dqonoff_mutex);
out_fmt:
--
1.6.4.2
^ permalink raw reply related [flat|nested] 3+ messages in thread