[PATCH] ext3: fix journal_release_buffer() race

		CPU0				CPU1

	journal_get_write_access(bh)
	 (Add buffer to t_reserved_list)

					journal_get_write_access(bh)
					 (It's already on t_reserved_list:
					  nothing to do)

	 (We decide we don't want to
	  journal the buffer after all)
	journal_release_buffer()
	 (It gets pulled off the transaction)

					journal_dirty_metadata()
					 (The buffer isn't on the reserved
					  list!  The kernel explodes)

Simple fix: just leave the buffer on t_reserved_list in
journal_release_buffer().  If nobody ends up claiming the buffer then it will
get thrown away at start of transaction commit.

diff --git a/fs/jbd/commit.c b/fs/jbd/commit.c
index 54f7862a37171e2f3e21f8eb81e6efeecc798c70..2580162cad52e379120cdc8a12e955eff6478422 100644 (file)
--- a/fs/jbd/commit.c
+++ b/fs/jbd/commit.c
@@ -169,10 +169,23 @@ void journal_commit_transaction(journal_t *journal)
         * that multiple journal_get_write_access() calls to the same
         * buffer are perfectly permissable.
         */
-       while (commit_transaction->t_reserved_list) {
-               jh = commit_transaction->t_reserved_list;
-               JBUFFER_TRACE(jh, "reserved, unused: refile");
-               journal_refile_buffer(journal, jh);
+       {
+               int nr = 0;
+               while (commit_transaction->t_reserved_list) {
+                       jh = commit_transaction->t_reserved_list;
+                       JBUFFER_TRACE(jh, "reserved, unused: refile");
+                       journal_refile_buffer(journal, jh);
+                       nr++;
+               }
+               if (nr) {
+                       static int noisy;
+
+                       if (noisy < 10) {
+                               noisy++;
+                               printk("%s: freed %d reserved buffers\n",
+                                       __FUNCTION__, nr);
+                       }
+               }
        }
 
        /*

diff --git a/fs/jbd/transaction.c b/fs/jbd/transaction.c
index 54e16b97fdaa7408891398bad56d47ae316a01fa..12ad174e7662200576bee1ba1490069071201c8a 100644 (file)
--- a/fs/jbd/transaction.c
+++ b/fs/jbd/transaction.c
@@ -1168,37 +1168,24 @@ out:
  * journal_release_buffer: undo a get_write_access without any buffer
  * updates, if the update decided in the end that it didn't need access.
  *
- * journal_get_write_access() can block, so it is quite possible for a
- * journaling component to decide after the write access is returned
- * that global state has changed and the update is no longer required.
- *
  * The caller passes in the number of credits which should be put back for
  * this buffer (zero or one).
+ *
+ * We leave the buffer attached to t_reserved_list because even though this
+ * handle doesn't want it, some other concurrent handle may want to journal
+ * this buffer.  If that handle is curently in between get_write_access() and
+ * journal_dirty_metadata() then it expects the buffer to be reserved.  If
+ * we were to rip it off t_reserved_list here, the other handle will explode
+ * when journal_dirty_metadata is presented with a non-reserved buffer.
+ *
+ * If nobody really wants to journal this buffer then it will be thrown
+ * away at the start of commit.
  */
 void
 journal_release_buffer(handle_t *handle, struct buffer_head *bh, int credits)
 {
-       transaction_t *transaction = handle->h_transaction;
-       journal_t *journal = transaction->t_journal;
-       struct journal_head *jh = bh2jh(bh);
-
-       JBUFFER_TRACE(jh, "entry");
-
-       /* If the buffer is reserved but not modified by this
-        * transaction, then it is safe to release it.  In all other
-        * cases, just leave the buffer as it is. */
-
-       jbd_lock_bh_state(bh);
-       spin_lock(&journal->j_list_lock);
-       if (jh->b_jlist == BJ_Reserved && jh->b_transaction == transaction &&
-           !buffer_jbddirty(jh2bh(jh))) {
-               JBUFFER_TRACE(jh, "unused: refiling it");
-               __journal_refile_buffer(jh);
-       }
-       spin_unlock(&journal->j_list_lock);
-       jbd_unlock_bh_state(bh);
+       BUFFER_TRACE(bh, "entry");
        handle->h_buffer_credits += credits;
-       JBUFFER_TRACE(jh, "exit");
 }
 
 /**