Re: [PATCH 09/15] jbd2: replace barriers with explicit flush / FUA usage

[Jan Kara <jack@suse.cz>]

[-- Attachment #1: Type: text/plain, Size: 2765 bytes --]

On Wed 18-08-10 05:29:17, Christoph Hellwig wrote:
> Switch to the WRITE_FLUSH_FUA flag for journal commits and remove the
> EOPNOTSUPP detection for barriers.
  Well, in the context of just this patch series, the patch is OK so you
can add
  Acked-by: Jan Kara <jack@suse.cz>
But as soon as blkdev_issue_flush() stops draining the queue, commit when
JBD2_FEATURE_INCOMPAT_ASYNC_COMMIT is set breaks. Do you plan to fix
that separately? Actually, attached is a patch which should fix
ASYNC_COMMIT code... The patch is totally untested, I just wrote it
because it's simpler than explaining what needs to be done ;).

								Honza
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Index: linux-2.6/fs/jbd2/commit.c
> ===================================================================
> --- linux-2.6.orig/fs/jbd2/commit.c	2010-08-17 16:52:37.111011684 +0200
> +++ linux-2.6/fs/jbd2/commit.c	2010-08-17 16:53:50.531029420 +0200
> @@ -134,25 +134,10 @@ static int journal_submit_commit_record(
>  
>  	if (journal->j_flags & JBD2_BARRIER &&
>  	    !JBD2_HAS_INCOMPAT_FEATURE(journal,
> -				       JBD2_FEATURE_INCOMPAT_ASYNC_COMMIT)) {
> -		ret = submit_bh(WRITE_SYNC_PLUG | WRITE_BARRIER, bh);
> -		if (ret == -EOPNOTSUPP) {
> -			printk(KERN_WARNING
> -			       "JBD: barrier-based sync failed on %s - "
> -			       "disabling barriers\n", journal->j_devname);
> -			spin_lock(&journal->j_state_lock);
> -			journal->j_flags &= ~JBD2_BARRIER;
> -			spin_unlock(&journal->j_state_lock);
> -
> -			/* And try again, without the barrier */
> -			lock_buffer(bh);
> -			set_buffer_uptodate(bh);
> -			clear_buffer_dirty(bh);
> -			ret = submit_bh(WRITE_SYNC_PLUG, bh);
> -		}
> -	} else {
> +				       JBD2_FEATURE_INCOMPAT_ASYNC_COMMIT))
> +		ret = submit_bh(WRITE_SYNC_PLUG | WRITE_FLUSH_FUA, bh);
> +	else
>  		ret = submit_bh(WRITE_SYNC_PLUG, bh);
> -	}
>  
>  	*cbh = bh;
>  	return ret;
> @@ -167,29 +152,8 @@ static int journal_wait_on_commit_record
>  {
>  	int ret = 0;
>  
> -retry:
>  	clear_buffer_dirty(bh);
>  	wait_on_buffer(bh);
> -	if (buffer_eopnotsupp(bh) && (journal->j_flags & JBD2_BARRIER)) {
> -		printk(KERN_WARNING
> -		       "JBD2: wait_on_commit_record: sync failed on %s - "
> -		       "disabling barriers\n", journal->j_devname);
> -		spin_lock(&journal->j_state_lock);
> -		journal->j_flags &= ~JBD2_BARRIER;
> -		spin_unlock(&journal->j_state_lock);
> -
> -		lock_buffer(bh);
> -		clear_buffer_dirty(bh);
> -		set_buffer_uptodate(bh);
> -		bh->b_end_io = journal_end_buffer_io_sync;
> -
> -		ret = submit_bh(WRITE_SYNC_PLUG, bh);
> -		if (ret) {
> -			unlock_buffer(bh);
> -			return ret;
> -		}
> -		goto retry;
> -	}
>  
>  	if (unlikely(!buffer_uptodate(bh)))
>  		ret = -EIO;
> 
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

[-- Attachment #2: 0001-jbd2-Modify-ASYNC_COMMIT-code-to-not-rely-on-queue-d.patch --]
[-- Type: text/x-patch, Size: 2255 bytes --]

>From 21a76eb2010faf0c4507a238d2d110bac9842146 Mon Sep 17 00:00:00 2001
From: Jan Kara <jack@suse.cz>
Date: Wed, 18 Aug 2010 15:56:56 +0200
Subject: [PATCH] jbd2: Modify ASYNC_COMMIT code to not rely on queue draining on barrier

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.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/jbd2/commit.c |   28 +++++++++++++++-------------
 1 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
index 6a25e51..4c52f78 100644
--- a/fs/jbd2/commit.c
+++ b/fs/jbd2/commit.c
@@ -667,6 +667,15 @@ 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
@@ -685,19 +694,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
@@ -811,6 +807,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.6.4.2