[PATCH] nfs: avoid possible deadlock bug when setting up delegation.

[PATCH] nfs: avoid possible deadlock bug when setting up delegation.

[Tao Guo]

Unlock clp->cl_lock before taking inode->i_lock when setting up
delegation to avoid possible deadlock bug.

Signed-off-by: Tao Guo <guotao-U4AKAne5IzAR5TUyvShJeg@public.gmane.org>
---
 fs/nfs/delegation.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/fs/nfs/delegation.c b/fs/nfs/delegation.c
index ea61d26..0983930 100644
--- a/fs/nfs/delegation.c
+++ b/fs/nfs/delegation.c
@@ -233,6 +233,7 @@ int nfs_inode_set_delegation(struct inode *inode, struct rpc_cred *cred, struct
 		if (memcmp(&delegation->stateid, &old_delegation->stateid,
 					sizeof(old_delegation->stateid)) == 0 &&
 				delegation->type == old_delegation->type) {
+			spin_unlock(&clp->cl_lock);
 			goto out;
 		}
 		/*
@@ -245,6 +246,7 @@ int nfs_inode_set_delegation(struct inode *inode, struct rpc_cred *cred, struct
 		if (delegation->type <= old_delegation->type) {
 			freeme = delegation;
 			delegation = NULL;
+			spin_unlock(&clp->cl_lock);
 			goto out;
 		}
 		freeme = nfs_detach_delegation_locked(nfsi, NULL, clp);
@@ -253,6 +255,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;
+	spin_unlock(&clp->cl_lock);
 
 	/* Ensure we revalidate the attributes and page cache! */
 	spin_lock(&inode->i_lock);
@@ -260,7 +263,6 @@ int nfs_inode_set_delegation(struct inode *inode, struct rpc_cred *cred, struct
 	spin_unlock(&inode->i_lock);
 
 out:
-	spin_unlock(&clp->cl_lock);
 	if (delegation != NULL)
 		nfs_free_delegation(delegation);
 	if (freeme != NULL)
-- 
1.6.3.3

Re: [PATCH] nfs: avoid possible deadlock bug when setting up delegation.

[Trond Myklebust]

On Thu, 2010-05-20 at 13:42 +0800, Tao Guo wrote: 
> Unlock clp->cl_lock before taking inode->i_lock when setting up
> delegation to avoid possible deadlock bug.

What deadlock? inode->i_lock should always be the innermost lock, so it
should always be safe to take it while already holding the nfs_client
lock.

If somebody is taking the nfs_client lock while holding the inode lock,
then that would be the deadlock bug that needs to be fixed.

Trond

Re: [PATCH] nfs: avoid possible deadlock bug when setting up delegation.

[Tao Guo]

On Thu, May 20, 2010 at 8:57 PM, Trond Myklebust
<trond.myklebust@fys.uio.no> wrote:
> On Thu, 2010-05-20 at 13:42 +0800, Tao Guo wrote:
>> Unlock clp->cl_lock before taking inode->i_lock when setting up
>> delegation to avoid possible deadlock bug.
>
> What deadlock? inode->i_lock should always be the innermost lock, so =
it
> should always be safe to take it while already holding the nfs_client
> lock.
here is: http://marc.info/?l=3Dlinux-nfs&m=3D127424502009958&w=3D2
>
> If somebody is taking the nfs_client lock while holding the inode loc=
k,
> then that would be the deadlock bug that needs to be fixed.
>
mm, I will reconsider that carefully~
> Trond
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" =
in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at =C2=A0http://vger.kernel.org/majordomo-info.ht=
ml
>



--=20
tao.

Re: [PATCH] nfs: avoid possible deadlock bug when setting up delegation.

[Trond Myklebust]

On Thu, 2010-05-20 at 21:20 +0800, Tao Guo wrote: 
> On Thu, May 20, 2010 at 8:57 PM, Trond Myklebust
> <trond.myklebust@fys.uio.no> wrote:
> > On Thu, 2010-05-20 at 13:42 +0800, Tao Guo wrote:
> >> Unlock clp->cl_lock before taking inode->i_lock when setting up
> >> delegation to avoid possible deadlock bug.
> >
> > What deadlock? inode->i_lock should always be the innermost lock, so it
> > should always be safe to take it while already holding the nfs_client
> > lock.
> here is: http://marc.info/?l=linux-nfs&m=127424502009958&w=2
> >

That would be a pnfs bug... Please fix it so that it adheres to the
locking rules in mainline.

Trond