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

[Tao Ma]

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. ;)

Regards,
Tao
> Br,
> Li Dongyang
>> Regards,
>> Tao
>>
>>> On Friday 09 April 2010 02:41:26 Sunil Mushran wrote:
>>>> 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:
>>> _______________________________________________
>>> Ocfs2-devel mailing list
>>> Ocfs2-devel at oss.oracle.com
>>> http://oss.oracle.com/mailman/listinfo/ocfs2-devel