On Thu, Apr 22, 2010 at 10:48:31AM -0400, J. Bruce Fields wrote:
> On Thu, Apr 22, 2010 at 10:32:23AM -0400, J. Bruce Fields wrote:
> > Enforce the rules about compound op ordering.
> >
> > Motivated by implementing RECLAIM_COMPLETE, for which the client is
> > implicit in the current session, so it is important to ensure a
> > succesful SEQUENCE proceeds the RECLAIM_COMPLETE.
>
> The other problem here is that while we have a reference count on the
> session itself preventing it from going away till the compound is done,
> I don't see what prevents the associated clientid from going away.
>
> To fix that, and to be more polite to 4.0 clients, I think we want to
> also add a client pointer to the compound_state structure, keep count of
> the number of compounds in progress which reference that client, and not
> start the client's expiry timer until we've encoded the reply to the
> compound.
Benny--I coded up a simple (possibly incorrect) implementation of this,
and then remembered that this was more or less what your
state-lock-reduction-prep patch series did. Do you have a more recent
version of those patches?
--b.
>
> One question there is whether it's really correct to assume that a
> single compound can reference only one client. (I don't think rfc 3530
> explicitly forbids a single compound referring to multiple clients. rfc
> 5661 explicitly allows it in the case of DESTROY_CLIENTID, though that's
> a special case.)
>
> --b.
>
> >
> > Signed-off-by: J. Bruce Fields <bfields@xxxxxxxxxxxxxx>
> > ---
> > fs/nfsd/nfs4proc.c | 44 ++++++++++++++++++++++++++++++--------------
> > fs/nfsd/nfs4state.c | 13 +++++++++++++
> > 2 files changed, 43 insertions(+), 14 deletions(-)
> >
> > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > index 37514c4..e147dbc 100644
> > --- a/fs/nfsd/nfs4proc.c
> > +++ b/fs/nfsd/nfs4proc.c
> > @@ -968,20 +968,36 @@ static struct nfsd4_operation nfsd4_ops[];
> > static const char *nfsd4_op_name(unsigned opnum);
> >
> > /*
> > - * Enforce NFSv4.1 COMPOUND ordering rules.
> > + * Enforce NFSv4.1 COMPOUND ordering rules:
> > *
> > - * TODO:
> > - * - enforce NFS4ERR_NOT_ONLY_OP,
> > - * - DESTROY_SESSION MUST be the final operation in the COMPOUND request.
> > + * Also note, enforced elsewhere:
> > + * - SEQUENCE other than as first op results in
> > + * NFS4ERR_SEQUENCE_POS. (Enforced in nfsd4_sequence().)
> > + * - BIND_CONN_TO_SESSION must be the only op in its compound
> > + * (Will be enforced in nfsd4_bind_conn_to_session().)
> > + * - DESTROY_SESSION must be the final operation in a compound, if
> > + * sessionid's in SEQUENCE and DESTROY_SESSION are the same.
> > + * (Enforced in nfsd4_destroy_session().)
> > */
> > -static bool nfs41_op_ordering_ok(struct nfsd4_compoundargs *args)
> > +static __be32 nfs41_check_op_ordering(struct nfsd4_compoundargs *args)
> > {
> > - if (args->minorversion && args->opcnt > 0) {
> > - struct nfsd4_op *op = &args->ops[0];
> > - return (op->status == nfserr_op_illegal) ||
> > - (nfsd4_ops[op->opnum].op_flags & ALLOWED_AS_FIRST_OP);
> > - }
> > - return true;
> > + struct nfsd4_op *op = &args->ops[0];
> > +
> > + /* These ordering requirements don't apply to NFSv4.0: */
> > + if (args->minorversion == 0)
> > + return nfs_ok;
> > + /* This is weird, but OK, not our problem: */
> > + if (args->opcnt == 0)
> > + return nfs_ok;
> > + if (op->status == nfserr_op_illegal)
> > + return nfs_ok;
> > + if (!(nfsd4_ops[op->opnum].op_flags & ALLOWED_AS_FIRST_OP))
> > + return nfserr_op_not_in_session;
> > + if (op->opnum == OP_SEQUENCE)
> > + return nfs_ok;
> > + if (args->opcnt != 1)
> > + return nfserr_not_only_op;
> > + return nfs_ok;
> > }
> >
> > /*
> > @@ -1023,13 +1039,13 @@ nfsd4_proc_compound(struct svc_rqst *rqstp,
> > if (args->minorversion > nfsd_supported_minorversion)
> > goto out;
> >
> > - if (!nfs41_op_ordering_ok(args)) {
> > + status = nfs41_check_op_ordering(args);
> > + if (status) {
> > op = &args->ops[0];
> > - op->status = nfserr_sequence_pos;
> > + op->status = status;
> > goto encode_op;
> > }
> >
> > - status = nfs_ok;
> > while (!status && resp->opcnt < args->opcnt) {
> > op = &args->ops[resp->opcnt++];
> >
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 5051ade..e444829 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -1365,6 +1365,14 @@ out:
> > return status;
> > }
> >
> > +static bool nfsd4_last_compound_op(struct svc_rqst *rqstp)
> > +{
> > + struct nfsd4_compoundres *resp = rqstp->rq_resp;
> > + struct nfsd4_compoundargs *argp = rqstp->rq_argp;
> > +
> > + return argp->opcnt == resp->opcnt;
> > +}
> > +
> > __be32
> > nfsd4_destroy_session(struct svc_rqst *r,
> > struct nfsd4_compound_state *cstate,
> > @@ -1380,6 +1388,11 @@ nfsd4_destroy_session(struct svc_rqst *r,
> > * - Do we need to clear any callback info from previous session?
> > */
> >
> > + if (!memcmp(&sessionid->sessionid, &cstate->session->se_sessionid,
> > + sizeof(struct nfs4_sessionid))) {
> > + if (!nfsd4_last_compound_op(r))
> > + return nfserr_not_only_op;
> > + }
> > dump_sessionid(__func__, &sessionid->sessionid);
> > spin_lock(&sessionid_lock);
> > ses = find_in_sessionid_hashtbl(&sessionid->sessionid);
> > --
> > 1.6.3.3
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
