Messages in this thread | | | Subject | Re: [PATCH] Fix DIO EIO error caused by race between jbd_commit_transaction() and journal_try_to_drop_buffers() | From | Mingming Cao <> | Date | Fri, 16 May 2008 10:30:35 -0700 |
| |
On Fri, 2008-05-16 at 10:17 -0700, Badari Pulavarty wrote:
> > @@ -1713,7 +1753,31 @@ 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); > > + > > + /* > > + * In the case of concurrent direct IO and buffered IO, > > + * There are a number of places where we > > + * could race with journal_commit_transaction(), the later still > > + * helds the reference to the buffers to free while processing them. > > + * try_to_free_buffers() failed to free those buffers, > > + * resulting in an unexpected EIO error > > + * returns back to the generic_file_direct_IO() > > + * > > + * So let's wait for the current transaction finished flush > > + * dirty data buffers before we try to free those buffers > > + * again. This wait is needed by direct IO code path only, > > + * gfp_mask __GFP_REPEAT is passed from the direct IO code > > + * path to flag if we need to wait and retry free buffers. > > + */ > > + if (ret == 0 && dio) { > > drop "dio" variable and compare here, like > if (ret == 0 && (gfp_mask & __GFP_REPEAT) >
Okay, will do. Also will update the patch with other format changes you suggested.
> > + spin_lock(&journal->j_state_lock); > > + journal_wait_for_transaction_sync_data(journal); > > + ret = try_to_free_buffers(page); > > + spin_unlock(&journal->j_state_lock); > > + } >
Also, This patch changed the struct journal_s to add a new wait queue, so that journal_try_to_free_buffers() could only need to wait for data to be commited, rather than using j_wait_done_commit queue and wait for the whole transaction being committed. It might break other fs that uses journal_s, thus not worth it. Will update the patch.
| |