Bruce, Oddly enough I didn't receive the patch you're commenting on into
my inbox. It already happened before on this list and I've no idea what
could have went wrong. (I also have a gmail account subscribed on this list
and I can't find it there, even in the spam folder :-/)
comments below...
On Apr. 24, 2010, 0:24 +0300, "J. Bruce Fields" <bfields@xxxxxxxxxxxxxx> wrote:
> 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?
Yup. though untested with latest bits.
I'll resend it as a reply to this message.
>
> --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.)
I don't think so, if the compound starts with SEQUENCE there's no way
to replace the session/clientid for the compound. All operations
not associated with a sessions must be the only ops in the compound
(with the exception of CREATE_SESSION that can be preceded with a
SEQUENCE but it doesn't change the client id not the effective session
for the compound)
Benny
>>
>> --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
