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

From: J. Bruce Fields
Date: Mon Apr 08 2013 - 09:56:37 EST

Next message: Hector Palacios: "Re: [PATCH RFC] ARM: dts: mxs: leave card detect out of common mmcpins config"
Previous message: Yanchuan Nian: "Re: [PATCH v2] nfsd: fix bug on nfs4 stateid deallocation"
In reply to: Yanchuan Nian: "Re: [PATCH v2] nfsd: fix bug on nfs4 stateid deallocation"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxxx>
> > 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@xxxxxxxxxx>
> >
> > 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
> >
> > /*
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Next message: Hector Palacios: "Re: [PATCH RFC] ARM: dts: mxs: leave card detect out of common mmcpins config"
Previous message: Yanchuan Nian: "Re: [PATCH v2] nfsd: fix bug on nfs4 stateid deallocation"
In reply to: Yanchuan Nian: "Re: [PATCH v2] nfsd: fix bug on nfs4 stateid deallocation"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]