[PATCH 0/2] nfsd: fi_delegees bugfix and cleanup

[PATCH 0/2] nfsd: fi_delegees bugfix and cleanup

[Jeff Layton]

Hi Bruce,

As I mentioned on IRC yesterday, here are a couple of patches to the
fi_delegees handling. The first one fixes a bug. I think it's pretty
unlikely to be hit in most cases, so I'm not sure if it's suitable for
stable. Your call there.

The second is just a cleanup patch -- fi_delegees is always handled
under spinlock so it really doesn't need to be an atomic_t.

Jeff Layton (2):
  nfsd: fix fi_delegees leak when fi_had_conflict returns true
  nfsd: fi_delegees doesn't need to be an atomic_t

 fs/nfsd/nfs4state.c | 8 ++++----
 fs/nfsd/state.h     | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

-- 
2.1.0

[PATCH 1/2] nfsd: fix fi_delegees leak when fi_had_conflict returns true

[Jeff Layton]

Currently, nfs4_set_delegation takes a reference to an existing
delegation and then checks to see if there is a conflict. If there is
one, then it doesn't release that reference.

Change the code to take the reference after the check and only if there
is no conflict.

Signed-off-by: Jeff Layton <jlayton@primarydata.com>
---
 fs/nfsd/nfs4state.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 5f62e8051e2b..86bc2882bffc 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3898,11 +3898,11 @@ nfs4_set_delegation(struct nfs4_client *clp, struct svc_fh *fh,
 		status = nfs4_setlease(dp);
 		goto out;
 	}
-	atomic_inc(&fp->fi_delegees);
 	if (fp->fi_had_conflict) {
 		status = -EAGAIN;
 		goto out_unlock;
 	}
+	atomic_inc(&fp->fi_delegees);
 	hash_delegation_locked(dp, fp);
 	status = 0;
 out_unlock:
-- 
2.1.0

[PATCH 2/2] nfsd: fi_delegees doesn't need to be an atomic_t

[Jeff Layton]

fi_delegees is always handled under the fi_lock, so there's no need to
use an atomic_t for this field.

Signed-off-by: Jeff Layton <jlayton@primarydata.com>
---
 fs/nfsd/nfs4state.c | 8 ++++----
 fs/nfsd/state.h     | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 86bc2882bffc..55e8afeff8db 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -688,7 +688,7 @@ static void nfs4_put_deleg_lease(struct nfs4_file *fp)
 	struct file *filp = NULL;
 
 	spin_lock(&fp->fi_lock);
-	if (fp->fi_deleg_file && atomic_dec_and_test(&fp->fi_delegees))
+	if (fp->fi_deleg_file && --fp->fi_delegees == 0)
 		swap(filp, fp->fi_deleg_file);
 	spin_unlock(&fp->fi_lock);
 
@@ -3856,12 +3856,12 @@ static int nfs4_setlease(struct nfs4_delegation *dp)
 	/* Race breaker */
 	if (fp->fi_deleg_file) {
 		status = 0;
-		atomic_inc(&fp->fi_delegees);
+		++fp->fi_delegees;
 		hash_delegation_locked(dp, fp);
 		goto out_unlock;
 	}
 	fp->fi_deleg_file = filp;
-	atomic_set(&fp->fi_delegees, 1);
+	fp->fi_delegees = 1;
 	hash_delegation_locked(dp, fp);
 	spin_unlock(&fp->fi_lock);
 	spin_unlock(&state_lock);
@@ -3902,7 +3902,7 @@ nfs4_set_delegation(struct nfs4_client *clp, struct svc_fh *fh,
 		status = -EAGAIN;
 		goto out_unlock;
 	}
-	atomic_inc(&fp->fi_delegees);
+	++fp->fi_delegees;
 	hash_delegation_locked(dp, fp);
 	status = 0;
 out_unlock:
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 28264a72fa60..afadb7c2e357 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -489,7 +489,7 @@ struct nfs4_file {
 	atomic_t		fi_access[2];
 	u32			fi_share_deny;
 	struct file		*fi_deleg_file;
-	atomic_t		fi_delegees;
+	int			fi_delegees;
 	struct knfsd_fh		fi_fhandle;
 	bool			fi_had_conflict;
 };
-- 
2.1.0

Re: [PATCH 0/2] nfsd: fi_delegees bugfix and cleanup

[J. Bruce Fields]

On Sat, Dec 13, 2014 at 09:11:38AM -0500, Jeff Layton wrote:
> As I mentioned on IRC yesterday, here are a couple of patches to the
> fi_delegees handling. The first one fixes a bug. I think it's pretty
> unlikely to be hit in most cases, so I'm not sure if it's suitable for
> stable. Your call there.
> 
> The second is just a cleanup patch -- fi_delegees is always handled
> under spinlock so it really doesn't need to be an atomic_t.

Sorry for the slow response, applying for 3.19 and stable (well, the
first anyway), may not get that passed along till -rc2.

--b.

> 
> Jeff Layton (2):
>   nfsd: fix fi_delegees leak when fi_had_conflict returns true
>   nfsd: fi_delegees doesn't need to be an atomic_t
> 
>  fs/nfsd/nfs4state.c | 8 ++++----
>  fs/nfsd/state.h     | 2 +-
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> -- 
> 2.1.0
>