On Mon, Mar 22, 2010 at 07:14:13PM +0300, Dmitry Monakhov wrote: > > > > Just to be clear, since I realized I wrote fsync() when I should have > > written fs/ext4/fsync.c, my proposal was to put this check in > > ext4_sync_file(). > This means that we will end up with two io-barriers in a row > for external journal case: > ext4_sync_file() > ->jbd2_log_start_commit() > ->blkdev_issue_flush(j_fs_dev) > /* seems following flush is mandatory to guarantee the metadata > * consistency */ > ->blkdev_issue_flush(j_fs_dev) Well, not if we only issue a barrier in the external barrier case.... but I agree the logic may start getting ugly. > I've already started to work with the patch but was surprised with > commit logic. > ->__jbd2_log_start_commit(target) > journal->j_commit_request = target > ->wake_up(&journal->j_wait_commit) > ->kjournald2 > transaction = journal->j_running_transaction > ->jbd2_journal_commit_transaction(journal); > commit_transaction = journal->j_running_transaction > ... > /* So j_commit_request is used only for comparison. But actually > committing journal->j_running_transaction->t_tid transaction > instead of j_commit_request. Why? > */ Yeah, I don't think jbd2_log_start_commit() has the semantics that are currently documented. We will return 0 if we wake up the commit thread, yes --- but since we only check j_commit_request, and it's a geq test, it might very well be the case that the transaction was committed long ago, and so we end up kicking off a transaction commit when one is not needed. Or, it may be that a transaction is just already being committed (and in fact is just about to be completed), and so the wake_up() call is a no-op, and so we don't get a barrier when one is needed (and expected) by ext4_sync_file(). We need to look at this very closely, but I think jbd2_log_start_commit() needs to check to see whether there is a current committing transaction, and whether it is the one which is that has been requested as a target. If so, and a barrier is requested, I think we need to have jbd2_log_start_commit() do the barrier. There is a risk of having a double barrier in that case, but modifying the flag on the currently committing transaction has the danger of being lost by kjournald --- or I guess alternatively we could have kjournald check that flag while holding j_state_lock, which might be better. If the currently running transaction is the requested target, then that's easy; we can set the flag, and then wake up the j_wait_commit wait queue. In any case, log_start_commit() doesn't currently distinguish between whether the targetted commit is the running or the committing transaction when it returns 0, and I think that's a problem. - Ted -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
