[Ocfs2-devel] [PATCH 2/2] Ocfs2: Handle O_DIRECT writes with coherency option.

[Tao Ma <tao.ma@oracle.com>]

? 2010-10-10 18:59, Joel Becker wrote:
> On Sat, Oct 09, 2010 at 07:26:42PM +0800, Tristan Ye wrote:
>> -	/* concurrent O_DIRECT writes are allowed */
>> -	rw_level = !direct_io;
>> +	/*
>> +	 * concurrent O_DIRECT writes are allowed with
>> +	 * mount_option "coherency=buffered".
>> +	 */
>> +	if (direct_io) {
>> +		rw_level = !(osb->s_mount_opt&  OCFS2_MOUNT_COHERENCY_BUFFERED);
>> +	} else
>> +		rw_level = !direct_io;
>> +
> 	I think I'd like:
>
> 	if (direct_io&&  (osb->s_mount_opt&  OCFS2_MOUNT_COHERENCY_BUFFERED))
> 		rw_level = 0;
> 	else
> 		rw_level = 1;
>
> It actually matches your comment much better.  But since we're going to
> be using it again later, perhaps you should set 'int full_coherency =
> (osb->s_mount_opt&  OCFS2_MOUNT_COHERENCY_BUFFERED)' up at the top of
> the function and then do:
>
> 	rw_level = (!direct_io || full_coherency)
yeah, it looks more natural.
>>   	ret = ocfs2_rw_lock(inode, rw_level);
>>   	if (ret<  0) {
>>   		mlog_errno(ret);
>>   		goto out_sems;
>>   	}
>>
>> +	/*
>> +	 * O_DIRECT writes with "coherency=full" need to take EX cluster
>> +	 * inode_lock to guarantee coherency.
>> +	 */
>> +	if ((direct_io)&&
>> +	    !(osb->s_mount_opt&  OCFS2_MOUNT_COHERENCY_BUFFERED)) {
> 	Then this check can be:
>
> 	if (direct_io&&  full_coherency) {
> 		/*
> 		 * We need to take and drop the inode lock to force
> 		 * other nodes to drop their caches.  Buffered I/O
> 		 * already does this in write_begin().
> 		 */
>
>> +		ret = ocfs2_inode_lock(inode, NULL, 1);
>> +		if (ret<  0) {
>> +			mlog_errno(ret);
>> +			goto out_sems;
>> +		}
>> +		
>> +		/*
>> +		 * Safe to drop the inode_lock immediately since we're just
>> +		 * telling other nodes to flush their cache.
>> +		 */
> 	And you don't need this comment.
I also have another concern. Do we really need the exclusive lock? I 
think a PR lock should
let others to flush the cache for us.

Regards,
Tao