lkml.org 
[lkml]   [2010]   [Jul]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [regression] Crash in wb_clear_pending()
On 05/07/10 21.32, Christoph Hellwig wrote:
> On Mon, Jul 05, 2010 at 09:24:39PM +0200, Jens Axboe wrote:
>> The oops itself looks like a recurrence of the missing RCU grace or
>> too early stack wakeup, which should be a 1-2 liner once it's found.
>
> See the previous thread. There's at least two issues:
>
> - wb_do_writeback checks work->state after it's been freed when we do
> the second test_bit for WS_ONSTACK
> - bdi_work_free accesses work->state after waking up the caller doing
> bdi_wait_on_work_done, which might have re-used the stack space
> allocated for the work item.
>
> The fix for that is to get rid of the fragile work->state stuff and the
> bit wakeups by just using a completion and using that as indicator
> for the stack wait. That's the main change the above patch does. In
> addition it also merges the two structures used for the writeback
> requests. Onl doing the completion and earlier list removal would
> be something like the untested patch below:

If those two late ON_STACK checks is the only issue left there,
why not just apply the below for 2.6.35?

diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index 0609607..15ce6ab 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -90,12 +90,13 @@ int writeback_in_progress(struct backing_dev_info *bdi)
static void bdi_work_free(struct rcu_head *head)
{
struct bdi_work *work = container_of(head, struct bdi_work, rcu_head);
+ int on_stack = test_bit(WS_ONSTACK, &work->state);

clear_bit(WS_INPROGRESS, &work->state);
smp_mb__after_clear_bit();
wake_up_bit(&work->state, WS_INPROGRESS);

- if (!test_bit(WS_ONSTACK, &work->state))
+ if (!on_stack)
kfree(work);
}

@@ -854,6 +855,7 @@ long wb_do_writeback(struct bdi_writeback *wb, int force_wait)

while ((work = get_next_work_item(bdi, wb)) != NULL) {
struct wb_writeback_args args = work->args;
+ int on_stack = test_bit(WS_ONSTACK, &work->state);

/*
* Override sync mode, in case we must wait for completion
@@ -865,7 +867,7 @@ long wb_do_writeback(struct bdi_writeback *wb, int force_wait)
* If this isn't a data integrity operation, just notify
* that we have seen this work and we are now starting it.
*/
- if (!test_bit(WS_ONSTACK, &work->state))
+ if (!on_stack)
wb_clear_pending(wb, work);

wrote += wb_writeback(wb, &args);
@@ -874,7 +876,7 @@ long wb_do_writeback(struct bdi_writeback *wb, int force_wait)
* This is a data integrity writeback, so only do the
* notification when we have completed the work.
*/
- if (test_bit(WS_ONSTACK, &work->state))
+ if (on_stack)
wb_clear_pending(wb, work);
}

--
Jens Axboe



\
 
 \ /
  Last update: 2010-07-05 21:49    [from the cache]
©2003-2011 Jasper Spaans