From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kara Subject: Re: ext4/quota lockdep complaint: false positive? Date: Tue, 5 Jan 2010 01:15:42 +0100 Message-ID: <20100105001542.GC3252@quack.suse.cz> References: Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Jan Kara , linux-ext4@vger.kernel.org To: Theodore Ts'o Return-path: Received: from cantor2.suse.de ([195.135.220.15]:55272 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753563Ab0AEAPl (ORCPT ); Mon, 4 Jan 2010 19:15:41 -0500 Content-Disposition: inline In-Reply-To: Sender: linux-ext4-owner@vger.kernel.org List-ID: 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: [] ext4_get_blocks+0x37/0x2d4 > > but task is already holding lock: > (&sb->s_type->i_mutex_key#12/4){+.+...}, at: [] 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){+.+...}: > [] __lock_acquire+0xa16/0xb75 > [] lock_acquire+0x93/0xb1 > [] __mutex_lock_common+0x32/0x314 > [] mutex_lock_nested+0x35/0x3d > [] vfs_load_quota_inode+0x172/0x40a > [] vfs_quota_on_path+0x44/0x4d > [] ext4_quota_on+0x122/0x171 > [] do_quotactl+0xdb/0x3da > [] sys_quotactl+0x27d/0x29d > [] syscall_call+0x7/0xb > > -> #1 (&s->s_dquot.dqptr_sem){++++..}: > [] __lock_acquire+0xa16/0xb75 > [] lock_acquire+0x93/0xb1 > [] down_read+0x39/0x76 > [] dquot_release_reserved_space+0x2d/0x91 > [] vfs_dq_release_reservation_block+0x49/0x55 > [] ext4_get_blocks+0x248/0x2d4 > [] mpage_da_map_blocks+0x86/0x347 > [] ext4_da_writepages+0x2ae/0x452 > [] do_writepages+0x1c/0x29 > [] writeback_single_inode+0xd9/0x293 > [] write_inode_now+0x80/0xc8 > [] vfs_load_quota_inode+0x105/0x40a > [] vfs_quota_on_path+0x44/0x4d > [] ext4_quota_on+0x122/0x171 > [] do_quotactl+0xdb/0x3da > [] sys_quotactl+0x27d/0x29d > [] syscall_call+0x7/0xb > > -> #0 (&ei->i_data_sem){++++..}: > [] __lock_acquire+0x918/0xb75 > [] lock_acquire+0x93/0xb1 > [] down_read+0x39/0x76 > [] ext4_get_blocks+0x37/0x2d4 > [] ext4_getblk+0x56/0x141 > [] ext4_bread+0x18/0x59 > [] ext4_quota_write+0xed/0x263 > [] write_blk+0x33/0x3b > [] do_insert_tree+0x1eb/0x2bd > [] do_insert_tree+0x258/0x2bd > [] do_insert_tree+0x258/0x2bd > [] do_insert_tree+0x258/0x2bd > [] qtree_write_dquot+0x5f/0x118 > [] v2_write_dquot+0x25/0x27 > [] dquot_acquire+0x89/0xde > [] ext4_acquire_dquot+0x64/0x7e > [] dqget+0x29d/0x2d9 > [] dquot_transfer+0x89/0x508 > [] vfs_dq_transfer+0x64/0x7f > [] ext4_setattr+0xa4/0x2c0 > [] notify_change+0x164/0x2aa > [] chown_common+0x68/0x7a > [] sys_fchownat+0x57/0x72 > [] 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: [] chown_common+0x5d/0x7a > #1: (jbd2_handle){+.+...}, at: [] start_this_handle+0x42d/0x478 > #2: (&dquot->dq_lock){+.+...}, at: [] dquot_acquire+0x25/0xde > #3: (&s->s_dquot.dqio_mutex){+.+...}, at: [] dquot_acquire+0x35/0xde > #4: (&sb->s_type->i_mutex_key#12/4){+.+...}, at: [] ext4_quota_write+0xb9/0x263 > > stack backtrace: > Pid: 9834, comm: chown Not tainted 2.6.32-06811-ge47eb9c #226 > Call Trace: > [] ? printk+0x14/0x16 > [] print_circular_bug+0x8a/0x96 > [] __lock_acquire+0x918/0xb75 > [] lock_acquire+0x93/0xb1 > [] ? ext4_get_blocks+0x37/0x2d4 > [] down_read+0x39/0x76 > [] ? ext4_get_blocks+0x37/0x2d4 > [] ext4_get_blocks+0x37/0x2d4 > [] ? mark_lock+0x1e/0x1d9 > [] ext4_getblk+0x56/0x141 > [] ? __mutex_lock_common+0x30a/0x314 > [] ext4_bread+0x18/0x59 > [] ? mutex_lock_nested+0x35/0x3d > [] ext4_quota_write+0xed/0x263 > [] write_blk+0x33/0x3b > [] do_insert_tree+0x1eb/0x2bd > [] do_insert_tree+0x258/0x2bd > [] do_insert_tree+0x258/0x2bd > [] do_insert_tree+0x258/0x2bd > [] qtree_write_dquot+0x5f/0x118 > [] ? qtree_read_dquot+0x62/0x1bb > [] v2_write_dquot+0x25/0x27 > [] dquot_acquire+0x89/0xde > [] ext4_acquire_dquot+0x64/0x7e > [] dqget+0x29d/0x2d9 > [] dquot_transfer+0x89/0x508 > [] ? _spin_unlock+0x22/0x25 > [] ? dqput+0x1de/0x1f3 > [] vfs_dq_transfer+0x64/0x7f > [] ext4_setattr+0xa4/0x2c0 > [] notify_change+0x164/0x2aa > [] chown_common+0x68/0x7a > [] sys_fchownat+0x57/0x72 > [] syscall_call+0x7/0xb > -- Jan Kara SUSE Labs, CR