From mboxrd@z Thu Jan 1 00:00:00 1970 From: Zheng Liu Subject: Re: [PATCH] ext4: fix a potential deadlock in __ext4_es_shrink() Date: Sun, 13 Jul 2014 15:00:15 +0800 Message-ID: <20140713070014.GA9301@gmail.com> References: <20140707233326.GA6076@bbox> <1405193603-15614-1-git-send-email-tytso@mit.edu> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Ext4 Developers List , Minchan Kim , stable@vger.kernel.org To: Theodore Ts'o Return-path: Content-Disposition: inline In-Reply-To: <1405193603-15614-1-git-send-email-tytso@mit.edu> Sender: stable-owner@vger.kernel.org List-Id: linux-ext4.vger.kernel.org On Sat, Jul 12, 2014 at 03:33:23PM -0400, Theodore Ts'o wrote: > This fixes the following lockdep complaint: > > [ INFO: possible circular locking dependency detected ] > 3.16.0-rc2-mm1+ #7 Tainted: G O > ------------------------------------------------------- > kworker/u24:0/4356 is trying to acquire lock: > (&(&sbi->s_es_lru_lock)->rlock){+.+.-.}, at: [] __ext4_es_shrink+0x4f/0x2e0 > > but task is already holding lock: > (&ei->i_es_lock){++++-.}, at: [] ext4_es_insert_extent+0x71/0x180 > > which lock already depends on the new lock. > > Possible unsafe locking scenario: > > CPU0 CPU1 > ---- ---- > lock(&ei->i_es_lock); > lock(&(&sbi->s_es_lru_lock)->rlock); > lock(&ei->i_es_lock); > lock(&(&sbi->s_es_lru_lock)->rlock); > > *** DEADLOCK *** > > 6 locks held by kworker/u24:0/4356: > #0: ("writeback"){.+.+.+}, at: [] process_one_work+0x180/0x560 > #1: ((&(&wb->dwork)->work)){+.+.+.}, at: [] process_one_work+0x180/0x560 > #2: (&type->s_umount_key#22){++++++}, at: [] grab_super_passive+0x44/0x90 > #3: (jbd2_handle){+.+...}, at: [] start_this_handle+0x189/0x5f0 > #4: (&ei->i_data_sem){++++..}, at: [] ext4_map_blocks+0x132/0x550 > #5: (&ei->i_es_lock){++++-.}, at: [] ext4_es_insert_extent+0x71/0x180 > > stack backtrace: > CPU: 0 PID: 4356 Comm: kworker/u24:0 Tainted: G O 3.16.0-rc2-mm1+ #7 > Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011 > Workqueue: writeback bdi_writeback_workfn (flush-253:0) > ffffffff8213dce0 ffff880014b07538 ffffffff815df0bb 0000000000000007 > ffffffff8213e040 ffff880014b07588 ffffffff815db3dd ffff880014b07568 > ffff880014b07610 ffff88003b868930 ffff88003b868908 ffff88003b868930 > Call Trace: > [] dump_stack+0x4e/0x68 > [] print_circular_bug+0x1fb/0x20c > [] __lock_acquire+0x163e/0x1d00 > [] ? retint_restore_args+0xe/0xe > [] ? __slab_alloc+0x4a8/0x4ce > [] ? __ext4_es_shrink+0x4f/0x2e0 > [] lock_acquire+0x87/0x120 > [] ? __ext4_es_shrink+0x4f/0x2e0 > [] ? ext4_es_free_extent+0x5d/0x70 > [] _raw_spin_lock+0x39/0x50 > [] ? __ext4_es_shrink+0x4f/0x2e0 > [] ? kmem_cache_alloc+0x18b/0x1a0 > [] __ext4_es_shrink+0x4f/0x2e0 > [] ext4_es_insert_extent+0xc8/0x180 > [] ext4_map_blocks+0x1c4/0x550 > [] ext4_writepages+0x6d4/0xd00 > ... > > Reported-by: Minchan Kim > Signed-off-by: Theodore Ts'o > Reported-by: Minchan Kim > Cc: stable@vger.kernel.org > Cc: Zheng Liu Thanks for fixing this. It looks good to me. Reviewed-by: Zheng Liu I will pick it up into my patch set for improving es shrinker and look at whether or not it can reduce the latency. Thanks, - Zheng > --- > fs/ext4/extents_status.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c > index 3f5c188..0b7e28e 100644 > --- a/fs/ext4/extents_status.c > +++ b/fs/ext4/extents_status.c > @@ -966,10 +966,10 @@ retry: > continue; > } > > - if (ei->i_es_lru_nr == 0 || ei == locked_ei) > + if (ei->i_es_lru_nr == 0 || ei == locked_ei || > + !write_trylock(&ei->i_es_lock)) > continue; > > - write_lock(&ei->i_es_lock); > shrunk = __es_try_to_reclaim_extents(ei, nr_to_scan); > if (ei->i_es_lru_nr == 0) > list_del_init(&ei->i_es_lru); > -- > 2.0.0 >