MIME-Version: 1.0
Reply-To: tigran.mkrtchyan@desy.de
In-Reply-To: <4EE61BE4.9060808@tonian.com>
References: <1323621708-25138-1-git-send-email-tigran.mkrtchyan@desy.de>
	<1323621708-25138-2-git-send-email-tigran.mkrtchyan@desy.de>
	<4EE61BE4.9060808@tonian.com>
Date: Mon, 12 Dec 2011 17:42:31 +0100
Message-ID: <CAGue13qbq2v8dbQaOPyb1DDfWdO3T9CAD8gkQRg3OpwkRYCohw@mail.gmail.com>
Subject: Re: [PATH v3 1/5] nfsd41: handle current stateid in open and close
From: Tigran Mkrtchyan <tigran.mkrtchyan@desy.de>
To: Benny Halevy <bhalevy@tonian.com>
Cc: Tigran Mkrtchyan <kofemann@googlemail.com>, linux-nfs@vger.kernel.org,
        Tigran Mkrtchyan <kofemann@gmail.com>
Content-Type: text/plain; charset=UTF-8
Sender: linux-nfs-owner@vger.kernel.org

On Mon, Dec 12, 2011 at 4:21 PM, Benny Halevy <bhalevy@tonian.com> wrote:
> On 2011-12-11 18:41, Tigran Mkrtchyan wrote:
>> From: Tigran Mkrtchyan <kofemann@gmail.com>
>>
>>
>> Signed-off-by: Tigran Mkrtchyan <kofemann@gmail.com>
>> ---
>>  fs/nfsd/current_stateid.h |   11 ++++++++++
>>  fs/nfsd/nfs4proc.c        |   47 ++++++++++++++++++++++++++++++++++++++------
>>  fs/nfsd/nfs4state.c       |   36 ++++++++++++++++++++++++++++++++++
>>  fs/nfsd/xdr4.h            |    2 +
>>  4 files changed, 89 insertions(+), 7 deletions(-)
>>  create mode 100644 fs/nfsd/current_stateid.h
>>
>> diff --git a/fs/nfsd/current_stateid.h b/fs/nfsd/current_stateid.h
>> new file mode 100644
>> index 0000000..a83dd50
>> --- /dev/null
>> +++ b/fs/nfsd/current_stateid.h
>> @@ -0,0 +1,11 @@
>> +#ifndef _NFSD4_CURRENT_STATE_H
>> +#define _NFSD4_CURRENT_STATE_H
>> +
>> +#include "state.h"
>> +#include "xdr4.h"
>> +
>> +extern void nfsd4_set_openstateid(struct nfsd4_compound_state *, struct nfsd4_open *);
>> +extern void nfsd4_get_closestateid(struct nfsd4_compound_state *, struct nfsd4_close *);
>> +extern void nfsd4_set_closestateid(struct nfsd4_compound_state *, struct nfsd4_close *);
>> +
>> +#endif   /* _NFSD4_CURRENT_STATE_H */
>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>> index fa38336..4b8d81a 100644
>> --- a/fs/nfsd/nfs4proc.c
>> +++ b/fs/nfsd/nfs4proc.c
>> @@ -39,6 +39,7 @@
>>  #include "cache.h"
>>  #include "xdr4.h"
>>  #include "vfs.h"
>> +#include "current_stateid.h"
>>
>>  #define NFSDDBG_FACILITY             NFSDDBG_PROC
>>
>> @@ -556,7 +557,6 @@ nfsd4_create(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>>                                    create->cr_name, create->cr_namelen,
>>                                    &create->cr_iattr, S_IFSOCK, 0, &resfh);
>>               break;
>> -
>
> nit: I assume this cosmetic hunk can be dropped :)
>
>>       case NF4FIFO:
>>               status = nfsd_create(rqstp, &cstate->current_fh,
>>                                    create->cr_name, create->cr_namelen,
>> @@ -1001,6 +1001,8 @@ static inline void nfsd4_increment_op_stats(u32 opnum)
>>  typedef __be32(*nfsd4op_func)(struct svc_rqst *, struct nfsd4_compound_state *,
>>                             void *);
>>  typedef u32(*nfsd4op_rsize)(struct svc_rqst *, struct nfsd4_op *op);
>> +typedef void(*stateid_setter)(struct nfsd4_compound_state *, void *);
>> +typedef void(*stateid_getter)(struct nfsd4_compound_state *, void *);
>>
>>  enum nfsd4_op_flags {
>>       ALLOWED_WITHOUT_FH = 1 << 0,    /* No current filehandle required */
>> @@ -1026,6 +1028,9 @@ enum nfsd4_op_flags {
>>        * the v4.0 case).
>>        */
>>       OP_CACHEME = 1 << 6,
>> +     CONSUMES_CURRENT_STATEID = 1 << 7,
>> +     PROVIDES_CURRENT_STATEID = 1 << 8,
>> +     CLEARS_CURRENT_STATEID = 1 << 9,
>>  };
>>
>>  struct nfsd4_operation {
>> @@ -1034,6 +1039,8 @@ struct nfsd4_operation {
>>       char *op_name;
>>       /* Try to get response size before operation */
>>       nfsd4op_rsize op_rsize_bop;
>> +     stateid_setter op_get_currentstateid;
>> +     stateid_getter op_set_currentstateid;
>>  };
>>
>>  static struct nfsd4_operation nfsd4_ops[];
>> @@ -1116,6 +1123,14 @@ static bool need_wrongsec_check(struct svc_rqst *rqstp)
>>       return !(nextd->op_flags & OP_HANDLES_WRONGSEC);
>>  }
>>
>> +static void
>> +nfsd4_clear_currentstateid(struct nfsd4_compound_state *cstate)
>> +{
>> +     if (cstate->has_stateid) {
>> +             memset(&cstate->current_stateid, 0, sizeof(stateid_t));
>> +             cstate->has_stateid = false;
>> +     }
>
> I'm a bit confused. In PATCH 0/5 you wrote:
> "struct nfsd4_compound_state contains pointer to current stateid"
> maybe it changes later in another patch. I'll be patient and keep reading ;-)
>
> Also, you use both a belt and suspenders.
> Instead of has_stateid I think you can just use the value of the
> invalid stateid and all zeroes is a valid, special stateid.
>
>> +}
>>  /*
>>   * COMPOUND call.
>>   */
>> @@ -1196,6 +1211,9 @@ nfsd4_proc_compound(struct svc_rqst *rqstp,
>>
>>               opdesc = OPDESC(op);
>>
>> +             BUG_ON(opdesc->op_flags & PROVIDES_CURRENT_STATEID &&
>> +                     opdesc->op_flags & CLEARS_CURRENT_STATEID);
>> +
>
> If so, couldn't the op just use nfsd4_clear_currentstateid()
> as its op_set_currentstateid?
>
>>               if (!cstate->current_fh.fh_dentry) {
>>                       if (!(opdesc->op_flags & ALLOWED_WITHOUT_FH)) {
>>                               op->status = nfserr_nofilehandle;
>> @@ -1216,13 +1234,25 @@ nfsd4_proc_compound(struct svc_rqst *rqstp,
>>               if (op->status)
>>                       goto encode_op;
>>
>> -             if (opdesc->op_func)
>> +             if (opdesc->op_func) {
>> +                     if (opdesc->op_flags & CONSUMES_CURRENT_STATEID)
>> +                             opdesc->op_get_currentstateid(cstate, &op->u);
>>                       op->status = opdesc->op_func(rqstp, cstate, &op->u);
>> -             else
>> +             } else
>>                       BUG_ON(op->status == nfs_ok);
>>
>> -             if (!op->status && need_wrongsec_check(rqstp))
>> -                     op->status = check_nfsd_access(cstate->current_fh.fh_export, rqstp);
>> +             if (!op->status) {
>> +                     if (opdesc->op_flags & PROVIDES_CURRENT_STATEID) {
>> +                             opdesc->op_set_currentstateid(cstate, &op->u);
>> +                             cstate->has_stateid = true;
>> +                     }
>> +
>> +                     if (opdesc->op_flags & CLEARS_CURRENT_STATEID)
>> +                             nfsd4_clear_currentstateid(cstate);
>> +
>> +                     if (need_wrongsec_check(rqstp))
>> +                             op->status = check_nfsd_access(cstate->current_fh.fh_export, rqstp);
>
> Hmmm, if check_nfsd_access returns with a failure there's no point
> in setting the current_stateid, is it?
>
>> +             }
>>
>>  encode_op:
>>               /* Only from SEQUENCE */
>> @@ -1411,9 +1441,11 @@ static struct nfsd4_operation nfsd4_ops[] = {
>>       },
>>       [OP_CLOSE] = {
>>               .op_func = (nfsd4op_func)nfsd4_close,
>> -             .op_flags = OP_MODIFIES_SOMETHING,
>> +             .op_flags = OP_MODIFIES_SOMETHING | CONSUMES_CURRENT_STATEID,
>
> Looks like this also requires PROVIDES_CURRENT_STATEID
> (or just ditch the flag so there can't be disagreement between the two :)

this is corrected in patch 2.

Tigran.
>
>>               .op_name = "OP_CLOSE",
>>               .op_rsize_bop = (nfsd4op_rsize)nfsd4_status_stateid_rsize,
>> +             .op_get_currentstateid = (stateid_getter)nfsd4_get_closestateid,
>> +             .op_set_currentstateid = (stateid_setter)nfsd4_set_closestateid,
>>       },
>>       [OP_COMMIT] = {
>>               .op_func = (nfsd4op_func)nfsd4_commit,
>> @@ -1481,9 +1513,10 @@ static struct nfsd4_operation nfsd4_ops[] = {
>>       },
>>       [OP_OPEN] = {
>>               .op_func = (nfsd4op_func)nfsd4_open,
>> -             .op_flags = OP_HANDLES_WRONGSEC | OP_MODIFIES_SOMETHING,
>> +             .op_flags = OP_HANDLES_WRONGSEC | OP_MODIFIES_SOMETHING | PROVIDES_CURRENT_STATEID,
>>               .op_name = "OP_OPEN",
>>               .op_rsize_bop = (nfsd4op_rsize)nfsd4_open_rsize,
>> +             .op_set_currentstateid = (stateid_setter)nfsd4_set_openstateid,
>>       },
>>       [OP_OPEN_CONFIRM] = {
>>               .op_func = (nfsd4op_func)nfsd4_open_confirm,
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 47e94e3..278e0af 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -51,10 +51,12 @@ time_t nfsd4_grace = 90;
>>  static time_t boot_time;
>>  static stateid_t zerostateid;             /* bits all 0 */
>>  static stateid_t onestateid;              /* bits all 1 */
>> +static stateid_t currentstateid;         /* other all 0, seqid 1 */
>>  static u64 current_sessionid = 1;
>>
>>  #define ZERO_STATEID(stateid) (!memcmp((stateid), &zerostateid, sizeof(stateid_t)))
>>  #define ONE_STATEID(stateid)  (!memcmp((stateid), &onestateid, sizeof(stateid_t)))
>> +#define CURRENT_STATEID(stateid) (!memcmp((stateid), &currentstateid, sizeof(stateid_t)))
>>
>>  /* forward declarations */
>>  static int check_for_locks(struct nfs4_file *filp, struct nfs4_lockowner *lowner);
>> @@ -4423,6 +4425,8 @@ nfs4_state_init(void)
>>               INIT_LIST_HEAD(&lock_ownerstr_hashtbl[i]);
>>       }
>>       memset(&onestateid, ~0, sizeof(stateid_t));
>> +     /* seqid 1, other all 0 */
>> +     currentstateid.si_generation = 1;
>
>
>>       INIT_LIST_HEAD(&close_lru);
>>       INIT_LIST_HEAD(&client_lru);
>>       INIT_LIST_HEAD(&del_recall_lru);
>> @@ -4545,3 +4549,35 @@ nfs4_state_shutdown(void)
>>       nfs4_unlock_state();
>>       nfsd4_destroy_callback_queue();
>>  }
>> +
>> +static void
>> +get_stateid(struct nfsd4_compound_state *cstate, stateid_t *stateid)
>> +{
>> +     if (cstate->minorversion && CURRENT_STATEID(stateid))
>> +             memcpy(stateid, &cstate->current_stateid, sizeof(stateid_t));
>> +}
>> +
>> +static void
>> +put_stateid(struct nfsd4_compound_state *cstate, stateid_t *stateid)
>> +{
>> +     if (cstate->minorversion)
>> +             memcpy(&cstate->current_stateid, stateid, sizeof(stateid_t));
>
> you can also set has_stateid to true here...
> (If you keep it. I'm not convinced yet it's really needed)
>
>> +}
>> +
>> +void
>> +nfsd4_set_openstateid(struct nfsd4_compound_state *cstate, struct nfsd4_open *open)
>> +{
>> +     put_stateid(cstate, &open->op_stateid);
>> +}
>> +
>> +void
>> +nfsd4_get_closestateid(struct nfsd4_compound_state *cstate, struct nfsd4_close *close)
>> +{
>> +     get_stateid(cstate, &close->cl_stateid);
>> +}
>> +
>> +void
>> +nfsd4_set_closestateid(struct nfsd4_compound_state *cstate, struct nfsd4_close *close)
>> +{
>> +     get_stateid(cstate, &close->cl_stateid);
>> +}
>> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
>> index 2364747..b8435d20 100644
>> --- a/fs/nfsd/xdr4.h
>> +++ b/fs/nfsd/xdr4.h
>> @@ -54,6 +54,8 @@ struct nfsd4_compound_state {
>>       size_t                  iovlen;
>>       u32                     minorversion;
>>       u32                     status;
>> +     stateid_t               current_stateid;
>> +     bool    has_stateid;
>>  };
>>
>>  static inline bool nfsd4_has_session(struct nfsd4_compound_state *cs)
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html