ocfs2: avoid direct write if we fall back to buffered v3

PATCH New Li Dongyang 3 participants 9 messages Apr 17, 2010 – May 4, 2010 OCFS2

Share & tools

Li Dongyang

author

2010-04-17 09:49 UTC #0 lore raw

PATCH 1/1 ocfs2: avoid direct write if we fall back to buffered v3

when we fall back to buffered write from direct write, we call __generic_file_aio_write but that will end up doing direct write even we are only prepared to do buffered write because the file has O_DIRECT flag set. This is a fix for
https://bugzilla.novell.com/show_bug.cgi?id=591039
revised with Joel's comments.

Signed-off-by Li Dongyang <[email protected]>

---
 fs/ocfs2/file.c |   25 ++++++++++++++-----------
 1 files changed, 14 insertions(+), 11 deletions(-)

fs/ocfs2/file.c +14 ~~-11~~

index de059f4..0c15be9 100644
@@ -1973,18 +1973,18 @@ relock:
 	/* communicate with ocfs2_dio_end_io */
 	ocfs2_iocb_set_rw_locked(iocb, rw_level);
 
-	if (direct_io) {
-		ret = generic_segment_checks(iov, &nr_segs, &ocount,
-					     VERIFY_READ);
-		if (ret)
-			goto out_dio;
+	ret = generic_segment_checks(iov, &nr_segs, &ocount,
+				     VERIFY_READ);
+	if (ret)
+		goto out_dio;
 
-		count = ocount;
-		ret = generic_write_checks(file, ppos, &count,
-					   S_ISBLK(inode->i_mode));
-		if (ret)
-			goto out_dio;
+	count = ocount;
+	ret = generic_write_checks(file, ppos, &count,
+				   S_ISBLK(inode->i_mode));
+	if (ret)
+		goto out_dio;
 
+	if (direct_io) {
 		written = generic_file_direct_write(iocb, iov, &nr_segs, *ppos,
 						    ppos, count, ocount);
 		if (written < 0) {

@@ -1999,7 +1999,10 @@ relock:
 			goto out_dio;
 		}
 	} else {
-		written = __generic_file_aio_write(iocb, iov, nr_segs, ppos);
+		current->backing_dev_info = file->f_mapping->backing_dev_info;
+		written = generic_file_buffered_write(iocb, iov, nr_segs, *ppos
+						      ppos, count, 0);
+		current->backing_dev_info = NULL;
 	}
 
 out_dio:

Signature · 1 line

1.6.4.2

Li Dongyang

author

2010-04-26 09:56 UTC #1 lore raw

PATCH 1/1 ocfs2: avoid direct write if we fall back to buffered v3

Quoted 48 lines · On Saturday 17 April 2010 17:49:10 Li Dongyang wrote:

Signed-off-by Li Dongyang <[email protected]>

---
 fs/ocfs2/file.c |   25 ++++++++++++++-----------
 1 files changed, 14 insertions(+), 11 deletions(-)

fs/ocfs2/file.c +10 ~~-10~~

index de059f4..0c15be9 100644
@@ -1973,18 +1973,18 @@ relock:
 	/* communicate with ocfs2_dio_end_io */
 	ocfs2_iocb_set_rw_locked(iocb, rw_level);
 
-	if (direct_io) {
-		ret = generic_segment_checks(iov, &nr_segs, &ocount,
-					     VERIFY_READ);
-		if (ret)
-			goto out_dio;
+	ret = generic_segment_checks(iov, &nr_segs, &ocount,
+				     VERIFY_READ);
+	if (ret)
+		goto out_dio;
 
-		count = ocount;
-		ret = generic_write_checks(file, ppos, &count,
-					   S_ISBLK(inode->i_mode));
-		if (ret)
-			goto out_dio;
+	count = ocount;
+	ret = generic_write_checks(file, ppos, &count,
+				   S_ISBLK(inode->i_mode));
+	if (ret)
+		goto out_dio;
 
+	if (direct_io) {
 		written = generic_file_direct_write(iocb, iov, &nr_segs, *ppos,
 						    ppos, count, ocount);
 		if (written < 0) {

@@ -1999,7 +1999,10 @@ relock:
 			goto out_dio;

fs/ocfs2/file.c relock: · line 2008

Quoted 10 lines

fs/ocfs2/file.c +4 -1

 		}
 	} else {
-		written = __generic_file_aio_write(iocb, iov, nr_segs, ppos);
+		current->backing_dev_info = file->f_mapping->backing_dev_info;
+		written = generic_file_buffered_write(iocb, iov, nr_segs, *ppos
+						      ppos, count, 0);
+		current->backing_dev_info = NULL;
 	}
 
 out_dio:

Joel, any comments on this update?
Br,
Li Dongyang

Joel Becker

2010-04-26 22:24 UTC #2 lore raw

PATCH 1/1 ocfs2: avoid direct write if we fall back to buffered v3

In reply to Li Dongyang

Quoted 8 lines · Li Dongyang

when we fall back to buffered write from direct write, we call __generic_file_aio_write but that will end up doing direct write even we are only prepared to do buffered write because the file has O_DIRECT flag set. This is a fix for
https://bugzilla.novell.com/show_bug.cgi?id=591039
revised with Joel's comments.
Signed-off-by Li Dongyang <[email protected]>

This looks good to me. I'd like Mark's ack before I push it to
merge-window.

Joel

Signature · 8 lines

"Sometimes one pays most for the things one gets for nothing."
        - Albert Einstein

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127

Mark Fasheh

2010-04-30 20:37 UTC #3 lore raw

PATCH 1/1 ocfs2: avoid direct write if we fall back to buffered v3

Quoted 8 lines · Joel Becker

Quoted 7 lines · Li Dongyang
when we fall back to buffered write from direct write, we call __generic_file_aio_write but that will end up doing direct write even we are only prepared to do buffered write because the file has O_DIRECT flag set. This is a fix for
https://bugzilla.novell.com/show_bug.cgi?id=591039
revised with Joel's comments.

Quoted 4 lines

Quoted 1 line
Signed-off-by Li Dongyang <[email protected]>
This looks good to me. I'd like Mark's ack before I push it to
merge-window.

Looks good to me. Nice catch on the backing_dev_info bits.

Acked-by Mark Fasheh <[email protected]>

	--Mark

--
Mark Fasheh

Joel Becker

2010-04-30 20:47 UTC #4 lore raw

PATCH 1/1 ocfs2: avoid direct write if we fall back to buffered v3

In reply to Li Dongyang

Quoted 6 lines · Li Dongyang

when we fall back to buffered write from direct write, we call __generic_file_aio_write but that will end up doing direct write even we are only prepared to do buffered write because the file has O_DIRECT flag set. This is a fix for
https://bugzilla.novell.com/show_bug.cgi?id=591039
revised with Joel's comments.

Have you tested this version? I got a compile error:

fs/ocfs2/file.c relock: · line 2006

Quoted 10 lines

fs/ocfs2/file.c +4 -1

@@ -1999,7 +1999,10 @@ relock:
 			goto out_dio;
 		}
 	} else {
-		written = __generic_file_aio_write(iocb, iov, nr_segs, ppos);
+		current->backing_dev_info = file->f_mapping->backing_dev_info;
+		written = generic_file_buffered_write(iocb, iov, nr_segs, *ppos
+						      ppos, count, 0);
+		current->backing_dev_info = NULL;
 	}

- written = generic_file_buffered_write(iocb, iov, nr_segs, *ppos
+ written = generic_file_buffered_write(iocb, iov, nr_segs, *ppos,

Joel

Signature · 9 lines

"I think it would be a good idea."  
        - Mahatma Ghandi, when asked what he thought of Western
          civilization

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127

Joel Becker

2010-04-30 20:52 UTC #5 lore raw

PATCH 1/1 ocfs2: avoid direct write if we fall back to buffered v3

Quoted 1 line · Joel Becker

Have you tested this version? I got a compile error:

I want to be clear - has this patch (v3) been run against the
fsstress that caused the bug report in the first place? Does the fsstress now pass? Let me know so I can send it upstream.

Joel

Signature · 9 lines

Life's Little Instruction Book #314

	"Never underestimate the power of forgiveness."

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127

Li Dongyang

author

2010-05-04 01:37 UTC #6 lore raw

PATCH 1/1 ocfs2: avoid direct write if we fall back to buffered v3

Quoted 9 lines · On Saturday 01 May 2010 04:52:43 Joel Becker wrote:

Quoted 1 line · Joel Becker
Have you tested this version? I got a compile error:
I want to be clear - has this patch (v3) been run against the
fsstress that caused the bug report in the first place? Does the fsstress now pass? Let me know so I can send it upstream.
Joel

it's a typo I made on my desktop, I first test it on my 2-node boxes, and it had already running fsstress for at least 70hrs(50 hrs with the OCFS2_INODE_SKIP_ORPHAN_DIR patches),
then I checked out the ocfs2/master branch on my desktop in case of the differences between ocfs2/master and linus tree(like the block check in ocfs2_direct_IO_get_blocks). and made changes, then generated the patch, and a typo is made here, what a silly mistake!
sorry for the inconvenience here.
Br,
Li Dongyang

Joel Becker

2010-05-04 01:47 UTC #7 lore raw

PATCH 1/1 ocfs2: avoid direct write if we fall back to buffered v3

Quoted 10 lines · Li Dongyang

Quoted 9 lines · On Saturday 01 May 2010 04:52:43 Joel Becker wrote:
Quoted 1 line · Joel Becker
Have you tested this version? I got a compile error:
I want to be clear - has this patch (v3) been run against the
fsstress that caused the bug report in the first place? Does the fsstress now pass? Let me know so I can send it upstream.
Joel

Quoted 7 lines

it's a typo I made on my desktop, I first test it on my 2-node boxes, and it had already running fsstress for at least 70hrs(50 hrs with the OCFS2_INODE_SKIP_ORPHAN_DIR patches),
then I checked out the ocfs2/master branch on my desktop in case of the differences between ocfs2/master and linus tree(like the block check in ocfs2_direct_IO_get_blocks). and made changes, then generated the patch, and a typo is made here, what a silly mistake!

And you are sure that the changes made by-hand on your desktop
are the same as made on your 2-node machines?

Joel

Signature · 8 lines

"I don't even butter my bread; I consider that cooking."
         - Katherine Cebrian

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127

Li Dongyang

author

2010-05-04 02:10 UTC #8 lore raw

PATCH 1/1 ocfs2: avoid direct write if we fall back to buffered v3

Quoted 19 lines · On Tuesday 04 May 2010 09:47:23 Joel Becker wrote:

Quoted 10 lines · Li Dongyang
Quoted 8 lines · On Saturday 01 May 2010 04:52:43 Joel Becker wrote:
Quoted 1 line · Joel Becker
Have you tested this version? I got a compile error:
I want to be clear - has this patch (v3) been run against the
fsstress that caused the bug report in the first place? Does the fsstress now pass? Let me know so I can send it upstream.
Joel
Quoted 7 lines
it's a typo I made on my desktop, I first test it on my 2-node boxes, and it had already running fsstress for at least 70hrs(50 hrs with the OCFS2_INODE_SKIP_ORPHAN_DIR patches),
then I checked out the ocfs2/master branch on my desktop in case of the differences between ocfs2/master and linus tree(like the block check in ocfs2_direct_IO_get_blocks). and made changes, then generated the patch, and a typo is made here, what a silly mistake!

Quoted 5 lines

And you are sure that the changes made by-hand on your desktop
are the same as made on your 2-node machines?
Joel

I just got a diff on my 2-node boxes, and checked it char to char, the only difference is the v3 patch lack the comma you pointed.

Br,
Li Dongyang