![]() | |||||||||||
Messages in this thread Complete thread (EXPERIMENTAL)Patch in this message |
On Thu 01-05-08 08:16:21, Badari Pulavarty wrote:
Hi Andrew & Jan,
As Mingming pointed out there are few other places where we could hold
the bh reference. Note also that we accumulate references to buffers in the
wbuf[] list and we need that for submit_bh() which consumes one bh
reference. Generally, it seems to me as a too fragile and impractical
rule "nobody can hold bh reference when not holding page lock" which is
basically what it comes down to if you really want to be sure that
journal_try_to_free_buffers() succeeds. And also note that in principle
there are other places which hold references to buffers without holding the
page lock - for example writepage() in ordered mode (although this one is
in practice hardly triggerable). So how we could fix at least the races
with commit code is to implement launder_page() callback for ext3/4 which
would wait for the previous transaction commit in case the page has buffers
that are part of that commit (I don't want this logic in
journal_try_to_free_buffers() as that is called also on memory-reclaim
path, but journal_launder_page() is fine with me). I was able to reproduce the customer problem involving DIO (invalidate_inode_pages2) problem by writing simple testcase to keep writing to a file using buffered writes and DIO writes forever in a loop. I see DIO writes fail with -EIO. After a long debug, found 2 cases how this could happen. These are race conditions with journal_try_to_free_buffers() and journal_commit_transaction(). 1) journal_submit_data_buffers() tries to get bh_state lock. If try lock fails, it drops the j_list_lock and sleeps for bh_state lock, while holding a reference on the buffer. In the meanwhile, journal_try_to_free_buffers() can clean up the journal head could call try_to_free_buffers(). try_to_free_buffers() would fail due to the reference held by journal_submit_data_buffers() - which in turn causes failues for DIO (invalidate_inode_pages2()). 2) When the buffer is on t_locked_list waiting for IO to finish, we hold a reference and give up the cpu, if we can't get bh_state lock. This causes try_to_free_buffers() to fail. Fix is to drop the reference on the buffer if we can't get bh_state lock, give up the cpu and re-try the whole operation - instead of waiting for the vh_state lock. Does this look like a resonable fix ? I am not sure how we are going to gurantee that by the time journal_try_to_free_buffers() get called, the page has buffers are not as part of the current transaction commit(which could be different than the one we waited in ext3_launder_page())? It seems more realistic to fix the races one by one to me. There is still a window that journal_submit_data_buffers() already removed the jh from the bh (when found the buffers are already being synced), but still keep a reference to the buffer head. journal_try_to_free_buffers() could be called. In that case try_to_free_buffers() will be called since there is no jh related to this buffer, and failed due to journal_submit_data_buffers() hasn't finish the cleanup business yet. For this new race, we could just grab the j_list_lock when re-try try_to_free_buffers() to force waiting for journal_commit_transaction() to finish it flush work. But not sure if this is acceptable approach? Patch like this? Comments? Mingming --------------------------------------------------------------------- There are a few cases direct IO could race with kjournal flushing data buffers which could result direct IO return EIO error.1) journal_submit_data_buffers() tries to get bh_state lock. If try lock fails, it drops the j_list_lock and sleeps for bh_state lock, while holding a reference on the buffer. In the meanwhile, journal_try_to_free_buffers() can clean up the journal head could call try_to_free_buffers(). try_to_free_buffers() would fail due to the reference held by journal_submit_data_buffers() - which in turn causes failues for DIO (invalidate_inode_pages2()). 2) When the buffer is on t_locked_list waiting for IO to finish, we hold a reference and give up the cpu, if we can't get bh_state lock. This causes try_to_free_buffers() to fail. 3) when journal_submit_data_buffers() saw the buffer is dirty but failed to lock the buffer bh1, journal_submit_data_buffers() released the j_list_lock and submit other buffers collected from previous check, with the reference to bh1 still hold. During this time journal_try_to_free_buffers() could clean up the journal head of bh1 and remove it from the t_syncdata_list. Then try_to_free_buffers() would fail because the reference held by journal_submit_data_buffers() 4) journal_submit_data_buffers() already removed the jh from the bh (when found the buffers are already being synced), but still keep a reference to the buffer head. journal_try_to_free_buffers() could be called. In that case try_to_free_buffers() will be called since there is no jh related to this buffer, and failed due to journal_submit_data_buffers() hasn't finish the cleanup business yet. Fix for first three races is to drop the reference on the buffer head when release the j_list_lock, give up the cpu and re-try the whole operation. This patch also fixes the race that data buffers has been flushed to disk and journal head is cleard by journal_submit_data_buffers() but did not get a chance to release buffer head reference before the journal_try_to_free_buffers() kicked in. Signed-off-by: Badari Pulavarty <pbadari@us.ibm.com> Signed-off-by: Mingming Cao <mcao@us.ibm.com> --- fs/jbd/commit.c | 21 ++++++++++++++++----- fs/jbd/transaction.c | 13 +++++++++++++ 2 files changed, 29 insertions(+), 5 deletions(-)Index: linux-2.6.26-rc1/fs/jbd/commit.c [view this diff only] ===================================================================
--- linux-2.6.26-rc1.orig/fs/jbd/commit.c 2008-05-03 11:59:44.000000000 -0700
+++ linux-2.6.26-rc1/fs/jbd/commit.c 2008-05-09 14:44:36.000000000 -0700
@@ -79,12 +79,16 @@ nope:
/*
* Try to acquire jbd_lock_bh_state() against the buffer, when j_list_lock is
- * held. For ranking reasons we must trylock. If we lose, schedule away and
+ * held. For ranking reasons we must trylock. If we lose, unlock the buffer
+ * if needed, drop the reference on the buffer, schedule away and
* return 0. j_list_lock is dropped in this case.
*/
-static int inverted_lock(journal_t *journal, struct buffer_head *bh)
+static int inverted_lock(journal_t *journal, struct buffer_head *bh, int locked)
{
if (!jbd_trylock_bh_state(bh)) {
+ if (locked)
+ unlock_buffer(bh);
+ put_bh(bh);
spin_unlock(&journal->j_list_lock);
schedule();
return 0;
@@ -209,19 +213,24 @@ write_out_data:
if (buffer_dirty(bh)) {
if (test_set_buffer_locked(bh)) {
BUFFER_TRACE(bh, "needs blocking lock");
+ put_bh(bh);
spin_unlock(&journal->j_list_lock);
/* Write out all data to prevent deadlocks */
journal_do_submit_data(wbuf, bufs);
bufs = 0;
- lock_buffer(bh);
spin_lock(&journal->j_list_lock);
+ continue;
}
locked = 1;
}
- /* We have to get bh_state lock. Again out of order, sigh. */
- if (!inverted_lock(journal, bh)) {
- jbd_lock_bh_state(bh);
+ /*
+ * We have to get bh_state lock. If the try lock fails,
+ * release the ref on the buffer, give up cpu and retry the
+ * whole operation.
+ */
+ if (!inverted_lock(journal, bh, locked)) {
spin_lock(&journal->j_list_lock);
+ continue;
}
/* Someone already cleaned up the buffer? */
if (!buffer_jbd(bh)
@@ -430,8 +439,7 @@ void journal_commit_transaction(journal_
err = -EIO;
spin_lock(&journal->j_list_lock);
}
- if (!inverted_lock(journal, bh)) {
- put_bh(bh);
+ if (!inverted_lock(journal, bh, 0)) {
spin_lock(&journal->j_list_lock);
continue;
}
Index: linux-2.6.26-rc1/fs/jbd/transaction.c
===================================================================
--- linux-2.6.26-rc1.orig/fs/jbd/transaction.c 2008-05-03 11:59:44.000000000 -0700
+++ linux-2.6.26-rc1/fs/jbd/transaction.c 2008-05-09 09:53:57.000000000 -0700
@@ -1714,6 +1714,19 @@ int journal_try_to_free_buffers(journal_
goto busy;
} while ((bh = bh->b_this_page) != head);
ret = try_to_free_buffers(page);
+ if (ret == 0) {
+ /*
+ * it is possible that journal_submit_data_buffers()
+ * still holds the bh ref even if clears the jh
+ * after journal_remove_journal_head,
+ * which leads to try_to_free_buffers() failed
+ * let's wait for journal_submit_data_buffers()
+ * to finishing remove the bh from the sync_data_list
+ */
+ spin_lock(&journal->j_list_lock);
+ ret = try_to_free_buffers(page);
+ spin_unlock(&journal->j_list_lock);
+ }
busy:
return ret;
}
| ||||||||||
![]() | |||||||||||
| Last update: 2008-05-10 00:31 [W:1.099 / U:0.100 seconds] ©2003-2005 Jasper Spaans | |||||||||||