Re: [nfs:testing 56/61] fs/nfs/dir.c:1092:26: sparse: incompatible types in comparison expression (different address spaces)

[NeilBrown <neilb@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 2279 bytes --]

On Mon, 04 Aug 2014 06:20:02 +0800 kbuild test robot <fengguang.wu@intel.com>
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.
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/
??

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]