diff mbox

[v4,09/10] nfsd: clean up nfs4_set_delegation

Message ID 1405696416-32585-10-git-send-email-jlayton@primarydata.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jeff Layton July 18, 2014, 3:13 p.m. UTC 
Move the alloc_init_deleg call into nfs4_set_delegation and change the
function to return a pointer to the delegation or an IS_ERR return. This
allows us to skip allocating a delegation if the file has already
experienced a lease conflict.

Signed-off-by: Jeff Layton <jlayton@primarydata.com>
---
 fs/nfsd/nfs4state.c | 34 ++++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)

Comments

Christoph Hellwig July 18, 2014, 5:19 p.m. UTC | #1 
> +nfs4_set_delegation(struct nfs4_client *clp, struct svc_fh *fh,
> +		    struct nfs4_file *fp)
>  {
> -	int status = 0;
> +	int status;
> +	struct nfs4_delegation *dp;
>  
>  	if (fp->fi_had_conflict)
> -		return -EAGAIN;
> +		return ERR_PTR(-EAGAIN);
> +
> +	dp = alloc_init_deleg(clp, fh);
> +	if (!dp)
> +		return ERR_PTR(-ENOMEM);
> +
>  	get_nfs4_file(fp);

Seems like we should pass the file pointer to alloc_init_deleg as well
so it can set that one one up and grab the reference without any lock?

Otherwise looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>
--
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  http://vger.kernel.org/majordomo-info.html
Jeff Layton July 18, 2014, 5:23 p.m. UTC | #2 
On Fri, 18 Jul 2014 10:19:30 -0700
Christoph Hellwig <hch@infradead.org> wrote:

> > +nfs4_set_delegation(struct nfs4_client *clp, struct svc_fh *fh,
> > +		    struct nfs4_file *fp)
> >  {
> > -	int status = 0;
> > +	int status;
> > +	struct nfs4_delegation *dp;
> >  
> >  	if (fp->fi_had_conflict)
> > -		return -EAGAIN;
> > +		return ERR_PTR(-EAGAIN);
> > +
> > +	dp = alloc_init_deleg(clp, fh);
> > +	if (!dp)
> > +		return ERR_PTR(-ENOMEM);
> > +
> >  	get_nfs4_file(fp);
> 
> Seems like we should pass the file pointer to alloc_init_deleg as well
> so it can set that one one up and grab the reference without any lock?
> 
> Otherwise looks good,
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>

I actually started to do a patch that did that, but the error handling
turned out to be a little twisted. Might be reasonable for a future
cleanup though.
diff mbox

Patch

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 1834d9bd5935..a2c6c85adfc7 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3553,12 +3553,20 @@  out_fput:
 	return status;
 }
 
-static int nfs4_set_delegation(struct nfs4_delegation *dp, struct nfs4_file *fp)
+static struct nfs4_delegation *
+nfs4_set_delegation(struct nfs4_client *clp, struct svc_fh *fh,
+		    struct nfs4_file *fp)
 {
-	int status = 0;
+	int status;
+	struct nfs4_delegation *dp;
 
 	if (fp->fi_had_conflict)
-		return -EAGAIN;
+		return ERR_PTR(-EAGAIN);
+
+	dp = alloc_init_deleg(clp, fh);
+	if (!dp)
+		return ERR_PTR(-ENOMEM);
+
 	get_nfs4_file(fp);
 	spin_lock(&state_lock);
 	spin_lock(&fp->fi_lock);
@@ -3566,7 +3574,8 @@  static int nfs4_set_delegation(struct nfs4_delegation *dp, struct nfs4_file *fp)
 	if (!fp->fi_lease) {
 		spin_unlock(&fp->fi_lock);
 		spin_unlock(&state_lock);
-		return nfs4_setlease(dp);
+		status = nfs4_setlease(dp);
+		goto out;
 	}
 	atomic_inc(&fp->fi_delegees);
 	if (fp->fi_had_conflict) {
@@ -3574,10 +3583,16 @@  static int nfs4_set_delegation(struct nfs4_delegation *dp, struct nfs4_file *fp)
 		goto out_unlock;
 	}
 	hash_delegation_locked(dp, fp);
+	status = 0;
 out_unlock:
 	spin_unlock(&fp->fi_lock);
 	spin_unlock(&state_lock);
-	return status;
+out:
+	if (status) {
+		nfs4_put_delegation(dp);
+		return ERR_PTR(status);
+	}
+	return dp;
 }
 
 static void nfsd4_open_deleg_none_ext(struct nfsd4_open *open, int status)
@@ -3651,12 +3666,9 @@  nfs4_open_delegation(struct svc_fh *fh, struct nfsd4_open *open,
 		default:
 			goto out_no_deleg;
 	}
-	dp = alloc_init_deleg(clp, fh);
-	if (dp == NULL)
+	dp = nfs4_set_delegation(clp, fh, stp->st_file);
+	if (IS_ERR(dp))
 		goto out_no_deleg;
-	status = nfs4_set_delegation(dp, stp->st_file);
-	if (status)
-		goto out_free;
 
 	memcpy(&open->op_delegate_stateid, &dp->dl_stid.sc_stateid, sizeof(dp->dl_stid.sc_stateid));
 
@@ -3664,8 +3676,6 @@  nfs4_open_delegation(struct svc_fh *fh, struct nfsd4_open *open,
 		STATEID_VAL(&dp->dl_stid.sc_stateid));
 	open->op_delegate_type = NFS4_OPEN_DELEGATE_READ;
 	return;
-out_free:
-	nfs4_put_delegation(dp);
 out_no_deleg:
 	open->op_delegate_type = NFS4_OPEN_DELEGATE_NONE;
 	if (open->op_claim_type == NFS4_OPEN_CLAIM_PREVIOUS &&