[PATCH] NFS: Fix RCU warnings in nfs_inode_return_delegation_noreclaim()

[PATCH] NFS: Fix RCU warnings in nfs_inode_return_delegation_noreclaim()

[David Howells]

Fix a number of RCU warnings in nfs_inode_return_delegation_noreclaim().  They
look like simple cases of missing rcu_read_lock/unlock() calls.

===================================================
[ INFO: suspicious rcu_dereference_check() usage. ]
---------------------------------------------------
fs/nfs/delegation.c:332 invoked rcu_dereference_check() without protection!

other info that might help us debug this:


rcu_scheduler_active = 1, debug_locks = 0
2 locks held by mount.nfs4/2281:
 #0:  (&type->s_umount_key#34){+.+...}, at: [<ffffffff810b25b4>] deactivate_super+0x60/0x80
 #1:  (iprune_sem){+.+...}, at: [<ffffffff810c332a>] invalidate_inodes+0x39/0x13a

stack backtrace:
Pid: 2281, comm: mount.nfs4 Not tainted 2.6.34-rc1-cachefs #110
Call Trace:
 [<ffffffff8105149f>] lockdep_rcu_dereference+0xaa/0xb2
 [<ffffffffa00b4591>] nfs_inode_return_delegation_noreclaim+0x5b/0xa0 [nfs]
 [<ffffffffa0095d63>] nfs4_clear_inode+0x11/0x1e [nfs]
 [<ffffffff810c2d92>] clear_inode+0x9e/0xf8
 [<ffffffff810c3028>] dispose_list+0x67/0x10e
 [<ffffffff810c340d>] invalidate_inodes+0x11c/0x13a
 [<ffffffff810b1dc1>] generic_shutdown_super+0x42/0xf4
 [<ffffffff810b1ebe>] kill_anon_super+0x11/0x4f
 [<ffffffffa009893c>] nfs4_kill_super+0x3f/0x72 [nfs]
 [<ffffffff810b25bc>] deactivate_super+0x68/0x80
 [<ffffffff810c6744>] mntput_no_expire+0xbb/0xf8
 [<ffffffff810c681b>] release_mounts+0x9a/0xb0
 [<ffffffff810c689b>] put_mnt_ns+0x6a/0x79
 [<ffffffffa00983a1>] nfs_follow_remote_path+0x5a/0x146 [nfs]
 [<ffffffffa0098334>] ? nfs_do_root_mount+0x82/0x95 [nfs]
 [<ffffffffa00985a9>] nfs4_try_mount+0x75/0xaf [nfs]
 [<ffffffffa0098874>] nfs4_get_sb+0x291/0x31a [nfs]
 [<ffffffff810b2059>] vfs_kern_mount+0xb8/0x177
 [<ffffffff810b2176>] do_kern_mount+0x48/0xe8
 [<ffffffff810c810b>] do_mount+0x782/0x7f9
 [<ffffffff810c8205>] sys_mount+0x83/0xbe
 [<ffffffff81001eeb>] system_call_fastpath+0x16/0x1b

Also on:

fs/nfs/delegation.c:215 invoked rcu_dereference_check() without protection!
 [<ffffffff8105149f>] lockdep_rcu_dereference+0xaa/0xb2
 [<ffffffffa00b4223>] nfs_inode_set_delegation+0xfe/0x219 [nfs]
 [<ffffffffa00a9c6f>] nfs4_opendata_to_nfs4_state+0x2c2/0x30d [nfs]
 [<ffffffffa00aa15d>] nfs4_do_open+0x2a6/0x3a6 [nfs]
 ...

And:

fs/nfs/delegation.c:40 invoked rcu_dereference_check() without protection!
 [<ffffffff8105149f>] lockdep_rcu_dereference+0xaa/0xb2
 [<ffffffffa00b3bef>] nfs_free_delegation+0x3d/0x6e [nfs]
 [<ffffffffa00b3e71>] nfs_do_return_delegation+0x26/0x30 [nfs]
 [<ffffffffa00b406a>] __nfs_inode_return_delegation+0x1ef/0x1fe [nfs]
 [<ffffffffa00b448a>] nfs_client_return_marked_delegations+0xc9/0x124 [nfs]
 ...

Signed-off-by: David Howells <dhowells@redhat.com>
---

 fs/nfs/delegation.c |   15 ++++++++++++---
 1 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index 2563beb..a77c735 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -37,8 +37,10 @@ static void nfs_free_delegation(struct nfs_delegation *delegation)
 {
 	struct rpc_cred *cred;
 
+	rcu_read_lock();
 	cred = rcu_dereference(delegation->cred);
 	rcu_assign_pointer(delegation->cred, NULL);
+	rcu_read_unlock();
 	call_rcu(&delegation->rcu, nfs_free_delegation_callback);
 	if (cred)
 		put_rpccred(cred);
@@ -212,10 +214,12 @@ int nfs_inode_set_delegation(struct inode *inode, struct rpc_cred *cred, struct
 	spin_lock_init(&delegation->lock);
 
 	spin_lock(&clp->cl_lock);
+	rcu_read_lock();
 	if (rcu_dereference(nfsi->delegation) != NULL) {
 		if (memcmp(&delegation->stateid, &nfsi->delegation->stateid,
 					sizeof(delegation->stateid)) == 0 &&
 				delegation->type == nfsi->delegation->type) {
+			rcu_read_unlock();
 			goto out;
 		}
 		/*
@@ -228,6 +232,7 @@ int nfs_inode_set_delegation(struct inode *inode, struct rpc_cred *cred, struct
 		if (delegation->type <= nfsi->delegation->type) {
 			freeme = delegation;
 			delegation = NULL;
+			rcu_read_lock();
 			goto out;
 		}
 		freeme = nfs_detach_delegation_locked(nfsi, NULL);
@@ -236,6 +241,7 @@ int nfs_inode_set_delegation(struct inode *inode, struct rpc_cred *cred, struct
 	nfsi->delegation_state = delegation->type;
 	rcu_assign_pointer(nfsi->delegation, delegation);
 	delegation = NULL;
+	rcu_read_unlock();
 
 	/* Ensure we revalidate the attributes and page cache! */
 	spin_lock(&inode->i_lock);
@@ -327,15 +333,18 @@ void nfs_inode_return_delegation_noreclaim(struct inode *inode)
 {
 	struct nfs_client *clp = NFS_SERVER(inode)->nfs_client;
 	struct nfs_inode *nfsi = NFS_I(inode);
-	struct nfs_delegation *delegation;
+	struct nfs_delegation *delegation = NULL;
 
+	rcu_read_lock();
 	if (rcu_dereference(nfsi->delegation) != NULL) {
 		spin_lock(&clp->cl_lock);
 		delegation = nfs_detach_delegation_locked(nfsi, NULL);
 		spin_unlock(&clp->cl_lock);
-		if (delegation != NULL)
-			nfs_do_return_delegation(inode, delegation, 0);
 	}
+	rcu_read_unlock();
+
+	if (delegation)
+		nfs_do_return_delegation(inode, delegation, 0);
 }
 
 int nfs_inode_return_delegation(struct inode *inode)

Re: [PATCH] NFS: Fix RCU warnings in nfs_inode_return_delegation_noreclaim()

[Trond Myklebust]

On Tue, 2010-03-16 at 11:51 +0000, David Howells wrote: 
> Fix a number of RCU warnings in nfs_inode_return_delegation_noreclaim().  They
> look like simple cases of missing rcu_read_lock/unlock() calls.
> 
> ===================================================
> [ INFO: suspicious rcu_dereference_check() usage. ]
> ---------------------------------------------------
> fs/nfs/delegation.c:332 invoked rcu_dereference_check() without protection!
> 
> other info that might help us debug this:
> 
> 
> rcu_scheduler_active = 1, debug_locks = 0
> 2 locks held by mount.nfs4/2281:
>  #0:  (&type->s_umount_key#34){+.+...}, at: [<ffffffff810b25b4>] deactivate_super+0x60/0x80
>  #1:  (iprune_sem){+.+...}, at: [<ffffffff810c332a>] invalidate_inodes+0x39/0x13a
> 
> stack backtrace:
> Pid: 2281, comm: mount.nfs4 Not tainted 2.6.34-rc1-cachefs #110
> Call Trace:
>  [<ffffffff8105149f>] lockdep_rcu_dereference+0xaa/0xb2
>  [<ffffffffa00b4591>] nfs_inode_return_delegation_noreclaim+0x5b/0xa0 [nfs]
>  [<ffffffffa0095d63>] nfs4_clear_inode+0x11/0x1e [nfs]
>  [<ffffffff810c2d92>] clear_inode+0x9e/0xf8
>  [<ffffffff810c3028>] dispose_list+0x67/0x10e
>  [<ffffffff810c340d>] invalidate_inodes+0x11c/0x13a
>  [<ffffffff810b1dc1>] generic_shutdown_super+0x42/0xf4
>  [<ffffffff810b1ebe>] kill_anon_super+0x11/0x4f
>  [<ffffffffa009893c>] nfs4_kill_super+0x3f/0x72 [nfs]
>  [<ffffffff810b25bc>] deactivate_super+0x68/0x80
>  [<ffffffff810c6744>] mntput_no_expire+0xbb/0xf8
>  [<ffffffff810c681b>] release_mounts+0x9a/0xb0
>  [<ffffffff810c689b>] put_mnt_ns+0x6a/0x79
>  [<ffffffffa00983a1>] nfs_follow_remote_path+0x5a/0x146 [nfs]
>  [<ffffffffa0098334>] ? nfs_do_root_mount+0x82/0x95 [nfs]
>  [<ffffffffa00985a9>] nfs4_try_mount+0x75/0xaf [nfs]
>  [<ffffffffa0098874>] nfs4_get_sb+0x291/0x31a [nfs]
>  [<ffffffff810b2059>] vfs_kern_mount+0xb8/0x177
>  [<ffffffff810b2176>] do_kern_mount+0x48/0xe8
>  [<ffffffff810c810b>] do_mount+0x782/0x7f9
>  [<ffffffff810c8205>] sys_mount+0x83/0xbe
>  [<ffffffff81001eeb>] system_call_fastpath+0x16/0x1b
> 
> Also on:
> 
> fs/nfs/delegation.c:215 invoked rcu_dereference_check() without protection!
>  [<ffffffff8105149f>] lockdep_rcu_dereference+0xaa/0xb2
>  [<ffffffffa00b4223>] nfs_inode_set_delegation+0xfe/0x219 [nfs]
>  [<ffffffffa00a9c6f>] nfs4_opendata_to_nfs4_state+0x2c2/0x30d [nfs]
>  [<ffffffffa00aa15d>] nfs4_do_open+0x2a6/0x3a6 [nfs]
>  ...
> 
> And:
> 
> fs/nfs/delegation.c:40 invoked rcu_dereference_check() without protection!
>  [<ffffffff8105149f>] lockdep_rcu_dereference+0xaa/0xb2
>  [<ffffffffa00b3bef>] nfs_free_delegation+0x3d/0x6e [nfs]
>  [<ffffffffa00b3e71>] nfs_do_return_delegation+0x26/0x30 [nfs]
>  [<ffffffffa00b406a>] __nfs_inode_return_delegation+0x1ef/0x1fe [nfs]
>  [<ffffffffa00b448a>] nfs_client_return_marked_delegations+0xc9/0x124 [nfs]
>  ...
> 
> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
> 
>  fs/nfs/delegation.c |   15 ++++++++++++---
>  1 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
> index 2563beb..a77c735 100644
> --- a/fs/nfs/delegation.c
> +++ b/fs/nfs/delegation.c
> @@ -37,8 +37,10 @@ static void nfs_free_delegation(struct nfs_delegation *delegation)
>  {
>  	struct rpc_cred *cred;
>  
> +	rcu_read_lock();
>  	cred = rcu_dereference(delegation->cred);
>  	rcu_assign_pointer(delegation->cred, NULL);
> +	rcu_read_unlock();
>  	call_rcu(&delegation->rcu, nfs_free_delegation_callback);
>  	if (cred)
>  		put_rpccred(cred);

That's bogus. We're in the process of freeing the delegation, so we
don't need to rely on rcu to read delegation->cred.

Better to just convert that rcu_dereference() into an ordinary pointer
dereference.

> @@ -212,10 +214,12 @@ int nfs_inode_set_delegation(struct inode *inode, struct rpc_cred *cred, struct
>  	spin_lock_init(&delegation->lock);
>  
>  	spin_lock(&clp->cl_lock);
> +	rcu_read_lock();
>  	if (rcu_dereference(nfsi->delegation) != NULL) {
>  		if (memcmp(&delegation->stateid, &nfsi->delegation->stateid,
>  					sizeof(delegation->stateid)) == 0 &&
>  				delegation->type == nfsi->delegation->type) {
> +			rcu_read_unlock();
>  			goto out;
>  		}
>  		/*

The spinlock already provides protection. Again we can just convert the
rcu_dereference() into a pointer dereference.

> @@ -228,6 +232,7 @@ int nfs_inode_set_delegation(struct inode *inode, struct rpc_cred *cred, struct
>  		if (delegation->type <= nfsi->delegation->type) {
>  			freeme = delegation;
>  			delegation = NULL;
> +			rcu_read_lock();
>  			goto out;
>  		}
>  		freeme = nfs_detach_delegation_locked(nfsi, NULL);
> @@ -236,6 +241,7 @@ int nfs_inode_set_delegation(struct inode *inode, struct rpc_cred *cred, struct
>  	nfsi->delegation_state = delegation->type;
>  	rcu_assign_pointer(nfsi->delegation, delegation);
>  	delegation = NULL;
> +	rcu_read_unlock();
>  
>  	/* Ensure we revalidate the attributes and page cache! */
>  	spin_lock(&inode->i_lock);
> @@ -327,15 +333,18 @@ void nfs_inode_return_delegation_noreclaim(struct inode *inode)
>  {
>  	struct nfs_client *clp = NFS_SERVER(inode)->nfs_client;
>  	struct nfs_inode *nfsi = NFS_I(inode);
> -	struct nfs_delegation *delegation;
> +	struct nfs_delegation *delegation = NULL;
>  
> +	rcu_read_lock();
>  	if (rcu_dereference(nfsi->delegation) != NULL) {
>  		spin_lock(&clp->cl_lock);
>  		delegation = nfs_detach_delegation_locked(nfsi, NULL);
>  		spin_unlock(&clp->cl_lock);
> -		if (delegation != NULL)
> -			nfs_do_return_delegation(inode, delegation, 0);
>  	}
> +	rcu_read_unlock();

We cannot hold the rcu read lock across the entire RPC call in
nfs_do_return_delegation(). All we want to do above is to check that
nfsi->delegation != NULL.

> +
> +	if (delegation)
> +		nfs_do_return_delegation(inode, delegation, 0);
>  }
>  
>  int nfs_inode_return_delegation(struct inode *inode)
> 

Re: [PATCH] NFS: Fix RCU warnings in nfs_inode_return_delegation_noreclaim()

[David Howells]

Trond Myklebust <Trond.Myklebust@netapp.com> wrote:

> > +	rcu_read_lock();
> >  	cred = rcu_dereference(delegation->cred);
> >  	rcu_assign_pointer(delegation->cred, NULL);
> > +	rcu_read_unlock();
> >  	call_rcu(&delegation->rcu, nfs_free_delegation_callback);
> >  	if (cred)
> >  		put_rpccred(cred);
> 
> That's bogus. We're in the process of freeing the delegation, so we
> don't need to rely on rcu to read delegation->cred.
> 
> Better to just convert that rcu_dereference() into an ordinary pointer
> dereference.

Now that I take a second look at this code, it looks wrong.  You shouldn't
really start dissassembling the delegation record until the RCU callback
triggers as it may be in use by someone up to that point.  If you know it
isn't in use at this point, why use call_rcu()?

> The spinlock already provides protection. Again we can just convert the
> rcu_dereference() into a pointer dereference.

That seems reasonable.

> We cannot hold the rcu read lock across the entire RPC call in
> nfs_do_return_delegation(). All we want to do above is to check that
> nfsi->delegation != NULL.

Good point.

However, that leads me to note that nfs_detach_delegation_locked() then must
be using rcu_dereference() unnecessarily.  Also nfs_do_return_delegation()
must be too.

I think if all you're doing is checking the state of the pointer, you don't
need the interpolated memory barrier, since you've no need to synchronise what
the pointer points to.

David

Re: [PATCH] NFS: Fix RCU warnings in nfs_inode_return_delegation_noreclaim()

[Paul E. McKenney]

On Tue, Mar 16, 2010 at 11:51:30AM +0000, David Howells wrote:
> Fix a number of RCU warnings in nfs_inode_return_delegation_noreclaim().  They
> look like simple cases of missing rcu_read_lock/unlock() calls.
> 
> ===================================================
> [ INFO: suspicious rcu_dereference_check() usage. ]
> ---------------------------------------------------
> fs/nfs/delegation.c:332 invoked rcu_dereference_check() without protection!
> 
> other info that might help us debug this:

Some thoughts on accounting for the update-side locks below.

							Thanx, Paul

> rcu_scheduler_active = 1, debug_locks = 0
> 2 locks held by mount.nfs4/2281:
>  #0:  (&type->s_umount_key#34){+.+...}, at: [<ffffffff810b25b4>] deactivate_super+0x60/0x80
>  #1:  (iprune_sem){+.+...}, at: [<ffffffff810c332a>] invalidate_inodes+0x39/0x13a
> 
> stack backtrace:
> Pid: 2281, comm: mount.nfs4 Not tainted 2.6.34-rc1-cachefs #110
> Call Trace:
>  [<ffffffff8105149f>] lockdep_rcu_dereference+0xaa/0xb2
>  [<ffffffffa00b4591>] nfs_inode_return_delegation_noreclaim+0x5b/0xa0 [nfs]
>  [<ffffffffa0095d63>] nfs4_clear_inode+0x11/0x1e [nfs]
>  [<ffffffff810c2d92>] clear_inode+0x9e/0xf8
>  [<ffffffff810c3028>] dispose_list+0x67/0x10e
>  [<ffffffff810c340d>] invalidate_inodes+0x11c/0x13a
>  [<ffffffff810b1dc1>] generic_shutdown_super+0x42/0xf4
>  [<ffffffff810b1ebe>] kill_anon_super+0x11/0x4f
>  [<ffffffffa009893c>] nfs4_kill_super+0x3f/0x72 [nfs]
>  [<ffffffff810b25bc>] deactivate_super+0x68/0x80
>  [<ffffffff810c6744>] mntput_no_expire+0xbb/0xf8
>  [<ffffffff810c681b>] release_mounts+0x9a/0xb0
>  [<ffffffff810c689b>] put_mnt_ns+0x6a/0x79
>  [<ffffffffa00983a1>] nfs_follow_remote_path+0x5a/0x146 [nfs]
>  [<ffffffffa0098334>] ? nfs_do_root_mount+0x82/0x95 [nfs]
>  [<ffffffffa00985a9>] nfs4_try_mount+0x75/0xaf [nfs]
>  [<ffffffffa0098874>] nfs4_get_sb+0x291/0x31a [nfs]
>  [<ffffffff810b2059>] vfs_kern_mount+0xb8/0x177
>  [<ffffffff810b2176>] do_kern_mount+0x48/0xe8
>  [<ffffffff810c810b>] do_mount+0x782/0x7f9
>  [<ffffffff810c8205>] sys_mount+0x83/0xbe
>  [<ffffffff81001eeb>] system_call_fastpath+0x16/0x1b
> 
> Also on:
> 
> fs/nfs/delegation.c:215 invoked rcu_dereference_check() without protection!
>  [<ffffffff8105149f>] lockdep_rcu_dereference+0xaa/0xb2
>  [<ffffffffa00b4223>] nfs_inode_set_delegation+0xfe/0x219 [nfs]
>  [<ffffffffa00a9c6f>] nfs4_opendata_to_nfs4_state+0x2c2/0x30d [nfs]
>  [<ffffffffa00aa15d>] nfs4_do_open+0x2a6/0x3a6 [nfs]
>  ...
> 
> And:
> 
> fs/nfs/delegation.c:40 invoked rcu_dereference_check() without protection!
>  [<ffffffff8105149f>] lockdep_rcu_dereference+0xaa/0xb2
>  [<ffffffffa00b3bef>] nfs_free_delegation+0x3d/0x6e [nfs]
>  [<ffffffffa00b3e71>] nfs_do_return_delegation+0x26/0x30 [nfs]
>  [<ffffffffa00b406a>] __nfs_inode_return_delegation+0x1ef/0x1fe [nfs]
>  [<ffffffffa00b448a>] nfs_client_return_marked_delegations+0xc9/0x124 [nfs]
>  ...
> 
> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
> 
>  fs/nfs/delegation.c |   15 ++++++++++++---
>  1 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
> index 2563beb..a77c735 100644
> --- a/fs/nfs/delegation.c
> +++ b/fs/nfs/delegation.c
> @@ -37,8 +37,10 @@ static void nfs_free_delegation(struct nfs_delegation *delegation)
>  {
>  	struct rpc_cred *cred;
> 
> +	rcu_read_lock();
>  	cred = rcu_dereference(delegation->cred);
>  	rcu_assign_pointer(delegation->cred, NULL);

The lock is probably held here, in which case something like the
following would work well without needing the artificial rcu_read_lock()
and rcu_read_unlock():

	cred = rcu_dereference_check(delegation->cred,
				     lockdep_is_held(&delegation->lock));

> +	rcu_read_unlock();
>  	call_rcu(&delegation->rcu, nfs_free_delegation_callback);
>  	if (cred)
>  		put_rpccred(cred);
> @@ -212,10 +214,12 @@ int nfs_inode_set_delegation(struct inode *inode, struct rpc_cred *cred, struct
>  	spin_lock_init(&delegation->lock);
> 
>  	spin_lock(&clp->cl_lock);
> +	rcu_read_lock();
>  	if (rcu_dereference(nfsi->delegation) != NULL) {

Same here, though I am not sure whether clp->cl_lock or something in
nfs_inode should be used.

>  		if (memcmp(&delegation->stateid, &nfsi->delegation->stateid,
>  					sizeof(delegation->stateid)) == 0 &&
>  				delegation->type == nfsi->delegation->type) {
> +			rcu_read_unlock();
>  			goto out;
>  		}
>  		/*
> @@ -228,6 +232,7 @@ int nfs_inode_set_delegation(struct inode *inode, struct rpc_cred *cred, struct
>  		if (delegation->type <= nfsi->delegation->type) {
>  			freeme = delegation;
>  			delegation = NULL;
> +			rcu_read_lock();
>  			goto out;
>  		}
>  		freeme = nfs_detach_delegation_locked(nfsi, NULL);
> @@ -236,6 +241,7 @@ int nfs_inode_set_delegation(struct inode *inode, struct rpc_cred *cred, struct
>  	nfsi->delegation_state = delegation->type;
>  	rcu_assign_pointer(nfsi->delegation, delegation);
>  	delegation = NULL;
> +	rcu_read_unlock();
> 
>  	/* Ensure we revalidate the attributes and page cache! */
>  	spin_lock(&inode->i_lock);
> @@ -327,15 +333,18 @@ void nfs_inode_return_delegation_noreclaim(struct inode *inode)
>  {
>  	struct nfs_client *clp = NFS_SERVER(inode)->nfs_client;
>  	struct nfs_inode *nfsi = NFS_I(inode);
> -	struct nfs_delegation *delegation;
> +	struct nfs_delegation *delegation = NULL;
> 
> +	rcu_read_lock();
>  	if (rcu_dereference(nfsi->delegation) != NULL) {

Same here.

>  		spin_lock(&clp->cl_lock);
>  		delegation = nfs_detach_delegation_locked(nfsi, NULL);
>  		spin_unlock(&clp->cl_lock);
> -		if (delegation != NULL)
> -			nfs_do_return_delegation(inode, delegation, 0);
>  	}
> +	rcu_read_unlock();
> +
> +	if (delegation)
> +		nfs_do_return_delegation(inode, delegation, 0);
>  }
> 
>  int nfs_inode_return_delegation(struct inode *inode)
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] NFS: Fix RCU warnings in nfs_inode_return_delegation_noreclaim()

[Trond Myklebust]

On Tue, 2010-03-16 at 10:17 -0700, Paul E. McKenney wrote: 
> On Tue, Mar 16, 2010 at 11:51:30AM +0000, David Howells wrote:
> > diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
> > index 2563beb..a77c735 100644
> > --- a/fs/nfs/delegation.c
> > +++ b/fs/nfs/delegation.c
> > @@ -37,8 +37,10 @@ static void nfs_free_delegation(struct nfs_delegation *delegation)
> >  {
> >  	struct rpc_cred *cred;
> > 
> > +	rcu_read_lock();
> >  	cred = rcu_dereference(delegation->cred);
> >  	rcu_assign_pointer(delegation->cred, NULL);
> 
> The lock is probably held here, in which case something like the
> following would work well without needing the artificial rcu_read_lock()
> and rcu_read_unlock():

No. The lock is not held here. At this point, the delegation has been
detached from the inode that pointed to it, and so we can free up its
contents.

We still need the call_rcu() to free up the allocated memory in order to
ensure that some process doing lockless traversal of the
clp->cl_delegations list doesn't crash.

> 	cred = rcu_dereference_check(delegation->cred,
> 				     lockdep_is_held(&delegation->lock));
> 
> > +	rcu_read_unlock();
> >  	call_rcu(&delegation->rcu, nfs_free_delegation_callback);
> >  	if (cred)
> >  		put_rpccred(cred);
> > @@ -212,10 +214,12 @@ int nfs_inode_set_delegation(struct inode *inode, struct rpc_cred *cred, struct
> >  	spin_lock_init(&delegation->lock);
> > 
> >  	spin_lock(&clp->cl_lock);
> > +	rcu_read_lock();
> >  	if (rcu_dereference(nfsi->delegation) != NULL) {
> 
> Same here, though I am not sure whether clp->cl_lock or something in
> nfs_inode should be used.

Yes. As I indicated to David in another email, the clp->cl_lock protects
us here, so it looks as if your suggestion above would be perfect.

> >  		if (memcmp(&delegation->stateid, &nfsi->delegation->stateid,
> >  					sizeof(delegation->stateid)) == 0 &&
> >  				delegation->type == nfsi->delegation->type) {
> > +			rcu_read_unlock();
> >  			goto out;
> >  		}
> >  		/*
> > @@ -228,6 +232,7 @@ int nfs_inode_set_delegation(struct inode *inode, struct rpc_cred *cred, struct
> >  		if (delegation->type <= nfsi->delegation->type) {
> >  			freeme = delegation;
> >  			delegation = NULL;
> > +			rcu_read_lock();
> >  			goto out;
> >  		}
> >  		freeme = nfs_detach_delegation_locked(nfsi, NULL);

Re: [PATCH] NFS: Fix RCU warnings in nfs_inode_return_delegation_noreclaim()

[David Howells]

Trond Myklebust <Trond.Myklebust@netapp.com> wrote:

> > The lock is probably held here, in which case something like the
> > following would work well without needing the artificial rcu_read_lock()
> > and rcu_read_unlock():
> 
> No. The lock is not held here. At this point, the delegation has been
> detached from the inode that pointed to it, and so we can free up its
> contents.
> 
> We still need the call_rcu() to free up the allocated memory in order to
> ensure that some process doing lockless traversal of the
> clp->cl_delegations list doesn't crash.

In that case, surely you can't detach the credentials pointer until the
callback is invoked?

David

Re: [PATCH] NFS: Fix RCU warnings in nfs_inode_return_delegation_noreclaim()

[Trond Myklebust]

On Tue, 2010-03-16 at 17:40 +0000, David Howells wrote: 
> Trond Myklebust <Trond.Myklebust@netapp.com> wrote:
> 
> > > The lock is probably held here, in which case something like the
> > > following would work well without needing the artificial rcu_read_lock()
> > > and rcu_read_unlock():
> > 
> > No. The lock is not held here. At this point, the delegation has been
> > detached from the inode that pointed to it, and so we can free up its
> > contents.
> > 
> > We still need the call_rcu() to free up the allocated memory in order to
> > ensure that some process doing lockless traversal of the
> > clp->cl_delegations list doesn't crash.
> 
> In that case, surely you can't detach the credentials pointer until the
> callback is invoked?

We can, because the process that is traversing the list has to lock the
delegation and test whether it is still attached or not before it can
dereference and use the contents.

In fact, that credential pointer is only really meant to be used when we
need to return the delegation. Once we get to the nfs_free_delegation()
bit, it is no longer needed.

Cheers
  Trond

Re: [PATCH] NFS: Fix RCU warnings in nfs_inode_return_delegation_noreclaim()

[Paul E. McKenney]

On Tue, Mar 16, 2010 at 01:35:54PM -0400, Trond Myklebust wrote:
> On Tue, 2010-03-16 at 10:17 -0700, Paul E. McKenney wrote: 
> > On Tue, Mar 16, 2010 at 11:51:30AM +0000, David Howells wrote:
> > > diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
> > > index 2563beb..a77c735 100644
> > > --- a/fs/nfs/delegation.c
> > > +++ b/fs/nfs/delegation.c
> > > @@ -37,8 +37,10 @@ static void nfs_free_delegation(struct nfs_delegation *delegation)
> > >  {
> > >  	struct rpc_cred *cred;
> > > 
> > > +	rcu_read_lock();
> > >  	cred = rcu_dereference(delegation->cred);
> > >  	rcu_assign_pointer(delegation->cred, NULL);
> > 
> > The lock is probably held here, in which case something like the
> > following would work well without needing the artificial rcu_read_lock()
> > and rcu_read_unlock():
> 
> No. The lock is not held here. At this point, the delegation has been
> detached from the inode that pointed to it, and so we can free up its
> contents.

OK.  Is there some reference counter or pointer that can be checked to
verify that this data structure really is in a state that prevents
RCU readers from finding it?

> We still need the call_rcu() to free up the allocated memory in order to
> ensure that some process doing lockless traversal of the
> clp->cl_delegations list doesn't crash.

OK.

> > 	cred = rcu_dereference_check(delegation->cred,
> > 				     lockdep_is_held(&delegation->lock));
> > 
> > > +	rcu_read_unlock();
> > >  	call_rcu(&delegation->rcu, nfs_free_delegation_callback);
> > >  	if (cred)
> > >  		put_rpccred(cred);
> > > @@ -212,10 +214,12 @@ int nfs_inode_set_delegation(struct inode *inode, struct rpc_cred *cred, struct
> > >  	spin_lock_init(&delegation->lock);
> > > 
> > >  	spin_lock(&clp->cl_lock);
> > > +	rcu_read_lock();
> > >  	if (rcu_dereference(nfsi->delegation) != NULL) {
> > 
> > Same here, though I am not sure whether clp->cl_lock or something in
> > nfs_inode should be used.
> 
> Yes. As I indicated to David in another email, the clp->cl_lock protects
> us here, so it looks as if your suggestion above would be perfect.

Sounds good!

							Thanx, Paul

> > >  		if (memcmp(&delegation->stateid, &nfsi->delegation->stateid,
> > >  					sizeof(delegation->stateid)) == 0 &&
> > >  				delegation->type == nfsi->delegation->type) {
> > > +			rcu_read_unlock();
> > >  			goto out;
> > >  		}
> > >  		/*
> > > @@ -228,6 +232,7 @@ int nfs_inode_set_delegation(struct inode *inode, struct rpc_cred *cred, struct
> > >  		if (delegation->type <= nfsi->delegation->type) {
> > >  			freeme = delegation;
> > >  			delegation = NULL;
> > > +			rcu_read_lock();
> > >  			goto out;
> > >  		}
> > >  		freeme = nfs_detach_delegation_locked(nfsi, NULL);
> 

Re: [PATCH] NFS: Fix RCU warnings in nfs_inode_return_delegation_noreclaim()

[Trond Myklebust]

On Tue, 2010-03-16 at 11:10 -0700, Paul E. McKenney wrote: 
> On Tue, Mar 16, 2010 at 01:35:54PM -0400, Trond Myklebust wrote:
> > On Tue, 2010-03-16 at 10:17 -0700, Paul E. McKenney wrote: 
> > > On Tue, Mar 16, 2010 at 11:51:30AM +0000, David Howells wrote:
> > > > diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
> > > > index 2563beb..a77c735 100644
> > > > --- a/fs/nfs/delegation.c
> > > > +++ b/fs/nfs/delegation.c
> > > > @@ -37,8 +37,10 @@ static void nfs_free_delegation(struct nfs_delegation *delegation)
> > > >  {
> > > >  	struct rpc_cred *cred;
> > > > 
> > > > +	rcu_read_lock();
> > > >  	cred = rcu_dereference(delegation->cred);
> > > >  	rcu_assign_pointer(delegation->cred, NULL);
> > > 
> > > The lock is probably held here, in which case something like the
> > > following would work well without needing the artificial rcu_read_lock()
> > > and rcu_read_unlock():
> > 
> > No. The lock is not held here. At this point, the delegation has been
> > detached from the inode that pointed to it, and so we can free up its
> > contents.
> 
> OK.  Is there some reference counter or pointer that can be checked to
> verify that this data structure really is in a state that prevents
> RCU readers from finding it?

Yes. The RCU readers are supposed to grab the delegation->lock and then
check the contents of the delegation->inode.

Cheers
  Trond

Re: [PATCH] NFS: Fix RCU warnings in nfs_inode_return_delegation_noreclaim()

[Paul E. McKenney]

On Tue, Mar 16, 2010 at 02:43:15PM -0400, Trond Myklebust wrote:
> On Tue, 2010-03-16 at 11:10 -0700, Paul E. McKenney wrote: 
> > On Tue, Mar 16, 2010 at 01:35:54PM -0400, Trond Myklebust wrote:
> > > On Tue, 2010-03-16 at 10:17 -0700, Paul E. McKenney wrote: 
> > > > On Tue, Mar 16, 2010 at 11:51:30AM +0000, David Howells wrote:
> > > > > diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
> > > > > index 2563beb..a77c735 100644
> > > > > --- a/fs/nfs/delegation.c
> > > > > +++ b/fs/nfs/delegation.c
> > > > > @@ -37,8 +37,10 @@ static void nfs_free_delegation(struct nfs_delegation *delegation)
> > > > >  {
> > > > >  	struct rpc_cred *cred;
> > > > > 
> > > > > +	rcu_read_lock();
> > > > >  	cred = rcu_dereference(delegation->cred);
> > > > >  	rcu_assign_pointer(delegation->cred, NULL);
> > > > 
> > > > The lock is probably held here, in which case something like the
> > > > following would work well without needing the artificial rcu_read_lock()
> > > > and rcu_read_unlock():
> > > 
> > > No. The lock is not held here. At this point, the delegation has been
> > > detached from the inode that pointed to it, and so we can free up its
> > > contents.
> > 
> > OK.  Is there some reference counter or pointer that can be checked to
> > verify that this data structure really is in a state that prevents
> > RCU readers from finding it?
> 
> Yes. The RCU readers are supposed to grab the delegation->lock and then
> check the contents of the delegation->inode.

So would something like the following work, then?

	cred = rcu_dereference_check(delegation->cred,
				     delegation->inode == NULL);

Or would some other check condition be more appropriate?

							Thanx, Paul