[patch] O_DIRECT: fix the splitting up of contiguous I/O

[patch] O_DIRECT: fix the splitting up of contiguous I/O

[Jeff Moyer]

Hi,

commit c2c6ca4 (direct-io: do not merge logically non-contiguous
requests) introduced a bug whereby all O_DIRECT I/Os were submitted a
page at a time to the block layer.  The problem is that the code
expected dio->block_in_file to correspond to the current page in the
dio.  In fact, it corresponds to the previous page submitted via
submit_page_section.  This was purely an oversight, as the
dio->cur_page_fs_offset field was introduced for just this purpose.
This patch simply uses the correct variable when calculating whether
there is a mismatch between contiguous logical blocks and contiguous
physical blocks (as described in the comments).

I also switched the if conditional following this check to an else if,
to ensure that we never call dio_bio_submit twice for the same dio (in
theory, this should not happen, anyway).

I've tested this by running blktrace and verifying that a 64KB I/O was
submitted as a single I/O.  I also ran the patched kernel through
xfstests' aio tests using xfs, ext4 (with 1k and 4k block sizes) and
btrfs and verified that there were no regressions as compared to an
unpatched kernel.

Comments, as always, are welcome.

Cheers,
Jeff

Signed-off-by: Jeff Moyer <jmoyer@redhat.com>
Acked-by: Josef Bacik <jbacik@redhat.com>

diff --git a/fs/direct-io.c b/fs/direct-io.c
index 7600aac..445901c 100644
--- a/fs/direct-io.c
+++ b/fs/direct-io.c
@@ -632,7 +632,7 @@ static int dio_send_cur_page(struct dio *dio)
 	int ret = 0;
 
 	if (dio->bio) {
-		loff_t cur_offset = dio->block_in_file << dio->blkbits;
+		loff_t cur_offset = dio->cur_page_fs_offset;
 		loff_t bio_next_offset = dio->logical_offset_in_bio +
 			dio->bio->bi_size;
 
@@ -657,7 +657,7 @@ static int dio_send_cur_page(struct dio *dio)
 		 * Submit now if the underlying fs is about to perform a
 		 * metadata read
 		 */
-		if (dio->boundary)
+		else if (dio->boundary)
 			dio_bio_submit(dio);
 	}
 

Re: [patch] O_DIRECT: fix the splitting up of contiguous I/O

[Christoph Hellwig]

Andrew, can you please send this on to Linus and -stable ASAP?  It's
causing massive problems for our users.

On Thu, Aug 12, 2010 at 04:50:59PM -0400, Jeff Moyer wrote:
> Hi,
> 
> commit c2c6ca4 (direct-io: do not merge logically non-contiguous
> requests) introduced a bug whereby all O_DIRECT I/Os were submitted a
> page at a time to the block layer.  The problem is that the code
> expected dio->block_in_file to correspond to the current page in the
> dio.  In fact, it corresponds to the previous page submitted via
> submit_page_section.  This was purely an oversight, as the
> dio->cur_page_fs_offset field was introduced for just this purpose.
> This patch simply uses the correct variable when calculating whether
> there is a mismatch between contiguous logical blocks and contiguous
> physical blocks (as described in the comments).
> 
> I also switched the if conditional following this check to an else if,
> to ensure that we never call dio_bio_submit twice for the same dio (in
> theory, this should not happen, anyway).
> 
> I've tested this by running blktrace and verifying that a 64KB I/O was
> submitted as a single I/O.  I also ran the patched kernel through
> xfstests' aio tests using xfs, ext4 (with 1k and 4k block sizes) and
> btrfs and verified that there were no regressions as compared to an
> unpatched kernel.
> 
> Comments, as always, are welcome.
> 
> Cheers,
> Jeff
> 
> Signed-off-by: Jeff Moyer <jmoyer@redhat.com>
> Acked-by: Josef Bacik <jbacik@redhat.com>
> 
> diff --git a/fs/direct-io.c b/fs/direct-io.c
> index 7600aac..445901c 100644
> --- a/fs/direct-io.c
> +++ b/fs/direct-io.c
> @@ -632,7 +632,7 @@ static int dio_send_cur_page(struct dio *dio)
>  	int ret = 0;
>  
>  	if (dio->bio) {
> -		loff_t cur_offset = dio->block_in_file << dio->blkbits;
> +		loff_t cur_offset = dio->cur_page_fs_offset;
>  		loff_t bio_next_offset = dio->logical_offset_in_bio +
>  			dio->bio->bi_size;
>  
> @@ -657,7 +657,7 @@ static int dio_send_cur_page(struct dio *dio)
>  		 * Submit now if the underlying fs is about to perform a
>  		 * metadata read
>  		 */
> -		if (dio->boundary)
> +		else if (dio->boundary)
>  			dio_bio_submit(dio);
>  	}
>  
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
---end quoted text---

Re: [Btrfs-devel] [patch] O_DIRECT: fix the splitting up of contiguous I/O

[Chris Samuel]

[-- Attachment #1: Type: Text/Plain, Size: 496 bytes --]

On Fri, 3 Sep 2010 12:25:01 am Christoph Hellwig wrote:

> Andrew, can you please send this on to Linus and -stable ASAP?
> It's causing massive problems for our users.

Did this patch get dropped ?   I don't see it in Linus's git..

https://patchwork.kernel.org/patch/119333/

cheers,
Chris
-- 
 Chris Samuel  :  http://www.csamuel.org/  :  Melbourne, VIC

This email may come with a PGP signature as a file. Do not panic.
For more info see: http://en.wikipedia.org/wiki/OpenPGP

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 482 bytes --]

Re: [Btrfs-devel] [patch] O_DIRECT: fix the splitting up of contiguous I/O

[Andrew Morton]

On Sun, 5 Sep 2010 22:56:08 +1000 Chris Samuel <chris@csamuel.org> wrote:

> On Fri, 3 Sep 2010 12:25:01 am Christoph Hellwig wrote:
> 
> > Andrew, can you please send this on to Linus and -stable ASAP?
> > It's causing massive problems for our users.
> 
> Did this patch get dropped ? 

Nope.  I have it queued for 2.6.36 and -stable, will send it in soon. 
Everything is a bit lagged at present due to conferences, summer,
encroaching senility, etc.

Re: [Btrfs-devel] [patch] O_DIRECT: fix the splitting up of contiguous I/O

[Chris Samuel]

On 06/09/10 02:36, Andrew Morton wrote:

> On Sun, 5 Sep 2010 22:56:08 +1000 Chris Samuel <chris@csamuel.org> wrote:
> 
>> > Did this patch get dropped ? 
>
> Nope.  I have it queued for 2.6.36 and -stable, will send it in soon. 
> Everything is a bit lagged at present due to conferences, summer,
> encroaching senility, etc.

Mea culpa - I mixed up the date of the original email with the
date of Christoph's email and so thought he'd sent that back
in August.  Looks like I'm the one with senility issues.. ;-)

cheers,
Chris
-- 
 Chris Samuel  :  http://www.csamuel.org/  :  Melbourne, VIC