fs: fix deadlocks in writeback_if_idle

int writeback_inodes_sb_if_idle(struct super_block *sb) {

fs/fs-writeback.c +6 -5

 	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)) {

The issue of writeback_inodes_sb being synchronous so far as it has to wait until the work has been dequeued is another subtlety. That is a funny interface though, really. It has 3 callers, sync, quota, and ubifs. From a quick look, quota and ubifs seem to think it is some kind of synchronous writeout API.

It also really sucks that it can get deadlocked behind an unrelated item in a workqueue. I think it should just get converted over to the async-submission style as well.

Quoted 5 lines · Nick Piggin

The issue of writeback_inodes_sb being synchronous so far as it has to wait until the work has been dequeued is another subtlety. That is a funny interface though, really. It has 3 callers, sync, quota, and ubifs. From a quick look, quota and ubifs seem to think it is some kind of synchronous writeout API.

Yes, they expect it and it used to be the case before per-bdi flusher
threads existed (because the function submitted IO on its own). Then it was changed to an async interface in per-bdi flusher thread patches and then back again to a synchronous one... Sad history...

Quoted 3 lines

It also really sucks that it can get deadlocked behind an unrelated item in a workqueue. I think it should just get converted over to the async-submission style as well.

Here I don't quite agree. After my patches (currently in -mm tree) all
work items have reasonably well defined lifetime so no livelocks should occur.

After all writeback thread is doing its best to do as much IO as possible (and hopefully saturates the storage) so given all the IO work items we cannot do much better. Where I see a room for improvement is that work items usually try to achieve a common goal - for example when we get two items "write all dirty pages", we have mostly fulfilled the second one after finishing the first one but we start from the beginning when processing the second request. But it seems non-trivial to do this request merging especially for different types of requests...

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.

Signed-off-by Nick Piggin <[email protected]>

---
 fs/fs-writeback.c |   32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

Index: linux-2.6/fs/fs-writeback.c
===================================================================

fs/fs-writeback.c +9 -7

@@ -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);

Quoted 36 lines

*
* 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)
{

fs/fs-writeback.c +6 -7

 	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);

fs/fs-writeback.c EXPORT_SYMBOL(writeback_inodes_sb_if_idl · line 1190

Quoted 10 lines

fs/fs-writeback.c +3 -3

-		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);
 

static inline int writeback_inodes_sb_if_idle(struct super_block *sb)

{
	return writeback_inodes_sb_nr_if_idle(sb, get_nr_dirty_pages());
}

In writeback.h, No?

But it has a single user so please just kill it.

Also writeback_inodes_sb_nr_if_idle() has a single user. Combined with above, two users. Why not open code it in the two sites. It should be much clearer to understand what the magic is all about?

Quoted 52 lines · Boaz Harrosh

Quoted 36 lines

*
* 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)
{

fs/fs-writeback.c +6 -7

 	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);

fs/fs-writeback.c EXPORT_SYMBOL(writeback_inodes_sb_if_idl · line 1190

Quoted 10 lines

fs/fs-writeback.c +3 -3

-		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);
 

static inline int writeback_inodes_sb_if_idle(struct super_block *sb)

{
	return writeback_inodes_sb_nr_if_idle(sb, get_nr_dirty_pages());
}

Quoted 1 line

In writeback.h, No?

I didn't care enough to move it :P I don't know if it matters.

Quoted 6 lines

But it has a single user so please just kill it.

Also writeback_inodes_sb_nr_if_idle() has a single user. Combined with above, two users. Why not open code it in the two sites. It should be much clearer to understand what the magic is all about?

The filesystem shouldn't be aware of the details (the "magic") of how to kick writeback, so I think the abstraction is right as is.

Thanks,
Nick

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 <[email protected]>

---
 fs/fs-writeback.c |   32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

Index: linux-2.6/fs/fs-writeback.c
===================================================================

fs/fs-writeback.c 2010-11-23 20:59:10.000000000 +1100 renamed from fs/fs-writeback.c 2010-11-23 20:57:23.000000000 +1100 +1 -0

@@ -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.
+ *

Excerpts from Nick Piggin's message of 2010-11-23 05:02:39 -0500:

[ ... ]

Quoted 32 lines

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 <[email protected]>

---
 fs/fs-writeback.c |   32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

Index: linux-2.6/fs/fs-writeback.c
===================================================================

fs/fs-writeback.c 2010-11-23 20:59:10.000000000 +1100 renamed from fs/fs-writeback.c 2010-11-23 20:57:23.000000000 +1100 +3 -1

@@ -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);

fs/fs-writeback.c 2010-11-23 20:59:10.000000000 +1100 EXPORT_SYMBOL(writeback_inodes_sb); · line 1162

Quoted 3 lines

fs/fs-writeback.c 2010-11-23 20:59:10.000000000 +1100 renamed from fs/fs-writeback.c 2010-11-23 20:57:23.000000000 +1100 +1 -2

-        writeback_inodes_sb(sb);
-        up_read(&sb->s_umount);
+        bdi_start_writeback(sb->s_bdi, get_nr_dirty_pages());

I'll put on my skis and channel Christoph for a minute. This isn't quite the same as the original. writeback_inodes_sb() writes inodes on a specific super block, while bdi_start_writeback() writes them for any SB on that bdi.

For btrfs there's only one bdi per SB, but for most everyone else a disk with a bunch of partitions is going to have multiple filesystems on the same bdi.

My original btrfs patch just exported the bdi_ funcs so that btrfs could do the above internally. But Christoph objected, and I think he's right. We should either give everyone a bdi or make sure the writeback func kicks only one filesystem.

Quoted 36 lines

Excerpts from Nick Piggin's message of 2010-11-23 05:02:39 -0500:

[ ... ]

Quoted 32 lines

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 <[email protected]>

---
 fs/fs-writeback.c |   32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

Index: linux-2.6/fs/fs-writeback.c
===================================================================

fs/fs-writeback.c 2010-11-23 20:59:10.000000000 +1100 renamed from fs/fs-writeback.c 2010-11-23 20:57:23.000000000 +1100 +3 -1

@@ -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);

Quoted 8 lines

fs/fs-writeback.c 2010-11-23 20:59:10.000000000 +1100 EXPORT_SYMBOL(writeback_inodes_sb); · line 1162

Quoted 3 lines

fs/fs-writeback.c 2010-11-23 20:59:10.000000000 +1100 renamed from fs/fs-writeback.c 2010-11-23 20:57:23.000000000 +1100 +1 -2

-        writeback_inodes_sb(sb);
-        up_read(&sb->s_umount);
+        bdi_start_writeback(sb->s_bdi, get_nr_dirty_pages());

I'll put on my skis and channel Christoph for a minute. This isn't quite the same as the original. writeback_inodes_sb() writes inodes on a specific super block, while bdi_start_writeback() writes them for any SB on that bdi.

Right.

Quoted 4 lines

For btrfs there's only one bdi per SB, but for most everyone else a disk with a bunch of partitions is going to have multiple filesystems on the same bdi.

Quoted 4 lines

My original btrfs patch just exported the bdi_ funcs so that btrfs could do the above internally. But Christoph objected, and I think he's right. We should either give everyone a bdi or make sure the writeback func kicks only one filesystem.

Well it's just kicking the writeback thread, and it will writeback from that particular sb.

It makes no further guarantees, and anyway
the sb has to compete for normal writeback within this bdi.

I think Christoph is right because filesystems should not really know about how bdi writeback queueing works. But I don't know if it's worth doing anything more complex for this functionality?

Excerpts from Nick Piggin's message of 2010-11-23 07:52:23 -0500:

Quoted 57 lines · Chris Mason

Quoted 36 lines

Excerpts from Nick Piggin's message of 2010-11-23 05:02:39 -0500:

[ ... ]

Quoted 32 lines

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 <[email protected]>

---
 fs/fs-writeback.c |   32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

Index: linux-2.6/fs/fs-writeback.c
===================================================================

fs/fs-writeback.c 2010-11-23 20:59:10.000000000 +1100 renamed from fs/fs-writeback.c 2010-11-23 20:57:23.000000000 +1100 +3 -1

@@ -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);

Quoted 8 lines

fs/fs-writeback.c 2010-11-23 20:59:10.000000000 +1100 EXPORT_SYMBOL(writeback_inodes_sb); · line 1162

Quoted 3 lines

fs/fs-writeback.c 2010-11-23 20:59:10.000000000 +1100 renamed from fs/fs-writeback.c 2010-11-23 20:57:23.000000000 +1100 +1 -2

-        writeback_inodes_sb(sb);
-        up_read(&sb->s_umount);
+        bdi_start_writeback(sb->s_bdi, get_nr_dirty_pages());

I'll put on my skis and channel Christoph for a minute. This isn't quite the same as the original. writeback_inodes_sb() writes inodes on a specific super block, while bdi_start_writeback() writes them for any SB on that bdi.

Right.

Quoted 8 lines

For btrfs there's only one bdi per SB, but for most everyone else a disk with a bunch of partitions is going to have multiple filesystems on the same bdi.

My original btrfs patch just exported the bdi_ funcs so that btrfs could do the above internally. But Christoph objected, and I think he's right. We should either give everyone a bdi or make sure the writeback func kicks only one filesystem.

Quoted 2 lines

Well it's just kicking the writeback thread, and it will writeback from that particular sb.

Hmmm? It will writeback for all the SBs on that bdi. In the current form that ext4 uses, that gets pretty expensive if you have a bunch of large partitions and you're only running out of space on one of them.

For the _nr variant that btrfs uses, it's worse for the filesystems that don't have a 1:1 bdi<->sb mapping. It might not actually write any of the pages from the SB that is out of space.

Quoted 7 lines

It makes no further guarantees, and anyway
the sb has to compete for normal writeback within this bdi.

I think Christoph is right because filesystems should not really know about how bdi writeback queueing works. But I don't know if it's worth doing anything more complex for this functionality?

I think we should make a writeback_inodes_sb_unlocked() that doesn't warn when the semaphore isn't held. That should be enough given where btrfs and ext4 are calling it from.

Quoted 3 lines

For the _nr variant that btrfs uses, it's worse for the filesystems that don't have a 1:1 bdi<->sb mapping. It might not actually write any of the pages from the SB that is out of space.

That's true, but it might not write anything anyway (and after we check whether writeout is in progress, the writeout thread could go to sleep and not do anything anyway).

So it's a pretty hacky interface anyway. If you want to do anything deterministic, you obviously need real coupling between producer and consumer. This should only be a performance tweak (or a workaround hack in worst case).

Quoted 8 lines

Quoted 7 lines

It makes no further guarantees, and anyway
the sb has to compete for normal writeback within this bdi.

I think Christoph is right because filesystems should not really know about how bdi writeback queueing works. But I don't know if it's worth doing anything more complex for this functionality?

Quoted 3 lines

I think we should make a writeback_inodes_sb_unlocked() that doesn't warn when the semaphore isn't held. That should be enough given where btrfs and ext4 are calling it from.

It doesn't solve the bugs -- calling and waiting for writeback is still broken because completion requires i_mutex and it is called from under i_mutex.

Quoted 4 lines · Nick Piggin

Quoted 3 lines

For the _nr variant that btrfs uses, it's worse for the filesystems that don't have a 1:1 bdi<->sb mapping. It might not actually write any of the pages from the SB that is out of space.

Quoted 8 lines

That's true, but it might not write anything anyway (and after we check whether writeout is in progress, the writeout thread could go to sleep and not do anything anyway).

So it's a pretty hacky interface anyway. If you want to do anything deterministic, you obviously need real coupling between producer and consumer. This should only be a performance tweak (or a workaround hack in worst case).

Yes, the current interface is a band aid for the problem and better
interface is welcome. But it's not trivial to do better...

Quoted 12 lines

Quoted 11 lines

Quoted 7 lines

It makes no further guarantees, and anyway
the sb has to compete for normal writeback within this bdi.

I think Christoph is right because filesystems should not really know about how bdi writeback queueing works. But I don't know if it's worth doing anything more complex for this functionality?

I think we should make a writeback_inodes_sb_unlocked() that doesn't warn when the semaphore isn't held. That should be enough given where btrfs and ext4 are calling it from.

Quoted 3 lines

It doesn't solve the bugs -- calling and waiting for writeback is still broken because completion requires i_mutex and it is called from under i_mutex.

Well, as I wrote in my previous email, only ext4 has the problem with
i_mutex and I personally view it as a bug. But ultimately it's Ted's call to decide.

Quoted 31 lines · Jan Kara

Quoted 4 lines · Nick Piggin

Quoted 3 lines

For the _nr variant that btrfs uses, it's worse for the filesystems that don't have a 1:1 bdi<->sb mapping. It might not actually write any of the pages from the SB that is out of space.

Quoted 8 lines

That's true, but it might not write anything anyway (and after we check whether writeout is in progress, the writeout thread could go to sleep and not do anything anyway).

So it's a pretty hacky interface anyway. If you want to do anything deterministic, you obviously need real coupling between producer and consumer. This should only be a performance tweak (or a workaround hack in worst case).

Yes, the current interface is a band aid for the problem and better
interface is welcome. But it's not trivial to do better...

Quoted 15 lines

Quoted 11 lines

Quoted 7 lines

It makes no further guarantees, and anyway
the sb has to compete for normal writeback within this bdi.

I think Christoph is right because filesystems should not really know about how bdi writeback queueing works. But I don't know if it's worth doing anything more complex for this functionality?

I think we should make a writeback_inodes_sb_unlocked() that doesn't warn when the semaphore isn't held. That should be enough given where btrfs and ext4 are calling it from.

It doesn't solve the bugs -- calling and waiting for writeback is still broken because completion requires i_mutex and it is called from under i_mutex.

Quoted 3 lines

Well, as I wrote in my previous email, only ext4 has the problem with
i_mutex and I personally view it as a bug. But ultimately it's Ted's call to decide.

Well, for now, the easiest and simplest fix is my patch, I think. The objection is that we may not write out anything for the specified sb, but the current implementation provides no such guarantees at all anyway, so I don't think it's a big issue.

	On Thu, 25 Nov 2010 14:53:56 +1100

Quoted 5 lines · Nick Piggin

On Wed, Nov 24, 2010 at 02:10:28PM +0100, Jan Kara wrote:
Well, for now, the easiest and simplest fix is my patch, I think. The objection is that we may not write out anything for the specified sb, but the current implementation provides no such guarantees at all anyway, so I don't think it's a big issue.

Well yes. We take something which will fail occasionally and with your patch replace it with something which will fail a bit more often. Why don't we go all the way and do something which will fail *even more often*. Namely, just delete the damn function in the hope that the resulting failures will provoke the ext4 crew into doing something sane this time?

Guys, this delalloc thing *sucks*. And here we are just sticking new bandaids on top of the old bandaids. And the btrfs approach isn't exactly a thing of glory, either.

So... nope. I won't be applying Nick's patch. Please fix this thing properly - you have a whole month!

Quoted 8 lines · Andrew Morton

	On Thu, 25 Nov 2010 14:53:56 +1100

Quoted 5 lines · Nick Piggin

On Wed, Nov 24, 2010 at 02:10:28PM +0100, Jan Kara wrote:
Well, for now, the easiest and simplest fix is my patch, I think. The objection is that we may not write out anything for the specified sb, but the current implementation provides no such guarantees at all anyway, so I don't think it's a big issue.

Quoted 6 lines

Well yes. We take something which will fail occasionally and with your patch replace it with something which will fail a bit more often. Why don't we go all the way and do something which will fail *even more often*. Namely, just delete the damn function in the hope that the resulting failures will provoke the ext4 crew into doing something sane this time?

I just need it fixed because the deadlocks are constantly hanging my tests and/or switching off lockdep.

Quoted 4 lines

Guys, this delalloc thing *sucks*. And here we are just sticking new bandaids on top of the old bandaids. And the btrfs approach isn't exactly a thing of glory, either.

Quoted 2 lines

So... nope. I won't be applying Nick's patch. Please fix this thing properly - you have a whole month!

Testers have less. It would be better to fix it now and rip it out at the start of the next merge window if you're that way inclined :)

Quoted 8 lines · Chris Mason

Quoted 7 lines

Quoted 4 lines

My original btrfs patch just exported the bdi_ funcs so that btrfs could do the above internally. But Christoph objected, and I think he's right. We should either give everyone a bdi or make sure the writeback func kicks only one filesystem.

Well it's just kicking the writeback thread, and it will writeback from that particular sb.

Quoted 3 lines

Hmmm? It will writeback for all the SBs on that bdi. In the current form that ext4 uses, that gets pretty expensive if you have a bunch of large partitions and you're only running out of space on one of them.

Right. But if the bdi has writeback in progress (which would be most of the time, on a busy filesystem), writeback_if_idle doesn't do anything, and it is happy just for the background writeback to eventually get around to writing out for us.

Quoted 9 lines · On Wed, 24 Nov 2010 12:03:43 +1100 Nick Piggin <[email protected]> wrote:

Quoted 8 lines · Chris Mason

Quoted 7 lines

Quoted 4 lines

My original btrfs patch just exported the bdi_ funcs so that btrfs could do the above internally. But Christoph objected, and I think he's right. We should either give everyone a bdi or make sure the writeback func kicks only one filesystem.

Well it's just kicking the writeback thread, and it will writeback from that particular sb.

Quoted 8 lines

Quoted 3 lines

Hmmm? It will writeback for all the SBs on that bdi. In the current form that ext4 uses, that gets pretty expensive if you have a bunch of large partitions and you're only running out of space on one of them.

Right. But if the bdi has writeback in progress (which would be most of the time, on a busy filesystem), writeback_if_idle doesn't do anything, and it is happy just for the background writeback to eventually get around to writing out for us.

That doesn't work if you're running btfs (apparently short for "busticated filesystem") because the bdi-per-sb thing carefully hid the information which you're looking for.

We still don't have a fix for this bug yet, it appears, btw.

For btrfs there's only one bdi per SB, but for most everyone else a disk with a bunch of partitions is going to have multiple filesystems on the same bdi.

Quoted 3 lines · On Tue, 23 Nov 2010 07:34:07 -0500 Chris Mason <[email protected]> wrote:

For btrfs there's only one bdi per SB, but for most everyone else a disk with a bunch of partitions is going to have multiple filesystems on the same bdi.

um, please explain why that wasn't idiotic? The BDI is a representation of a backing device and it's *supposed* to provide visibility into what's happening against other partitions on the same device. Creating a BDI per SB (it didn't even occur to me to think that a filesystem was even able to do this) breaks that.

Quoted 7 lines · Andrew Morton

Quoted 3 lines · On Tue, 23 Nov 2010 07:34:07 -0500 Chris Mason <[email protected]> wrote:

For btrfs there's only one bdi per SB, but for most everyone else a disk with a bunch of partitions is going to have multiple filesystems on the same bdi.

Quoted 6 lines

um, please explain why that wasn't idiotic? The BDI is a representation of a backing device and it's *supposed* to provide visibility into what's happening against other partitions on the same device. Creating a BDI per SB (it didn't even occur to me to think that a filesystem was even able to do this) breaks that.

In btrfs an SB my span multiple partitions. How else can it be solved?

Quoted 3 lines · On Tue, 23 Nov 2010 07:34:07 -0500 Chris Mason <[email protected]> wrote:

For btrfs there's only one bdi per SB, but for most everyone else a disk with a bunch of partitions is going to have multiple filesystems on the same bdi.

um, please explain why that wasn't idiotic? The BDI is a representation of a backing device and it's *supposed* to provide visibility into what's happening against other partitions on the same device. Creating a BDI per SB (it didn't even occur to me to think that a filesystem was even able to do this) breaks that.

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.

Signed-off-by Nick Piggin <[email protected]>

---
 fs/fs-writeback.c |   32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

Index: linux-2.6/fs/fs-writeback.c
===================================================================

fs/fs-writeback.c +12 -11

@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

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.

Signed-off-by Nick Piggin <[email protected]>

---
 fs/fs-writeback.c |   32 ++++++++++++++++++++------------
 1 file changed, 20 insertions(+), 12 deletions(-)

Index: linux-2.6/fs/fs-writeback.c
===================================================================

fs/fs-writeback.c +12 -11

@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html