Messages in this thread Patch in this message | | | Subject | Re: [PATCH] Fix commit of ordered data buffers | From | "Zhang, Yanmin" <> | Date | Thu, 28 Sep 2006 16:31:06 +0800 |
| |
On Tue, 2006-09-12 at 05:05, Jan Kara wrote: > Hi Andrew, > > here is the patch that came out of the thread "set_page_buffer_dirty > should skip unmapped buffers". It fixes several flaws in the code > writing out ordered data buffers during commit. It definitely fixed the > problem Badari was seeing with fsx-linux test. Could you include it > into -mm? Since there are quite complex interactions with other JBD code > and the locking is kind of ugly, I'd leave it in -mm for a while whether > some bug does not emerge ;). Thanks. > > Honza I also worked on it because I didn't know you were working on it until I located the root cause and tried to check bugzilla.
I reviewed your patch.
+ if (!inverted_lock(journal, bh)) { + jbd_lock_bh_state(bh); + spin_lock(&journal->j_list_lock); + } Should journal->j_list_lock be unlocked before jbd_lock_bh_state(bh)?
The fsx-linux test issue is a race between journal_commit_transaction and journal_dirty_data. After journal_commit_transaction adds buffer_head pointers to wbuf, it might unlock journal->j_list_lock. Although all buffer head in wbuf are locked, does that prevent journal_dirty_data from unlinking the buffer head from the transaction and fsx-linux from truncating it?
I'm not a journal expert. But I want to discuss it.
My investigation is below (Scenario):
fsx-linux starts journal_dirty_data and journal_dirty_data links a jh to journal->j_running_transaction's t_sync_datalist, kjournald might not write the buffer to disk quickly, but saves it to array wbuf. Then, fsx-linux starts the second journal_dirty_data of a new transaction might submit the same buffer head and move the jh to the new transaction's t_sync_datalist. Then, fsx-linux truncates the last a couple of buffers of a page. Then, block_write_full_page calls invalidatepage to invalidate the last a couple of buffers of the page, so the journal_heads of the buffer_head are unlinked and are marked as unmapped. Then, fsx-linux extend the file and does a msync after changing the page content by mmaping the page, so the page (inclduing the last buffer head) is marked dirty again. Then, kjournald's journal_commit_transaction goes through wbuf to submit_bh all dirty buffers, but one buffer head is already marked as unmapped. A bug check is triggerred.
>From above scenario, as long as the late calls doesn't try to lock the buffer head, the race condition still exists.
I think the right way is to let journal_dirty_data to wait till wbuf is flushed.
Below is my patch. Any idea?
Signed-off-by: Zhang Yanmin <yanmin.zhang@intel.com>
---
diff -Nraup linux-2.6.18-rc7/fs/jbd/commit.c linux-2.6.18-rc7_jbd/fs/jbd/commit.c --- linux-2.6.18-rc7/fs/jbd/commit.c 2006-09-20 08:57:12.000000000 +0800 +++ linux-2.6.18-rc7_jbd/fs/jbd/commit.c 2006-09-27 16:33:14.000000000 +0800 @@ -384,6 +384,8 @@ write_out_data: spin_lock(&journal->j_list_lock); } + wake_up(&journal->j_wait_commit_sync_datalist_free); + /* * Wait for all previously submitted IO to complete. */ diff -Nraup linux-2.6.18-rc7/fs/jbd/journal.c linux-2.6.18-rc7_jbd/fs/jbd/journal.c --- linux-2.6.18-rc7/fs/jbd/journal.c 2006-09-20 08:57:12.000000000 +0800 +++ linux-2.6.18-rc7_jbd/fs/jbd/journal.c 2006-09-27 16:11:27.000000000 +0800 @@ -656,6 +656,7 @@ static journal_t * journal_init_common ( init_waitqueue_head(&journal->j_wait_transaction_locked); init_waitqueue_head(&journal->j_wait_logspace); + init_waitqueue_head(&journal->j_wait_commit_sync_datalist_free); init_waitqueue_head(&journal->j_wait_done_commit); init_waitqueue_head(&journal->j_wait_checkpoint); init_waitqueue_head(&journal->j_wait_commit); diff -Nraup linux-2.6.18-rc7/fs/jbd/transaction.c linux-2.6.18-rc7_jbd/fs/jbd/transaction.c --- linux-2.6.18-rc7/fs/jbd/transaction.c 2006-09-20 08:57:12.000000000 +0800 +++ linux-2.6.18-rc7_jbd/fs/jbd/transaction.c 2006-09-28 14:42:05.000000000 +0800 @@ -965,6 +965,8 @@ int journal_dirty_data(handle_t *handle, * never, ever allow this to happen: there's nothing we can do * about it in this layer. */ + +repeat_ifcase2: jbd_lock_bh_state(bh); spin_lock(&journal->j_list_lock); if (jh->b_transaction) { @@ -1032,6 +1034,32 @@ int journal_dirty_data(handle_t *handle, time if it is redirtied */ } + if (jh->b_transaction != NULL && + journal->j_committing_transaction == + jh->b_transaction && + jh->b_jlist == BJ_SyncData) { + + wait_queue_head_t *queue_head; + spin_unlock(&journal->j_list_lock); + jbd_unlock_bh_state(bh); + + if (need_brelse) { + BUFFER_TRACE(bh, "brelse"); + __brelse(bh); + need_brelse = 0; + } + + queue_head = + &journal->j_wait_commit_sync_datalist_free; + wait_event(*queue_head, + !(jh->b_transaction != NULL && + journal->j_committing_transaction == + jh->b_transaction && + jh->b_jlist == BJ_SyncData)); + + goto repeat_ifcase2; + } + /* journal_clean_data_list() may have got there first */ if (jh->b_transaction != NULL) { JBUFFER_TRACE(jh, "unfile from commit"); diff -Nraup linux-2.6.18-rc7/include/linux/jbd.h linux-2.6.18-rc7_jbd/include/linux/jbd.h --- linux-2.6.18-rc7/include/linux/jbd.h 2006-09-20 08:57:13.000000000 +0800 +++ linux-2.6.18-rc7_jbd/include/linux/jbd.h 2006-09-27 16:29:47.000000000 +0800 @@ -583,6 +583,8 @@ struct transaction_s * @j_wait_transaction_locked: Wait queue for waiting for a locked transaction * to start committing, or for a barrier lock to be released * @j_wait_logspace: Wait queue for waiting for checkpointing to complete + * @j_wait_commit_sync_datalist_free: Wait queue for waiting for commit + * transaction sync_datalist becomes null * @j_wait_done_commit: Wait queue for waiting for commit to complete * @j_wait_checkpoint: Wait queue to trigger checkpointing * @j_wait_commit: Wait queue to trigger commit @@ -686,6 +688,12 @@ struct journal_s /* Wait queue for waiting for checkpointing to complete */ wait_queue_head_t j_wait_logspace; + /* + * Wait queue for waiting for commit transaction + * sync_datalist becomes null + */ + wait_queue_head_t j_wait_commit_sync_datalist_free; + /* Wait queue for waiting for commit to complete */ wait_queue_head_t j_wait_done_commit; - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |