recent bdi changes result in WARN_ON on luksClose / dm

recent bdi changes result in WARN_ON on luksClose / dm

[Sebastian Andrzej Siewior]

| Orion:/# cryptsetup luksClose crypto
|------------[ cut here ]------------
|WARNING: at /home/bigeasy/git/cryptodev-2.6/mm/backing-dev.c:637 bdi_destroy+0x44/0x60()
|Modules linked in: cbc sha256_generic dm_crypt [last unloaded: mv_cesa]
|[<c0028dd8>] (unwind_backtrace+0x0/0xdc) from [<c003adb0>] (warn_slowpath_common+0x4c/0x80)
|[<c003adb0>] (warn_slowpath_common+0x4c/0x80) from [<c007f2e0>] (bdi_destroy+0x44/0x60)
|[<c007f2e0>] (bdi_destroy+0x44/0x60) from [<c012e3a0>] (blk_release_queue+0x3c/0x54)
|[<c012e3a0>] (blk_release_queue+0x3c/0x54) from [<c01399a8>] (kobject_release+0x5c/0x74)
|[<c01399a8>] (kobject_release+0x5c/0x74) from [<c013a6ac>] (kref_put+0x68/0x7c)
|[<c013a6ac>] (kref_put+0x68/0x7c) from [<c01b7098>] (dm_put+0x148/0x178)
|[<c01b7098>] (dm_put+0x148/0x178) from [<c01bb82c>] (dev_remove+0x98/0xb4)
|[<c01bb82c>] (dev_remove+0x98/0xb4) from [<c01bbec0>] (dm_ctl_ioctl+0x21c/0x29c)
|[<c01bbec0>] (dm_ctl_ioctl+0x21c/0x29c) from [<c00a153c>] (vfs_ioctl+0x2c/0x8c)
|[<c00a153c>] (vfs_ioctl+0x2c/0x8c) from [<c00a1bac>] (do_vfs_ioctl+0x524/0x590)
|[<c00a1bac>] (do_vfs_ioctl+0x524/0x590) from [<c00a1c50>] (sys_ioctl+0x38/0x5c)
|[<c00a1c50>] (sys_ioctl+0x38/0x5c) from [<c0023e80>] (ret_fast_syscall+0x0/0x2c)
|---[ end trace b3c544a14b51605c ]---

This is on v2.6.31-1335-g86d7101. A bisect between a12e4d3 and 89af571
leads to 
|commit 66f3b8e2e103a0b93b945764d98e9ba46cb926dd
|Author: Jens Axboe <jens.axboe@oracle.com>
|Date:   Wed Sep 2 09:19:46 2009 +0200
|
|    writeback: move dirty inodes from super_block to backing_dev_info
|    
|    This is a first step at introducing per-bdi flusher threads. We should
|    have no change in behaviour, although sb_has_dirty_inodes() is now
|    ridiculously expensive, as there's no easy way to answer that question.
|    Not a huge problem, since it'll be deleted in subsequent patches.
|    
|    Signed-off-by: Jens Axboe <jens.axboe@oracle.com>
|
|:040000 040000 08d15594747641d496850833378666fccc301872 9bd487d9aa4778cc2b4faabd94f9e4e378f09ed4 M      fs
|:040000 040000 84292cade24750adfd88aa4ef104209573ea8d7e 986c9bf3552ecc52337c69d62fd39ab67e928f3c M      include
|:040000 040000 8feddafc4d976199f9842b2f138f93e395afc746 d33936f4946f1ff4e4a5c7a7eab226b174d104d8 M      mm

Sebastian

Re: recent bdi changes result in WARN_ON on luksClose / dm

[Jens Axboe]

On Sat, Sep 12 2009, Sebastian Andrzej Siewior wrote:
> | Orion:/# cryptsetup luksClose crypto
> |------------[ cut here ]------------
> |WARNING: at /home/bigeasy/git/cryptodev-2.6/mm/backing-dev.c:637 bdi_destroy+0x44/0x60()
> |Modules linked in: cbc sha256_generic dm_crypt [last unloaded: mv_cesa]
> |[<c0028dd8>] (unwind_backtrace+0x0/0xdc) from [<c003adb0>] (warn_slowpath_common+0x4c/0x80)
> |[<c003adb0>] (warn_slowpath_common+0x4c/0x80) from [<c007f2e0>] (bdi_destroy+0x44/0x60)
> |[<c007f2e0>] (bdi_destroy+0x44/0x60) from [<c012e3a0>] (blk_release_queue+0x3c/0x54)
> |[<c012e3a0>] (blk_release_queue+0x3c/0x54) from [<c01399a8>] (kobject_release+0x5c/0x74)
> |[<c01399a8>] (kobject_release+0x5c/0x74) from [<c013a6ac>] (kref_put+0x68/0x7c)
> |[<c013a6ac>] (kref_put+0x68/0x7c) from [<c01b7098>] (dm_put+0x148/0x178)
> |[<c01b7098>] (dm_put+0x148/0x178) from [<c01bb82c>] (dev_remove+0x98/0xb4)
> |[<c01bb82c>] (dev_remove+0x98/0xb4) from [<c01bbec0>] (dm_ctl_ioctl+0x21c/0x29c)
> |[<c01bbec0>] (dm_ctl_ioctl+0x21c/0x29c) from [<c00a153c>] (vfs_ioctl+0x2c/0x8c)
> |[<c00a153c>] (vfs_ioctl+0x2c/0x8c) from [<c00a1bac>] (do_vfs_ioctl+0x524/0x590)
> |[<c00a1bac>] (do_vfs_ioctl+0x524/0x590) from [<c00a1c50>] (sys_ioctl+0x38/0x5c)
> |[<c00a1c50>] (sys_ioctl+0x38/0x5c) from [<c0023e80>] (ret_fast_syscall+0x0/0x2c)
> |---[ end trace b3c544a14b51605c ]---

So dm is killing the queue and associated backing device, while dirty
inodes still exist. That's not very nice. Perhaps dm_lock_for_deletion()
should ensure that the backing is flushed?

-- 
Jens Axboe

Re: recent bdi changes result in WARN_ON on luksClose / dm

[Jens Axboe]

On Sat, Sep 12 2009, Jens Axboe wrote:
> On Sat, Sep 12 2009, Sebastian Andrzej Siewior wrote:
> > | Orion:/# cryptsetup luksClose crypto
> > |------------[ cut here ]------------
> > |WARNING: at /home/bigeasy/git/cryptodev-2.6/mm/backing-dev.c:637 bdi_destroy+0x44/0x60()
> > |Modules linked in: cbc sha256_generic dm_crypt [last unloaded: mv_cesa]
> > |[<c0028dd8>] (unwind_backtrace+0x0/0xdc) from [<c003adb0>] (warn_slowpath_common+0x4c/0x80)
> > |[<c003adb0>] (warn_slowpath_common+0x4c/0x80) from [<c007f2e0>] (bdi_destroy+0x44/0x60)
> > |[<c007f2e0>] (bdi_destroy+0x44/0x60) from [<c012e3a0>] (blk_release_queue+0x3c/0x54)
> > |[<c012e3a0>] (blk_release_queue+0x3c/0x54) from [<c01399a8>] (kobject_release+0x5c/0x74)
> > |[<c01399a8>] (kobject_release+0x5c/0x74) from [<c013a6ac>] (kref_put+0x68/0x7c)
> > |[<c013a6ac>] (kref_put+0x68/0x7c) from [<c01b7098>] (dm_put+0x148/0x178)
> > |[<c01b7098>] (dm_put+0x148/0x178) from [<c01bb82c>] (dev_remove+0x98/0xb4)
> > |[<c01bb82c>] (dev_remove+0x98/0xb4) from [<c01bbec0>] (dm_ctl_ioctl+0x21c/0x29c)
> > |[<c01bbec0>] (dm_ctl_ioctl+0x21c/0x29c) from [<c00a153c>] (vfs_ioctl+0x2c/0x8c)
> > |[<c00a153c>] (vfs_ioctl+0x2c/0x8c) from [<c00a1bac>] (do_vfs_ioctl+0x524/0x590)
> > |[<c00a1bac>] (do_vfs_ioctl+0x524/0x590) from [<c00a1c50>] (sys_ioctl+0x38/0x5c)
> > |[<c00a1c50>] (sys_ioctl+0x38/0x5c) from [<c0023e80>] (ret_fast_syscall+0x0/0x2c)
> > |---[ end trace b3c544a14b51605c ]---
> 
> So dm is killing the queue and associated backing device, while dirty
> inodes still exist. That's not very nice. Perhaps dm_lock_for_deletion()
> should ensure that the backing is flushed?

Don't count this as a real fix, I'll investigate closer on monday. Does
the warning go away with this hack? It'll ensure that pending dirty
inodes are flushed.

diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index b4845b1..ead4a9b 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -357,6 +357,17 @@ int dm_open_count(struct mapped_device *md)
 	return atomic_read(&md->open_count);
 }
 
+static void dm_flush_dirty_io(struct mapped_device *md)
+{
+	struct writeback_control wbc = {
+		.bdi		= &md->queue->backing_dev_info,
+		.sync_mode	= WB_SYNC_ALL,
+		.nr_to_write	= LONG_MAX,
+	};
+
+	bdi_start_writeback(&wbc);
+}
+
 /*
  * Guarantees nothing is using the device before it's deleted.
  */
@@ -373,6 +384,9 @@ int dm_lock_for_deletion(struct mapped_device *md)
 
 	spin_unlock(&_minor_lock);
 
+	if (!r)
+		dm_flush_dirty_io(md);
+
 	return r;
 }
 

-- 
Jens Axboe

Re: recent bdi changes result in WARN_ON on luksClose / dm

[Jens Axboe]

On Sat, Sep 12 2009, Jens Axboe wrote:
> On Sat, Sep 12 2009, Jens Axboe wrote:
> > On Sat, Sep 12 2009, Sebastian Andrzej Siewior wrote:
> > > | Orion:/# cryptsetup luksClose crypto
> > > |------------[ cut here ]------------
> > > |WARNING: at /home/bigeasy/git/cryptodev-2.6/mm/backing-dev.c:637 bdi_destroy+0x44/0x60()
> > > |Modules linked in: cbc sha256_generic dm_crypt [last unloaded: mv_cesa]
> > > |[<c0028dd8>] (unwind_backtrace+0x0/0xdc) from [<c003adb0>] (warn_slowpath_common+0x4c/0x80)
> > > |[<c003adb0>] (warn_slowpath_common+0x4c/0x80) from [<c007f2e0>] (bdi_destroy+0x44/0x60)
> > > |[<c007f2e0>] (bdi_destroy+0x44/0x60) from [<c012e3a0>] (blk_release_queue+0x3c/0x54)
> > > |[<c012e3a0>] (blk_release_queue+0x3c/0x54) from [<c01399a8>] (kobject_release+0x5c/0x74)
> > > |[<c01399a8>] (kobject_release+0x5c/0x74) from [<c013a6ac>] (kref_put+0x68/0x7c)
> > > |[<c013a6ac>] (kref_put+0x68/0x7c) from [<c01b7098>] (dm_put+0x148/0x178)
> > > |[<c01b7098>] (dm_put+0x148/0x178) from [<c01bb82c>] (dev_remove+0x98/0xb4)
> > > |[<c01bb82c>] (dev_remove+0x98/0xb4) from [<c01bbec0>] (dm_ctl_ioctl+0x21c/0x29c)
> > > |[<c01bbec0>] (dm_ctl_ioctl+0x21c/0x29c) from [<c00a153c>] (vfs_ioctl+0x2c/0x8c)
> > > |[<c00a153c>] (vfs_ioctl+0x2c/0x8c) from [<c00a1bac>] (do_vfs_ioctl+0x524/0x590)
> > > |[<c00a1bac>] (do_vfs_ioctl+0x524/0x590) from [<c00a1c50>] (sys_ioctl+0x38/0x5c)
> > > |[<c00a1c50>] (sys_ioctl+0x38/0x5c) from [<c0023e80>] (ret_fast_syscall+0x0/0x2c)
> > > |---[ end trace b3c544a14b51605c ]---
> > 
> > So dm is killing the queue and associated backing device, while dirty
> > inodes still exist. That's not very nice. Perhaps dm_lock_for_deletion()
> > should ensure that the backing is flushed?
> 
> Don't count this as a real fix, I'll investigate closer on monday. Does
> the warning go away with this hack? It'll ensure that pending dirty
> inodes are flushed.

Though I do wonder why we have dirty inodes there, if dm has ensured
that the device has no other openers left. Something to investigate for
monday.

-- 
Jens Axboe

Re: recent bdi changes result in WARN_ON on luksClose / dm

[Sebastian Andrzej Siewior]

* Jens Axboe | 2009-09-12 21:58:40 [+0200]:

>Don't count this as a real fix, I'll investigate closer on monday. Does
>the warning go away with this hack? It'll ensure that pending dirty
>inodes are flushed.

That warning goes away but another pops up:

|------------[ cut here ]------------
|WARNING: at /home/bigeasy/git/cryptodev-2.6/fs/fs-writeback.c:504 writeback_inodes_wb+0x2cc/0x49c()
|Modules linked in: sha256_generic cbc dm_crypt
|[<c0028dd8>] (unwind_backtrace+0x0/0xdc) from [<c003adb0>] (warn_slowpath_common+0x4c/0x80)
|[<c003adb0>] (warn_slowpath_common+0x4c/0x80) from [<c00b0998>] (writeback_inodes_wb+0x2cc/0x49c)
|[<c00b0998>] (writeback_inodes_wb+0x2cc/0x49c) from [<c00b0c98>] (wb_writeback+0x130/0x180)
|[<c00b0c98>] (wb_writeback+0x130/0x180) from [<c00b10f8>] (wb_do_writeback+0x7c/0x1d0)
|[<c00b10f8>] (wb_do_writeback+0x7c/0x1d0) from [<c00b1284>] (bdi_writeback_task+0x38/0xb0)
|[<c00b1284>] (bdi_writeback_task+0x38/0xb0) from [<c007f788>] (bdi_start_fn+0x74/0xe0)
|[<c007f788>] (bdi_start_fn+0x74/0xe0) from [<c004f2cc>] (kthread+0x7c/0x84)
|[<c004f2cc>] (kthread+0x7c/0x84) from [<c0024858>] (kernel_thread_exit+0x0/0x8)
|---[ end trace e1f395241bdb07ff ]---

Sebastian

Re: recent bdi changes result in WARN_ON on luksClose / dm

[Jens Axboe]

On Sun, Sep 13 2009, Sebastian Andrzej Siewior wrote:
> * Jens Axboe | 2009-09-12 21:58:40 [+0200]:
> 
> >Don't count this as a real fix, I'll investigate closer on monday. Does
> >the warning go away with this hack? It'll ensure that pending dirty
> >inodes are flushed.
> 
> That warning goes away but another pops up:

Oh right, just goes to show that it's doing this backwards. We need the
umount rwsem for WB_SYNC_ALL. I'll look at this tomorrow.

-- 
Jens Axboe

Re: recent bdi changes result in WARN_ON on luksClose / dm

[Jens Axboe]

On Sun, Sep 13 2009, Jens Axboe wrote:
> On Sun, Sep 13 2009, Sebastian Andrzej Siewior wrote:
> > * Jens Axboe | 2009-09-12 21:58:40 [+0200]:
> > 
> > >Don't count this as a real fix, I'll investigate closer on monday. Does
> > >the warning go away with this hack? It'll ensure that pending dirty
> > >inodes are flushed.
> > 
> > That warning goes away but another pops up:
> 
> Oh right, just goes to show that it's doing this backwards. We need the
> umount rwsem for WB_SYNC_ALL. I'll look at this tomorrow.

I think the safest option is to ensure that we just move these entries
to our default bdi, which doesn't go away. This should fix the warning,
can you test whether it works as well?

diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index fd93566..27f82dc 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -668,7 +668,17 @@ void bdi_destroy(struct backing_dev_info *bdi)
 {
 	int i;
 
-	WARN_ON(bdi_has_dirty_io(bdi));
+	/*
+	 * Splice our entries to the default_backing_dev_info, if this
+	 * bdi disappears
+	 */
+	if (bdi_has_dirty_io(bdi)) {
+		struct bdi_writeback *dst = &default_backing_dev_info.wb;
+
+		list_splice(&bdi->wb.b_dirty, &dst->b_dirty);
+		list_splice(&bdi->wb.b_io, &dst->b_io);
+		list_splice(&bdi->wb.b_more_io, &dst->b_more_io);
+	}
 
 	bdi_unregister(bdi);
 

-- 
Jens Axboe

Re: recent bdi changes result in WARN_ON on luksClose / dm

[Sebastian Andrzej Siewior]

* Jens Axboe | 2009-09-14 11:05:33 [+0200]:

>I think the safest option is to ensure that we just move these entries
>to our default bdi, which doesn't go away. This should fix the warning,
>can you test whether it works as well?
Yep, I don't see anything with this patch. Thanks.

Sebastian