Hello,
On Thu 05-05-11 17:57:16, Tao Ma wrote:
> From: Tao Ma <boyu.mt@xxxxxxxxxx>
>
> In do_get_write_access, we check journal_head->b_jlist and if it
> is BJ_Shadow, we will sleep until we remove it from t_shadow_list
> in jbd2_journal_commit_transaction, but it isn't protected by any
> lock. So if we uses some cached b_jlist and before schedule,
> jbd2_journal_commit_transaction has already waken up all
> the waiting thread. As a result, this thread will never be waken up.
I had a look at the code and I think it's more complicated than this.
The code is:
prepare_to_wait(wqh, &wait.wait, TASK_UNINTERRUPTIBLE);
if (jh->b_jlist != BJ_Shadow)
break;
schedule();
You're right that jh->b_jlist != BJ_Shadow test is done without any lock.
But prepare_to_wait() does set_current_state() which implies a memory
barrier. The comment there says:
/*
* set_current_state() includes a barrier so that the write of current->state
* is correctly serialised wrt the caller's subsequent test of whether to
* actually sleep:
*
* set_current_state(TASK_UNINTERRUPTIBLE);
* if (do_i_need_to_sleep())
* schedule();
*
* If the caller does not need such serialisation then use __set_current_state()
*/
So we are guaranteed that either we see that jh->b_jlist != BJ_Shadow or
the waking process sees us in the wait queue and removes us.
Well, not quite. The waking code is:
journal_file_buffer(jh, commit_transaction, BJ_Forget);
/* Wake up any transactions which were waiting for this
IO to complete */
wake_up_bit(&bh->b_state, BH_Unshadow);
And that's where the problem actually is. Even the comment before
wake_up_bit() warns that:
* In order for this to function properly, as it uses waitqueue_active()
* internally, some kind of memory barrier must be done prior to calling
* this. Typically, this will be smp_mb__after_clear_bit(), but in some
* cases where bitflags are manipulated non-atomically under a lock, one
* may need to use a less regular barrier, such fs/inode.c's smp_mb(),
* because spin_unlock() does not guarantee a memory barrier.
I'll send proper fix in a moment.
Honza
--
Jan Kara <jack@xxxxxxx>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
