Re: [PATCH][take 5] JBD2: Fix race between free buffer and commit trasanction

[Mingming Cao <cmm@us.ibm.com>]

On Fri, 2008-05-30 at 11:54 +0530, Aneesh Kumar K.V wrote:
> On Wed, May 28, 2008 at 05:18:19PM -0700, Mingming Cao wrote:
> > Index: linux-2.6.26-rc3/fs/jbd2/transaction.c
> > ===================================================================
> > --- linux-2.6.26-rc3.orig/fs/jbd2/transaction.c	2008-05-28 16:10:41.000000000 -0700
> > +++ linux-2.6.26-rc3/fs/jbd2/transaction.c	2008-05-28 16:13:16.000000000 -0700
> > @@ -1656,12 +1656,42 @@ out:
> >  	return;
> >  }
> > 
> > +/*
> > + * jbd2_journal_try_to_free_buffers() could race with jbd2_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.
> > + *
> > + * jbd2_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.
> > + */
> 
> We are taking the spin_lock again in the function ??
> 

Thanks.  I noticed this yesterday and have sent Andrew updated patch to 
replace the one he just added to mm tree but forget to copy to the list. Here is the updated patch,

Mingming

---
 fs/jbd2/transaction.c |   61 +++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 58 insertions(+), 3 deletions(-)

Index: linux-2.6.26-rc4/fs/jbd2/transaction.c
===================================================================
--- linux-2.6.26-rc4.orig/fs/jbd2/transaction.c	2008-05-29 12:21:40.000000000 -0700
+++ linux-2.6.26-rc4/fs/jbd2/transaction.c	2008-05-29 12:38:30.000000000 -0700
@@ -1656,12 +1656,43 @@ out:
 	return;
 }
 
+/*
+ * jbd2_journal_try_to_free_buffers() could race with
+ * jbd2_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.
+ *
+ * jbd2_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.
+ *
+ */
+static void jbd2_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);
+	jbd2_log_wait_commit(journal, tid);
+}
 
 /**
  * int jbd2_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: we use the mask to detect how hard should we try to release
+ * buffers. If __GFP_WAIT and __GFP_FS is set, we wait for commit code to
+ * release the buffers.
  *
  *
  * For all the buffers on this page,
@@ -1690,9 +1721,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?
+ *
+ * Return 0 on failure, 1 on success
  */
 int jbd2_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;
@@ -1708,7 +1741,8 @@ int jbd2_journal_try_to_free_buffers(jou
 		/*
 		 * We take our own ref against the journal_head here to avoid
 		 * having to add tons of locking around each instance of
-		 * jbd2_journal_remove_journal_head() and jbd2_journal_put_journal_head().
+		 * jbd2_journal_remove_journal_head() and
+		 * jbd2_journal_put_journal_head().
 		 */
 		jh = jbd2_journal_grab_journal_head(bh);
 		if (!jh)
@@ -1721,7 +1755,28 @@ int jbd2_journal_try_to_free_buffers(jou
 		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 jbd2_journal_try_to_free_buffers()
+	 * could race with jbd2_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)) {
+		jbd2_journal_wait_for_transaction_sync_data(journal);
+		ret = try_to_free_buffers(page);
+	}
+
 busy:
 	return ret;
 }