PATCH] fix potential latency issues in JBD's journal code

PATCH] fix potential latency issues in JBD's journal code

[Arjan van de Ven]

From: Arjan van de Ven <arjan@linux.intel.com>
Date: Fri, 4 Jul 2008 19:26:16 -0700
Subject: [PATCH] fix potential latency issues in JBD's journal code
CC: akpm@linux-foundation.org
CC: jens.axboe@oracle.com

This is a follow-on to commit 18ce3751ccd488c78d3827e9f6bf54e6322676fb
(Properly notify block layer of sync writes)
which fixed some severe latency issues due to a "submit IO and then wait for it"
pattern, which got fixed by properly informing the block layer that the IOs in
question are going to be waited on immediately after a batch submission.

In the JBD layer, some of the core journal routines have the exact same pattern
of code, and... surprising (or not)... they're also not using the WRITE_SYNC
variant to inform the blocklayer.

This patch modifies two key places that submit IO that then immediately will
get waited on. The JBD code is slightly convoluted, but after some chasing of
abstraction layers, these instances seem to really be of this pattern.
There's one case in checkpoint.c which is another candidate, but I've not
been able to get my head around the code enough to verify that that one
really is of this pattern as well, so for now I'll leave that one as is.

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
---
 fs/jbd/commit.c |    4 ++--
 fs/jbd/revoke.c |    2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/jbd/commit.c b/fs/jbd/commit.c
index 5a8ca61..0b348de 100644
--- a/fs/jbd/commit.c
+++ b/fs/jbd/commit.c
@@ -165,7 +165,7 @@ static void journal_do_submit_data(struct buffer_head **wbuf, int bufs)
 	for (i = 0; i < bufs; i++) {
 		wbuf[i]->b_end_io = end_buffer_write_sync;
 		/* We use-up our safety reference in submit_bh() */
-		submit_bh(WRITE, wbuf[i]);
+		submit_bh(WRITE_SYNC, wbuf[i]);
 	}
 }
 
@@ -622,7 +622,7 @@ start_journal_io:
 				clear_buffer_dirty(bh);
 				set_buffer_uptodate(bh);
 				bh->b_end_io = journal_end_buffer_io_sync;
-				submit_bh(WRITE, bh);
+				submit_bh(WRITE_SYNC, bh);
 			}
 			cond_resched();
 
diff --git a/fs/jbd/revoke.c b/fs/jbd/revoke.c
index 1bb43e9..366ee20 100644
--- a/fs/jbd/revoke.c
+++ b/fs/jbd/revoke.c
@@ -616,7 +616,7 @@ static void flush_descriptor(journal_t *journal,
 	set_buffer_jwrite(bh);
 	BUFFER_TRACE(bh, "write");
 	set_buffer_dirty(bh);
-	ll_rw_block(SWRITE, 1, &bh);
+	ll_rw_block(SWRITE_SYNC, 1, &bh);
 }
 #endif
 
-- 
1.5.5.1


-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: PATCH] fix potential latency issues in JBD's journal code

[Arjan van de Ven]

Helps if I send the patch that compiles ;)

From: Arjan van de Ven <arjan@linux.intel.com>
Subject: [PATCH] fix potential latency issues in JBD's journal code

This is a follow-on to commit 18ce3751ccd488c78d3827e9f6bf54e6322676fb
 (Properly notify block layer of sync writes)
 which fixed some severe latency issues due to a "submit IO and then wait for it"
 pattern, which got fixed by properly informing the block layer that the IOs in
 question are going to be waited on immediately after a batch submission.

In the JBD layer, some of the core journal routines have the exact same pattern
of code, and... surprising (or not)... they're also not using the WRITE_SYNC
variant to inform the blocklayer.

This patch modifies two key places that submit IO that then immediately will
get waited on. The JBD code is slightly convoluted, but after some chasing of
abstraction layers, these instances seem to really be of this pattern.
There's one case in checkpoint.c which is another candidate, but I've not
been able to get my head around the code enough to verify that this one
really is of this pattern.

Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
---
 fs/jbd/commit.c |    5 +++--
 fs/jbd/revoke.c |    3 ++-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/jbd/commit.c b/fs/jbd/commit.c
index 5a8ca61..f3bc0b0 100644
--- a/fs/jbd/commit.c
+++ b/fs/jbd/commit.c
@@ -15,6 +15,7 @@
 
 #include <linux/time.h>
 #include <linux/fs.h>
+#include <linux/bio.h>
 #include <linux/jbd.h>
 #include <linux/errno.h>
 #include <linux/slab.h>
@@ -165,7 +166,7 @@ static void journal_do_submit_data(struct buffer_head **wbuf, int bufs)
 	for (i = 0; i < bufs; i++) {
 		wbuf[i]->b_end_io = end_buffer_write_sync;
 		/* We use-up our safety reference in submit_bh() */
-		submit_bh(WRITE, wbuf[i]);
+		submit_bh(WRITE_SYNC, wbuf[i]);
 	}
 }
 
@@ -622,7 +623,7 @@ start_journal_io:
 				clear_buffer_dirty(bh);
 				set_buffer_uptodate(bh);
 				bh->b_end_io = journal_end_buffer_io_sync;
-				submit_bh(WRITE, bh);
+				submit_bh(WRITE_SYNC, bh);
 			}
 			cond_resched();
 
diff --git a/fs/jbd/revoke.c b/fs/jbd/revoke.c
index 1bb43e9..ce92dd5 100644
--- a/fs/jbd/revoke.c
+++ b/fs/jbd/revoke.c
@@ -67,6 +67,7 @@
 #include <linux/slab.h>
 #include <linux/list.h>
 #include <linux/init.h>
+#include <linux/bio.h>
 #endif
 #include <linux/log2.h>
 
@@ -616,7 +617,7 @@ static void flush_descriptor(journal_t *journal,
 	set_buffer_jwrite(bh);
 	BUFFER_TRACE(bh, "write");
 	set_buffer_dirty(bh);
-	ll_rw_block(SWRITE, 1, &bh);
+	ll_rw_block(SWRITE_SYNC, 1, &bh);
 }
 #endif
 
-- 
1.5.5.1



-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: PATCH] fix potential latency issues in JBD's journal code

[Theodore Tso]

Acked-by: "Theodore Ts'o" <tytso@mit.edu>

I will make the corresponding change for ext4.  Is the "Properly
notify block layer of sync writes" going to be pushed to mainline?  I
can't add SWRITE_SYNC to the ext4 tree without growing a dependency to
this patch.

> This patch modifies two key places that submit IO that then immediately will
> get waited on. The JBD code is slightly convoluted, but after some chasing of
> abstraction layers, these instances seem to really be of this pattern.
> There's one case in checkpoint.c which is another candidate, but I've not
> been able to get my head around the code enough to verify that that one
> really is of this pattern as well, so for now I'll leave that one as is.

You mean the ll_rw_block() SWRITE in __flush_patch?  Yep, seems to be
of the same pattern.

						- Ted

Re: PATCH] fix potential latency issues in JBD's journal code

[Arjan van de Ven]

On Sat, 5 Jul 2008 10:13:35 -0400
Theodore Tso <tytso@mit.edu> wrote:

> Acked-by: "Theodore Ts'o" <tytso@mit.edu>
> 
> I will make the corresponding change for ext4.  Is the "Properly
> notify block layer of sync writes" going to be pushed to mainline?  I
> can't add SWRITE_SYNC to the ext4 tree without growing a dependency to
> this patch.

you have stale bits .. it's in Mainline for like at least 24 hours
already ;=)

-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: PATCH] fix potential latency issues in JBD's journal code

[Theodore Tso]

On Fri, Jul 04, 2008 at 07:29:29PM -0700, Arjan van de Ven wrote:
> @@ -165,7 +165,7 @@ static void journal_do_submit_data(struct buffer_head **wbuf, int bufs)
>  	for (i = 0; i < bufs; i++) {
>  		wbuf[i]->b_end_io = end_buffer_write_sync;
>  		/* We use-up our safety reference in submit_bh() */
> -		submit_bh(WRITE, wbuf[i]);
> +		submit_bh(WRITE_SYNC, wbuf[i]);
>  	}
>  }

So I started looking at this patch more closely when trying to
replicate it for ext4.  Don't you want to only use WRITE_SYNC() only
for the very last time in the loop?  Otherwise you end up unplugging
the queue after each bufferhead, which wouldn't be a good thing, right?

    	  	     		       		- Ted

Re: PATCH] fix potential latency issues in JBD's journal code

[Arjan van de Ven]

On Sun, 6 Jul 2008 00:15:02 -0400
Theodore Tso <tytso@MIT.EDU> wrote:

> On Fri, Jul 04, 2008 at 07:29:29PM -0700, Arjan van de Ven wrote:
> > @@ -165,7 +165,7 @@ static void journal_do_submit_data(struct
> > buffer_head **wbuf, int bufs) for (i = 0; i < bufs; i++) {
> >  		wbuf[i]->b_end_io = end_buffer_write_sync;
> >  		/* We use-up our safety reference in submit_bh() */
> > -		submit_bh(WRITE, wbuf[i]);
> > +		submit_bh(WRITE_SYNC, wbuf[i]);
> >  	}
> >  }
> 
> So I started looking at this patch more closely when trying to
> replicate it for ext4.  Don't you want to only use WRITE_SYNC() only
> for the very last time in the loop?  Otherwise you end up unplugging
> the queue after each bufferhead, which wouldn't be a good thing,
> right?

it's debatable. Because this submit-bh() will sometimes block... it
wouldn't get IO started for that case. I think this is where I'd like
Jens to say what he thinks the rules are; clearly the elevator needs to
treat all of these guys as sync, but maybe the plugging should be ..
flexible.


-- 
If you want to reach me at my work email, use arjan@linux.intel.com
For development, discussion and tips for power savings, 
visit http://www.lesswatts.org

Re: PATCH] fix potential latency issues in JBD's journal code

[Theodore Tso]

On Sat, Jul 05, 2008 at 10:30:21PM -0700, Arjan van de Ven wrote:
> it's debatable. Because this submit-bh() will sometimes block... it
> wouldn't get IO started for that case. I think this is where I'd like
> Jens to say what he thinks the rules are; clearly the elevator needs to
> treat all of these guys as sync, but maybe the plugging should be ..
> flexible.

Stupid idea.... maybe the unplugging should be done on the wait side?
Has this been considered already?

						- Ted