[PATCH v2] nfsd: fix bug on nfs4 stateid deallocation

[PATCH v2] nfsd: fix bug on nfs4 stateid deallocation

[ycnian]

NFS4_OO_PURGE_CLOSE is not handled properly. To avoid memory leak, nfs4 
stateid which is pointed by oo_last_closed_stid is freed in nfsd4_close(), 
but NFS4_OO_PURGE_CLOSE isn't cleared meanwhile. So the stateid released in 
THIS close procedure may be freed immediately in the coming encoding function.
Sorry that Signed-off-by was forgotten in last version.

Signed-off-by: Yanchuan Nian <ycnian@gmail.com>
---
 fs/nfsd/nfs4state.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index cc41bf4..d972db8 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -3827,6 +3827,7 @@ nfsd4_close(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 
 	nfsd4_close_open_stateid(stp);
 	release_last_closed_stateid(oo);
+	oo->oo_flags &= ~NFS4_OO_PURGE_CLOSE;
 	oo->oo_last_closed_stid = stp;
 
 	if (list_empty(&oo->oo_owner.so_stateids)) {
-- 
1.7.4.4

Re: [PATCH v2] nfsd: fix bug on nfs4 stateid deallocation

[J. Bruce Fields]

On Mon, Mar 11, 2013 at 08:46:14AM +0800, ycnian@gmail.com wrote:
> NFS4_OO_PURGE_CLOSE is not handled properly. To avoid memory leak, nfs4 
> stateid which is pointed by oo_last_closed_stid is freed in nfsd4_close(), 
> but NFS4_OO_PURGE_CLOSE isn't cleared meanwhile. So the stateid released in 
> THIS close procedure may be freed immediately in the coming encoding function.

OK, makes sense.  This code is confusing, I wonder if there's some way
we could make it simpler.

> Sorry that Signed-off-by was forgotten in last version.

Do you have a test that reproduces this bug?

--b.

> 
> Signed-off-by: Yanchuan Nian <ycnian@gmail.com>
> ---
>  fs/nfsd/nfs4state.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index cc41bf4..d972db8 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -3827,6 +3827,7 @@ nfsd4_close(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  
>  	nfsd4_close_open_stateid(stp);
>  	release_last_closed_stateid(oo);
> +	oo->oo_flags &= ~NFS4_OO_PURGE_CLOSE;
>  	oo->oo_last_closed_stid = stp;
>  
>  	if (list_empty(&oo->oo_owner.so_stateids)) {
> -- 
> 1.7.4.4
> 

Re: [PATCH v2] nfsd: fix bug on nfs4 stateid deallocation

[J. Bruce Fields]

On Wed, Mar 13, 2013 at 11:04:54PM +0800, Yanchuan Nian wrote:
> 2013/3/11 J. Bruce Fields <bfields@fieldses.org>
> 
> > On Mon, Mar 11, 2013 at 08:46:14AM +0800, ycnian@gmail.com wrote:
> > > NFS4_OO_PURGE_CLOSE is not handled properly. To avoid memory leak, nfs4
> > > stateid which is pointed by oo_last_closed_stid is freed in
> > nfsd4_close(),
> > > but NFS4_OO_PURGE_CLOSE isn't cleared meanwhile. So the stateid released
> > in
> > > THIS close procedure may be freed immediately in the coming encoding
> > function.
> >
> > OK, makes sense.  This code is confusing, I wonder if there's some way
> > we could make it simpler.
> >
> 
> The only purpose of NFS4_OO_PURGE_CLOSE is to decide whether the stateid
> pointed by last-closed-stateid should be freed or not. I wonder if this
> flag is necessary. oo_last_closed_stid is set only in CLOSE, so in other
> procedures, if the pointer is not NULL, it must be set in previous
> procedure, we can free it directly. In CLOSE procedure, we also free it
> directly before asigning the new released stateid to it in nfsd4_close(),
> and then we can ignore it in nfsd4_encode_close(). What do you think?

Yeah, something like that would be simpler, I think.  Maybe the
following?  (On top of your patch.)

> I don't quite understand the difficulties in CLOSE retransmission. RFC3530
> suggests we deallocate the stateid in next validly sequenced request. If we
> don't do so, what will happen?

Nothing bad.  It's just that the stateid isn't useful any more past that
point, so we may as well get rid of it.  (If the client sends the close
with sequence number s, and then resends the close, we want to be able
to reply.  But once it sends another operation with sequence s+1, it's
saying that it's received the answer to the close now.  And any further
attempt to send something with sequence number s would fail.)

> I don't see this suggestion in RFC5661, and
> the stateid is deallocated directly in NFSv4.1, why? Thanks very much.

In 4.1 these stateowner-based sequence numbers aren't used any more.
(They're still there, but they're ignored.)  Instead 4.1 depends on the
SEQUENCE operation to provide ordering and exactly-once semantics.

--b.

commit 4e603ce0a56deeac0a83d067a863f96f0619c26d
Author: J. Bruce Fields <bfields@redhat.com>
Date:   Mon Apr 1 16:37:12 2013 -0400

    nfsd4: cleanup handling of nfsv4.0 closed stateid's
    
    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 285a0c8..aa6e77f 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -575,7 +575,7 @@ static void unhash_openowner(struct nfs4_openowner *oo)
 	}
 }
 
-static void release_last_closed_stateid(struct nfs4_openowner *oo)
+void release_last_closed_stateid(struct nfs4_openowner *oo)
 {
 	struct nfs4_ol_stateid *s = oo->oo_last_closed_stid;
 
@@ -3760,26 +3760,6 @@ out:
 	return status;
 }
 
-void nfsd4_purge_closed_stateid(struct nfs4_stateowner *so)
-{
-	struct nfs4_openowner *oo;
-	struct nfs4_ol_stateid *s;
-
-	if (!so->so_is_open_owner)
-		return;
-	oo = openowner(so);
-	s = oo->oo_last_closed_stid;
-	if (!s)
-		return;
-	if (!(oo->oo_flags & NFS4_OO_PURGE_CLOSE)) {
-		/* Release the last_closed_stid on the next seqid bump: */
-		oo->oo_flags |= NFS4_OO_PURGE_CLOSE;
-		return;
-	}
-	oo->oo_flags &= ~NFS4_OO_PURGE_CLOSE;
-	release_last_closed_stateid(oo);
-}
-
 static void nfsd4_close_open_stateid(struct nfs4_ol_stateid *s)
 {
 	unhash_open_stateid(s);
@@ -3816,9 +3796,7 @@ nfsd4_close(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	memcpy(&close->cl_stateid, &stp->st_stid.sc_stateid, sizeof(stateid_t));
 
 	nfsd4_close_open_stateid(stp);
-	release_last_closed_stateid(oo);
-	oo->oo_flags &= ~NFS4_OO_PURGE_CLOSE;
-	oo->oo_last_closed_stid = stp;
+	close->cl_closed_stateid = stp;
 
 	if (list_empty(&oo->oo_owner.so_stateids)) {
 		if (cstate->minorversion) {
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index e3dc58d..2324638 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -527,6 +527,7 @@ nfsd4_decode_close(struct nfsd4_compoundargs *argp, struct nfsd4_close *close)
 {
 	DECODE_HEAD;
 
+	close->cl_closed_stateid = NULL;
 	READ_BUF(4);
 	READ32(close->cl_seqid);
 	return nfsd4_decode_stateid(argp, &close->cl_stateid);
@@ -1707,8 +1708,7 @@ static void write_cinfo(__be32 **p, struct nfsd4_change_info *c)
  * Note that we increment sequence id's here, at the last moment, so we're sure
  * we know whether the error to be returned is a sequence id mutating error.
  */
-
-static void encode_seqid_op_tail(struct nfsd4_compoundres *resp, __be32 *save, __be32 nfserr)
+static void encode_seqid_op_tail(struct nfsd4_compoundres *resp, __be32 *save, __be32 nfserr, struct nfs4_ol_stateid *close_stp)
 {
 	struct nfs4_stateowner *stateowner = resp->cstate.replay_owner;
 
@@ -1719,7 +1719,13 @@ static void encode_seqid_op_tail(struct nfsd4_compoundres *resp, __be32 *save, _
 			  (char *)resp->p - (char *)save;
 		memcpy(stateowner->so_replay.rp_buf, save,
 			stateowner->so_replay.rp_buflen);
-		nfsd4_purge_closed_stateid(stateowner);
+		if (stateowner->so_is_open_owner) {
+			struct nfs4_openowner *oo = openowner(stateowner);
+
+			release_last_closed_stateid(oo);
+			if (close_stp)
+				oo->oo_last_closed_stid = close_stp;
+		}
 	}
 }
 
@@ -2667,7 +2673,7 @@ nfsd4_encode_close(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_c
 	if (!nfserr)
 		nfsd4_encode_stateid(resp, &close->cl_stateid);
 
-	encode_seqid_op_tail(resp, save, nfserr);
+	encode_seqid_op_tail(resp, save, nfserr, close->cl_closed_stateid);
 	return nfserr;
 }
 
@@ -2770,7 +2776,7 @@ nfsd4_encode_lock(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_lo
 	else if (nfserr == nfserr_denied)
 		nfsd4_encode_lock_denied(resp, &lock->lk_denied);
 
-	encode_seqid_op_tail(resp, save, nfserr);
+	encode_seqid_op_tail(resp, save, nfserr, NULL);
 	return nfserr;
 }
 
@@ -2790,7 +2796,7 @@ nfsd4_encode_locku(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_l
 	if (!nfserr)
 		nfsd4_encode_stateid(resp, &locku->lu_stateid);
 
-	encode_seqid_op_tail(resp, save, nfserr);
+	encode_seqid_op_tail(resp, save, nfserr, NULL);
 	return nfserr;
 }
 
@@ -2885,7 +2891,7 @@ nfsd4_encode_open(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_op
 	}
 	/* XXX save filehandle here */
 out:
-	encode_seqid_op_tail(resp, save, nfserr);
+	encode_seqid_op_tail(resp, save, nfserr, NULL);
 	return nfserr;
 }
 
@@ -2897,7 +2903,7 @@ nfsd4_encode_open_confirm(struct nfsd4_compoundres *resp, __be32 nfserr, struct
 	if (!nfserr)
 		nfsd4_encode_stateid(resp, &oc->oc_resp_stateid);
 
-	encode_seqid_op_tail(resp, save, nfserr);
+	encode_seqid_op_tail(resp, save, nfserr, NULL);
 	return nfserr;
 }
 
@@ -2909,7 +2915,7 @@ nfsd4_encode_open_downgrade(struct nfsd4_compoundres *resp, __be32 nfserr, struc
 	if (!nfserr)
 		nfsd4_encode_stateid(resp, &od->od_stateid);
 
-	encode_seqid_op_tail(resp, save, nfserr);
+	encode_seqid_op_tail(resp, save, nfserr, NULL);
 	return nfserr;
 }
 
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 327552b..1401599 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -363,7 +363,6 @@ struct nfs4_openowner {
 	struct nfs4_ol_stateid *oo_last_closed_stid;
 	time_t			oo_time; /* time of placement on so_close_lru */
 #define NFS4_OO_CONFIRMED   1
-#define NFS4_OO_PURGE_CLOSE 2
 #define NFS4_OO_NEW         4
 	unsigned char		oo_flags;
 };
@@ -371,7 +370,7 @@ struct nfs4_openowner {
 struct nfs4_lockowner {
 	struct nfs4_stateowner	lo_owner; /* must be first element */
 	struct list_head	lo_owner_ino_hash; /* hash by owner,file */
-	struct list_head        lo_perstateid; /* for lockowners only */
+	struct list_head        lo_perstateid;
 	struct list_head	lo_list; /* for temporary uses */
 };
 
@@ -485,7 +484,7 @@ extern struct nfs4_client_reclaim *nfs4_client_to_reclaim(const char *name,
 							struct nfsd_net *nn);
 extern bool nfs4_has_reclaimed_state(const char *name, struct nfsd_net *nn);
 extern void release_session_client(struct nfsd4_session *);
-extern void nfsd4_purge_closed_stateid(struct nfs4_stateowner *);
+extern void release_last_closed_stateid(struct nfs4_openowner *);
 
 /* nfs4recover operations */
 extern int nfsd4_client_tracking_init(struct net *net);
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index 40e05e6..34766df 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -91,7 +91,8 @@ struct nfsd4_access {
 
 struct nfsd4_close {
 	u32		cl_seqid;           /* request */
-	stateid_t	cl_stateid;         /* request+response */
+	stateid_t	cl_stateid;         /* request */
+	struct nfs4_ol_stateid *cl_closed_stateid; /* response */
 };
 
 struct nfsd4_commit {

Re: [PATCH v2] nfsd: fix bug on nfs4 stateid deallocation

[Yanchuan Nian]

On Mon, Apr 01, 2013 at 09:50:44PM -0400, J. Bruce Fields wrote:
> On Wed, Mar 13, 2013 at 11:04:54PM +0800, Yanchuan Nian wrote:
> > 2013/3/11 J. Bruce Fields <bfields@fieldses.org>
> > 
> > > On Mon, Mar 11, 2013 at 08:46:14AM +0800, ycnian@gmail.com wrote:
> > > > NFS4_OO_PURGE_CLOSE is not handled properly. To avoid memory leak, nfs4
> > > > stateid which is pointed by oo_last_closed_stid is freed in
> > > nfsd4_close(),
> > > > but NFS4_OO_PURGE_CLOSE isn't cleared meanwhile. So the stateid released
> > > in
> > > > THIS close procedure may be freed immediately in the coming encoding
> > > function.
> > >
> > > OK, makes sense.  This code is confusing, I wonder if there's some way
> > > we could make it simpler.
> > >
> > 
> > The only purpose of NFS4_OO_PURGE_CLOSE is to decide whether the stateid
> > pointed by last-closed-stateid should be freed or not. I wonder if this
> > flag is necessary. oo_last_closed_stid is set only in CLOSE, so in other
> > procedures, if the pointer is not NULL, it must be set in previous
> > procedure, we can free it directly. In CLOSE procedure, we also free it
> > directly before asigning the new released stateid to it in nfsd4_close(),
> > and then we can ignore it in nfsd4_encode_close(). What do you think?
> 
> Yeah, something like that would be simpler, I think.  Maybe the
> following?  (On top of your patch.)

This patch may cause memory leak in NFSv4.1. If the stateid released is the last one in nfs4_openowner, nfs4_openowner will be deallocated immediately, and NULL will be assigned to cstate->replay_owner at the same time. In this situation encode_seqid_op_tail() cannot be called. To avoid this problem, we can free the stateid just before or after nfs4_opwnowner deallocation.

Another question, cl_stateid in struct nfsd4_close will be returned to the client, so original comment is right even though applying this patch. Can this struct be commented like this.

struct nfsd4_close {
        u32             cl_seqid;           /* request */
        stateid_t       cl_stateid;         /* request+response */
        struct nfs4_ol_stateid *cl_closed_stateid; /* used during processing */
};



> 
> > I don't quite understand the difficulties in CLOSE retransmission. RFC3530
> > suggests we deallocate the stateid in next validly sequenced request. If we
> > don't do so, what will happen?
> 
> Nothing bad.  It's just that the stateid isn't useful any more past that
> point, so we may as well get rid of it.  (If the client sends the close
> with sequence number s, and then resends the close, we want to be able
> to reply.  But once it sends another operation with sequence s+1, it's
> saying that it's received the answer to the close now.  And any further
> attempt to send something with sequence number s would fail.)
> 
> > I don't see this suggestion in RFC5661, and
> > the stateid is deallocated directly in NFSv4.1, why? Thanks very much.
> 
> In 4.1 these stateowner-based sequence numbers aren't used any more.
> (They're still there, but they're ignored.)  Instead 4.1 depends on the
> SEQUENCE operation to provide ordering and exactly-once semantics.
> 
> --b.
> 
> commit 4e603ce0a56deeac0a83d067a863f96f0619c26d
> Author: J. Bruce Fields <bfields@redhat.com>
> Date:   Mon Apr 1 16:37:12 2013 -0400
> 
>     nfsd4: cleanup handling of nfsv4.0 closed stateid's
>     
>     Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 285a0c8..aa6e77f 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -575,7 +575,7 @@ static void unhash_openowner(struct nfs4_openowner *oo)
>  	}
>  }
>  
> -static void release_last_closed_stateid(struct nfs4_openowner *oo)
> +void release_last_closed_stateid(struct nfs4_openowner *oo)
>  {
>  	struct nfs4_ol_stateid *s = oo->oo_last_closed_stid;
>  
> @@ -3760,26 +3760,6 @@ out:
>  	return status;
>  }
>  
> -void nfsd4_purge_closed_stateid(struct nfs4_stateowner *so)
> -{
> -	struct nfs4_openowner *oo;
> -	struct nfs4_ol_stateid *s;
> -
> -	if (!so->so_is_open_owner)
> -		return;
> -	oo = openowner(so);
> -	s = oo->oo_last_closed_stid;
> -	if (!s)
> -		return;
> -	if (!(oo->oo_flags & NFS4_OO_PURGE_CLOSE)) {
> -		/* Release the last_closed_stid on the next seqid bump: */
> -		oo->oo_flags |= NFS4_OO_PURGE_CLOSE;
> -		return;
> -	}
> -	oo->oo_flags &= ~NFS4_OO_PURGE_CLOSE;
> -	release_last_closed_stateid(oo);
> -}
> -
>  static void nfsd4_close_open_stateid(struct nfs4_ol_stateid *s)
>  {
>  	unhash_open_stateid(s);
> @@ -3816,9 +3796,7 @@ nfsd4_close(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	memcpy(&close->cl_stateid, &stp->st_stid.sc_stateid, sizeof(stateid_t));
>  
>  	nfsd4_close_open_stateid(stp);
> -	release_last_closed_stateid(oo);
> -	oo->oo_flags &= ~NFS4_OO_PURGE_CLOSE;
> -	oo->oo_last_closed_stid = stp;
> +	close->cl_closed_stateid = stp;
>  
>  	if (list_empty(&oo->oo_owner.so_stateids)) {
>  		if (cstate->minorversion) {
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index e3dc58d..2324638 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -527,6 +527,7 @@ nfsd4_decode_close(struct nfsd4_compoundargs *argp, struct nfsd4_close *close)
>  {
>  	DECODE_HEAD;
>  
> +	close->cl_closed_stateid = NULL;
>  	READ_BUF(4);
>  	READ32(close->cl_seqid);
>  	return nfsd4_decode_stateid(argp, &close->cl_stateid);
> @@ -1707,8 +1708,7 @@ static void write_cinfo(__be32 **p, struct nfsd4_change_info *c)
>   * Note that we increment sequence id's here, at the last moment, so we're sure
>   * we know whether the error to be returned is a sequence id mutating error.
>   */
> -
> -static void encode_seqid_op_tail(struct nfsd4_compoundres *resp, __be32 *save, __be32 nfserr)
> +static void encode_seqid_op_tail(struct nfsd4_compoundres *resp, __be32 *save, __be32 nfserr, struct nfs4_ol_stateid *close_stp)
>  {
>  	struct nfs4_stateowner *stateowner = resp->cstate.replay_owner;
>  
> @@ -1719,7 +1719,13 @@ static void encode_seqid_op_tail(struct nfsd4_compoundres *resp, __be32 *save, _
>  			  (char *)resp->p - (char *)save;
>  		memcpy(stateowner->so_replay.rp_buf, save,
>  			stateowner->so_replay.rp_buflen);
> -		nfsd4_purge_closed_stateid(stateowner);
> +		if (stateowner->so_is_open_owner) {
> +			struct nfs4_openowner *oo = openowner(stateowner);
> +
> +			release_last_closed_stateid(oo);
> +			if (close_stp)
> +				oo->oo_last_closed_stid = close_stp;
> +		}
>  	}
>  }
>  
> @@ -2667,7 +2673,7 @@ nfsd4_encode_close(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_c
>  	if (!nfserr)
>  		nfsd4_encode_stateid(resp, &close->cl_stateid);
>  
> -	encode_seqid_op_tail(resp, save, nfserr);
> +	encode_seqid_op_tail(resp, save, nfserr, close->cl_closed_stateid);
>  	return nfserr;
>  }
>  
> @@ -2770,7 +2776,7 @@ nfsd4_encode_lock(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_lo
>  	else if (nfserr == nfserr_denied)
>  		nfsd4_encode_lock_denied(resp, &lock->lk_denied);
>  
> -	encode_seqid_op_tail(resp, save, nfserr);
> +	encode_seqid_op_tail(resp, save, nfserr, NULL);
>  	return nfserr;
>  }
>  
> @@ -2790,7 +2796,7 @@ nfsd4_encode_locku(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_l
>  	if (!nfserr)
>  		nfsd4_encode_stateid(resp, &locku->lu_stateid);
>  
> -	encode_seqid_op_tail(resp, save, nfserr);
> +	encode_seqid_op_tail(resp, save, nfserr, NULL);
>  	return nfserr;
>  }
>  
> @@ -2885,7 +2891,7 @@ nfsd4_encode_open(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_op
>  	}
>  	/* XXX save filehandle here */
>  out:
> -	encode_seqid_op_tail(resp, save, nfserr);
> +	encode_seqid_op_tail(resp, save, nfserr, NULL);
>  	return nfserr;
>  }
>  
> @@ -2897,7 +2903,7 @@ nfsd4_encode_open_confirm(struct nfsd4_compoundres *resp, __be32 nfserr, struct
>  	if (!nfserr)
>  		nfsd4_encode_stateid(resp, &oc->oc_resp_stateid);
>  
> -	encode_seqid_op_tail(resp, save, nfserr);
> +	encode_seqid_op_tail(resp, save, nfserr, NULL);
>  	return nfserr;
>  }
>  
> @@ -2909,7 +2915,7 @@ nfsd4_encode_open_downgrade(struct nfsd4_compoundres *resp, __be32 nfserr, struc
>  	if (!nfserr)
>  		nfsd4_encode_stateid(resp, &od->od_stateid);
>  
> -	encode_seqid_op_tail(resp, save, nfserr);
> +	encode_seqid_op_tail(resp, save, nfserr, NULL);
>  	return nfserr;
>  }
>  
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index 327552b..1401599 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -363,7 +363,6 @@ struct nfs4_openowner {
>  	struct nfs4_ol_stateid *oo_last_closed_stid;
>  	time_t			oo_time; /* time of placement on so_close_lru */
>  #define NFS4_OO_CONFIRMED   1
> -#define NFS4_OO_PURGE_CLOSE 2
>  #define NFS4_OO_NEW         4
>  	unsigned char		oo_flags;
>  };
> @@ -371,7 +370,7 @@ struct nfs4_openowner {
>  struct nfs4_lockowner {
>  	struct nfs4_stateowner	lo_owner; /* must be first element */
>  	struct list_head	lo_owner_ino_hash; /* hash by owner,file */
> -	struct list_head        lo_perstateid; /* for lockowners only */
> +	struct list_head        lo_perstateid;
>  	struct list_head	lo_list; /* for temporary uses */
>  };
>  
> @@ -485,7 +484,7 @@ extern struct nfs4_client_reclaim *nfs4_client_to_reclaim(const char *name,
>  							struct nfsd_net *nn);
>  extern bool nfs4_has_reclaimed_state(const char *name, struct nfsd_net *nn);
>  extern void release_session_client(struct nfsd4_session *);
> -extern void nfsd4_purge_closed_stateid(struct nfs4_stateowner *);
> +extern void release_last_closed_stateid(struct nfs4_openowner *);
>  
>  /* nfs4recover operations */
>  extern int nfsd4_client_tracking_init(struct net *net);
> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> index 40e05e6..34766df 100644
> --- a/fs/nfsd/xdr4.h
> +++ b/fs/nfsd/xdr4.h
> @@ -91,7 +91,8 @@ struct nfsd4_access {
>  
>  struct nfsd4_close {
>  	u32		cl_seqid;           /* request */
> -	stateid_t	cl_stateid;         /* request+response */
> +	stateid_t	cl_stateid;         /* request */
> +	struct nfs4_ol_stateid *cl_closed_stateid; /* response */
>  };
>  
>  struct nfsd4_commit {

Re: [PATCH v2] nfsd: fix bug on nfs4 stateid deallocation

[J. Bruce Fields]

On Wed, Apr 03, 2013 at 06:58:43PM +0800, Yanchuan Nian wrote:
> On Mon, Apr 01, 2013 at 09:50:44PM -0400, J. Bruce Fields wrote:
> > On Wed, Mar 13, 2013 at 11:04:54PM +0800, Yanchuan Nian wrote:
> > > 2013/3/11 J. Bruce Fields <bfields@fieldses.org>
> > > 
> > > > On Mon, Mar 11, 2013 at 08:46:14AM +0800, ycnian@gmail.com wrote:
> > > > > NFS4_OO_PURGE_CLOSE is not handled properly. To avoid memory leak, nfs4
> > > > > stateid which is pointed by oo_last_closed_stid is freed in
> > > > nfsd4_close(),
> > > > > but NFS4_OO_PURGE_CLOSE isn't cleared meanwhile. So the stateid released
> > > > in
> > > > > THIS close procedure may be freed immediately in the coming encoding
> > > > function.
> > > >
> > > > OK, makes sense.  This code is confusing, I wonder if there's some way
> > > > we could make it simpler.
> > > >
> > > 
> > > The only purpose of NFS4_OO_PURGE_CLOSE is to decide whether the stateid
> > > pointed by last-closed-stateid should be freed or not. I wonder if this
> > > flag is necessary. oo_last_closed_stid is set only in CLOSE, so in other
> > > procedures, if the pointer is not NULL, it must be set in previous
> > > procedure, we can free it directly. In CLOSE procedure, we also free it
> > > directly before asigning the new released stateid to it in nfsd4_close(),
> > > and then we can ignore it in nfsd4_encode_close(). What do you think?
> > 
> > Yeah, something like that would be simpler, I think.  Maybe the
> > following?  (On top of your patch.)
> 

> This patch may cause memory leak in NFSv4.1. If the stateid released
> is the last one in nfs4_openowner, nfs4_openowner will be deallocated
> immediately, and NULL will be assigned to cstate->replay_owner at the
> same time. In this situation encode_seqid_op_tail() cannot be called.
> To avoid this problem, we can free the stateid just before or after
> nfs4_opwnowner deallocation.

Yes, thanks for catching that!:

 	nfsd4_close_open_stateid(stp);
-	close->cl_closed_stateid = stp;
+	if (cstate->minorversion) {
+		unhash_stid(&stp->st_stid);
+		free_generic_stateid(stp);
+	} else
+		close->cl_closed_stateid = stp;
 
 	if (list_empty(&oo->oo_owner.so_stateids)) {
 		if (cstate->minorversion) {

> Another question, cl_stateid in struct nfsd4_close will be returned to
> the client, so original comment is right even though applying this
> patch. Can this struct be commented like this.
> 
> struct nfsd4_close {
>         u32             cl_seqid;           /* request */
>         stateid_t       cl_stateid;         /* request+response */
>         struct nfs4_ol_stateid *cl_closed_stateid; /* used during processing */

Agreed; done.  The result is the following (untested).

--b.

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 3e1101a..9bd6653 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -674,7 +674,7 @@ static void unhash_openowner(struct nfs4_openowner *oo)
 	}
 }
 
-static void release_last_closed_stateid(struct nfs4_openowner *oo)
+void release_last_closed_stateid(struct nfs4_openowner *oo)
 {
 	struct nfs4_ol_stateid *s = oo->oo_last_closed_stid;
 
@@ -3783,26 +3783,6 @@ out:
 	return status;
 }
 
-void nfsd4_purge_closed_stateid(struct nfs4_stateowner *so)
-{
-	struct nfs4_openowner *oo;
-	struct nfs4_ol_stateid *s;
-
-	if (!so->so_is_open_owner)
-		return;
-	oo = openowner(so);
-	s = oo->oo_last_closed_stid;
-	if (!s)
-		return;
-	if (!(oo->oo_flags & NFS4_OO_PURGE_CLOSE)) {
-		/* Release the last_closed_stid on the next seqid bump: */
-		oo->oo_flags |= NFS4_OO_PURGE_CLOSE;
-		return;
-	}
-	oo->oo_flags &= ~NFS4_OO_PURGE_CLOSE;
-	release_last_closed_stateid(oo);
-}
-
 static void nfsd4_close_open_stateid(struct nfs4_ol_stateid *s)
 {
 	unhash_open_stateid(s);
@@ -3839,9 +3819,11 @@ nfsd4_close(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	memcpy(&close->cl_stateid, &stp->st_stid.sc_stateid, sizeof(stateid_t));
 
 	nfsd4_close_open_stateid(stp);
-	release_last_closed_stateid(oo);
-	oo->oo_flags &= ~NFS4_OO_PURGE_CLOSE;
-	oo->oo_last_closed_stid = stp;
+	if (cstate->minorversion) {
+		unhash_stid(&stp->st_stid);
+		free_generic_stateid(stp);
+	} else
+		close->cl_closed_stateid = stp;
 
 	if (list_empty(&oo->oo_owner.so_stateids)) {
 		if (cstate->minorversion) {
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 700de01..60e196f 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -527,6 +527,7 @@ nfsd4_decode_close(struct nfsd4_compoundargs *argp, struct nfsd4_close *close)
 {
 	DECODE_HEAD;
 
+	close->cl_closed_stateid = NULL;
 	READ_BUF(4);
 	READ32(close->cl_seqid);
 	return nfsd4_decode_stateid(argp, &close->cl_stateid);
@@ -1707,8 +1708,7 @@ static void write_cinfo(__be32 **p, struct nfsd4_change_info *c)
  * Note that we increment sequence id's here, at the last moment, so we're sure
  * we know whether the error to be returned is a sequence id mutating error.
  */
-
-static void encode_seqid_op_tail(struct nfsd4_compoundres *resp, __be32 *save, __be32 nfserr)
+static void encode_seqid_op_tail(struct nfsd4_compoundres *resp, __be32 *save, __be32 nfserr, struct nfs4_ol_stateid *close_stp)
 {
 	struct nfs4_stateowner *stateowner = resp->cstate.replay_owner;
 
@@ -1719,7 +1719,13 @@ static void encode_seqid_op_tail(struct nfsd4_compoundres *resp, __be32 *save, _
 			  (char *)resp->p - (char *)save;
 		memcpy(stateowner->so_replay.rp_buf, save,
 			stateowner->so_replay.rp_buflen);
-		nfsd4_purge_closed_stateid(stateowner);
+		if (stateowner->so_is_open_owner) {
+			struct nfs4_openowner *oo = openowner(stateowner);
+
+			release_last_closed_stateid(oo);
+			if (close_stp)
+				oo->oo_last_closed_stid = close_stp;
+		}
 	}
 }
 
@@ -2667,7 +2673,7 @@ nfsd4_encode_close(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_c
 	if (!nfserr)
 		nfsd4_encode_stateid(resp, &close->cl_stateid);
 
-	encode_seqid_op_tail(resp, save, nfserr);
+	encode_seqid_op_tail(resp, save, nfserr, close->cl_closed_stateid);
 	return nfserr;
 }
 
@@ -2770,7 +2776,7 @@ nfsd4_encode_lock(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_lo
 	else if (nfserr == nfserr_denied)
 		nfsd4_encode_lock_denied(resp, &lock->lk_denied);
 
-	encode_seqid_op_tail(resp, save, nfserr);
+	encode_seqid_op_tail(resp, save, nfserr, NULL);
 	return nfserr;
 }
 
@@ -2790,7 +2796,7 @@ nfsd4_encode_locku(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_l
 	if (!nfserr)
 		nfsd4_encode_stateid(resp, &locku->lu_stateid);
 
-	encode_seqid_op_tail(resp, save, nfserr);
+	encode_seqid_op_tail(resp, save, nfserr, NULL);
 	return nfserr;
 }
 
@@ -2885,7 +2891,7 @@ nfsd4_encode_open(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_op
 	}
 	/* XXX save filehandle here */
 out:
-	encode_seqid_op_tail(resp, save, nfserr);
+	encode_seqid_op_tail(resp, save, nfserr, NULL);
 	return nfserr;
 }
 
@@ -2897,7 +2903,7 @@ nfsd4_encode_open_confirm(struct nfsd4_compoundres *resp, __be32 nfserr, struct
 	if (!nfserr)
 		nfsd4_encode_stateid(resp, &oc->oc_resp_stateid);
 
-	encode_seqid_op_tail(resp, save, nfserr);
+	encode_seqid_op_tail(resp, save, nfserr, NULL);
 	return nfserr;
 }
 
@@ -2909,7 +2915,7 @@ nfsd4_encode_open_downgrade(struct nfsd4_compoundres *resp, __be32 nfserr, struc
 	if (!nfserr)
 		nfsd4_encode_stateid(resp, &od->od_stateid);
 
-	encode_seqid_op_tail(resp, save, nfserr);
+	encode_seqid_op_tail(resp, save, nfserr, NULL);
 	return nfserr;
 }
 
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index f6ae4db..477f027 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -355,7 +355,6 @@ struct nfs4_openowner {
 	struct nfs4_ol_stateid *oo_last_closed_stid;
 	time_t			oo_time; /* time of placement on so_close_lru */
 #define NFS4_OO_CONFIRMED   1
-#define NFS4_OO_PURGE_CLOSE 2
 #define NFS4_OO_NEW         4
 	unsigned char		oo_flags;
 };
@@ -363,7 +362,7 @@ struct nfs4_openowner {
 struct nfs4_lockowner {
 	struct nfs4_stateowner	lo_owner; /* must be first element */
 	struct list_head	lo_owner_ino_hash; /* hash by owner,file */
-	struct list_head        lo_perstateid; /* for lockowners only */
+	struct list_head        lo_perstateid;
 	struct list_head	lo_list; /* for temporary uses */
 };
 
@@ -477,7 +476,7 @@ extern struct nfs4_client_reclaim *nfs4_client_to_reclaim(const char *name,
 							struct nfsd_net *nn);
 extern bool nfs4_has_reclaimed_state(const char *name, struct nfsd_net *nn);
 extern void put_client_renew(struct nfs4_client *clp);
-extern void nfsd4_purge_closed_stateid(struct nfs4_stateowner *);
+extern void release_last_closed_stateid(struct nfs4_openowner *);
 
 /* nfs4recover operations */
 extern int nfsd4_client_tracking_init(struct net *net);
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index 40e05e6..4d501db 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -91,7 +91,8 @@ struct nfsd4_access {
 
 struct nfsd4_close {
 	u32		cl_seqid;           /* request */
-	stateid_t	cl_stateid;         /* request+response */
+	stateid_t	cl_stateid;         /* request */
+	struct nfs4_ol_stateid *cl_closed_stateid; /* used during processing */
 };
 
 struct nfsd4_commit {

Re: [PATCH v2] nfsd: fix bug on nfs4 stateid deallocation

[Yanchuan Nian]

On Wed, Apr 03, 2013 at 02:55:08PM -0400, J. Bruce Fields wrote:
> On Wed, Apr 03, 2013 at 06:58:43PM +0800, Yanchuan Nian wrote:
> > On Mon, Apr 01, 2013 at 09:50:44PM -0400, J. Bruce Fields wrote:
> > > On Wed, Mar 13, 2013 at 11:04:54PM +0800, Yanchuan Nian wrote:
> > > > 2013/3/11 J. Bruce Fields <bfields@fieldses.org>
> > > > 
> > > > > On Mon, Mar 11, 2013 at 08:46:14AM +0800, ycnian@gmail.com wrote:
> > > > > > NFS4_OO_PURGE_CLOSE is not handled properly. To avoid memory leak, nfs4
> > > > > > stateid which is pointed by oo_last_closed_stid is freed in
> > > > > nfsd4_close(),
> > > > > > but NFS4_OO_PURGE_CLOSE isn't cleared meanwhile. So the stateid released
> > > > > in
> > > > > > THIS close procedure may be freed immediately in the coming encoding
> > > > > function.
> > > > >
> > > > > OK, makes sense.  This code is confusing, I wonder if there's some way
> > > > > we could make it simpler.
> > > > >
> > > > 
> > > > The only purpose of NFS4_OO_PURGE_CLOSE is to decide whether the stateid
> > > > pointed by last-closed-stateid should be freed or not. I wonder if this
> > > > flag is necessary. oo_last_closed_stid is set only in CLOSE, so in other
> > > > procedures, if the pointer is not NULL, it must be set in previous
> > > > procedure, we can free it directly. In CLOSE procedure, we also free it
> > > > directly before asigning the new released stateid to it in nfsd4_close(),
> > > > and then we can ignore it in nfsd4_encode_close(). What do you think?
> > > 
> > > Yeah, something like that would be simpler, I think.  Maybe the
> > > following?  (On top of your patch.)
> > 
> 
> > This patch may cause memory leak in NFSv4.1. If the stateid released
> > is the last one in nfs4_openowner, nfs4_openowner will be deallocated
> > immediately, and NULL will be assigned to cstate->replay_owner at the
> > same time. In this situation encode_seqid_op_tail() cannot be called.
> > To avoid this problem, we can free the stateid just before or after
> > nfs4_opwnowner deallocation.
> 
> Yes, thanks for catching that!:
> 
>  	nfsd4_close_open_stateid(stp);
> -	close->cl_closed_stateid = stp;
> +	if (cstate->minorversion) {
> +		unhash_stid(&stp->st_stid);
> +		free_generic_stateid(stp);
> +	} else
> +		close->cl_closed_stateid = stp;
>  
>  	if (list_empty(&oo->oo_owner.so_stateids)) {
>  		if (cstate->minorversion) {
> 
> > Another question, cl_stateid in struct nfsd4_close will be returned to
> > the client, so original comment is right even though applying this
> > patch. Can this struct be commented like this.
> > 
> > struct nfsd4_close {
> >         u32             cl_seqid;           /* request */
> >         stateid_t       cl_stateid;         /* request+response */
> >         struct nfs4_ol_stateid *cl_closed_stateid; /* used during processing */
> 
> Agreed; done.  The result is the following (untested).

Yes, it works on my machine. Just the comment of cl_stateid in struct
nfsd4_close is still a little misleading.
> 
> --b.
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 3e1101a..9bd6653 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -674,7 +674,7 @@ static void unhash_openowner(struct nfs4_openowner *oo)
>  	}
>  }
>  
> -static void release_last_closed_stateid(struct nfs4_openowner *oo)
> +void release_last_closed_stateid(struct nfs4_openowner *oo)
>  {
>  	struct nfs4_ol_stateid *s = oo->oo_last_closed_stid;
>  
> @@ -3783,26 +3783,6 @@ out:
>  	return status;
>  }
>  
> -void nfsd4_purge_closed_stateid(struct nfs4_stateowner *so)
> -{
> -	struct nfs4_openowner *oo;
> -	struct nfs4_ol_stateid *s;
> -
> -	if (!so->so_is_open_owner)
> -		return;
> -	oo = openowner(so);
> -	s = oo->oo_last_closed_stid;
> -	if (!s)
> -		return;
> -	if (!(oo->oo_flags & NFS4_OO_PURGE_CLOSE)) {
> -		/* Release the last_closed_stid on the next seqid bump: */
> -		oo->oo_flags |= NFS4_OO_PURGE_CLOSE;
> -		return;
> -	}
> -	oo->oo_flags &= ~NFS4_OO_PURGE_CLOSE;
> -	release_last_closed_stateid(oo);
> -}
> -
>  static void nfsd4_close_open_stateid(struct nfs4_ol_stateid *s)
>  {
>  	unhash_open_stateid(s);
> @@ -3839,9 +3819,11 @@ nfsd4_close(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	memcpy(&close->cl_stateid, &stp->st_stid.sc_stateid, sizeof(stateid_t));
>  
>  	nfsd4_close_open_stateid(stp);
> -	release_last_closed_stateid(oo);
> -	oo->oo_flags &= ~NFS4_OO_PURGE_CLOSE;
> -	oo->oo_last_closed_stid = stp;
> +	if (cstate->minorversion) {
> +		unhash_stid(&stp->st_stid);
> +		free_generic_stateid(stp);
> +	} else
> +		close->cl_closed_stateid = stp;
>  
>  	if (list_empty(&oo->oo_owner.so_stateids)) {
>  		if (cstate->minorversion) {
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 700de01..60e196f 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -527,6 +527,7 @@ nfsd4_decode_close(struct nfsd4_compoundargs *argp, struct nfsd4_close *close)
>  {
>  	DECODE_HEAD;
>  
> +	close->cl_closed_stateid = NULL;
>  	READ_BUF(4);
>  	READ32(close->cl_seqid);
>  	return nfsd4_decode_stateid(argp, &close->cl_stateid);
> @@ -1707,8 +1708,7 @@ static void write_cinfo(__be32 **p, struct nfsd4_change_info *c)
>   * Note that we increment sequence id's here, at the last moment, so we're sure
>   * we know whether the error to be returned is a sequence id mutating error.
>   */
> -
> -static void encode_seqid_op_tail(struct nfsd4_compoundres *resp, __be32 *save, __be32 nfserr)
> +static void encode_seqid_op_tail(struct nfsd4_compoundres *resp, __be32 *save, __be32 nfserr, struct nfs4_ol_stateid *close_stp)
>  {
>  	struct nfs4_stateowner *stateowner = resp->cstate.replay_owner;
>  
> @@ -1719,7 +1719,13 @@ static void encode_seqid_op_tail(struct nfsd4_compoundres *resp, __be32 *save, _
>  			  (char *)resp->p - (char *)save;
>  		memcpy(stateowner->so_replay.rp_buf, save,
>  			stateowner->so_replay.rp_buflen);
> -		nfsd4_purge_closed_stateid(stateowner);
> +		if (stateowner->so_is_open_owner) {
> +			struct nfs4_openowner *oo = openowner(stateowner);
> +
> +			release_last_closed_stateid(oo);
> +			if (close_stp)
> +				oo->oo_last_closed_stid = close_stp;
> +		}
>  	}
>  }
>  
> @@ -2667,7 +2673,7 @@ nfsd4_encode_close(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_c
>  	if (!nfserr)
>  		nfsd4_encode_stateid(resp, &close->cl_stateid);
>  
> -	encode_seqid_op_tail(resp, save, nfserr);
> +	encode_seqid_op_tail(resp, save, nfserr, close->cl_closed_stateid);
>  	return nfserr;
>  }
>  
> @@ -2770,7 +2776,7 @@ nfsd4_encode_lock(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_lo
>  	else if (nfserr == nfserr_denied)
>  		nfsd4_encode_lock_denied(resp, &lock->lk_denied);
>  
> -	encode_seqid_op_tail(resp, save, nfserr);
> +	encode_seqid_op_tail(resp, save, nfserr, NULL);
>  	return nfserr;
>  }
>  
> @@ -2790,7 +2796,7 @@ nfsd4_encode_locku(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_l
>  	if (!nfserr)
>  		nfsd4_encode_stateid(resp, &locku->lu_stateid);
>  
> -	encode_seqid_op_tail(resp, save, nfserr);
> +	encode_seqid_op_tail(resp, save, nfserr, NULL);
>  	return nfserr;
>  }
>  
> @@ -2885,7 +2891,7 @@ nfsd4_encode_open(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_op
>  	}
>  	/* XXX save filehandle here */
>  out:
> -	encode_seqid_op_tail(resp, save, nfserr);
> +	encode_seqid_op_tail(resp, save, nfserr, NULL);
>  	return nfserr;
>  }
>  
> @@ -2897,7 +2903,7 @@ nfsd4_encode_open_confirm(struct nfsd4_compoundres *resp, __be32 nfserr, struct
>  	if (!nfserr)
>  		nfsd4_encode_stateid(resp, &oc->oc_resp_stateid);
>  
> -	encode_seqid_op_tail(resp, save, nfserr);
> +	encode_seqid_op_tail(resp, save, nfserr, NULL);
>  	return nfserr;
>  }
>  
> @@ -2909,7 +2915,7 @@ nfsd4_encode_open_downgrade(struct nfsd4_compoundres *resp, __be32 nfserr, struc
>  	if (!nfserr)
>  		nfsd4_encode_stateid(resp, &od->od_stateid);
>  
> -	encode_seqid_op_tail(resp, save, nfserr);
> +	encode_seqid_op_tail(resp, save, nfserr, NULL);
>  	return nfserr;
>  }
>  
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index f6ae4db..477f027 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -355,7 +355,6 @@ struct nfs4_openowner {
>  	struct nfs4_ol_stateid *oo_last_closed_stid;
>  	time_t			oo_time; /* time of placement on so_close_lru */
>  #define NFS4_OO_CONFIRMED   1
> -#define NFS4_OO_PURGE_CLOSE 2
>  #define NFS4_OO_NEW         4
>  	unsigned char		oo_flags;
>  };
> @@ -363,7 +362,7 @@ struct nfs4_openowner {
>  struct nfs4_lockowner {
>  	struct nfs4_stateowner	lo_owner; /* must be first element */
>  	struct list_head	lo_owner_ino_hash; /* hash by owner,file */
> -	struct list_head        lo_perstateid; /* for lockowners only */
> +	struct list_head        lo_perstateid;
>  	struct list_head	lo_list; /* for temporary uses */
>  };
>  
> @@ -477,7 +476,7 @@ extern struct nfs4_client_reclaim *nfs4_client_to_reclaim(const char *name,
>  							struct nfsd_net *nn);
>  extern bool nfs4_has_reclaimed_state(const char *name, struct nfsd_net *nn);
>  extern void put_client_renew(struct nfs4_client *clp);
> -extern void nfsd4_purge_closed_stateid(struct nfs4_stateowner *);
> +extern void release_last_closed_stateid(struct nfs4_openowner *);
>  
>  /* nfs4recover operations */
>  extern int nfsd4_client_tracking_init(struct net *net);
> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> index 40e05e6..4d501db 100644
> --- a/fs/nfsd/xdr4.h
> +++ b/fs/nfsd/xdr4.h
> @@ -91,7 +91,8 @@ struct nfsd4_access {
>  
>  struct nfsd4_close {
>  	u32		cl_seqid;           /* request */
> -	stateid_t	cl_stateid;         /* request+response */
> +	stateid_t	cl_stateid;         /* request */
> +	struct nfs4_ol_stateid *cl_closed_stateid; /* used during processing */
>  };
>  
>  struct nfsd4_commit {

Re: [PATCH v2] nfsd: fix bug on nfs4 stateid deallocation

[J. Bruce Fields]

On Thu, Apr 04, 2013 at 10:04:03PM +0800, Yanchuan Nian wrote:
> On Wed, Apr 03, 2013 at 02:55:08PM -0400, J. Bruce Fields wrote:
> > On Wed, Apr 03, 2013 at 06:58:43PM +0800, Yanchuan Nian wrote:
> > > On Mon, Apr 01, 2013 at 09:50:44PM -0400, J. Bruce Fields wrote:
> > > > On Wed, Mar 13, 2013 at 11:04:54PM +0800, Yanchuan Nian wrote:
> > > > > 2013/3/11 J. Bruce Fields <bfields@fieldses.org>
> > > > > 
> > > > > > On Mon, Mar 11, 2013 at 08:46:14AM +0800, ycnian@gmail.com wrote:
> > > > > > > NFS4_OO_PURGE_CLOSE is not handled properly. To avoid memory leak, nfs4
> > > > > > > stateid which is pointed by oo_last_closed_stid is freed in
> > > > > > nfsd4_close(),
> > > > > > > but NFS4_OO_PURGE_CLOSE isn't cleared meanwhile. So the stateid released
> > > > > > in
> > > > > > > THIS close procedure may be freed immediately in the coming encoding
> > > > > > function.
> > > > > >
> > > > > > OK, makes sense.  This code is confusing, I wonder if there's some way
> > > > > > we could make it simpler.
> > > > > >
> > > > > 
> > > > > The only purpose of NFS4_OO_PURGE_CLOSE is to decide whether the stateid
> > > > > pointed by last-closed-stateid should be freed or not. I wonder if this
> > > > > flag is necessary. oo_last_closed_stid is set only in CLOSE, so in other
> > > > > procedures, if the pointer is not NULL, it must be set in previous
> > > > > procedure, we can free it directly. In CLOSE procedure, we also free it
> > > > > directly before asigning the new released stateid to it in nfsd4_close(),
> > > > > and then we can ignore it in nfsd4_encode_close(). What do you think?
> > > > 
> > > > Yeah, something like that would be simpler, I think.  Maybe the
> > > > following?  (On top of your patch.)
> > > 
> > 
> > > This patch may cause memory leak in NFSv4.1. If the stateid released
> > > is the last one in nfs4_openowner, nfs4_openowner will be deallocated
> > > immediately, and NULL will be assigned to cstate->replay_owner at the
> > > same time. In this situation encode_seqid_op_tail() cannot be called.
> > > To avoid this problem, we can free the stateid just before or after
> > > nfs4_opwnowner deallocation.
> > 
> > Yes, thanks for catching that!:
> > 
> >  	nfsd4_close_open_stateid(stp);
> > -	close->cl_closed_stateid = stp;
> > +	if (cstate->minorversion) {
> > +		unhash_stid(&stp->st_stid);
> > +		free_generic_stateid(stp);
> > +	} else
> > +		close->cl_closed_stateid = stp;
> >  
> >  	if (list_empty(&oo->oo_owner.so_stateids)) {
> >  		if (cstate->minorversion) {
> > 
> > > Another question, cl_stateid in struct nfsd4_close will be returned to
> > > the client, so original comment is right even though applying this
> > > patch. Can this struct be commented like this.
> > > 
> > > struct nfsd4_close {
> > >         u32             cl_seqid;           /* request */
> > >         stateid_t       cl_stateid;         /* request+response */
> > >         struct nfs4_ol_stateid *cl_closed_stateid; /* used during processing */
> > 
> > Agreed; done.  The result is the following (untested).
> 
> Yes, it works on my machine. Just the comment of cl_stateid in struct
> nfsd4_close is still a little misleading.

Whoops, fixed.

Actually, I find encode_seqid_op_tail completely confusing, and I don't
see why it's necessary--it would seem more straightforward to do the
seqid bump in the procedure itself.  How about the following?

--b.

commit e58235072acd52ecab71d498b2b06633a2a0c376
Author: J. Bruce Fields <bfields@redhat.com>
Date:   Mon Apr 1 16:37:12 2013 -0400

    nfsd4: cleanup handling of nfsv4.0 closed stateid's
    
    Closed stateid's are kept around a little while to handle close replays
    in the 4.0 case.  So we stash them in the last-used stateid in the
    oo_last_closed_stateid field of the open owner.  We can free that in
    encode_seqid_op_tail once the seqid on the open owner is next
    incremented.  But we don't want to do that on the close itself; so we
    set NFS4_OO_PURGE_CLOSE flag set on the open owner, skip freeing it the
    first time through encode_seqid_op_tail, then when we see that flag set
    next time we free it.
    
    This is unnecessarily baroque.
    
    Instead, just move the logic that increments the seqid out of the xdr
    code and into the operation code itself.
    
    The justification given for the current placement is that we need to
    wait till the last minute to be sure we know whether the status is a
    sequence-id-mutating error or not, but examination of the code shows
    that can't actually happen.
    
    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index a9b707b..609e1e2 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -415,7 +415,8 @@ out:
 	nfsd4_cleanup_open_state(open, status);
 	if (open->op_openowner)
 		cstate->replay_owner = &open->op_openowner->oo_owner;
-	else
+	nfsd4_bump_seqid(cstate, status);
+	if (!cstate->replay_owner)
 		nfs4_unlock_state();
 	return status;
 }
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 795b24d..bcd2339 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -720,6 +720,28 @@ dump_sessionid(const char *fn, struct nfs4_sessionid *sessionid)
 }
 #endif
 
+/*
+ * Bump the seqid on cstate->replay_owner, and clear replay_owner if it
+ * won't be used for replay.
+ */
+void nfsd4_bump_seqid(struct nfsd4_compound_state *cstate, __be32 nfserr)
+{
+	struct nfs4_stateowner *so = cstate->replay_owner;
+
+	if (nfserr == nfserr_replay_me)
+		return;
+
+	if (!seqid_mutating_err(ntohl(nfserr))) {
+		cstate->replay_owner = NULL;
+		return;
+	}
+	if (!so)
+		return;
+	if (so->so_is_open_owner)
+		release_last_closed_stateid(openowner(so));
+	so->so_seqid++;
+	return;
+}
 
 static void
 gen_sessionid(struct nfsd4_session *ses)
@@ -3702,6 +3724,7 @@ nfsd4_open_confirm(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	nfsd4_client_record_create(oo->oo_owner.so_client);
 	status = nfs_ok;
 out:
+	nfsd4_bump_seqid(cstate, status);
 	if (!cstate->replay_owner)
 		nfs4_unlock_state();
 	return status;
@@ -3785,31 +3808,12 @@ nfsd4_open_downgrade(struct svc_rqst *rqstp,
 	memcpy(&od->od_stateid, &stp->st_stid.sc_stateid, sizeof(stateid_t));
 	status = nfs_ok;
 out:
+	nfsd4_bump_seqid(cstate, status);
 	if (!cstate->replay_owner)
 		nfs4_unlock_state();
 	return status;
 }
 
-void nfsd4_purge_closed_stateid(struct nfs4_stateowner *so)
-{
-	struct nfs4_openowner *oo;
-	struct nfs4_ol_stateid *s;
-
-	if (!so->so_is_open_owner)
-		return;
-	oo = openowner(so);
-	s = oo->oo_last_closed_stid;
-	if (!s)
-		return;
-	if (!(oo->oo_flags & NFS4_OO_PURGE_CLOSE)) {
-		/* Release the last_closed_stid on the next seqid bump: */
-		oo->oo_flags |= NFS4_OO_PURGE_CLOSE;
-		return;
-	}
-	oo->oo_flags &= ~NFS4_OO_PURGE_CLOSE;
-	release_last_closed_stateid(oo);
-}
-
 static void nfsd4_close_open_stateid(struct nfs4_ol_stateid *s)
 {
 	unhash_open_stateid(s);
@@ -3838,17 +3842,20 @@ nfsd4_close(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 					&close->cl_stateid,
 					NFS4_OPEN_STID|NFS4_CLOSED_STID,
 					&stp, nn);
+	nfsd4_bump_seqid(cstate, status);
 	if (status)
 		goto out; 
 	oo = openowner(stp->st_stateowner);
-	status = nfs_ok;
 	update_stateid(&stp->st_stid.sc_stateid);
 	memcpy(&close->cl_stateid, &stp->st_stid.sc_stateid, sizeof(stateid_t));
 
 	nfsd4_close_open_stateid(stp);
-	release_last_closed_stateid(oo);
-	oo->oo_flags &= ~NFS4_OO_PURGE_CLOSE;
-	oo->oo_last_closed_stid = stp;
+
+	if (cstate->minorversion) {
+		unhash_stid(&stp->st_stid);
+		free_generic_stateid(stp);
+	} else
+		oo->oo_last_closed_stid = stp;
 
 	if (list_empty(&oo->oo_owner.so_stateids)) {
 		if (cstate->minorversion) {
@@ -4270,6 +4277,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 out:
 	if (status && new_state)
 		release_lockowner(lock_sop);
+	nfsd4_bump_seqid(cstate, status);
 	if (!cstate->replay_owner)
 		nfs4_unlock_state();
 	if (file_lock)
@@ -4439,6 +4447,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	memcpy(&locku->lu_stateid, &stp->st_stid.sc_stateid, sizeof(stateid_t));
 
 out:
+	nfsd4_bump_seqid(cstate, status);
 	if (!cstate->replay_owner)
 		nfs4_unlock_state();
 	if (file_lock)
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index 700de01..a5e8a64 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -1701,28 +1701,6 @@ static void write_cinfo(__be32 **p, struct nfsd4_change_info *c)
 								\
 	save = resp->p;
 
-/*
- * Routine for encoding the result of a "seqid-mutating" NFSv4 operation.  This
- * is where sequence id's are incremented, and the replay cache is filled.
- * Note that we increment sequence id's here, at the last moment, so we're sure
- * we know whether the error to be returned is a sequence id mutating error.
- */
-
-static void encode_seqid_op_tail(struct nfsd4_compoundres *resp, __be32 *save, __be32 nfserr)
-{
-	struct nfs4_stateowner *stateowner = resp->cstate.replay_owner;
-
-	if (seqid_mutating_err(ntohl(nfserr)) && stateowner) {
-		stateowner->so_seqid++;
-		stateowner->so_replay.rp_status = nfserr;
-		stateowner->so_replay.rp_buflen =
-			  (char *)resp->p - (char *)save;
-		memcpy(stateowner->so_replay.rp_buf, save,
-			stateowner->so_replay.rp_buflen);
-		nfsd4_purge_closed_stateid(stateowner);
-	}
-}
-
 /* Encode as an array of strings the string given with components
  * separated @sep, escaped with esc_enter and esc_exit.
  */
@@ -2667,7 +2645,6 @@ nfsd4_encode_close(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_c
 	if (!nfserr)
 		nfsd4_encode_stateid(resp, &close->cl_stateid);
 
-	encode_seqid_op_tail(resp, save, nfserr);
 	return nfserr;
 }
 
@@ -2770,7 +2747,6 @@ nfsd4_encode_lock(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_lo
 	else if (nfserr == nfserr_denied)
 		nfsd4_encode_lock_denied(resp, &lock->lk_denied);
 
-	encode_seqid_op_tail(resp, save, nfserr);
 	return nfserr;
 }
 
@@ -2790,7 +2766,6 @@ nfsd4_encode_locku(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_l
 	if (!nfserr)
 		nfsd4_encode_stateid(resp, &locku->lu_stateid);
 
-	encode_seqid_op_tail(resp, save, nfserr);
 	return nfserr;
 }
 
@@ -2885,7 +2860,6 @@ nfsd4_encode_open(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_op
 	}
 	/* XXX save filehandle here */
 out:
-	encode_seqid_op_tail(resp, save, nfserr);
 	return nfserr;
 }
 
@@ -2897,7 +2871,6 @@ nfsd4_encode_open_confirm(struct nfsd4_compoundres *resp, __be32 nfserr, struct
 	if (!nfserr)
 		nfsd4_encode_stateid(resp, &oc->oc_resp_stateid);
 
-	encode_seqid_op_tail(resp, save, nfserr);
 	return nfserr;
 }
 
@@ -2909,7 +2882,6 @@ nfsd4_encode_open_downgrade(struct nfsd4_compoundres *resp, __be32 nfserr, struc
 	if (!nfserr)
 		nfsd4_encode_stateid(resp, &od->od_stateid);
 
-	encode_seqid_op_tail(resp, save, nfserr);
 	return nfserr;
 }
 
@@ -3567,6 +3539,7 @@ __be32 nfsd4_check_resp_size(struct nfsd4_compoundres *resp, u32 pad)
 void
 nfsd4_encode_operation(struct nfsd4_compoundres *resp, struct nfsd4_op *op)
 {
+	struct nfs4_stateowner *so = resp->cstate.replay_owner;
 	__be32 *statp;
 	__be32 *p;
 
@@ -3583,6 +3556,11 @@ nfsd4_encode_operation(struct nfsd4_compoundres *resp, struct nfsd4_op *op)
 	/* nfsd4_check_drc_limit guarantees enough room for error status */
 	if (!op->status)
 		op->status = nfsd4_check_resp_size(resp, 0);
+	if (so) {
+		so->so_replay.rp_status = op->status;
+		so->so_replay.rp_buflen = (char *)resp->p - (char *)(statp+1);
+		memcpy(so->so_replay.rp_buf, statp+1, so->so_replay.rp_buflen);
+	}
 status:
 	/*
 	 * Note: We write the status directly, instead of using WRITE32(),
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index 7674bc8..13ec485 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -355,7 +355,6 @@ struct nfs4_openowner {
 	struct nfs4_ol_stateid *oo_last_closed_stid;
 	time_t			oo_time; /* time of placement on so_close_lru */
 #define NFS4_OO_CONFIRMED   1
-#define NFS4_OO_PURGE_CLOSE 2
 #define NFS4_OO_NEW         4
 	unsigned char		oo_flags;
 };
@@ -363,7 +362,7 @@ struct nfs4_openowner {
 struct nfs4_lockowner {
 	struct nfs4_stateowner	lo_owner; /* must be first element */
 	struct list_head	lo_owner_ino_hash; /* hash by owner,file */
-	struct list_head        lo_perstateid; /* for lockowners only */
+	struct list_head        lo_perstateid;
 	struct list_head	lo_list; /* for temporary uses */
 };
 
@@ -477,7 +476,6 @@ extern struct nfs4_client_reclaim *nfs4_client_to_reclaim(const char *name,
 							struct nfsd_net *nn);
 extern bool nfs4_has_reclaimed_state(const char *name, struct nfsd_net *nn);
 extern void put_client_renew(struct nfs4_client *clp);
-extern void nfsd4_purge_closed_stateid(struct nfs4_stateowner *);
 
 /* nfs4recover operations */
 extern int nfsd4_client_tracking_init(struct net *net);
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index 40e05e6..3b271d2 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -623,6 +623,7 @@ extern __be32 nfsd4_test_stateid(struct svc_rqst *rqstp,
 		struct nfsd4_compound_state *, struct nfsd4_test_stateid *test_stateid);
 extern __be32 nfsd4_free_stateid(struct svc_rqst *rqstp,
 		struct nfsd4_compound_state *, struct nfsd4_free_stateid *free_stateid);
+extern void nfsd4_bump_seqid(struct nfsd4_compound_state *, __be32 nfserr);
 #endif
 
 /*

Re: [PATCH v2] nfsd: fix bug on nfs4 stateid deallocation

[Yanchuan Nian]

On Fri, Apr 05, 2013 at 10:20:42AM -0400, J. Bruce Fields wrote:
> On Thu, Apr 04, 2013 at 10:04:03PM +0800, Yanchuan Nian wrote:
> > On Wed, Apr 03, 2013 at 02:55:08PM -0400, J. Bruce Fields wrote:
> > > On Wed, Apr 03, 2013 at 06:58:43PM +0800, Yanchuan Nian wrote:
> > > > On Mon, Apr 01, 2013 at 09:50:44PM -0400, J. Bruce Fields wrote:
> > > > > On Wed, Mar 13, 2013 at 11:04:54PM +0800, Yanchuan Nian wrote:
> > > > > > 2013/3/11 J. Bruce Fields <bfields@fieldses.org>
> > > > > > 
> > > > > > > On Mon, Mar 11, 2013 at 08:46:14AM +0800, ycnian@gmail.com wrote:
> > > > > > > > NFS4_OO_PURGE_CLOSE is not handled properly. To avoid memory leak, nfs4
> > > > > > > > stateid which is pointed by oo_last_closed_stid is freed in
> > > > > > > nfsd4_close(),
> > > > > > > > but NFS4_OO_PURGE_CLOSE isn't cleared meanwhile. So the stateid released
> > > > > > > in
> > > > > > > > THIS close procedure may be freed immediately in the coming encoding
> > > > > > > function.
> > > > > > >
> > > > > > > OK, makes sense.  This code is confusing, I wonder if there's some way
> > > > > > > we could make it simpler.
> > > > > > >
> > > > > > 
> > > > > > The only purpose of NFS4_OO_PURGE_CLOSE is to decide whether the stateid
> > > > > > pointed by last-closed-stateid should be freed or not. I wonder if this
> > > > > > flag is necessary. oo_last_closed_stid is set only in CLOSE, so in other
> > > > > > procedures, if the pointer is not NULL, it must be set in previous
> > > > > > procedure, we can free it directly. In CLOSE procedure, we also free it
> > > > > > directly before asigning the new released stateid to it in nfsd4_close(),
> > > > > > and then we can ignore it in nfsd4_encode_close(). What do you think?
> > > > > 
> > > > > Yeah, something like that would be simpler, I think.  Maybe the
> > > > > following?  (On top of your patch.)
> > > > 
> > > 
> > > > This patch may cause memory leak in NFSv4.1. If the stateid released
> > > > is the last one in nfs4_openowner, nfs4_openowner will be deallocated
> > > > immediately, and NULL will be assigned to cstate->replay_owner at the
> > > > same time. In this situation encode_seqid_op_tail() cannot be called.
> > > > To avoid this problem, we can free the stateid just before or after
> > > > nfs4_opwnowner deallocation.
> > > 
> > > Yes, thanks for catching that!:
> > > 
> > >  	nfsd4_close_open_stateid(stp);
> > > -	close->cl_closed_stateid = stp;
> > > +	if (cstate->minorversion) {
> > > +		unhash_stid(&stp->st_stid);
> > > +		free_generic_stateid(stp);
> > > +	} else
> > > +		close->cl_closed_stateid = stp;
> > >  
> > >  	if (list_empty(&oo->oo_owner.so_stateids)) {
> > >  		if (cstate->minorversion) {
> > > 
> > > > Another question, cl_stateid in struct nfsd4_close will be returned to
> > > > the client, so original comment is right even though applying this
> > > > patch. Can this struct be commented like this.
> > > > 
> > > > struct nfsd4_close {
> > > >         u32             cl_seqid;           /* request */
> > > >         stateid_t       cl_stateid;         /* request+response */
> > > >         struct nfs4_ol_stateid *cl_closed_stateid; /* used during processing */
> > > 
> > > Agreed; done.  The result is the following (untested).
> > 
> > Yes, it works on my machine. Just the comment of cl_stateid in struct
> > nfsd4_close is still a little misleading.
> 
> Whoops, fixed.
> 
> Actually, I find encode_seqid_op_tail completely confusing, and I don't
> see why it's necessary--it would seem more straightforward to do the
> seqid bump in the procedure itself.  How about the following?

Sounds OK. I test it with some basic opens/closes. All goes well.
> 
> --b.
> 
> commit e58235072acd52ecab71d498b2b06633a2a0c376
> Author: J. Bruce Fields <bfields@redhat.com>
> Date:   Mon Apr 1 16:37:12 2013 -0400
> 
>     nfsd4: cleanup handling of nfsv4.0 closed stateid's
>     
>     Closed stateid's are kept around a little while to handle close replays
>     in the 4.0 case.  So we stash them in the last-used stateid in the
>     oo_last_closed_stateid field of the open owner.  We can free that in
>     encode_seqid_op_tail once the seqid on the open owner is next
>     incremented.  But we don't want to do that on the close itself; so we
>     set NFS4_OO_PURGE_CLOSE flag set on the open owner, skip freeing it the
>     first time through encode_seqid_op_tail, then when we see that flag set
>     next time we free it.
>     
>     This is unnecessarily baroque.
>     
>     Instead, just move the logic that increments the seqid out of the xdr
>     code and into the operation code itself.
>     
>     The justification given for the current placement is that we need to
>     wait till the last minute to be sure we know whether the status is a
>     sequence-id-mutating error or not, but examination of the code shows
>     that can't actually happen.
>     
>     Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index a9b707b..609e1e2 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -415,7 +415,8 @@ out:
>  	nfsd4_cleanup_open_state(open, status);
>  	if (open->op_openowner)
>  		cstate->replay_owner = &open->op_openowner->oo_owner;
> -	else
> +	nfsd4_bump_seqid(cstate, status);
> +	if (!cstate->replay_owner)
>  		nfs4_unlock_state();
>  	return status;
>  }
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 795b24d..bcd2339 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -720,6 +720,28 @@ dump_sessionid(const char *fn, struct nfs4_sessionid *sessionid)
>  }
>  #endif
>  
> +/*
> + * Bump the seqid on cstate->replay_owner, and clear replay_owner if it
> + * won't be used for replay.
> + */
> +void nfsd4_bump_seqid(struct nfsd4_compound_state *cstate, __be32 nfserr)
> +{
> +	struct nfs4_stateowner *so = cstate->replay_owner;
> +
> +	if (nfserr == nfserr_replay_me)
> +		return;
> +
> +	if (!seqid_mutating_err(ntohl(nfserr))) {
> +		cstate->replay_owner = NULL;
> +		return;
> +	}
> +	if (!so)
> +		return;
> +	if (so->so_is_open_owner)
> +		release_last_closed_stateid(openowner(so));
> +	so->so_seqid++;
> +	return;
> +}
>  
>  static void
>  gen_sessionid(struct nfsd4_session *ses)
> @@ -3702,6 +3724,7 @@ nfsd4_open_confirm(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	nfsd4_client_record_create(oo->oo_owner.so_client);
>  	status = nfs_ok;
>  out:
> +	nfsd4_bump_seqid(cstate, status);
>  	if (!cstate->replay_owner)
>  		nfs4_unlock_state();
>  	return status;
> @@ -3785,31 +3808,12 @@ nfsd4_open_downgrade(struct svc_rqst *rqstp,
>  	memcpy(&od->od_stateid, &stp->st_stid.sc_stateid, sizeof(stateid_t));
>  	status = nfs_ok;
>  out:
> +	nfsd4_bump_seqid(cstate, status);
>  	if (!cstate->replay_owner)
>  		nfs4_unlock_state();
>  	return status;
>  }
>  
> -void nfsd4_purge_closed_stateid(struct nfs4_stateowner *so)
> -{
> -	struct nfs4_openowner *oo;
> -	struct nfs4_ol_stateid *s;
> -
> -	if (!so->so_is_open_owner)
> -		return;
> -	oo = openowner(so);
> -	s = oo->oo_last_closed_stid;
> -	if (!s)
> -		return;
> -	if (!(oo->oo_flags & NFS4_OO_PURGE_CLOSE)) {
> -		/* Release the last_closed_stid on the next seqid bump: */
> -		oo->oo_flags |= NFS4_OO_PURGE_CLOSE;
> -		return;
> -	}
> -	oo->oo_flags &= ~NFS4_OO_PURGE_CLOSE;
> -	release_last_closed_stateid(oo);
> -}
> -
>  static void nfsd4_close_open_stateid(struct nfs4_ol_stateid *s)
>  {
>  	unhash_open_stateid(s);
> @@ -3838,17 +3842,20 @@ nfsd4_close(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  					&close->cl_stateid,
>  					NFS4_OPEN_STID|NFS4_CLOSED_STID,
>  					&stp, nn);
> +	nfsd4_bump_seqid(cstate, status);
>  	if (status)
>  		goto out; 
>  	oo = openowner(stp->st_stateowner);
> -	status = nfs_ok;
>  	update_stateid(&stp->st_stid.sc_stateid);
>  	memcpy(&close->cl_stateid, &stp->st_stid.sc_stateid, sizeof(stateid_t));
>  
>  	nfsd4_close_open_stateid(stp);
> -	release_last_closed_stateid(oo);
> -	oo->oo_flags &= ~NFS4_OO_PURGE_CLOSE;
> -	oo->oo_last_closed_stid = stp;
> +
> +	if (cstate->minorversion) {
> +		unhash_stid(&stp->st_stid);
> +		free_generic_stateid(stp);
> +	} else
> +		oo->oo_last_closed_stid = stp;
>  
>  	if (list_empty(&oo->oo_owner.so_stateids)) {
>  		if (cstate->minorversion) {
> @@ -4270,6 +4277,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  out:
>  	if (status && new_state)
>  		release_lockowner(lock_sop);
> +	nfsd4_bump_seqid(cstate, status);
>  	if (!cstate->replay_owner)
>  		nfs4_unlock_state();
>  	if (file_lock)
> @@ -4439,6 +4447,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	memcpy(&locku->lu_stateid, &stp->st_stid.sc_stateid, sizeof(stateid_t));
>  
>  out:
> +	nfsd4_bump_seqid(cstate, status);
>  	if (!cstate->replay_owner)
>  		nfs4_unlock_state();
>  	if (file_lock)
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index 700de01..a5e8a64 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -1701,28 +1701,6 @@ static void write_cinfo(__be32 **p, struct nfsd4_change_info *c)
>  								\
>  	save = resp->p;
>  
> -/*
> - * Routine for encoding the result of a "seqid-mutating" NFSv4 operation.  This
> - * is where sequence id's are incremented, and the replay cache is filled.
> - * Note that we increment sequence id's here, at the last moment, so we're sure
> - * we know whether the error to be returned is a sequence id mutating error.
> - */
> -
> -static void encode_seqid_op_tail(struct nfsd4_compoundres *resp, __be32 *save, __be32 nfserr)
> -{
> -	struct nfs4_stateowner *stateowner = resp->cstate.replay_owner;
> -
> -	if (seqid_mutating_err(ntohl(nfserr)) && stateowner) {
> -		stateowner->so_seqid++;
> -		stateowner->so_replay.rp_status = nfserr;
> -		stateowner->so_replay.rp_buflen =
> -			  (char *)resp->p - (char *)save;
> -		memcpy(stateowner->so_replay.rp_buf, save,
> -			stateowner->so_replay.rp_buflen);
> -		nfsd4_purge_closed_stateid(stateowner);
> -	}
> -}
> -
>  /* Encode as an array of strings the string given with components
>   * separated @sep, escaped with esc_enter and esc_exit.
>   */
> @@ -2667,7 +2645,6 @@ nfsd4_encode_close(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_c
>  	if (!nfserr)
>  		nfsd4_encode_stateid(resp, &close->cl_stateid);
>  
> -	encode_seqid_op_tail(resp, save, nfserr);
>  	return nfserr;
>  }
>  
> @@ -2770,7 +2747,6 @@ nfsd4_encode_lock(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_lo
>  	else if (nfserr == nfserr_denied)
>  		nfsd4_encode_lock_denied(resp, &lock->lk_denied);
>  
> -	encode_seqid_op_tail(resp, save, nfserr);
>  	return nfserr;
>  }
>  
> @@ -2790,7 +2766,6 @@ nfsd4_encode_locku(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_l
>  	if (!nfserr)
>  		nfsd4_encode_stateid(resp, &locku->lu_stateid);
>  
> -	encode_seqid_op_tail(resp, save, nfserr);
>  	return nfserr;
>  }
>  
> @@ -2885,7 +2860,6 @@ nfsd4_encode_open(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_op
>  	}
>  	/* XXX save filehandle here */
>  out:
> -	encode_seqid_op_tail(resp, save, nfserr);
>  	return nfserr;
>  }
>  
> @@ -2897,7 +2871,6 @@ nfsd4_encode_open_confirm(struct nfsd4_compoundres *resp, __be32 nfserr, struct
>  	if (!nfserr)
>  		nfsd4_encode_stateid(resp, &oc->oc_resp_stateid);
>  
> -	encode_seqid_op_tail(resp, save, nfserr);
>  	return nfserr;
>  }
>  
> @@ -2909,7 +2882,6 @@ nfsd4_encode_open_downgrade(struct nfsd4_compoundres *resp, __be32 nfserr, struc
>  	if (!nfserr)
>  		nfsd4_encode_stateid(resp, &od->od_stateid);
>  
> -	encode_seqid_op_tail(resp, save, nfserr);
>  	return nfserr;
>  }
>  
> @@ -3567,6 +3539,7 @@ __be32 nfsd4_check_resp_size(struct nfsd4_compoundres *resp, u32 pad)
>  void
>  nfsd4_encode_operation(struct nfsd4_compoundres *resp, struct nfsd4_op *op)
>  {
> +	struct nfs4_stateowner *so = resp->cstate.replay_owner;
>  	__be32 *statp;
>  	__be32 *p;
>  
> @@ -3583,6 +3556,11 @@ nfsd4_encode_operation(struct nfsd4_compoundres *resp, struct nfsd4_op *op)
>  	/* nfsd4_check_drc_limit guarantees enough room for error status */
>  	if (!op->status)
>  		op->status = nfsd4_check_resp_size(resp, 0);
> +	if (so) {
> +		so->so_replay.rp_status = op->status;
> +		so->so_replay.rp_buflen = (char *)resp->p - (char *)(statp+1);
> +		memcpy(so->so_replay.rp_buf, statp+1, so->so_replay.rp_buflen);
> +	}
>  status:
>  	/*
>  	 * Note: We write the status directly, instead of using WRITE32(),
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index 7674bc8..13ec485 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -355,7 +355,6 @@ struct nfs4_openowner {
>  	struct nfs4_ol_stateid *oo_last_closed_stid;
>  	time_t			oo_time; /* time of placement on so_close_lru */
>  #define NFS4_OO_CONFIRMED   1
> -#define NFS4_OO_PURGE_CLOSE 2
>  #define NFS4_OO_NEW         4
>  	unsigned char		oo_flags;
>  };
> @@ -363,7 +362,7 @@ struct nfs4_openowner {
>  struct nfs4_lockowner {
>  	struct nfs4_stateowner	lo_owner; /* must be first element */
>  	struct list_head	lo_owner_ino_hash; /* hash by owner,file */
> -	struct list_head        lo_perstateid; /* for lockowners only */
> +	struct list_head        lo_perstateid;
>  	struct list_head	lo_list; /* for temporary uses */
>  };
>  
> @@ -477,7 +476,6 @@ extern struct nfs4_client_reclaim *nfs4_client_to_reclaim(const char *name,
>  							struct nfsd_net *nn);
>  extern bool nfs4_has_reclaimed_state(const char *name, struct nfsd_net *nn);
>  extern void put_client_renew(struct nfs4_client *clp);
> -extern void nfsd4_purge_closed_stateid(struct nfs4_stateowner *);
>  
>  /* nfs4recover operations */
>  extern int nfsd4_client_tracking_init(struct net *net);
> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> index 40e05e6..3b271d2 100644
> --- a/fs/nfsd/xdr4.h
> +++ b/fs/nfsd/xdr4.h
> @@ -623,6 +623,7 @@ extern __be32 nfsd4_test_stateid(struct svc_rqst *rqstp,
>  		struct nfsd4_compound_state *, struct nfsd4_test_stateid *test_stateid);
>  extern __be32 nfsd4_free_stateid(struct svc_rqst *rqstp,
>  		struct nfsd4_compound_state *, struct nfsd4_free_stateid *free_stateid);
> +extern void nfsd4_bump_seqid(struct nfsd4_compound_state *, __be32 nfserr);
>  #endif
>  
>  /*

Re: [PATCH v2] nfsd: fix bug on nfs4 stateid deallocation

[J. Bruce Fields]

On Mon, Apr 08, 2013 at 09:54:18PM +0800, Yanchuan Nian wrote:
> On Fri, Apr 05, 2013 at 10:20:42AM -0400, J. Bruce Fields wrote:
> > Actually, I find encode_seqid_op_tail completely confusing, and I don't
> > see why it's necessary--it would seem more straightforward to do the
> > seqid bump in the procedure itself.  How about the following?
> 
> Sounds OK. I test it with some basic opens/closes. All goes well.

Good, thanks!

--b.

> > 
> > --b.
> > 
> > commit e58235072acd52ecab71d498b2b06633a2a0c376
> > Author: J. Bruce Fields <bfields@redhat.com>
> > Date:   Mon Apr 1 16:37:12 2013 -0400
> > 
> >     nfsd4: cleanup handling of nfsv4.0 closed stateid's
> >     
> >     Closed stateid's are kept around a little while to handle close replays
> >     in the 4.0 case.  So we stash them in the last-used stateid in the
> >     oo_last_closed_stateid field of the open owner.  We can free that in
> >     encode_seqid_op_tail once the seqid on the open owner is next
> >     incremented.  But we don't want to do that on the close itself; so we
> >     set NFS4_OO_PURGE_CLOSE flag set on the open owner, skip freeing it the
> >     first time through encode_seqid_op_tail, then when we see that flag set
> >     next time we free it.
> >     
> >     This is unnecessarily baroque.
> >     
> >     Instead, just move the logic that increments the seqid out of the xdr
> >     code and into the operation code itself.
> >     
> >     The justification given for the current placement is that we need to
> >     wait till the last minute to be sure we know whether the status is a
> >     sequence-id-mutating error or not, but examination of the code shows
> >     that can't actually happen.
> >     
> >     Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> > 
> > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > index a9b707b..609e1e2 100644
> > --- a/fs/nfsd/nfs4proc.c
> > +++ b/fs/nfsd/nfs4proc.c
> > @@ -415,7 +415,8 @@ out:
> >  	nfsd4_cleanup_open_state(open, status);
> >  	if (open->op_openowner)
> >  		cstate->replay_owner = &open->op_openowner->oo_owner;
> > -	else
> > +	nfsd4_bump_seqid(cstate, status);
> > +	if (!cstate->replay_owner)
> >  		nfs4_unlock_state();
> >  	return status;
> >  }
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 795b24d..bcd2339 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -720,6 +720,28 @@ dump_sessionid(const char *fn, struct nfs4_sessionid *sessionid)
> >  }
> >  #endif
> >  
> > +/*
> > + * Bump the seqid on cstate->replay_owner, and clear replay_owner if it
> > + * won't be used for replay.
> > + */
> > +void nfsd4_bump_seqid(struct nfsd4_compound_state *cstate, __be32 nfserr)
> > +{
> > +	struct nfs4_stateowner *so = cstate->replay_owner;
> > +
> > +	if (nfserr == nfserr_replay_me)
> > +		return;
> > +
> > +	if (!seqid_mutating_err(ntohl(nfserr))) {
> > +		cstate->replay_owner = NULL;
> > +		return;
> > +	}
> > +	if (!so)
> > +		return;
> > +	if (so->so_is_open_owner)
> > +		release_last_closed_stateid(openowner(so));
> > +	so->so_seqid++;
> > +	return;
> > +}
> >  
> >  static void
> >  gen_sessionid(struct nfsd4_session *ses)
> > @@ -3702,6 +3724,7 @@ nfsd4_open_confirm(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >  	nfsd4_client_record_create(oo->oo_owner.so_client);
> >  	status = nfs_ok;
> >  out:
> > +	nfsd4_bump_seqid(cstate, status);
> >  	if (!cstate->replay_owner)
> >  		nfs4_unlock_state();
> >  	return status;
> > @@ -3785,31 +3808,12 @@ nfsd4_open_downgrade(struct svc_rqst *rqstp,
> >  	memcpy(&od->od_stateid, &stp->st_stid.sc_stateid, sizeof(stateid_t));
> >  	status = nfs_ok;
> >  out:
> > +	nfsd4_bump_seqid(cstate, status);
> >  	if (!cstate->replay_owner)
> >  		nfs4_unlock_state();
> >  	return status;
> >  }
> >  
> > -void nfsd4_purge_closed_stateid(struct nfs4_stateowner *so)
> > -{
> > -	struct nfs4_openowner *oo;
> > -	struct nfs4_ol_stateid *s;
> > -
> > -	if (!so->so_is_open_owner)
> > -		return;
> > -	oo = openowner(so);
> > -	s = oo->oo_last_closed_stid;
> > -	if (!s)
> > -		return;
> > -	if (!(oo->oo_flags & NFS4_OO_PURGE_CLOSE)) {
> > -		/* Release the last_closed_stid on the next seqid bump: */
> > -		oo->oo_flags |= NFS4_OO_PURGE_CLOSE;
> > -		return;
> > -	}
> > -	oo->oo_flags &= ~NFS4_OO_PURGE_CLOSE;
> > -	release_last_closed_stateid(oo);
> > -}
> > -
> >  static void nfsd4_close_open_stateid(struct nfs4_ol_stateid *s)
> >  {
> >  	unhash_open_stateid(s);
> > @@ -3838,17 +3842,20 @@ nfsd4_close(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >  					&close->cl_stateid,
> >  					NFS4_OPEN_STID|NFS4_CLOSED_STID,
> >  					&stp, nn);
> > +	nfsd4_bump_seqid(cstate, status);
> >  	if (status)
> >  		goto out; 
> >  	oo = openowner(stp->st_stateowner);
> > -	status = nfs_ok;
> >  	update_stateid(&stp->st_stid.sc_stateid);
> >  	memcpy(&close->cl_stateid, &stp->st_stid.sc_stateid, sizeof(stateid_t));
> >  
> >  	nfsd4_close_open_stateid(stp);
> > -	release_last_closed_stateid(oo);
> > -	oo->oo_flags &= ~NFS4_OO_PURGE_CLOSE;
> > -	oo->oo_last_closed_stid = stp;
> > +
> > +	if (cstate->minorversion) {
> > +		unhash_stid(&stp->st_stid);
> > +		free_generic_stateid(stp);
> > +	} else
> > +		oo->oo_last_closed_stid = stp;
> >  
> >  	if (list_empty(&oo->oo_owner.so_stateids)) {
> >  		if (cstate->minorversion) {
> > @@ -4270,6 +4277,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >  out:
> >  	if (status && new_state)
> >  		release_lockowner(lock_sop);
> > +	nfsd4_bump_seqid(cstate, status);
> >  	if (!cstate->replay_owner)
> >  		nfs4_unlock_state();
> >  	if (file_lock)
> > @@ -4439,6 +4447,7 @@ nfsd4_locku(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >  	memcpy(&locku->lu_stateid, &stp->st_stid.sc_stateid, sizeof(stateid_t));
> >  
> >  out:
> > +	nfsd4_bump_seqid(cstate, status);
> >  	if (!cstate->replay_owner)
> >  		nfs4_unlock_state();
> >  	if (file_lock)
> > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> > index 700de01..a5e8a64 100644
> > --- a/fs/nfsd/nfs4xdr.c
> > +++ b/fs/nfsd/nfs4xdr.c
> > @@ -1701,28 +1701,6 @@ static void write_cinfo(__be32 **p, struct nfsd4_change_info *c)
> >  								\
> >  	save = resp->p;
> >  
> > -/*
> > - * Routine for encoding the result of a "seqid-mutating" NFSv4 operation.  This
> > - * is where sequence id's are incremented, and the replay cache is filled.
> > - * Note that we increment sequence id's here, at the last moment, so we're sure
> > - * we know whether the error to be returned is a sequence id mutating error.
> > - */
> > -
> > -static void encode_seqid_op_tail(struct nfsd4_compoundres *resp, __be32 *save, __be32 nfserr)
> > -{
> > -	struct nfs4_stateowner *stateowner = resp->cstate.replay_owner;
> > -
> > -	if (seqid_mutating_err(ntohl(nfserr)) && stateowner) {
> > -		stateowner->so_seqid++;
> > -		stateowner->so_replay.rp_status = nfserr;
> > -		stateowner->so_replay.rp_buflen =
> > -			  (char *)resp->p - (char *)save;
> > -		memcpy(stateowner->so_replay.rp_buf, save,
> > -			stateowner->so_replay.rp_buflen);
> > -		nfsd4_purge_closed_stateid(stateowner);
> > -	}
> > -}
> > -
> >  /* Encode as an array of strings the string given with components
> >   * separated @sep, escaped with esc_enter and esc_exit.
> >   */
> > @@ -2667,7 +2645,6 @@ nfsd4_encode_close(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_c
> >  	if (!nfserr)
> >  		nfsd4_encode_stateid(resp, &close->cl_stateid);
> >  
> > -	encode_seqid_op_tail(resp, save, nfserr);
> >  	return nfserr;
> >  }
> >  
> > @@ -2770,7 +2747,6 @@ nfsd4_encode_lock(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_lo
> >  	else if (nfserr == nfserr_denied)
> >  		nfsd4_encode_lock_denied(resp, &lock->lk_denied);
> >  
> > -	encode_seqid_op_tail(resp, save, nfserr);
> >  	return nfserr;
> >  }
> >  
> > @@ -2790,7 +2766,6 @@ nfsd4_encode_locku(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_l
> >  	if (!nfserr)
> >  		nfsd4_encode_stateid(resp, &locku->lu_stateid);
> >  
> > -	encode_seqid_op_tail(resp, save, nfserr);
> >  	return nfserr;
> >  }
> >  
> > @@ -2885,7 +2860,6 @@ nfsd4_encode_open(struct nfsd4_compoundres *resp, __be32 nfserr, struct nfsd4_op
> >  	}
> >  	/* XXX save filehandle here */
> >  out:
> > -	encode_seqid_op_tail(resp, save, nfserr);
> >  	return nfserr;
> >  }
> >  
> > @@ -2897,7 +2871,6 @@ nfsd4_encode_open_confirm(struct nfsd4_compoundres *resp, __be32 nfserr, struct
> >  	if (!nfserr)
> >  		nfsd4_encode_stateid(resp, &oc->oc_resp_stateid);
> >  
> > -	encode_seqid_op_tail(resp, save, nfserr);
> >  	return nfserr;
> >  }
> >  
> > @@ -2909,7 +2882,6 @@ nfsd4_encode_open_downgrade(struct nfsd4_compoundres *resp, __be32 nfserr, struc
> >  	if (!nfserr)
> >  		nfsd4_encode_stateid(resp, &od->od_stateid);
> >  
> > -	encode_seqid_op_tail(resp, save, nfserr);
> >  	return nfserr;
> >  }
> >  
> > @@ -3567,6 +3539,7 @@ __be32 nfsd4_check_resp_size(struct nfsd4_compoundres *resp, u32 pad)
> >  void
> >  nfsd4_encode_operation(struct nfsd4_compoundres *resp, struct nfsd4_op *op)
> >  {
> > +	struct nfs4_stateowner *so = resp->cstate.replay_owner;
> >  	__be32 *statp;
> >  	__be32 *p;
> >  
> > @@ -3583,6 +3556,11 @@ nfsd4_encode_operation(struct nfsd4_compoundres *resp, struct nfsd4_op *op)
> >  	/* nfsd4_check_drc_limit guarantees enough room for error status */
> >  	if (!op->status)
> >  		op->status = nfsd4_check_resp_size(resp, 0);
> > +	if (so) {
> > +		so->so_replay.rp_status = op->status;
> > +		so->so_replay.rp_buflen = (char *)resp->p - (char *)(statp+1);
> > +		memcpy(so->so_replay.rp_buf, statp+1, so->so_replay.rp_buflen);
> > +	}
> >  status:
> >  	/*
> >  	 * Note: We write the status directly, instead of using WRITE32(),
> > diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> > index 7674bc8..13ec485 100644
> > --- a/fs/nfsd/state.h
> > +++ b/fs/nfsd/state.h
> > @@ -355,7 +355,6 @@ struct nfs4_openowner {
> >  	struct nfs4_ol_stateid *oo_last_closed_stid;
> >  	time_t			oo_time; /* time of placement on so_close_lru */
> >  #define NFS4_OO_CONFIRMED   1
> > -#define NFS4_OO_PURGE_CLOSE 2
> >  #define NFS4_OO_NEW         4
> >  	unsigned char		oo_flags;
> >  };
> > @@ -363,7 +362,7 @@ struct nfs4_openowner {
> >  struct nfs4_lockowner {
> >  	struct nfs4_stateowner	lo_owner; /* must be first element */
> >  	struct list_head	lo_owner_ino_hash; /* hash by owner,file */
> > -	struct list_head        lo_perstateid; /* for lockowners only */
> > +	struct list_head        lo_perstateid;
> >  	struct list_head	lo_list; /* for temporary uses */
> >  };
> >  
> > @@ -477,7 +476,6 @@ extern struct nfs4_client_reclaim *nfs4_client_to_reclaim(const char *name,
> >  							struct nfsd_net *nn);
> >  extern bool nfs4_has_reclaimed_state(const char *name, struct nfsd_net *nn);
> >  extern void put_client_renew(struct nfs4_client *clp);
> > -extern void nfsd4_purge_closed_stateid(struct nfs4_stateowner *);
> >  
> >  /* nfs4recover operations */
> >  extern int nfsd4_client_tracking_init(struct net *net);
> > diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> > index 40e05e6..3b271d2 100644
> > --- a/fs/nfsd/xdr4.h
> > +++ b/fs/nfsd/xdr4.h
> > @@ -623,6 +623,7 @@ extern __be32 nfsd4_test_stateid(struct svc_rqst *rqstp,
> >  		struct nfsd4_compound_state *, struct nfsd4_test_stateid *test_stateid);
> >  extern __be32 nfsd4_free_stateid(struct svc_rqst *rqstp,
> >  		struct nfsd4_compound_state *, struct nfsd4_free_stateid *free_stateid);
> > +extern void nfsd4_bump_seqid(struct nfsd4_compound_state *, __be32 nfserr);
> >  #endif
> >  
> >  /*