[Ocfs2-devel] [PATCH 2/2] Ocfs2: Handle O_DIRECT writes with coherency option.
Tao Ma
tao.ma at oracle.com
Sun Oct 10 06:43:52 PDT 2010
于 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
More information about the Ocfs2-devel
mailing list