Attachment 308846 Details for Bug 446599 – Patch correcting jbd races

[patch] Patch correcting jbd races

dio-jbd-race-eio-error-fix-v8.patch (text/plain), 5.67 KB, created by Bryn M. Reeves on 2008-06-10 18:21:26 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Bryn M. Reeves

Created: 2008-06-10 18:21:26 UTC

Size: 5.67 KB

patch

obsolete

>JBD: fix race between journal_try_to_free_buffers() and jbd commit transaction
>
>From: Mingming Cao <cmm@us.ibm.com>
>
>journal_try_to_free_buffers() could race with jbd commit transaction when
>the later is holding the buffer reference while waiting for the data buffer
>to flush to disk. If the caller of journal_try_to_free_buffers() request
>tries hard to release the buffers, it will treat the failure as error and return
>back to the caller. We have seen the directo IO failed due to this race. 
>Some of the caller of releasepage() also expecting the buffer to be dropped
>when passed with GFP_KERNEL mask to the releasepage()->journal_try_to_free_buffers().
>
>With this patch, if the caller is passing the GFP_KERNEL to indicating this
>call could wait, in case of try_to_free_buffers() failed, let's waiting for
>journal_commit_transaction() to finish commit the current committing transaction
>, then try to free those buffers again with journal locked.
>
>This patch also fixed the problem that missing the the j_state_lock to protect
>updateing the current transaction's t_state to T_COMMIT.
>
>Signed-off-by: Mingming Cao <cmm@us.ibm.com>
>Reviewed-by: Badari Pulavarty <pbadari@us.ibm.com> 
>---
> fs/buffer.c          |    3 --
> fs/jbd/commit.c      |    2 +
> fs/jbd/transaction.c |   57 ++++++++++++++++++++++++++++++++++++++++++++++++---
> 3 files changed, 57 insertions(+), 5 deletions(-)
>
>Index: linux-2.6.18.ppc/fs/jbd/transaction.c
>===================================================================
>--- linux-2.6.18.ppc.orig/fs/jbd/transaction.c	2008-05-19 18:13:57.000000000 -0700
>+++ linux-2.6.18.ppc/fs/jbd/transaction.c	2008-05-29 13:42:00.000000000 -0700
>@@ -1606,13 +1606,41 @@ out:
> 	return;
> }
> 
>+/*
>+ * journal_try_to_free_buffers() could race with journal_commit_transaction()
>+ * The later might still hold the reference count to the buffers when inspecting
>+ * them on t_syncdata_list or t_locked_list.
>+ *
>+ * Journal_try_to_free_buffers() will call this function to
>+ * wait for the current transaction to finish syncing data buffers, before
>+ * try to free that buffer.
>+ *
>+ * Called with journal->j_state_lock hold.
>+ */
>+static void journal_wait_for_transaction_sync_data(journal_t *journal)
>+{
>+	transaction_t *transaction = NULL;
>+	tid_t tid;
>+
>+	spin_lock(&journal->j_state_lock);
>+	transaction = journal->j_committing_transaction;
>+
>+	if (!transaction){
>+		spin_unlock(&journal->j_state_lock);
>+		return;
>+	}
>+
>+	tid = transaction->t_tid;
>+	spin_unlock(&journal->j_state_lock);
>+	log_wait_commit(journal, tid);
>+}
> 
> /** 
>  * int journal_try_to_free_buffers() - try to free page buffers.
>  * @journal: journal for operation
>  * @page: to try and free
>- * @unused_gfp_mask: unused
>- *
>+ * @gfp_mask: specifies whether the call may block
>+ *		(__GFP_WAIT & __GFP_FS via GFP_KERNEL)
>  * 
>  * For all the buffers on this page,
>  * if they are fully written out ordered data, move them onto BUF_CLEAN
>@@ -1640,9 +1668,11 @@ out:
>  * journal_try_to_free_buffer() is changing its state.  But that
>  * cannot happen because we never reallocate freed data as metadata
>  * while the data is part of a transaction.  Yes?
>+ *
>+ * Returns 0 on failure, 1 on success
>  */
> int journal_try_to_free_buffers(journal_t *journal, 
>-				struct page *page, gfp_t unused_gfp_mask)
>+				struct page *page, gfp_t gfp_mask)
> {
> 	struct buffer_head *head;
> 	struct buffer_head *bh;
>@@ -1671,7 +1701,28 @@ int journal_try_to_free_buffers(journal_
> 		if (buffer_jbd(bh))
> 			goto busy;
> 	} while ((bh = bh->b_this_page) != head);
>+
> 	ret = try_to_free_buffers(page);
>+
>+ 	/*
>+	 * There are a number of places where journal_try_to_free_buffers()
>+	 * could race with journal_commit_transaction(), the later still
>+	 * holds the reference to the buffers to free while processing them.
>+	 * try_to_free_buffers() failed to free those buffers. Some of the
>+	 * caller of releasepage() request page buffers to be dropped, otherwise
>+	 * treat the fail-to-free as errors (such as generic_file_direct_IO())
>+	 *
>+	 * So, if the caller of try_to_release_page() wants the synchronous
>+	 * behaviour(i.e make sure buffers are dropped upon return),
>+	 * let's wait for the current transaction to finish flush of
>+	 * dirty data buffers, then try to free those buffers again,
>+	 * with the journal locked.
>+	 */
>+	if (ret == 0 && (gfp_mask & __GFP_WAIT) && (gfp_mask & __GFP_FS)) {
>+		journal_wait_for_transaction_sync_data(journal);
>+		ret = try_to_free_buffers(page);
>+	}
>+
> busy:
> 	return ret;
> }
>Index: linux-2.6.18.ppc/fs/buffer.c
>===================================================================
>--- linux-2.6.18.ppc.orig/fs/buffer.c	2008-05-19 18:13:57.000000000 -0700
>+++ linux-2.6.18.ppc/fs/buffer.c	2008-05-19 18:14:09.000000000 -0700
>@@ -1583,9 +1583,8 @@ static void discard_buffer(struct buffer
>  * Otherwise return zero.
>  *
>  * The @gfp_mask argument specifies whether I/O may be performed to release
>- * this page (__GFP_IO), and whether the call may block (__GFP_WAIT).
>+ * this page (__GFP_IO), and whether the call may block (__GFP_WAIT & __GFP_FS).
>  *
>- * NOTE: @gfp_mask may go away, and this function may become non-blocking.
>  */
> int try_to_release_page(struct page *page, gfp_t gfp_mask)
> {
>Index: linux-2.6.18.ppc/fs/jbd/commit.c
>===================================================================
>--- linux-2.6.18.ppc.orig/fs/jbd/commit.c	2008-05-16 14:20:32.000000000 -0700
>+++ linux-2.6.18.ppc/fs/jbd/commit.c	2008-05-19 18:15:56.000000000 -0700
>@@ -488,7 +488,9 @@ void journal_commit_transaction(journal_
> 	 * transaction!  Now comes the tricky part: we need to write out
> 	 * metadata.  Loop over the transaction's entire buffer list:
> 	 */
>+	spin_lock(&journal->j_state_lock);
> 	commit_transaction->t_state = T_COMMIT;
>+	spin_unlock(&journal->j_state_lock);
> 
> 	descriptor = NULL;
> 	bufs = 0;

Actions: View | Diff

Attachments on bug 446599: 305460 | 308846