Ocfs2 devel - Apr 2010 - [PATCH] ocfs2: avoid direct write if we fall back to buffered

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


Signed-off-by: Li Dongyang <lidongyang at novell.com>
---
 fs/ocfs2/file.c |   27 +++++++++++++++++----------
 1 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
index de059f4..707f2a2 100644
--- a/fs/ocfs2/file.c
+++ b/fs/ocfs2/file.c
@@ -1973,18 +1973,24 @@ 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,
+	count = ocount;
+	ret = generic_write_checks(file, ppos, &count,
 					   S_ISBLK(inode->i_mode));
-		if (ret)
-			goto out_dio;
+	if (ret)
+		goto out_dio;
+
+	ret = file_remove_suid(file);
+	if (ret)
+		goto out_dio;
 
+	file_update_time(file);
+
+	if (direct_io) {
 		written = generic_file_direct_write(iocb, iov, &nr_segs, *ppos,
 						    ppos, count, ocount);
 		if (written < 0) {
@@ -1999,7 +2005,8 @@ relock:
 			goto out_dio;
 		}
 	} else {
-		written = __generic_file_aio_write(iocb, iov, nr_segs, ppos);
+		written = generic_file_buffered_write(iocb, iov, nr_segs,
+				*ppos, ppos, count, 0);
 	}
 
 out_dio:
-- 
1.7.0.3

I cannot read the bugzilla. Now it maybe that that bz
cannot be made public. That's ok. But if that's the case,
can you explain the problem encountered. I am not qs
the fix... rather trying to understand why this has not
been reported before.

Li Dongyang wrote:
> 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
>
>
> Signed-off-by: Li Dongyang <lidongyang at novell.com>
> ---
>  fs/ocfs2/file.c |   27 +++++++++++++++++----------
>  1 files changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
> index de059f4..707f2a2 100644
> --- a/fs/ocfs2/file.c
> +++ b/fs/ocfs2/file.c
> @@ -1973,18 +1973,24 @@ 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,
> +	count = ocount;
> +	ret = generic_write_checks(file, ppos, &count,
>  					   S_ISBLK(inode->i_mode));
> -		if (ret)
> -			goto out_dio;
> +	if (ret)
> +		goto out_dio;
> +
> +	ret = file_remove_suid(file);
> +	if (ret)
> +		goto out_dio;
>  
> +	file_update_time(file);
> +
> +	if (direct_io) {
>  		written = generic_file_direct_write(iocb, iov, &nr_segs, *ppos,
>  						    ppos, count, ocount);
>  		if (written < 0) {
> @@ -1999,7 +2005,8 @@ relock:
>  			goto out_dio;
>  		}
>  	} else {
> -		written = __generic_file_aio_write(iocb, iov, nr_segs, ppos);
> +		written = generic_file_buffered_write(iocb, iov, nr_segs,
> +				*ppos, ppos, count, 0);
>  	}
>  
>  out_dio:
>

I still get a bug with this check and without my patch:


[16179.955148] (13400,1):ocfs2_truncate_file:465 ERROR: bug expression:
le64_to_cpu(fe->i_size) != i_size_read(inode)
[16179.955157] (13400,1):ocfs2_truncate_file:465 ERROR: Inode 254789, inode
i_size = 811008 != di i_size = 809011, i_flags = 0x1
the call trace is the same.


the problem is this check in ocfs2_direct_IO_get_blocks just check if we are
going beyond the blocks right now,
so if a direct write won't play with new blocks but extending the i_size
still get a pass, like the error above said, di->i_size is 809011, using 198
blocks and the direct write end up with i_size 811008, just same 198 blocks.


IMHO, we can add this check back and fix this check, or we don't try to do
direct write if we decided we can't in ocfs2_file_aio_write, after calling
ocfs2_prepare_inode_for_write as my patch said.


Comments? ;-)


Br,
Li Dongyang
>>> Sunil Mushran  04/10/10 1:42 AM >>>
  Li Dongyang wrote:
> On Friday 09 April 2010 11:32:10 Tao Ma wrote:
>> Hi Dongyang,
>>
>> Li Dongyang wrote:
>>> Hi, Tao,
>>>
>>> On Friday 09 April 2010 10:38:33 Tao Ma wrote:
>>>> Hi Dongyang,
>>>>
>>>> Li Dongyang wrote:
>>>>> This is because ocfs2_file_aio_write calls
>>>>> ocfs2_prepare_inode_for_write which sets direct_io to 0 if
it finds out
>>>>> that direct IO would extend the file. But later we call
>>>>> __generic_file_aio_write which end's up calling
>>>>> generic_file_direct_write because the file has O_DIRECT
flag.So every
>>>>> time we do a direct write extending the file, the
inode->i_size gets
>>>>> inconsistent with the i_size on disk because we call
>>>>> generic_file_direct_write, and if we do a truncate after
this, we will
>>>>> meet a bug in ocfs2_truncate_file.
>>>> yes we have O_DIRECT flag set and in __generic_file_aio_write
it will
>>>> call generic_file_direct_write first and then trigger to
>>>> ocfs2_direct_IO. In this function we will check again and
return 0. And
>>>> _generic_file_aio_write will fall back to buffered write if the
directIO
>>>> can't write. Am I wrong somehow?
>>> yes ocfs2_direct_IO has some check, but it just check if we are
>>> appending(the i_size <= offset), if the offset < i_size and
offset +
>>> count > i_size, it will do direct io anyway. seems we also can
fix this
>>> by adding a check to ocfs2_direct_IO.
>> It is done by ocfs2_direct_IO_get_blocks. Just debug the kernel and you
>> will get what I mean. ;)
> Do you mean this section in ocfs2_direct_IO_get_blocks:?
> /*
>  * Any write past EOF is not allowed because we'd be extending.
>  */
> if (create && (iblock + max_blocks) > inode_blocks) {
>     ret = -EIO;
>     goto bail;
> }
>
> I was using the linus tree 
> git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux-2.6.git
> and we don't have that check, but I can find this in the 
> git://git.kernel.org/pub/scm/linux/kernel/git/jlbec/ocfs2.git, introduced
by
> commit 564f8a3228879d6962edb3432d01bcd7499a67ec
>
> and now with this check I got what you mean, you are right, but I wonder
why
> the linus tree doesn't have this check? and are we suppose to do with
this?
> IMHO we can just push this commit to linus tree.
commit 5fe878ae7f82fbf0830dbfaee4c5ca18f3aee442
Author: Christoph Hellwig 
Date:   Tue Dec 15 16:47:50 2009 -0800

    direct-io: cleanup blockdev_direct_IO locking

This check was removed recently by the above patch.

On Thu, Apr 08, 2010 at 03:47:24PM +0800, Li Dongyang
wrote:
> 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
	We need to evaluate what __g_f_a_w() is doing and make sure
we're matching it appropriately for ocfs2.
> +	ret = file_remove_suid(file);
> +	if (ret)
> +		goto out_dio;
	NAK.  We do suid checks in ocfs2_prepare_inode_for_write().
Calling file_remove_suid() is outside of ocfs2's locking.  It calls
->setattr() which has its own rules in ocfs2.
> +	file_update_time(file);
	We have special behaviors regarding time updates for ocfs2
direct I/O.  This might want to live right next to the call to
generic_file_buffered_write().  But maybe not.  It needs to be checked.

Joel

-- 

To spot the expert, pick the one who predicts the job will take the
longest and cost the most.

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