| Message ID | 1304589436-14860-1-git-send-email-tm@tao.ma |
|---|---|
| State | Rejected, archived |
| Headers | show |
Hello, On Thu 05-05-11 17:57:16, Tao Ma wrote: > From: Tao Ma <boyu.mt@taobao.com> > > 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
On 05/05/2011 08:09 PM, Jan Kara wrote: > Hello, > > On Thu 05-05-11 17:57:16, Tao Ma wrote: >> From: Tao Ma <boyu.mt@taobao.com> >> >> 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. oh, great thanks for the fix and the detailed explanation about the memory barrier. Regards, Tao -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c index 05fa77a..2e837e8 100644 --- a/fs/jbd2/transaction.c +++ b/fs/jbd2/transaction.c @@ -693,7 +693,7 @@ repeat: * extra copy, not the primary copy, which gets * journaled. If the primary copy is already going to * disk then we cannot do copy-out here. */ - + spin_lock(&journal->j_list_lock); if (jh->b_jlist == BJ_Shadow) { DEFINE_WAIT_BIT(wait, &bh->b_state, BH_Unshadow); wait_queue_head_t *wqh; @@ -701,18 +701,24 @@ repeat: wqh = bit_waitqueue(&bh->b_state, BH_Unshadow); JBUFFER_TRACE(jh, "on shadow: sleep"); - jbd_unlock_bh_state(bh); /* commit wakes up all shadow buffers after IO */ for ( ; ; ) { prepare_to_wait(wqh, &wait.wait, TASK_UNINTERRUPTIBLE); if (jh->b_jlist != BJ_Shadow) break; + spin_unlock(&journal->j_list_lock); + jbd_unlock_bh_state(bh); schedule(); + jbd_lock_bh_state(bh); + spin_lock(&journal->j_list_lock); } + spin_unlock(&journal->j_list_lock); + jbd_unlock_bh_state(bh); finish_wait(wqh, &wait.wait); goto repeat; } + spin_unlock(&journal->j_list_lock); /* Only do the copy if the currently-owning transaction * still needs it. If it is on the Forget list, the