From: Tigran Mkrtchyan <[email protected]>
Signed-off-by: Tigran Mkrtchyan <[email protected]>
---
fs/nfsd/current_stateid.h | 44 ++++++++++++++++++++++++++++++++++++++++++++
fs/nfsd/nfs4proc.c | 22 ++++++++++++++++++----
fs/nfsd/nfs4state.c | 17 +++++++++++++++++
fs/nfsd/xdr4.h | 1 +
4 files changed, 80 insertions(+), 4 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..f1ad6e9
--- /dev/null
+++ b/fs/nfsd/current_stateid.h
@@ -0,0 +1,44 @@
+/*
+ * Copyright (c) 2001 The Regents of the University of Michigan.
+ * All rights reserved.
+ *
+ * Kendrick Smith <[email protected]>
+ * Andy Adamson <[email protected]>
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ *
+ * 1. Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution.
+ * 3. Neither the name of the University nor the names of its
+ * contributors may be used to endorse or promote products derived
+ * from this software without specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED ``AS IS'' AND ANY EXPRESS OR IMPLIED
+ * WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
+ * DISCLAIMED. IN NO EVENT SHALL THE REGENTS OR CONTRIBUTORS BE LIABLE
+ * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
+ * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
+ * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
+ * NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
+ * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ *
+ */
+
+#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 *cstate, struct nfsd4_open *open);
+extern void nfsd4_get_closestateid(struct nfsd4_compound_state *cstate, struct nfsd4_close *close);
+
+#endif /* _NFSD4_CURRENT_STATE_H */
diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index fa38336..bfed3cb 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
@@ -1001,6 +1002,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 +1029,8 @@ enum nfsd4_op_flags {
* the v4.0 case).
*/
OP_CACHEME = 1 << 6,
+ CONSUMES_CURRENT_STATEID = 1 << 7,
+ PROVIDES_CURRENT_STATEID = 1 << 8,
};
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[];
@@ -1216,11 +1223,16 @@ 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 && opdesc->op_flags & PROVIDES_CURRENT_STATEID)
+ opdesc->op_set_currentstateid(cstate, &op->u);
+
if (!op->status && need_wrongsec_check(rqstp))
op->status = check_nfsd_access(cstate->current_fh.fh_export, rqstp);
@@ -1411,9 +1423,10 @@ 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,
.op_name = "OP_CLOSE",
.op_rsize_bop = (nfsd4op_rsize)nfsd4_status_stateid_rsize,
+ .op_get_currentstateid = (stateid_getter)nfsd4_get_closestateid,
},
[OP_COMMIT] = {
.op_func = (nfsd4op_func)nfsd4_commit,
@@ -1481,9 +1494,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..9dc1de8 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), ¤tstateid, 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,16 @@ nfs4_state_shutdown(void)
nfs4_unlock_state();
nfsd4_destroy_callback_queue();
}
+
+void
+nfsd4_set_openstateid(struct nfsd4_compound_state *cstate, struct nfsd4_open *open)
+{
+ memcpy(&cstate->current_stateid, &open->op_stateid, sizeof(stateid_t));
+}
+
+void
+nfsd4_get_closestateid(struct nfsd4_compound_state *cstate, struct nfsd4_close *close)
+{
+ if (CURRENT_STATEID(&close->cl_stateid))
+ memcpy(&close->cl_stateid, &cstate->current_stateid, sizeof(stateid_t));
+}
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index 2364747..96f9966 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -54,6 +54,7 @@ struct nfsd4_compound_state {
size_t iovlen;
u32 minorversion;
u32 status;
+ stateid_t current_stateid;
};
static inline bool nfsd4_has_session(struct nfsd4_compound_state *cs)
--
1.7.7.3
On 2011-12-13 00:40, J. Bruce Fields wrote:
> On Thu, Dec 08, 2011 at 06:09:07PM +0200, Benny Halevy wrote:
>> On 2011-12-07 19:11, J. Bruce Fields wrote:
>>> On Wed, Dec 07, 2011 at 03:17:48PM +0200, Benny Halevy wrote:
>>>> On 2011-12-07 01:08, J. Bruce Fields wrote:
>>>>> It's a little verbose, but yes it does nicely collect the current
>>>>> stateid getting/setting into one place. Benny, is the more or less what
>>>>> you were thinking of?
>>>>
>>>> Yes it is, though I also liked your direction of just using the current
>>>> stateid for xdr decoding and encoding.
>>>
>>> I'd still have a slight preference for doing it that way, just because
>>> it would take fewer lines of code--but I can live with this too, so I'll
>>> leave the choice to Tigran's excellent taste....
>>>
>>>> { ~0, { { ~0, ~0 }, ~0 } } is ugly but compact :)
>>>>
>>>> And the following may be an overkill...
>>>> {
>>>> .si_generation = ~0,
>>>> .si_opaque = {
>>>> .so_clid = {
>>>> .cl_boot = ~0,
>>>> .cl_id = ~0,
>>>> },
>>>> .so_id = ~0
>>>> }
>>>> };
>>>
>>> Reminding myself of http://tools.ietf.org/html/rfc5661#section-8.2.3...
>>> "Stateid values whose "other" field is either all zeros or all ones are
>>> reserved."
>>>
>>> Maybe:
>>>
>>> #define all_ones {{~0,~0 },~0}
>>> #define all_zeroes {{ 0, 0 }, 0}
>>> const stateid_t one_stateid = {
>>> .si_generation = ~0,
>>> .si_opaque = all_ones
>>> };
>>
>> That looks like a good compromise to me.
>>
>>> const stateid_t current_stateid = {
>>> .si_generation = 1,
>>> .si_opaque = all_zeroes
>>> };
>>> ...
>>>
>>> Or you could ditch the all_zeroes; it's just there for some kind of
>>> symmetry.
>>
>> Yeah, static initialization to zero is guaranteed.
>
> OK, applying the following assuming no objections.--b.
Ack
Benny
>
> commit f32f3c2d3f09a586349ca9180885dc8741290fd9
> Author: J. Bruce Fields <[email protected]>
> Date: Mon Dec 12 15:00:35 2011 -0500
>
> nfsd4: initialize special stateid's at compile time
>
> Stateid's with "other" ("opaque") field all zeros or all ones are
> reserved. We define all_ones separately on the off chance there will be
> more such some day, though currently all the other special stateid's
> have zero other field.
>
> Signed-off-by: J. Bruce Fields <[email protected]>
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 6ab6779..213da7b 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -49,12 +49,20 @@
> time_t nfsd4_lease = 90; /* default lease time */
> time_t nfsd4_grace = 90;
> static time_t boot_time;
> -static stateid_t zerostateid; /* bits all 0 */
> -static stateid_t onestateid; /* bits all 1 */
> +
> +#define all_ones {{~0,~0},~0}
> +static const stateid_t one_stateid = {
> + .si_generation = ~0,
> + .si_opaque = all_ones,
> +};
> +static const stateid_t zero_stateid = {
> + /* all fields zero */
> +};
> +
> 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 ZERO_STATEID(stateid) (!memcmp((stateid), &zero_stateid, sizeof(stateid_t)))
> +#define ONE_STATEID(stateid) (!memcmp((stateid), &one_stateid, sizeof(stateid_t)))
>
> /* forward declarations */
> static int check_for_locks(struct nfs4_file *filp, struct nfs4_lockowner *lowner);
> @@ -4564,7 +4572,6 @@ nfs4_state_init(void)
> }
> for (i = 0; i < LOCKOWNER_INO_HASH_SIZE; i++)
> INIT_LIST_HEAD(&lockowner_ino_hashtbl[i]);
> - memset(&onestateid, ~0, sizeof(stateid_t));
> INIT_LIST_HEAD(&close_lru);
> INIT_LIST_HEAD(&client_lru);
> INIT_LIST_HEAD(&del_recall_lru);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Dec 08, 2011 at 06:09:07PM +0200, Benny Halevy wrote:
> On 2011-12-07 19:11, J. Bruce Fields wrote:
> > On Wed, Dec 07, 2011 at 03:17:48PM +0200, Benny Halevy wrote:
> >> On 2011-12-07 01:08, J. Bruce Fields wrote:
> >>> It's a little verbose, but yes it does nicely collect the current
> >>> stateid getting/setting into one place. Benny, is the more or less what
> >>> you were thinking of?
> >>
> >> Yes it is, though I also liked your direction of just using the current
> >> stateid for xdr decoding and encoding.
> >
> > I'd still have a slight preference for doing it that way, just because
> > it would take fewer lines of code--but I can live with this too, so I'll
> > leave the choice to Tigran's excellent taste....
> >
> >> { ~0, { { ~0, ~0 }, ~0 } } is ugly but compact :)
> >>
> >> And the following may be an overkill...
> >> {
> >> .si_generation = ~0,
> >> .si_opaque = {
> >> .so_clid = {
> >> .cl_boot = ~0,
> >> .cl_id = ~0,
> >> },
> >> .so_id = ~0
> >> }
> >> };
> >
> > Reminding myself of http://tools.ietf.org/html/rfc5661#section-8.2.3...
> > "Stateid values whose "other" field is either all zeros or all ones are
> > reserved."
> >
> > Maybe:
> >
> > #define all_ones {{~0,~0 },~0}
> > #define all_zeroes {{ 0, 0 }, 0}
> > const stateid_t one_stateid = {
> > .si_generation = ~0,
> > .si_opaque = all_ones
> > };
>
> That looks like a good compromise to me.
>
> > const stateid_t current_stateid = {
> > .si_generation = 1,
> > .si_opaque = all_zeroes
> > };
> > ...
> >
> > Or you could ditch the all_zeroes; it's just there for some kind of
> > symmetry.
>
> Yeah, static initialization to zero is guaranteed.
OK, applying the following assuming no objections.--b.
commit f32f3c2d3f09a586349ca9180885dc8741290fd9
Author: J. Bruce Fields <[email protected]>
Date: Mon Dec 12 15:00:35 2011 -0500
nfsd4: initialize special stateid's at compile time
Stateid's with "other" ("opaque") field all zeros or all ones are
reserved. We define all_ones separately on the off chance there will be
more such some day, though currently all the other special stateid's
have zero other field.
Signed-off-by: J. Bruce Fields <[email protected]>
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 6ab6779..213da7b 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -49,12 +49,20 @@
time_t nfsd4_lease = 90; /* default lease time */
time_t nfsd4_grace = 90;
static time_t boot_time;
-static stateid_t zerostateid; /* bits all 0 */
-static stateid_t onestateid; /* bits all 1 */
+
+#define all_ones {{~0,~0},~0}
+static const stateid_t one_stateid = {
+ .si_generation = ~0,
+ .si_opaque = all_ones,
+};
+static const stateid_t zero_stateid = {
+ /* all fields zero */
+};
+
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 ZERO_STATEID(stateid) (!memcmp((stateid), &zero_stateid, sizeof(stateid_t)))
+#define ONE_STATEID(stateid) (!memcmp((stateid), &one_stateid, sizeof(stateid_t)))
/* forward declarations */
static int check_for_locks(struct nfs4_file *filp, struct nfs4_lockowner *lowner);
@@ -4564,7 +4572,6 @@ nfs4_state_init(void)
}
for (i = 0; i < LOCKOWNER_INO_HASH_SIZE; i++)
INIT_LIST_HEAD(&lockowner_ino_hashtbl[i]);
- memset(&onestateid, ~0, sizeof(stateid_t));
INIT_LIST_HEAD(&close_lru);
INIT_LIST_HEAD(&client_lru);
INIT_LIST_HEAD(&del_recall_lru);
It's a little verbose, but yes it does nicely collect the current
stateid getting/setting into one place. Benny, is the more or less what
you were thinking of?
Nits inline below.
On Tue, Dec 06, 2011 at 10:44:07PM +0100, Tigran Mkrtchyan wrote:
> From: Tigran Mkrtchyan <[email protected]>
>
>
> Signed-off-by: Tigran Mkrtchyan <[email protected]>
> ---
> fs/nfsd/current_stateid.h | 44 ++++++++++++++++++++++++++++++++++++++++++++
> fs/nfsd/nfs4proc.c | 22 ++++++++++++++++++----
> fs/nfsd/nfs4state.c | 17 +++++++++++++++++
> fs/nfsd/xdr4.h | 1 +
> 4 files changed, 80 insertions(+), 4 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..f1ad6e9
> --- /dev/null
> +++ b/fs/nfsd/current_stateid.h
> @@ -0,0 +1,44 @@
> +/*
> + * Copyright (c) 2001 The Regents of the University of Michigan.
> + * All rights reserved.
> + *
> + * Kendrick Smith <[email protected]>
> + * Andy Adamson <[email protected]>
Kendrick and Andy have been busy! And they're both working at the
University again?
Anyway: on a header file containing only two function prototypes, I'd be
inclined to leave off any copyright notice unless someone's lawyers
absolutely insist on it....
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + *
> + * 1. Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in the
> + * documentation and/or other materials provided with the distribution.
> + * 3. Neither the name of the University nor the names of its
> + * contributors may be used to endorse or promote products derived
> + * from this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED ``AS IS'' AND ANY EXPRESS OR IMPLIED
> + * WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF
> + * MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> + * DISCLAIMED. IN NO EVENT SHALL THE REGENTS OR CONTRIBUTORS BE LIABLE
> + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
> + * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
> + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
> + * NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
> + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + *
> + */
> +
> +#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 *cstate, struct nfsd4_open *open);
> +extern void nfsd4_get_closestateid(struct nfsd4_compound_state *cstate, struct nfsd4_close *close);
> +
> +#endif /* _NFSD4_CURRENT_STATE_H */
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index fa38336..bfed3cb 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
>
> @@ -1001,6 +1002,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 +1029,8 @@ enum nfsd4_op_flags {
> * the v4.0 case).
> */
> OP_CACHEME = 1 << 6,
> + CONSUMES_CURRENT_STATEID = 1 << 7,
> + PROVIDES_CURRENT_STATEID = 1 << 8,
Or should we just use the fact that op_{get,put}_currentstateid will be
NULL whenever the respective flag would be set?
> };
>
> 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[];
> @@ -1216,11 +1223,16 @@ 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 && opdesc->op_flags & PROVIDES_CURRENT_STATEID)
> + opdesc->op_set_currentstateid(cstate, &op->u);
> +
> if (!op->status && need_wrongsec_check(rqstp))
> op->status = check_nfsd_access(cstate->current_fh.fh_export, rqstp);
>
> @@ -1411,9 +1423,10 @@ 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,
> .op_name = "OP_CLOSE",
> .op_rsize_bop = (nfsd4op_rsize)nfsd4_status_stateid_rsize,
> + .op_get_currentstateid = (stateid_getter)nfsd4_get_closestateid,
> },
> [OP_COMMIT] = {
> .op_func = (nfsd4op_func)nfsd4_commit,
> @@ -1481,9 +1494,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..9dc1de8 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), ¤tstateid, 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));
A project for another day, but the initialization of these constants has
always annoyed me--isn't there any compact way to define these at
compile-time?
--b.
> + /* 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,16 @@ nfs4_state_shutdown(void)
> nfs4_unlock_state();
> nfsd4_destroy_callback_queue();
> }
> +
> +void
> +nfsd4_set_openstateid(struct nfsd4_compound_state *cstate, struct nfsd4_open *open)
> +{
> + memcpy(&cstate->current_stateid, &open->op_stateid, sizeof(stateid_t));
> +}
> +
> +void
> +nfsd4_get_closestateid(struct nfsd4_compound_state *cstate, struct nfsd4_close *close)
> +{
> + if (CURRENT_STATEID(&close->cl_stateid))
> + memcpy(&close->cl_stateid, &cstate->current_stateid, sizeof(stateid_t));
> +}
> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
> index 2364747..96f9966 100644
> --- a/fs/nfsd/xdr4.h
> +++ b/fs/nfsd/xdr4.h
> @@ -54,6 +54,7 @@ struct nfsd4_compound_state {
> size_t iovlen;
> u32 minorversion;
> u32 status;
> + stateid_t current_stateid;
> };
>
> static inline bool nfsd4_has_session(struct nfsd4_compound_state *cs)
> --
> 1.7.7.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2011-12-07 01:08, J. Bruce Fields wrote:
> It's a little verbose, but yes it does nicely collect the current
> stateid getting/setting into one place. Benny, is the more or less what
> you were thinking of?
Yes it is, though I also liked your direction of just using the current
stateid for xdr decoding and encoding.
>
> Nits inline below.
>
> On Tue, Dec 06, 2011 at 10:44:07PM +0100, Tigran Mkrtchyan wrote:
>> From: Tigran Mkrtchyan <[email protected]>
>>
>>
>> Signed-off-by: Tigran Mkrtchyan <[email protected]>
>> ---
>> fs/nfsd/current_stateid.h | 44 ++++++++++++++++++++++++++++++++++++++++++++
>> fs/nfsd/nfs4proc.c | 22 ++++++++++++++++++----
>> fs/nfsd/nfs4state.c | 17 +++++++++++++++++
>> fs/nfsd/xdr4.h | 1 +
>> 4 files changed, 80 insertions(+), 4 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..f1ad6e9
>> --- /dev/null
>> +++ b/fs/nfsd/current_stateid.h
>> @@ -0,0 +1,44 @@
>> +/*
>> + * Copyright (c) 2001 The Regents of the University of Michigan.
>> + * All rights reserved.
>> + *
>> + * Kendrick Smith <[email protected]>
>> + * Andy Adamson <[email protected]>
>
> Kendrick and Andy have been busy! And they're both working at the
> University again?
>
> Anyway: on a header file containing only two function prototypes, I'd be
> inclined to leave off any copyright notice unless someone's lawyers
> absolutely insist on it....
>
>> + *
>> + * Redistribution and use in source and binary forms, with or without
>> + * modification, are permitted provided that the following conditions
>> + * are met:
>> + *
>> + * 1. Redistributions of source code must retain the above copyright
>> + * notice, this list of conditions and the following disclaimer.
>> + * 2. Redistributions in binary form must reproduce the above copyright
>> + * notice, this list of conditions and the following disclaimer in the
>> + * documentation and/or other materials provided with the distribution.
>> + * 3. Neither the name of the University nor the names of its
>> + * contributors may be used to endorse or promote products derived
>> + * from this software without specific prior written permission.
>> + *
>> + * THIS SOFTWARE IS PROVIDED ``AS IS'' AND ANY EXPRESS OR IMPLIED
>> + * WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF
>> + * MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
>> + * DISCLAIMED. IN NO EVENT SHALL THE REGENTS OR CONTRIBUTORS BE LIABLE
>> + * FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
>> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
>> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR
>> + * BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
>> + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
>> + * NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
>> + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>> + *
>> + */
>> +
>> +#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 *cstate, struct nfsd4_open *open);
>> +extern void nfsd4_get_closestateid(struct nfsd4_compound_state *cstate, struct nfsd4_close *close);
>> +
>> +#endif /* _NFSD4_CURRENT_STATE_H */
>> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
>> index fa38336..bfed3cb 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
>>
>> @@ -1001,6 +1002,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 +1029,8 @@ enum nfsd4_op_flags {
>> * the v4.0 case).
>> */
>> OP_CACHEME = 1 << 6,
>> + CONSUMES_CURRENT_STATEID = 1 << 7,
>> + PROVIDES_CURRENT_STATEID = 1 << 8,
>
> Or should we just use the fact that op_{get,put}_currentstateid will be
> NULL whenever the respective flag would be set?
>
~=
or we could use a no-op implementation of the method
for the respective ops.
>> };
>>
>> 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[];
>> @@ -1216,11 +1223,16 @@ 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 && opdesc->op_flags & PROVIDES_CURRENT_STATEID)
>> + opdesc->op_set_currentstateid(cstate, &op->u);
>> +
>> if (!op->status && need_wrongsec_check(rqstp))
>> op->status = check_nfsd_access(cstate->current_fh.fh_export, rqstp);
>>
>> @@ -1411,9 +1423,10 @@ 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,
>> .op_name = "OP_CLOSE",
>> .op_rsize_bop = (nfsd4op_rsize)nfsd4_status_stateid_rsize,
>> + .op_get_currentstateid = (stateid_getter)nfsd4_get_closestateid,
Even though CLOSE returns a stateid, this stateid is not useful to
the client and should be treated as deprecated. CLOSE "shuts down"
the state associated with all OPENs for the file by a single open-
owner. As noted above, CLOSE will either release all file-locking
state or return an error. Therefore, the stateid returned by CLOSE
is not useful for operations that follow. To help find any uses of
this stateid by clients, the server SHOULD return the invalid special
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
stateid (the "other" value is zero and the "seqid" field is
NFS4_UINT32_MAX, see Section 8.2.3).
Copying the returned stateid is crucial is the client is silly enough
to construct a COMPOUND that attempts to use the current_stateid after CLOSE.
>> },
>> [OP_COMMIT] = {
>> .op_func = (nfsd4op_func)nfsd4_commit,
>> @@ -1481,9 +1494,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..9dc1de8 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), ¤tstateid, 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));
>
> A project for another day, but the initialization of these constants has
> always annoyed me--isn't there any compact way to define these at
> compile-time?
{ ~0, { { ~0, ~0 }, ~0 } } is ugly but compact :)
And the following may be an overkill...
{
.si_generation = ~0,
.si_opaque = {
.so_clid = {
.cl_boot = ~0,
.cl_id = ~0,
},
.so_id = ~0
}
};
>
> --b.
>
>> + /* seqid 1, other all 0 */
>> + currentstateid.si_generation = 1;
should be cpu_to_be32(1), no?
>> INIT_LIST_HEAD(&close_lru);
>> INIT_LIST_HEAD(&client_lru);
>> INIT_LIST_HEAD(&del_recall_lru);
>> @@ -4545,3 +4549,16 @@ nfs4_state_shutdown(void)
>> nfs4_unlock_state();
>> nfsd4_destroy_callback_queue();
>> }
>> +
>> +void
>> +nfsd4_set_openstateid(struct nfsd4_compound_state *cstate, struct nfsd4_open *open)
>> +{
>> + memcpy(&cstate->current_stateid, &open->op_stateid, sizeof(stateid_t));
>> +}
>> +
>> +void
>> +nfsd4_get_closestateid(struct nfsd4_compound_state *cstate, struct nfsd4_close *close)
>> +{
>> + if (CURRENT_STATEID(&close->cl_stateid))
>> + memcpy(&close->cl_stateid, &cstate->current_stateid, sizeof(stateid_t));
So, if CLOSE if the first operation to use the current stateid
and no other operation has already set it, what do we get?
the zero stateid?
I believe we need to initialize the current_stateid for the compound
with the invalid stateid:
(the "other" value is zero and the "seqid" field is
NFS4_UINT32_MAX, see Section 8.2.3)
Benny
>> +}
>> diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
>> index 2364747..96f9966 100644
>> --- a/fs/nfsd/xdr4.h
>> +++ b/fs/nfsd/xdr4.h
>> @@ -54,6 +54,7 @@ struct nfsd4_compound_state {
>> size_t iovlen;
>> u32 minorversion;
>> u32 status;
>> + stateid_t current_stateid;
>> };
>>
>> static inline bool nfsd4_has_session(struct nfsd4_compound_state *cs)
>> --
>> 1.7.7.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to [email protected]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2011-12-07 19:11, J. Bruce Fields wrote:
> On Wed, Dec 07, 2011 at 03:17:48PM +0200, Benny Halevy wrote:
>> On 2011-12-07 01:08, J. Bruce Fields wrote:
>>> It's a little verbose, but yes it does nicely collect the current
>>> stateid getting/setting into one place. Benny, is the more or less what
>>> you were thinking of?
>>
>> Yes it is, though I also liked your direction of just using the current
>> stateid for xdr decoding and encoding.
>
> I'd still have a slight preference for doing it that way, just because
> it would take fewer lines of code--but I can live with this too, so I'll
> leave the choice to Tigran's excellent taste....
>
>> { ~0, { { ~0, ~0 }, ~0 } } is ugly but compact :)
>>
>> And the following may be an overkill...
>> {
>> .si_generation = ~0,
>> .si_opaque = {
>> .so_clid = {
>> .cl_boot = ~0,
>> .cl_id = ~0,
>> },
>> .so_id = ~0
>> }
>> };
>
> Reminding myself of http://tools.ietf.org/html/rfc5661#section-8.2.3...
> "Stateid values whose "other" field is either all zeros or all ones are
> reserved."
>
> Maybe:
>
> #define all_ones {{~0,~0 },~0}
> #define all_zeroes {{ 0, 0 }, 0}
> const stateid_t one_stateid = {
> .si_generation = ~0,
> .si_opaque = all_ones
> };
That looks like a good compromise to me.
> const stateid_t current_stateid = {
> .si_generation = 1,
> .si_opaque = all_zeroes
> };
> ...
>
> Or you could ditch the all_zeroes; it's just there for some kind of
> symmetry.
Yeah, static initialization to zero is guaranteed.
Benny
>
> --b.
On Wed, Dec 7, 2011 at 18:11, J. Bruce Fields <[email protected]> wrote:
> On Wed, Dec 07, 2011 at 03:17:48PM +0200, Benny Halevy wrote:
>> On 2011-12-07 01:08, J. Bruce Fields wrote:
>> > It's a little verbose, but yes it does nicely collect the current
>> > stateid getting/setting into one place. Benny, is the more or less what
>> > you were thinking of?
>>
>> Yes it is, though I also liked your direction of just using the current
>> stateid for xdr decoding and encoding.
>
> I'd still have a slight preference for doing it that way, just because
> it would take fewer lines of code--but I can live with this too, so I'll
> leave the choice to Tigran's excellent taste....
I will prefer current approach as it makes less disaster than
propagating the knowledge about stateid handling deep into xdr layer.
Tigran.
>
>> { ~0, { { ~0, ~0 }, ~0 } } is ugly but compact :)
>>
>> And the following may be an overkill...
>> {
>> .si_generation = ~0,
>> .si_opaque = {
>> .so_clid = {
>> .cl_boot = ~0,
>> .cl_id = ~0,
>> },
>> .so_id = ~0
>> }
>> };
>
> Reminding myself of http://tools.ietf.org/html/rfc5661#section-8.2.3...
> "Stateid values whose "other" field is either all zeros or all ones are
> reserved."
>
> Maybe:
>
> #define all_ones {{~0,~0 },~0}
> #define all_zeroes {{ 0, 0 }, 0}
> const stateid_t one_stateid = {
> .si_generation = ~0,
> .si_opaque = all_ones
> };
> const stateid_t current_stateid = {
> .si_generation = 1,
> .si_opaque = all_zeroes
> };
> ...
>
> Or you could ditch the all_zeroes; it's just there for some kind of
> symmetry.
>
> --b.
On Wed, Dec 07, 2011 at 03:17:48PM +0200, Benny Halevy wrote:
> On 2011-12-07 01:08, J. Bruce Fields wrote:
> > It's a little verbose, but yes it does nicely collect the current
> > stateid getting/setting into one place. Benny, is the more or less what
> > you were thinking of?
>
> Yes it is, though I also liked your direction of just using the current
> stateid for xdr decoding and encoding.
I'd still have a slight preference for doing it that way, just because
it would take fewer lines of code--but I can live with this too, so I'll
leave the choice to Tigran's excellent taste....
> { ~0, { { ~0, ~0 }, ~0 } } is ugly but compact :)
>
> And the following may be an overkill...
> {
> .si_generation = ~0,
> .si_opaque = {
> .so_clid = {
> .cl_boot = ~0,
> .cl_id = ~0,
> },
> .so_id = ~0
> }
> };
Reminding myself of http://tools.ietf.org/html/rfc5661#section-8.2.3...
"Stateid values whose "other" field is either all zeros or all ones are
reserved."
Maybe:
#define all_ones {{~0,~0 },~0}
#define all_zeroes {{ 0, 0 }, 0}
const stateid_t one_stateid = {
.si_generation = ~0,
.si_opaque = all_ones
};
const stateid_t current_stateid = {
.si_generation = 1,
.si_opaque = all_zeroes
};
...
Or you could ditch the all_zeroes; it's just there for some kind of
symmetry.
--b.
On Wed, Dec 07, 2011 at 03:17:48PM +0200, Benny Halevy wrote:
> I believe we need to initialize the current_stateid for the compound
> with the invalid stateid:
> (the "other" value is zero and the "seqid" field is
> NFS4_UINT32_MAX, see Section 8.2.3)
Sounds like a good idea.