From: Nick Piggin Subject: [patch] fs: fix deadlocks in writeback_if_idle Date: Tue, 23 Nov 2010 21:02:39 +1100 Message-ID: <20101123100239.GA4232@amd> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii To: linux-fsdevel@vger.kernel.org, Andrew Morton , Al Viro , linux-ext4@vger.kernel.org, linux-btrfs@vger.kernel.org, Jan Kara Received: from ipmail04.adl6.internode.on.net ([150.101.137.141]:53794 "EHLO ipmail04.adl6.internode.on.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751024Ab0KWKCu (ORCPT ); Tue, 23 Nov 2010 05:02:50 -0500 Content-Disposition: inline Sender: linux-ext4-owner@vger.kernel.org List-ID: Taking s_umount lock inside i_mutex can result in an ABBA deadlock: ======================================================= [ INFO: possible circular locking dependency detected ] 2.6.37-rc3+ #26 ------------------------------------------------------- append_writer/12828 is trying to acquire lock: (&type->s_umount_key#24){+++++.}, at: [] writeback_inodes_sb_if_idle+0x32/0x60 but task is already holding lock: (&sb->s_type->i_mutex_key#14){+.+.+.}, at: [] generic_file_aio_write+0x53/0xd0 which lock already depends on the new lock. the existing dependency chain (in reverse order) is: -> #3 (&sb->s_type->i_mutex_key#14){+.+.+.}: [] lock_acquire+0x95/0x1b0 [] __mutex_lock_common+0x59/0x480 [] mutex_lock_nested+0x3e/0x50 [] ext4_end_io_work+0x37/0xb0 [ext4] [] process_one_work+0x1b8/0x5a0 [] worker_thread+0x175/0x3a0 [] kthread+0x96/0xa0 [] kernel_thread_helper+0x4/0x10 -> #2 ((&io->work)){+.+...}: [] lock_acquire+0x95/0x1b0 [] process_one_work+0x1a4/0x5a0 [] worker_thread+0x175/0x3a0 [] kthread+0x96/0xa0 [] kernel_thread_helper+0x4/0x10 -> #1 (ext4-dio-unwritten){+.+...}: [] lock_acquire+0x95/0x1b0 [] flush_workqueue+0x148/0x540 [] ext4_sync_fs+0x3b/0x100 [ext4] [] __sync_filesystem+0x5e/0x90 [] sync_filesystem+0x32/0x60 [] generic_shutdown_super+0x2f/0x100 [] kill_block_super+0x2c/0x50 [] deactivate_locked_super+0x45/0x60 [] deactivate_super+0x45/0x60 [] mntput_no_expire+0xf0/0x190 [] sys_umount+0x79/0x3a0 [] system_call_fastpath+0x16/0x1b -> #0 (&type->s_umount_key#24){+++++.}: [] __lock_acquire+0x1382/0x1490 [] lock_acquire+0x95/0x1b0 [] down_read+0x42/0x60 [] writeback_inodes_sb_if_idle+0x32/0x60 [] ext4_da_write_begin+0x20d/0x310 [ext4] [] generic_file_buffered_write+0x114/0x2a0 [] __generic_file_aio_write+0x240/0x470 [] generic_file_aio_write+0x66/0xd0 [] ext4_file_write+0x3d/0xd0 [ext4] [] do_sync_write+0xd2/0x110 [] vfs_write+0xc8/0x190 [] sys_write+0x4c/0x80 [] system_call_fastpath+0x16/0x1b other info that might help us debug this: 1 lock held by append_writer/12828: #0: (&sb->s_type->i_mutex_key#14){+.+.+.}, at: [] generic_file_aio_write+0x53/0xd0 stack backtrace: Pid: 12828, comm: append_writer Not tainted 2.6.37-rc3+ #26 Call Trace: [] print_circular_bug+0xe9/0xf0 [] __lock_acquire+0x1382/0x1490 [] lock_acquire+0x95/0x1b0 [] ? writeback_inodes_sb_if_idle+0x32/0x60 [] ? sub_preempt_count+0x9d/0xd0 [] down_read+0x42/0x60 [] ? writeback_inodes_sb_if_idle+0x32/0x60 [] writeback_inodes_sb_if_idle+0x32/0x60 [] ext4_da_write_begin+0x20d/0x310 [ext4] [] ? up_read+0x1e/0x40 [] generic_file_buffered_write+0x114/0x2a0 [] ? current_fs_time+0x22/0x30 [] __generic_file_aio_write+0x240/0x470 [] ? generic_file_aio_write+0x53/0xd0 [] generic_file_aio_write+0x66/0xd0 [] ext4_file_write+0x3d/0xd0 [ext4] [] ? fsnotify+0x88/0x5e0 [] do_sync_write+0xd2/0x110 [] ? sub_preempt_count+0x9d/0xd0 [] ? trace_hardirqs_on_thunk+0x3a/0x3f [] ? smp_apic_timer_interrupt+0x6a/0xa0 [] vfs_write+0xc8/0x190 [] sys_write+0x4c/0x80 [] system_call_fastpath+0x16/0x1b Also, there can be an AA deadlock if the filesystem takes i_mutex in a workqueue, if it also waits for work completion while holding i_mutex. SysRq : Show Blocked State task PC stack pid father kworker/9:1 D 0000000000000000 6296 118 2 Call Trace: [] ? get_parent_ip+0x11/0x50 [] __mutex_lock_common+0x18c/0x480 [] ? ext4_end_io_work+0x37/0xb0 [ext4] [] ? ext4_end_io_work+0x37/0xb0 [ext4] [] mutex_lock_nested+0x3e/0x50 [] ext4_end_io_work+0x37/0xb0 [ext4] [] process_one_work+0x1b8/0x5a0 [] ? process_one_work+0x14e/0x5a0 [] ? ext4_end_io_work+0x0/0xb0 [ext4] [] worker_thread+0x175/0x3a0 [] ? worker_thread+0x0/0x3a0 [] kthread+0x96/0xa0 [] kernel_thread_helper+0x4/0x10 [] ? finish_task_switch+0x78/0x110 [] ? restore_args+0x0/0x30 [] ? kthread+0x0/0xa0 [] ? kernel_thread_helper+0x0/0x10 dbench D 0000000000000000 2872 2916 1 Call Trace: [] ? trace_hardirqs_off+0xd/0x10 [] schedule_timeout+0x1f5/0x350 [] ? mark_held_locks+0x6b/0xa0 [] ? _raw_spin_unlock_irq+0x2b/0x60 [] ? get_parent_ip+0x11/0x50 [] ? sub_preempt_count+0x9d/0xd0 [] wait_for_common+0x10d/0x190 [] ? default_wake_function+0x0/0x10 [] ? _raw_spin_unlock_bh+0x39/0x40 [] wait_for_completion+0x18/0x20 [] writeback_inodes_sb_nr+0xf7/0x120 [] writeback_inodes_sb+0x4d/0x60 [] ? writeback_inodes_sb_if_idle+0x32/0x60 [] writeback_inodes_sb_if_idle+0x3a/0x60 [] ext4_da_write_begin+0x20d/0x310 [] generic_file_buffered_write+0x114/0x2a0 Avoid both these issues by issuing completely asynchronous writeback request in writeback_inodes_sb_if_idle. Don't let that fool you into thinking these functions don't suck any more. ext4 now passes extensive stress testing with xfstests, fs_mark, dbench, with a writeback_inodes_if_idle call added directly into ext4_da_write_begin to trigger the path frequently. Previously it would spew lockdep stuff and hang in a number of ways very quickly. Signed-off-by: Nick Piggin --- fs/fs-writeback.c | 32 ++++++++++++++++++++------------ 1 file changed, 20 insertions(+), 12 deletions(-) Index: linux-2.6/fs/fs-writeback.c =================================================================== --- linux-2.6.orig/fs/fs-writeback.c 2010-11-23 20:57:23.000000000 +1100 +++ linux-2.6/fs/fs-writeback.c 2010-11-23 20:59:10.000000000 +1100 @@ -1152,16 +1152,17 @@ EXPORT_SYMBOL(writeback_inodes_sb); * * Invoke writeback_inodes_sb if no writeback is currently underway. * Returns 1 if writeback was started, 0 if not. + * + * Even if 1 is returned, writeback may not be started if memory allocation + * fails. This function makes no guarantees about anything. */ int writeback_inodes_sb_if_idle(struct super_block *sb) { if (!writeback_in_progress(sb->s_bdi)) { - down_read(&sb->s_umount); - writeback_inodes_sb(sb); - up_read(&sb->s_umount); + bdi_start_writeback(sb->s_bdi, get_nr_dirty_pages()); return 1; - } else - return 0; + } + return 0; } EXPORT_SYMBOL(writeback_inodes_sb_if_idle); @@ -1172,17 +1173,18 @@ EXPORT_SYMBOL(writeback_inodes_sb_if_idl * * Invoke writeback_inodes_sb if no writeback is currently underway. * Returns 1 if writeback was started, 0 if not. + * + * Even if 1 is returned, writeback may not be started if memory allocation + * fails. This function makes no guarantees about anything. */ int writeback_inodes_sb_nr_if_idle(struct super_block *sb, unsigned long nr) { if (!writeback_in_progress(sb->s_bdi)) { - down_read(&sb->s_umount); - writeback_inodes_sb_nr(sb, nr); - up_read(&sb->s_umount); + bdi_start_writeback(sb->s_bdi, nr); return 1; - } else - return 0; + } + return 0; } EXPORT_SYMBOL(writeback_inodes_sb_nr_if_idle);