[3/3] writeback: fix false positive WARN in __mark_inode_dirty
diff mbox

Message ID 20151201235852.36836.87208.stgit@dwillia2-desk3.jf.intel.com
State New
Headers show

Commit Message

Williams, Dan J Dec. 1, 2015, 11:58 p.m. UTC
This warning was added as a debugging aid way back in commit
500b067c5e6c "writeback: check for registered bdi in flusher add and
inode dirty" when we were switching over to per-bdi writeback.

Once the block device has been torn down it's no longer useful to
complain about unregistered bdi's.  Clear the writeback capability under
the the wb->list_lock(), so that __mark_inode_dirty has no opportunity
to race bdi_unregister() to this WARN() condition.

Alternatively we could just delete the warning...

Found this while testing block device remove from underneath an active
fs triggering traces like:

 WARNING: CPU: 6 PID: 2129 at fs/fs-writeback.c:2065 __mark_inode_dirty+0x261/0x350()
 bdi-block not registered
 [..]
 Call Trace:
  [<ffffffff81459f02>] dump_stack+0x44/0x62
  [<ffffffff810a1f32>] warn_slowpath_common+0x82/0xc0
  [<ffffffff810a1fcc>] warn_slowpath_fmt+0x5c/0x80
  [<ffffffff812830b1>] __mark_inode_dirty+0x261/0x350
  [<ffffffff8126d019>] generic_update_time+0x79/0xd0
  [<ffffffff8126d19d>] file_update_time+0xbd/0x110
  [<ffffffff812e4ab8>] ext4_dax_fault+0x68/0x110
  [<ffffffff811f7f3e>] __do_fault+0x4e/0xf0
  [<ffffffff811fc077>] handle_mm_fault+0x5e7/0x1b50
  [<ffffffff811fbae1>] ? handle_mm_fault+0x51/0x1b50
  [<ffffffff81068981>] __do_page_fault+0x191/0x3f0
  [<ffffffff81068caf>] trace_do_page_fault+0x4f/0x120
  [<ffffffff810630fa>] do_async_page_fault+0x1a/0xa0
  [<ffffffff819023f8>] async_page_fault+0x28/0x30

Cc: Jan Kara <jack@suse.com>
Cc: Tejun Heo <tj@kernel.org>
Cc: Jens Axboe <axboe@fb.com>
Cc: Dave Chinner <david@fromorbit.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 mm/backing-dev.c |    7 +++++++
 1 file changed, 7 insertions(+)


--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Jan Kara Dec. 2, 2015, 10:35 a.m. UTC | #1
On Tue 01-12-15 15:58:52, Dan Williams wrote:
> This warning was added as a debugging aid way back in commit
> 500b067c5e6c "writeback: check for registered bdi in flusher add and
> inode dirty" when we were switching over to per-bdi writeback.
> 
> Once the block device has been torn down it's no longer useful to
> complain about unregistered bdi's.  Clear the writeback capability under
> the the wb->list_lock(), so that __mark_inode_dirty has no opportunity
> to race bdi_unregister() to this WARN() condition.
> 
> Alternatively we could just delete the warning...
> 
> Found this while testing block device remove from underneath an active
> fs triggering traces like:
> 
>  WARNING: CPU: 6 PID: 2129 at fs/fs-writeback.c:2065 __mark_inode_dirty+0x261/0x350()
>  bdi-block not registered
>  [..]
>  Call Trace:
>   [<ffffffff81459f02>] dump_stack+0x44/0x62
>   [<ffffffff810a1f32>] warn_slowpath_common+0x82/0xc0
>   [<ffffffff810a1fcc>] warn_slowpath_fmt+0x5c/0x80
>   [<ffffffff812830b1>] __mark_inode_dirty+0x261/0x350
>   [<ffffffff8126d019>] generic_update_time+0x79/0xd0
>   [<ffffffff8126d19d>] file_update_time+0xbd/0x110
>   [<ffffffff812e4ab8>] ext4_dax_fault+0x68/0x110
>   [<ffffffff811f7f3e>] __do_fault+0x4e/0xf0
>   [<ffffffff811fc077>] handle_mm_fault+0x5e7/0x1b50
>   [<ffffffff811fbae1>] ? handle_mm_fault+0x51/0x1b50
>   [<ffffffff81068981>] __do_page_fault+0x191/0x3f0
>   [<ffffffff81068caf>] trace_do_page_fault+0x4f/0x120
>   [<ffffffff810630fa>] do_async_page_fault+0x1a/0xa0
>   [<ffffffff819023f8>] async_page_fault+0x28/0x30
> 
> Cc: Jan Kara <jack@suse.com>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Jens Axboe <axboe@fb.com>
> Cc: Dave Chinner <david@fromorbit.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

OK, looks good to me. There is still a tiny race window where the warning
could trigger but I don't think it's worth caring about. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  mm/backing-dev.c |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/mm/backing-dev.c b/mm/backing-dev.c
> index 8ed2ffd963c5..24e61acf74a7 100644
> --- a/mm/backing-dev.c
> +++ b/mm/backing-dev.c
> @@ -343,10 +343,17 @@ static void wb_shutdown(struct bdi_writeback *wb)
>  {
>  	/* Make sure nobody queues further work */
>  	spin_lock_bh(&wb->work_lock);
> +
>  	if (!test_and_clear_bit(WB_registered, &wb->state)) {
>  		spin_unlock_bh(&wb->work_lock);
>  		return;
>  	}
> +
> +	/* tell __mark_inode_dirty that writeback is no longer possible */
> +	spin_lock(&wb->list_lock);
> +	wb->bdi->capabilities |= BDI_CAP_NO_WRITEBACK;
> +	spin_unlock(&wb->list_lock);
> +
>  	spin_unlock_bh(&wb->work_lock);
>  
>  	/*
> 
>

Patch
diff mbox

diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index 8ed2ffd963c5..24e61acf74a7 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -343,10 +343,17 @@  static void wb_shutdown(struct bdi_writeback *wb)
 {
 	/* Make sure nobody queues further work */
 	spin_lock_bh(&wb->work_lock);
+
 	if (!test_and_clear_bit(WB_registered, &wb->state)) {
 		spin_unlock_bh(&wb->work_lock);
 		return;
 	}
+
+	/* tell __mark_inode_dirty that writeback is no longer possible */
+	spin_lock(&wb->list_lock);
+	wb->bdi->capabilities |= BDI_CAP_NO_WRITEBACK;
+	spin_unlock(&wb->list_lock);
+
 	spin_unlock_bh(&wb->work_lock);
 
 	/*