On Sun, Aug 3, 2014 at 7:03 PM, NeilBrown <neilb@xxxxxxx> wrote:
> On Mon, 04 Aug 2014 06:20:02 +0800 kbuild test robot <fengguang.wu@xxxxxxxxx>
> wrote:
>
>> tree: git://git.linux-nfs.org/projects/trondmy/linux-nfs.git testing
>> head: f682a398b2e24ae0a775ddf37cced83b897198ee
>> commit: d51ac1a8e9b86b2d17d349bb256869cab6522787 [56/61] NFS: prepare for RCU-walk support but pushing tests later in code.
>> reproduce: make C=1 CF=-D__CHECK_ENDIAN__
>>
>>
>> sparse warnings: (new ones prefixed by >>)
>>
>> >> fs/nfs/dir.c:1092:26: sparse: incompatible types in comparison expression (different address spaces)
>> >> fs/nfs/dir.c:1169:31: sparse: incompatible types in comparison expression (different address spaces)
>>
>> vim +1092 fs/nfs/dir.c
>>
>> 1086 struct nfs_fh *fhandle = NULL;
>> 1087 struct nfs_fattr *fattr = NULL;
>> 1088 struct nfs4_label *label = NULL;
>> 1089 int error;
>> 1090
>> 1091 if (flags & LOOKUP_RCU) {
>> > 1092 parent = rcu_dereference(dentry->d_parent);
>> 1093 dir = ACCESS_ONCE(parent->d_inode);
>> 1094 if (!dir)
>> 1095 return -ECHILD;
>
> Hmmm.. I suspect rcu_dereference doesn't really make sense here.
> After all, d_parent is not assigned with rcu_assign_ptr, and no-one else uses
> rcu_dereference for it.
>
> The issue is that, without locks, d_parent could change at any point.
> As dentries are freed with call_rcu it is safe to follow any pointers we find,
> but there is a limit how much we can trust them.
> It is very likely that any change to d_parent that mattered would increment
> some seqlock so that RCU-walk would eventually abort.
>
>
> So we may not need the
>
>> > 1169 if (parent != rcu_dereference(dentry->d_parent))
>> 1170 return -ECHILD;
>
> at the end, as a seqlock will probably catch any problem.
My main worry with that argument is whether or not the d_seq protected
lookups are guaranteed to always cover the parent. I can't see
anything in Documentation/filesystems/path-lookup.txt that indicates
that they must be.
> Without that we don't even need to store 'parent' at all, just
> dir = ACCESS_ONCE(dentry->d_parent->d_inode);
>
> If we keep it, which is probably safest, then using ACCESS_ONCE in place of
> the current rcu_dereference() make sense.
>
> parent = ACCESS_ONCE(dentry->d_parent);
> dir = ACCESS_ONCE(dir->d_inode);
>
> ...
>
> if (parent != ACCESS_ONCE(dentry->d_parent))
> return -ECHILD;
>
>
> Trond, would you like me to resend that patch, or do you want to just
> s/rcu_derefence/ACCESS_ONCE/
> ??
Could you send an incremental patch?
Thanks!
Trond
--
Trond Myklebust
Linux NFS client maintainer, PrimaryData
trond.myklebust@xxxxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
