Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755101Ab0BAPEn (ORCPT ); Mon, 1 Feb 2010 10:04:43 -0500 Received: from mx1.redhat.com ([209.132.183.28]:13389 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754253Ab0BAPEl (ORCPT ); Mon, 1 Feb 2010 10:04:41 -0500 Date: Mon, 1 Feb 2010 10:04:32 -0500 From: Vivek Goyal To: Gui Jianfeng Cc: Jens Axboe , linux kernel mailing list Subject: Re: [PATCH] Blk-cgroup: Fix potential deallock in blk-cgroup Message-ID: <20100201150432.GA3323@redhat.com> References: <4B669569.9070205@cn.fujitsu.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4B669569.9070205@cn.fujitsu.com> User-Agent: Mutt/1.5.19 (2009-01-05) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5827 Lines: 158 On Mon, Feb 01, 2010 at 04:48:41PM +0800, Gui Jianfeng wrote: > Hi > > I triggered a lockdep warnning as following. > > ======================================================= > [ INFO: possible circular locking dependency detected ] > 2.6.33-rc2 #1 > ------------------------------------------------------- > test_io_control/7357 is trying to acquire lock: > (blkio_list_lock){+.+...}, at: [] blkiocg_weight_write+0x82/0x9e > > but task is already holding lock: > (&(&blkcg->lock)->rlock){......}, at: [] blkiocg_weight_write+0x3b/0x9e > > which lock already depends on the new lock. > > > the existing dependency chain (in reverse order) is: > > -> #2 (&(&blkcg->lock)->rlock){......}: > [] validate_chain+0x8bc/0xb9c > [] __lock_acquire+0x723/0x789 > [] lock_acquire+0x90/0xa7 > [] _raw_spin_lock_irqsave+0x27/0x5a > [] blkiocg_add_blkio_group+0x1a/0x6d > [] cfq_get_queue+0x225/0x3de > [] cfq_set_request+0x217/0x42d > [] elv_set_request+0x17/0x26 > [] get_request+0x203/0x2c5 > [] get_request_wait+0x18/0x10e > [] __make_request+0x2ba/0x375 > [] generic_make_request+0x28d/0x30f > [] submit_bio+0x8a/0x8f > [] submit_bh+0xf0/0x10f > [] ll_rw_block+0xc0/0xf9 > [] ext3_find_entry+0x319/0x544 [ext3] > [] ext3_lookup+0x2c/0xb9 [ext3] > [] do_lookup+0xd3/0x172 > [] link_path_walk+0x5fb/0x95c > [] path_walk+0x3c/0x81 > [] do_path_lookup+0x21/0x8a > [] do_filp_open+0xf0/0x978 > [] open_exec+0x1b/0xb7 > [] do_execve+0xbb/0x266 > [] sys_execve+0x24/0x4a > [] ptregs_execve+0x12/0x18 > > -> #1 (&(&q->__queue_lock)->rlock){..-.-.}: > [] validate_chain+0x8bc/0xb9c > [] __lock_acquire+0x723/0x789 > [] lock_acquire+0x90/0xa7 > [] _raw_spin_lock_irqsave+0x27/0x5a > [] cfq_unlink_blkio_group+0x17/0x41 > [] blkiocg_destroy+0x72/0xc7 > [] cgroup_diput+0x4a/0xb2 > [] dentry_iput+0x93/0xb7 > [] d_kill+0x1c/0x36 > [] dput+0xf5/0xfe > [] do_rmdir+0x95/0xbe > [] sys_rmdir+0x10/0x12 > [] sysenter_do_call+0x12/0x32 > > -> #0 (blkio_list_lock){+.+...}: > [] validate_chain+0x61c/0xb9c > [] __lock_acquire+0x723/0x789 > [] lock_acquire+0x90/0xa7 > [] _raw_spin_lock+0x1e/0x4e > [] blkiocg_weight_write+0x82/0x9e > [] cgroup_file_write+0xc6/0x1c0 > [] vfs_write+0x8c/0x116 > [] sys_write+0x3b/0x60 > [] sysenter_do_call+0x12/0x32 > > other info that might help us debug this: > > 1 lock held by test_io_control/7357: > #0: (&(&blkcg->lock)->rlock){......}, at: [] blkiocg_weight_write+0x3b/0x9e > stack backtrace: > Pid: 7357, comm: test_io_control Not tainted 2.6.33-rc2 #1 > Call Trace: > [] print_circular_bug+0x91/0x9d > [] validate_chain+0x61c/0xb9c > [] __lock_acquire+0x723/0x789 > [] lock_acquire+0x90/0xa7 > [] ? blkiocg_weight_write+0x82/0x9e > [] _raw_spin_lock+0x1e/0x4e > [] ? blkiocg_weight_write+0x82/0x9e > [] blkiocg_weight_write+0x82/0x9e > [] cgroup_file_write+0xc6/0x1c0 > [] ? trace_hardirqs_off+0xb/0xd > [] ? cpu_clock+0x2e/0x44 > [] ? security_file_permission+0xf/0x11 > [] ? rw_verify_area+0x8a/0xad > [] ? cgroup_file_write+0x0/0x1c0 > [] vfs_write+0x8c/0x116 > [] sys_write+0x3b/0x60 > [] sysenter_do_call+0x12/0x32 > > To prevent deadlock, we should take locks as following sequence: > > blkio_list_lock -> queue_lock -> blkcg_lock. > > The following patch should fix this bug. > > Signed-off-by: Gui Jianfeng Thanks Gui. So, we acquired locks as follows. queue_lock -> blkcg_lock (cfq_get_queue -> blkiocg_add_blkio_group path) blkio_list_lock -> queue_lock (blkiocg_destroy -> blkio_unlink_group_fn) And now we are trying to take blkcg_lock --> blkio_list_lock which is a candidate for circular dependency. Looks good. Acked-by: Vivek Goyal Thanks Vivek > --- > block/blk-cgroup.c | 4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c > index 1fa2654..e7dbbaf 100644 > --- a/block/blk-cgroup.c > +++ b/block/blk-cgroup.c > @@ -147,16 +147,16 @@ blkiocg_weight_write(struct cgroup *cgroup, struct cftype *cftype, u64 val) > return -EINVAL; > > blkcg = cgroup_to_blkio_cgroup(cgroup); > + spin_lock(&blkio_list_lock); > spin_lock_irq(&blkcg->lock); > blkcg->weight = (unsigned int)val; > hlist_for_each_entry(blkg, n, &blkcg->blkg_list, blkcg_node) { > - spin_lock(&blkio_list_lock); > list_for_each_entry(blkiop, &blkio_list, list) > blkiop->ops.blkio_update_group_weight_fn(blkg, > blkcg->weight); > - spin_unlock(&blkio_list_lock); > } > spin_unlock_irq(&blkcg->lock); > + spin_unlock(&blkio_list_lock); > return 0; > } > > -- > 1.5.4.rc3 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/