On Mon 06-09-10 13:15:52, Andreas Dilger wrote:

On 2010-08-26, at 10:23, Tejun Heo wrote:

quoted

From 49f4cef00a1bd3c79fb2fe1f982c5157f0792867 Mon Sep 17 00:00:00 2001
From: Jan Kara <jack@suse.cz>

Currently JBD2 relies blkdev_issue_flush() draining the queue when ASYNC_COMMIT
feature is set. This property is going away so make JBD2 wait for buffers it
needs on its own before submitting the cache flush.

I finally had a chance to look at this patch more closely, and I think it
may be breaking the ASYNC_COMMIT functionality, by forcing a wait for all
of the data blocks _before_ the journal commit block is even submitted,
even though ASYNC_COMMIT is enabled.

  Yes, we do wait for *data* blocks before submitting commit block. We have
to do it even while checksumming because data blocks aren't part of the
checksum. Only metadata is checksummed and thus only metadata blocks
are safe to be waited for after the commit block is submitted.

									Honza

When ASYNC_COMMIT is enabled, it means that the journal transaction
coherency is handled by the commit block checksum of the transaction data
blocks, so the commit block can be submitted to the journal at the same
time as the transaction data blocks.  The flush on the journal device
(and the filesystem device, if they are separate) should happen after
both are submitted.

However, if ASYNC_COMMIT is NOT enabled, then the transaction data blocks
should be submitted and flushed before the journal commit block is
submitted, and then there should be a second cache flush afterward.

---

quoted

This patch is necessary before enabling flush/fua support in jbd2.
The flush-fua git tree has been udpated to included this between patch
24 and 25.

Thanks.

fs/jbd2/commit.c |   29 ++++++++++++++++-------------
1 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
index 7c068c1..8797fd1 100644
--- a/fs/jbd2/commit.c
+++ b/fs/jbd2/commit.c

@@ -701,6 +701,16 @@ start_journal_io:

		}
	}

+	err = journal_finish_inode_data_buffers(journal, commit_transaction);
+	if (err) {
+		printk(KERN_WARNING
+			"JBD2: Detected IO errors while flushing file data "
+		       "on %s\n", journal->j_devname);
+		if (journal->j_flags & JBD2_ABORT_ON_SYNCDATA_ERR)
+			jbd2_journal_abort(journal, err);
+		err = 0;
+	}
+
	/*
	 * If the journal is not located on the file system device,
	 * then we must flush the file system device before we issue

@@ -719,19 +729,6 @@ start_journal_io:

						 &cbh, crc32_sum);
		if (err)
			__jbd2_journal_abort_hard(journal);
-		if (journal->j_flags & JBD2_BARRIER)
-			blkdev_issue_flush(journal->j_dev, GFP_KERNEL, NULL,
-				BLKDEV_IFL_WAIT);
-	}
-
-	err = journal_finish_inode_data_buffers(journal, commit_transaction);
-	if (err) {
-		printk(KERN_WARNING
-			"JBD2: Detected IO errors while flushing file data "
-		       "on %s\n", journal->j_devname);
-		if (journal->j_flags & JBD2_ABORT_ON_SYNCDATA_ERR)
-			jbd2_journal_abort(journal, err);
-		err = 0;
	}

	/* Lo and behold: we have just managed to send a transaction to

@@ -845,6 +842,12 @@ wait_for_iobuf:

	}
	if (!err && !is_journal_aborted(journal))
		err = journal_wait_on_commit_record(journal, cbh);
+	if (JBD2_HAS_INCOMPAT_FEATURE(journal,
+				      JBD2_FEATURE_INCOMPAT_ASYNC_COMMIT) &&
+	    journal->j_flags & JBD2_BARRIER) {
+			blkdev_issue_flush(journal->j_dev, GFP_KERNEL, NULL,
+				BLKDEV_IFL_WAIT);
+	}

	if (err)
		jbd2_journal_abort(journal, err);
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Cheers, Andreas

-- 
Jan Kara [off-list ref]
SUSE Labs, CR