From: Dmitry <dmonakhov@openvz.org>
Subject: Re: [patch] fs: fix deadlocks in writeback_if_idle
Date: Tue, 23 Nov 2010 15:54:13 +0300
Message-ID: <8762vo9p56.fsf@dmon-lap.sw.ru>
References: <20101123100239.GA4232@amd>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
To: Nick Piggin <npiggin@kernel.dk>, linux-fsdevel@vger.kernel.org,
	Andrew Morton <akpm@linux-foundation.org>,
	Al Viro <viro@ZenIV.linux.org.uk>, linux-ext4@vger.kernel.org,
	linux-btrfs@v
Return-path: <linux-ext4-owner@vger.kernel.org>
Received: from mail-ew0-f46.google.com ([209.85.215.46]:47027 "EHLO
	mail-ew0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1751079Ab0KWMy2 (ORCPT
	<rfc822;linux-ext4@vger.kernel.org>); Tue, 23 Nov 2010 07:54:28 -0500
In-Reply-To: <20101123100239.GA4232@amd>
Sender: linux-ext4-owner@vger.kernel.org
List-ID: <linux-ext4.vger.kernel.org>

On Tue, 23 Nov 2010 21:02:39 +1100, Nick Piggin <npiggin@kernel.dk> wrote:
> 
> 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:
> 	[<ffffffff8113d6d2>] writeback_inodes_sb_if_idle+0x32/0x60
>  
>  but task is already holding lock:
>   (&sb->s_type->i_mutex_key#14){+.+.+.}, at:
> 	[<ffffffff810cc863>] 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){+.+.+.}:
>         [<ffffffff810852c5>] lock_acquire+0x95/0x1b0
>         [<ffffffff81601329>] __mutex_lock_common+0x59/0x480
>         [<ffffffff8160182e>] mutex_lock_nested+0x3e/0x50
>         [<ffffffffa003a147>] ext4_end_io_work+0x37/0xb0 [ext4]
>         [<ffffffff81068378>] process_one_work+0x1b8/0x5a0
>         [<ffffffff81069675>] worker_thread+0x175/0x3a0
>         [<ffffffff8106e246>] kthread+0x96/0xa0
>         [<ffffffff81003ed4>] kernel_thread_helper+0x4/0x10
>  
>  -> #2 ((&io->work)){+.+...}:
>         [<ffffffff810852c5>] lock_acquire+0x95/0x1b0
>         [<ffffffff81068364>] process_one_work+0x1a4/0x5a0
>         [<ffffffff81069675>] worker_thread+0x175/0x3a0
>         [<ffffffff8106e246>] kthread+0x96/0xa0
>         [<ffffffff81003ed4>] kernel_thread_helper+0x4/0x10
>  
>  -> #1 (ext4-dio-unwritten){+.+...}:
>         [<ffffffff810852c5>] lock_acquire+0x95/0x1b0
>         [<ffffffff81067bc8>] flush_workqueue+0x148/0x540
>         [<ffffffffa004761b>] ext4_sync_fs+0x3b/0x100 [ext4]
>         [<ffffffff8114304e>] __sync_filesystem+0x5e/0x90
>         [<ffffffff81143132>] sync_filesystem+0x32/0x60
>         [<ffffffff8111a97f>] generic_shutdown_super+0x2f/0x100
>         [<ffffffff8111aa7c>] kill_block_super+0x2c/0x50
>         [<ffffffff8111b1e5>] deactivate_locked_super+0x45/0x60
>         [<ffffffff8111b415>] deactivate_super+0x45/0x60
>         [<ffffffff81136430>] mntput_no_expire+0xf0/0x190
>         [<ffffffff811376a9>] sys_umount+0x79/0x3a0
>         [<ffffffff8100312b>] system_call_fastpath+0x16/0x1b
>  
>  -> #0 (&type->s_umount_key#24){+++++.}:
>         [<ffffffff81085122>] __lock_acquire+0x1382/0x1490
>         [<ffffffff810852c5>] lock_acquire+0x95/0x1b0
>         [<ffffffff81601ba2>] down_read+0x42/0x60
>         [<ffffffff8113d6d2>] writeback_inodes_sb_if_idle+0x32/0x60
>         [<ffffffffa0037efd>] ext4_da_write_begin+0x20d/0x310 [ext4]
>         [<ffffffff810cbbf4>] generic_file_buffered_write+0x114/0x2a0
>         [<ffffffff810cc5e0>] __generic_file_aio_write+0x240/0x470
>         [<ffffffff810cc876>] generic_file_aio_write+0x66/0xd0
>         [<ffffffffa002cfad>] ext4_file_write+0x3d/0xd0 [ext4]
>         [<ffffffff81117702>] do_sync_write+0xd2/0x110
>         [<ffffffff811179c8>] vfs_write+0xc8/0x190
>         [<ffffffff811183ec>] sys_write+0x4c/0x80
>         [<ffffffff8100312b>] 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:
> 	[<ffffffff810cc863>] generic_file_aio_write+0x53/0xd0
>  
>  stack backtrace:
>  Pid: 12828, comm: append_writer Not tainted 2.6.37-rc3+ #26
>  Call Trace:
>   [<ffffffff81082c39>] print_circular_bug+0xe9/0xf0
>   [<ffffffff81085122>] __lock_acquire+0x1382/0x1490
>   [<ffffffff810852c5>] lock_acquire+0x95/0x1b0
>   [<ffffffff8113d6d2>] ? writeback_inodes_sb_if_idle+0x32/0x60
>   [<ffffffff81606c3d>] ? sub_preempt_count+0x9d/0xd0
>   [<ffffffff81601ba2>] down_read+0x42/0x60
>   [<ffffffff8113d6d2>] ? writeback_inodes_sb_if_idle+0x32/0x60
>   [<ffffffff8113d6d2>] writeback_inodes_sb_if_idle+0x32/0x60
>   [<ffffffffa0037efd>] ext4_da_write_begin+0x20d/0x310 [ext4]
>   [<ffffffff81073dde>] ? up_read+0x1e/0x40
>   [<ffffffff810cbbf4>] generic_file_buffered_write+0x114/0x2a0
>   [<ffffffff8104fa22>] ? current_fs_time+0x22/0x30
>   [<ffffffff810cc5e0>] __generic_file_aio_write+0x240/0x470
>   [<ffffffff810cc863>] ? generic_file_aio_write+0x53/0xd0
>   [<ffffffff810cc876>] generic_file_aio_write+0x66/0xd0
>   [<ffffffffa002cfad>] ext4_file_write+0x3d/0xd0 [ext4]
>   [<ffffffff81150db8>] ? fsnotify+0x88/0x5e0
>   [<ffffffff81117702>] do_sync_write+0xd2/0x110
>   [<ffffffff81606c3d>] ? sub_preempt_count+0x9d/0xd0
>   [<ffffffff816027b9>] ? trace_hardirqs_on_thunk+0x3a/0x3f
>   [<ffffffff81022e9a>] ? smp_apic_timer_interrupt+0x6a/0xa0
>   [<ffffffff811179c8>] vfs_write+0xc8/0x190
>   [<ffffffff811183ec>] sys_write+0x4c/0x80
>   [<ffffffff8100312b>] 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:
> [<ffffffff81039431>] ? get_parent_ip+0x11/0x50
> [<ffffffff8160145c>] __mutex_lock_common+0x18c/0x480
> [<ffffffffa0042147>] ? ext4_end_io_work+0x37/0xb0 [ext4]
> [<ffffffffa0042147>] ? ext4_end_io_work+0x37/0xb0 [ext4]
> [<ffffffff8160182e>] mutex_lock_nested+0x3e/0x50
> [<ffffffffa0042147>] ext4_end_io_work+0x37/0xb0 [ext4]
> [<ffffffff81068378>] process_one_work+0x1b8/0x5a0
> [<ffffffff8106830e>] ? process_one_work+0x14e/0x5a0
> [<ffffffffa0042110>] ? ext4_end_io_work+0x0/0xb0 [ext4]
> [<ffffffff81069675>] worker_thread+0x175/0x3a0
> [<ffffffff81069500>] ? worker_thread+0x0/0x3a0
> [<ffffffff8106e246>] kthread+0x96/0xa0
> [<ffffffff81003ed4>] kernel_thread_helper+0x4/0x10
> [<ffffffff81039878>] ? finish_task_switch+0x78/0x110
> [<ffffffff816036c0>] ? restore_args+0x0/0x30
> [<ffffffff8106e1b0>] ? kthread+0x0/0xa0
> [<ffffffff81003ed0>] ? kernel_thread_helper+0x0/0x10
> 
> dbench        D 0000000000000000  2872  2916      1
> Call Trace:
> [<ffffffff810806fd>] ? trace_hardirqs_off+0xd/0x10
> [<ffffffff81600bb5>] schedule_timeout+0x1f5/0x350
> [<ffffffff8108380b>] ? mark_held_locks+0x6b/0xa0
> [<ffffffff8160314b>] ? _raw_spin_unlock_irq+0x2b/0x60
> [<ffffffff81039431>] ? get_parent_ip+0x11/0x50
> [<ffffffff81606c3d>] ? sub_preempt_count+0x9d/0xd0
> [<ffffffff815ffacd>] wait_for_common+0x10d/0x190
> [<ffffffff810426e0>] ? default_wake_function+0x0/0x10
> [<ffffffff81602ec9>] ? _raw_spin_unlock_bh+0x39/0x40
> [<ffffffff815ffbf8>] wait_for_completion+0x18/0x20
> [<ffffffff8113d1e7>] writeback_inodes_sb_nr+0xf7/0x120
> [<ffffffff8113d68d>] writeback_inodes_sb+0x4d/0x60
> [<ffffffff8113d6d2>] ?  writeback_inodes_sb_if_idle+0x32/0x60
> [<ffffffff8113d6da>] writeback_inodes_sb_if_idle+0x3a/0x60
> [<ffffffffa003fefd>] ext4_da_write_begin+0x20d/0x310
> [<ffffffff810cbbf4>] 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.
I'm still able to trigger strange lockups on fsstress, but in other
place. System hangs so quickly so i'm not succeed in grabbing logs yet.
> 
> Signed-off-by: Nick Piggin <npiggin@kernel.dk>
> 
> ---
>  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);
>  
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html