Skip to contentSkip to conversation

block, writeback: wait for writeback to finish before detaching wb

PATCH Superseded Tahsin Erdogan 3 participants 5 messages Mar 9, 2017

Subsystems (2)
Stable branch VFS
lore
Share & tools
lore

Tahsin Erdogan

author
#0 lore raw

PATCH 1/1 block, writeback: wait for writeback to finish before detaching wb

__blkdev_put() could surprise writeback thread by detaching the wb object from an inode that hasn't cleared the I_SYNC flag yet. This causes a NULL pointer dereference as seen below:

  BUG: unable to handle kernel NULL pointer dereference at (null)
  IP: locked_inode_to_wb_and_lock_list+0x38/0x440
  PGD 0
  Oops: 0000 [#1] SMP
  CPU: 0 PID: 34 Comm: kworker/u8:1 Not tainted 4.11.0-rc1+ #202
  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
  Workqueue: writeback wb_workfn (flush-8:16)
  task: ffff88013aa780c0 task.stack: ffffc9000012c000
  RIP: 0010:locked_inode_to_wb_and_lock_list+0x38/0x440
  RSP: 0018:ffffc9000012fb70 EFLAGS: 00010202
  RAX: 0000000000000001 RBX: 0000000000000000 RCX: 0000000000000018
  RDX: ffff88013aa780c0 RSI: ffff880139a478f8 RDI: ffff88013aa788b8
  RBP: ffffc9000012fba0 R08: 0000000000000001 R09: 0000000000000000
  R10: 00000000969da8e2 R11: 0000000000000000 R12: ffff880139a47858
  R13: ffff880139a478e0 R14: ffff880139a478f8 R15: ffff8801371f4058
  FS:  0000000000000000(0000) GS:ffff88013ae00000(0000) knlGS:0000000000000000
  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
  CR2: 0000000000000000 CR3: 0000000001012000 CR4: 00000000000006f0
  Call Trace:
   writeback_sb_inodes+0x3e1/0x7a0
   __writeback_inodes_wb+0x87/0xc0
   wb_writeback+0x2e7/0x5c0
   wb_workfn+0x2d1/0x9c0
   process_one_work+0x1d3/0x620
   worker_thread+0x126/0x4a0
   kthread+0x10a/0x140
   ret_from_fork+0x2e/0x40
  RIP: locked_inode_to_wb_and_lock_list+0x38/0x440 RSP: ffffc9000012fb70
  CR2: 0000000000000000
  ---[ end trace e0ea8a2695f4c86c ]---

Make __blkdev_put() wait for the I_SYNC flag to clear before detaching wb.

Fixes 43d1c0eb7e11 ("block: detach bdev inode from its wb in __blkdev_put()")
Signed-off-by Tahsin Erdogan <[email protected]>
---
 fs/block_dev.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)
fs/block_dev.c +7 -4
index 2eca00ec4370..70fb82fcedd0 100644
@@ -95,7 +95,7 @@ void kill_bdev(struct block_device *bdev)
 
 	invalidate_bh_lrus();
 	truncate_inode_pages(mapping, 0);
-}	
+}
 EXPORT_SYMBOL(kill_bdev);
 
 /* Invalidate clean unused buffers and pagecache. */

@@ -617,13 +617,13 @@ static loff_t block_llseek(struct file *file, loff_t offset, int whence)
 	inode_unlock(bd_inode);
 	return retval;
 }
-	
+
 int blkdev_fsync(struct file *filp, loff_t start, loff_t end, int datasync)
 {
 	struct inode *bd_inode = bdev_file_inode(filp);
 	struct block_device *bdev = I_BDEV(bd_inode);
 	int error;
-	
+
 	error = filemap_write_and_wait_range(filp->f_mapping, start, end);
 	if (error)
 		return error;

@@ -1038,7 +1038,7 @@ void bdput(struct block_device *bdev)
 }
 
 EXPORT_SYMBOL(bdput);
- 
+
 static struct block_device *bd_acquire(struct inode *inode)
 {
 	struct block_device *bdev;

@@ -1880,7 +1880,10 @@ static void __blkdev_put(struct block_device *bdev, fmode_t mode, int for_part)
 		 * Detaching bdev inode from its wb in __destroy_inode()
 		 * is too late: the queue which embeds its bdi (along with
 		 * root wb) can be gone as soon as we put_disk() below.
+		 * Before detaching wb, wait for any writeback activity for
+		 * inode to settle.
 		 */
+		inode_wait_for_writeback(bdev->bd_inode);
 		inode_detach_wb(bdev->bd_inode);
 	}
 	if (bdev->bd_contains == bdev) {
Signature · 1 line
2.12.0.246.ga2ecc84866-goog
#1 lore raw

(cc'ing Jan and quoting the whole message)

Quoted 74 lines · Tahsin Erdogan

__blkdev_put() could surprise writeback thread by detaching the wb object from an inode that hasn't cleared the I_SYNC flag yet. This causes a NULL pointer dereference as seen below:

  BUG: unable to handle kernel NULL pointer dereference at (null)
  IP: locked_inode_to_wb_and_lock_list+0x38/0x440
  PGD 0
  Oops: 0000 [#1] SMP
  CPU: 0 PID: 34 Comm: kworker/u8:1 Not tainted 4.11.0-rc1+ #202
  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
  Workqueue: writeback wb_workfn (flush-8:16)
  task: ffff88013aa780c0 task.stack: ffffc9000012c000
  RIP: 0010:locked_inode_to_wb_and_lock_list+0x38/0x440
  RSP: 0018:ffffc9000012fb70 EFLAGS: 00010202
  RAX: 0000000000000001 RBX: 0000000000000000 RCX: 0000000000000018
  RDX: ffff88013aa780c0 RSI: ffff880139a478f8 RDI: ffff88013aa788b8
  RBP: ffffc9000012fba0 R08: 0000000000000001 R09: 0000000000000000
  R10: 00000000969da8e2 R11: 0000000000000000 R12: ffff880139a47858
  R13: ffff880139a478e0 R14: ffff880139a478f8 R15: ffff8801371f4058
  FS:  0000000000000000(0000) GS:ffff88013ae00000(0000) knlGS:0000000000000000
  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
  CR2: 0000000000000000 CR3: 0000000001012000 CR4: 00000000000006f0
  Call Trace:
   writeback_sb_inodes+0x3e1/0x7a0
   __writeback_inodes_wb+0x87/0xc0
   wb_writeback+0x2e7/0x5c0
   wb_workfn+0x2d1/0x9c0
   process_one_work+0x1d3/0x620
   worker_thread+0x126/0x4a0
   kthread+0x10a/0x140
   ret_from_fork+0x2e/0x40
  RIP: locked_inode_to_wb_and_lock_list+0x38/0x440 RSP: ffffc9000012fb70
  CR2: 0000000000000000
  ---[ end trace e0ea8a2695f4c86c ]---

Make __blkdev_put() wait for the I_SYNC flag to clear before detaching wb.

Fixes 43d1c0eb7e11 ("block: detach bdev inode from its wb in __blkdev_put()")
Signed-off-by Tahsin Erdogan <[email protected]>
---
 fs/block_dev.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)
fs/block_dev.c +3 -3
index 2eca00ec4370..70fb82fcedd0 100644
@@ -95,7 +95,7 @@ void kill_bdev(struct block_device *bdev)
 
 	invalidate_bh_lrus();
 	truncate_inode_pages(mapping, 0);
-}	
+}
 EXPORT_SYMBOL(kill_bdev);
 
 /* Invalidate clean unused buffers and pagecache. */

@@ -617,13 +617,13 @@ static loff_t block_llseek(struct file *file, loff_t offset, int whence)
 	inode_unlock(bd_inode);
 	return retval;
 }
-	
+
 int blkdev_fsync(struct file *filp, loff_t start, loff_t end, int datasync)
 {
 	struct inode *bd_inode = bdev_file_inode(filp);
 	struct block_device *bdev = I_BDEV(bd_inode);
 	int error;
-	
+
 	error = filemap_write_and_wait_range(filp->f_mapping, start, end);
 	if (error)
 		return error;

fs/block_dev.c void bdput(struct block_device *bdev) · line 1041

Quoted 5 lines
fs/block_dev.c +1 -1
@@ -1038,7 +1038,7 @@ void bdput(struct block_device *bdev)
 }
 
 EXPORT_SYMBOL(bdput);
- 
+

White line contaminations.

Quoted 4 lines
fs/block_dev.c +0 -0
 static struct block_device *bd_acquire(struct inode *inode)
 {
 	struct block_device *bdev;

fs/block_dev.c static void __blkdev_put(struct block_device *bdev, fmode_t mode, int for_part) · line 1889

Quoted 10 lines
fs/block_dev.c +3 -0
@@ -1880,7 +1880,10 @@ static void __blkdev_put(struct block_device *bdev, fmode_t mode, int for_part)
 		 * Detaching bdev inode from its wb in __destroy_inode()
 		 * is too late: the queue which embeds its bdi (along with
 		 * root wb) can be gone as soon as we put_disk() below.
+		 * Before detaching wb, wait for any writeback activity for
+		 * inode to settle.
 		 */
+		inode_wait_for_writeback(bdev->bd_inode);
 		inode_detach_wb(bdev->bd_inode);
 	}
 	if (bdev->bd_contains == bdev) {

Given that there's write_inode_now(@sync == true) call right above, I'm not sure how waiting for I_SYNC once more time would make a difference. Can you please explain how to trigger the issue?

Thanks.

Signature · 1 line
tejun
author
#2 lore raw

PATCH v2 1/1 block, writeback: wait for writeback to finish before detaching wb

__blkdev_put() could surprise writeback thread by detaching the wb object from an inode that hasn't cleared the I_SYNC flag yet. This causes a NULL pointer dereference as seen below:

  BUG: unable to handle kernel NULL pointer dereference at (null)
  IP: locked_inode_to_wb_and_lock_list+0x38/0x440
  PGD 0
  Oops: 0000 [#1] SMP
  CPU: 0 PID: 34 Comm: kworker/u8:1 Not tainted 4.11.0-rc1+ #202
  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
  Workqueue: writeback wb_workfn (flush-8:16)
  task: ffff88013aa780c0 task.stack: ffffc9000012c000
  RIP: 0010:locked_inode_to_wb_and_lock_list+0x38/0x440
  RSP: 0018:ffffc9000012fb70 EFLAGS: 00010202
  RAX: 0000000000000001 RBX: 0000000000000000 RCX: 0000000000000018
  RDX: ffff88013aa780c0 RSI: ffff880139a478f8 RDI: ffff88013aa788b8
  RBP: ffffc9000012fba0 R08: 0000000000000001 R09: 0000000000000000
  R10: 00000000969da8e2 R11: 0000000000000000 R12: ffff880139a47858
  R13: ffff880139a478e0 R14: ffff880139a478f8 R15: ffff8801371f4058
  FS:  0000000000000000(0000) GS:ffff88013ae00000(0000) knlGS:0000000000000000
  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
  CR2: 0000000000000000 CR3: 0000000001012000 CR4: 00000000000006f0
  Call Trace:
   writeback_sb_inodes+0x3e1/0x7a0
   __writeback_inodes_wb+0x87/0xc0
   wb_writeback+0x2e7/0x5c0
   wb_workfn+0x2d1/0x9c0
   process_one_work+0x1d3/0x620
   worker_thread+0x126/0x4a0
   kthread+0x10a/0x140
   ret_from_fork+0x2e/0x40
  RIP: locked_inode_to_wb_and_lock_list+0x38/0x440 RSP: ffffc9000012fb70
  CR2: 0000000000000000
  ---[ end trace e0ea8a2695f4c86c ]---

Make __blkdev_put() wait for the I_SYNC flag to clear before detaching wb.

Fixes 43d1c0eb7e11 ("block: detach bdev inode from its wb in __blkdev_put()")
Signed-off-by Tahsin Erdogan <[email protected]>

---
v2:
Removed white space clean up changes

 fs/block_dev.c | 3 +++
 1 file changed, 3 insertions(+)
fs/block_dev.c +3 -0
index 2eca00ec4370..fdc71f9f8003 100644
@@ -1880,7 +1880,10 @@ static void __blkdev_put(struct block_device *bdev, fmode_t mode, int for_part)
 		 * Detaching bdev inode from its wb in __destroy_inode()
 		 * is too late: the queue which embeds its bdi (along with
 		 * root wb) can be gone as soon as we put_disk() below.
+		 * Before detaching wb, wait for any writeback activity for
+		 * inode to settle.
 		 */
+		inode_wait_for_writeback(bdev->bd_inode);
 		inode_detach_wb(bdev->bd_inode);
 	}
 	if (bdev->bd_contains == bdev) {
Signature · 1 line
2.12.0.246.ga2ecc84866-goog
author
#3 lore raw

Hi Tejun,

Quoted 2 lines

White line contaminations.

Fixed in v2.

Quoted 3 lines

Given that there's write_inode_now(@sync == true) call right above, I'm not sure how waiting for I_SYNC once more time would make a difference. Can you please explain how to trigger the issue?

bdev_write_inode() calls write_inode_now() only if I_DIRTY is set for the inode. It is possible that I_DIRTY flags have been cleared by the writeback thread, but it hasn't run node_sync_complete() yet.

I am able to reproduce the problem by adding articial delays in __blkdev_put() and writeback_sb_inodes(). Please see the repro patch below:

fs/block_dev.c +1 -0
index 2eca00ec4370..e3cfbba19a6c 100644
@@ -33,6 +33,7 @@
 #include <linux/task_io_accounting_ops.h>
 #include <linux/falloc.h>
 #include <linux/uaccess.h>
+#include <linux/delay.h>
 #include "internal.h"
 
 struct bdev_inode {

@@ -1875,6 +1876,8 @@ static void __blkdev_put(struct block_device

*bdev, fmode_t mode, int for_part)

                sync_blockdev(bdev);
                kill_bdev(bdev);

+ /* Allow time for writeback thread to clear I_DIRTY flags */

+               mdelay(200);
                bdev_write_inode(bdev);
                /*
                 * Detaching bdev inode from its wb in __destroy_inode()
fs/fs-writeback.c +5 -0
index ef600591d96f..55073cbc1823 100644
@@ -28,6 +28,7 @@
 #include <linux/tracepoint.h>
 #include <linux/device.h>
 #include <linux/memcontrol.h>
+#include <linux/delay.h>
 #include "internal.h"
 
 /*

@@ -1585,6 +1586,10 @@ static long writeback_sb_inodes(struct super_block *sb,
                        cond_resched();
                }
 
+               /* Allow __blkdev_put() to run inode_detach_wb() */
+               if (inode->i_ino == 0)
+                       mdelay(200);
+
                /*
                 * Requeue @inode if still dirty.  Be careful as @inode may
                 * have been switched to another wb in the meantime.
wb-detach-repro.sh new file +10 -0
new file mode 100755
index 000000000000..66892d71cd20
@@ -0,0 +1,10 @@
+#!/bin/bash
+set -ex
+
+echo 100 > /proc/sys/vm/dirty_expire_centisecs
+echo 100 > /proc/sys/vm/dirty_writeback_centisecs
+
+mount /dev/sdb /mnt/sdb
+sleep 0.9
+echo 10 > /proc/sys/vm/dirty_writeback_centisecs
+umount /dev/sdb
reviewer
#4 lore raw
In reply to Tejun Heo
Quoted 95 lines · Tejun Heo

(cc'ing Jan and quoting the whole message)

Quoted 74 lines · Tahsin Erdogan

__blkdev_put() could surprise writeback thread by detaching the wb object from an inode that hasn't cleared the I_SYNC flag yet. This causes a NULL pointer dereference as seen below:

  BUG: unable to handle kernel NULL pointer dereference at (null)
  IP: locked_inode_to_wb_and_lock_list+0x38/0x440
  PGD 0
  Oops: 0000 [#1] SMP
  CPU: 0 PID: 34 Comm: kworker/u8:1 Not tainted 4.11.0-rc1+ #202
  Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
  Workqueue: writeback wb_workfn (flush-8:16)
  task: ffff88013aa780c0 task.stack: ffffc9000012c000
  RIP: 0010:locked_inode_to_wb_and_lock_list+0x38/0x440
  RSP: 0018:ffffc9000012fb70 EFLAGS: 00010202
  RAX: 0000000000000001 RBX: 0000000000000000 RCX: 0000000000000018
  RDX: ffff88013aa780c0 RSI: ffff880139a478f8 RDI: ffff88013aa788b8
  RBP: ffffc9000012fba0 R08: 0000000000000001 R09: 0000000000000000
  R10: 00000000969da8e2 R11: 0000000000000000 R12: ffff880139a47858
  R13: ffff880139a478e0 R14: ffff880139a478f8 R15: ffff8801371f4058
  FS:  0000000000000000(0000) GS:ffff88013ae00000(0000) knlGS:0000000000000000
  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
  CR2: 0000000000000000 CR3: 0000000001012000 CR4: 00000000000006f0
  Call Trace:
   writeback_sb_inodes+0x3e1/0x7a0
   __writeback_inodes_wb+0x87/0xc0
   wb_writeback+0x2e7/0x5c0
   wb_workfn+0x2d1/0x9c0
   process_one_work+0x1d3/0x620
   worker_thread+0x126/0x4a0
   kthread+0x10a/0x140
   ret_from_fork+0x2e/0x40
  RIP: locked_inode_to_wb_and_lock_list+0x38/0x440 RSP: ffffc9000012fb70
  CR2: 0000000000000000
  ---[ end trace e0ea8a2695f4c86c ]---

Make __blkdev_put() wait for the I_SYNC flag to clear before detaching wb.

Fixes 43d1c0eb7e11 ("block: detach bdev inode from its wb in __blkdev_put()")
Signed-off-by Tahsin Erdogan <[email protected]>
---
 fs/block_dev.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)
fs/block_dev.c +3 -3
index 2eca00ec4370..70fb82fcedd0 100644
@@ -95,7 +95,7 @@ void kill_bdev(struct block_device *bdev)
 
 	invalidate_bh_lrus();
 	truncate_inode_pages(mapping, 0);
-}	
+}
 EXPORT_SYMBOL(kill_bdev);
 
 /* Invalidate clean unused buffers and pagecache. */

@@ -617,13 +617,13 @@ static loff_t block_llseek(struct file *file, loff_t offset, int whence)
 	inode_unlock(bd_inode);
 	return retval;
 }
-	
+
 int blkdev_fsync(struct file *filp, loff_t start, loff_t end, int datasync)
 {
 	struct inode *bd_inode = bdev_file_inode(filp);
 	struct block_device *bdev = I_BDEV(bd_inode);
 	int error;
-	
+
 	error = filemap_write_and_wait_range(filp->f_mapping, start, end);
 	if (error)
 		return error;

fs/block_dev.c void bdput(struct block_device *bdev) · line 1041

Quoted 5 lines
fs/block_dev.c +1 -1
@@ -1038,7 +1038,7 @@ void bdput(struct block_device *bdev)
 }
 
 EXPORT_SYMBOL(bdput);
- 
+

White line contaminations.

Quoted 10 lines
fs/block_dev.c +2 -0
 static struct block_device *bd_acquire(struct inode *inode)
 {
 	struct block_device *bdev;

@@ -1880,7 +1880,10 @@ static void __blkdev_put(struct block_device *bdev, fmode_t mode, int for_part)
 		 * Detaching bdev inode from its wb in __destroy_inode()
 		 * is too late: the queue which embeds its bdi (along with
 		 * root wb) can be gone as soon as we put_disk() below.
+		 * Before detaching wb, wait for any writeback activity for
+		 * inode to settle.
 		 */
Quoted 8 lines

fs/block_dev.c static void __blkdev_put(struct block_device *bdev, fmode_t mode, int for_part) · line 1889

Quoted 4 lines
fs/block_dev.c +1 -0
+		inode_wait_for_writeback(bdev->bd_inode);
 		inode_detach_wb(bdev->bd_inode);
 	}
 	if (bdev->bd_contains == bdev) {

Given that there's write_inode_now(@sync == true) call right above, I'm not sure how waiting for I_SYNC once more time would make a difference. Can you please explain how to trigger the issue?

Agreed that waiting for I_SYNC isn't going to cut it. Flusher thread (or possibly some other process) can be running inode_to_wb() without I_SYNC set. I have patches that fix this (remove inode_detach_wb() from __blkdev_put()) - the ones you were reviewing lately. I'll post v4 next week so that we can get those merged.

But if Tahsin has a reproducer, it would be nice. I was not able to hit the race in my testing.

Honza

Signature · 2 lines
Jan Kara <[email protected]>
SUSE Labs, CR