Includes:
. CB_GETATTR proc for nfs4_cb_procedures[]
. XDR encoding and decoding function for CB_GETATTR request/reply
. add nfs4_cb_fattr to nfs4_delegation for sending CB_GETATTR
and store file attributes from client's reply.
Signed-off-by: Dai Ngo <[email protected]>
---
fs/nfsd/nfs4callback.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++++
fs/nfsd/state.h | 17 +++++++
fs/nfsd/xdr4cb.h | 19 ++++++++
3 files changed, 153 insertions(+)
diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
index 4039ffcf90ba..ca3d72ef5fbc 100644
--- a/fs/nfsd/nfs4callback.c
+++ b/fs/nfsd/nfs4callback.c
@@ -87,6 +87,43 @@ static void encode_bitmap4(struct xdr_stream *xdr, const __u32 *bitmap,
WARN_ON_ONCE(xdr_stream_encode_uint32_array(xdr, bitmap, len) < 0);
}
+static void decode_bitmap4(struct xdr_stream *xdr, __u32 *bitmap,
+ size_t len)
+{
+ WARN_ON_ONCE(xdr_stream_decode_uint32_array(xdr, bitmap, len) < 0);
+}
+
+static int decode_attr_length(struct xdr_stream *xdr, uint32_t *attrlen)
+{
+ __be32 *p;
+
+ p = xdr_inline_decode(xdr, 4);
+ if (unlikely(!p))
+ return -EIO;
+ *attrlen = be32_to_cpup(p);
+ return 0;
+}
+
+static int decode_cb_getattr(struct xdr_stream *xdr, uint32_t *bitmap,
+ struct nfs4_cb_fattr *fattr)
+{
+ __be32 *ptr;
+
+ if (likely(bitmap[0] & FATTR4_WORD0_CHANGE)) {
+ ptr = xdr_inline_decode(xdr, 8);
+ if (unlikely(!ptr))
+ return -EIO;
+ xdr_decode_hyper(ptr, &fattr->ncf_cb_cinfo);
+ }
+ if (likely(bitmap[0] & FATTR4_WORD0_SIZE)) {
+ ptr = xdr_inline_decode(xdr, 8);
+ if (unlikely(!ptr))
+ return -EIO;
+ xdr_decode_hyper(ptr, &fattr->ncf_cb_fsize);
+ }
+ return 0;
+}
+
/*
* nfs_cb_opnum4
*
@@ -358,6 +395,26 @@ encode_cb_recallany4args(struct xdr_stream *xdr,
}
/*
+ * CB_GETATTR4args
+ * struct CB_GETATTR4args {
+ * nfs_fh4 fh;
+ * bitmap4 attr_request;
+ * };
+ *
+ * The size and change attributes are the only one
+ * guaranteed to be serviced by the client.
+ */
+static void
+encode_cb_getattr4args(struct xdr_stream *xdr, struct nfs4_cb_compound_hdr *hdr,
+ struct knfsd_fh *fh, struct nfs4_cb_fattr *fattr)
+{
+ encode_nfs_cb_opnum4(xdr, OP_CB_GETATTR);
+ encode_nfs_fh4(xdr, fh);
+ encode_bitmap4(xdr, fattr->ncf_cb_bmap, ARRAY_SIZE(fattr->ncf_cb_bmap));
+ hdr->nops++;
+}
+
+/*
* CB_SEQUENCE4args
*
* struct CB_SEQUENCE4args {
@@ -493,6 +550,29 @@ static void nfs4_xdr_enc_cb_null(struct rpc_rqst *req, struct xdr_stream *xdr,
}
/*
+ * 20.1. Operation 3: CB_GETATTR - Get Attributes
+ */
+static void nfs4_xdr_enc_cb_getattr(struct rpc_rqst *req,
+ struct xdr_stream *xdr, const void *data)
+{
+ const struct nfsd4_callback *cb = data;
+ struct nfs4_cb_fattr *ncf =
+ container_of(cb, struct nfs4_cb_fattr, ncf_getattr);
+ struct nfs4_delegation *dp =
+ container_of(ncf, struct nfs4_delegation, dl_cb_fattr);
+ struct nfs4_cb_compound_hdr hdr = {
+ .ident = cb->cb_clp->cl_cb_ident,
+ .minorversion = cb->cb_clp->cl_minorversion,
+ };
+
+ encode_cb_compound4args(xdr, &hdr);
+ encode_cb_sequence4args(xdr, cb, &hdr);
+ encode_cb_getattr4args(xdr, &hdr,
+ &dp->dl_stid.sc_file->fi_fhandle, &dp->dl_cb_fattr);
+ encode_cb_nops(&hdr);
+}
+
+/*
* 20.2. Operation 4: CB_RECALL - Recall a Delegation
*/
static void nfs4_xdr_enc_cb_recall(struct rpc_rqst *req, struct xdr_stream *xdr,
@@ -548,6 +628,42 @@ static int nfs4_xdr_dec_cb_null(struct rpc_rqst *req, struct xdr_stream *xdr,
}
/*
+ * 20.1. Operation 3: CB_GETATTR - Get Attributes
+ */
+static int nfs4_xdr_dec_cb_getattr(struct rpc_rqst *rqstp,
+ struct xdr_stream *xdr,
+ void *data)
+{
+ struct nfsd4_callback *cb = data;
+ struct nfs4_cb_compound_hdr hdr;
+ int status;
+ u32 bitmap[3] = {0};
+ u32 attrlen;
+ struct nfs4_cb_fattr *ncf =
+ container_of(cb, struct nfs4_cb_fattr, ncf_getattr);
+
+ status = decode_cb_compound4res(xdr, &hdr);
+ if (unlikely(status))
+ return status;
+
+ status = decode_cb_sequence4res(xdr, cb);
+ if (unlikely(status || cb->cb_seq_status))
+ return status;
+
+ status = decode_cb_op_status(xdr, OP_CB_GETATTR, &cb->cb_status);
+ if (status)
+ return status;
+ decode_bitmap4(xdr, bitmap, 3);
+ status = decode_attr_length(xdr, &attrlen);
+ if (status)
+ return status;
+ ncf->ncf_cb_cinfo = 0;
+ ncf->ncf_cb_fsize = 0;
+ status = decode_cb_getattr(xdr, bitmap, ncf);
+ return status;
+}
+
+/*
* 20.2. Operation 4: CB_RECALL - Recall a Delegation
*/
static int nfs4_xdr_dec_cb_recall(struct rpc_rqst *rqstp,
@@ -855,6 +971,7 @@ static const struct rpc_procinfo nfs4_cb_procedures[] = {
PROC(CB_NOTIFY_LOCK, COMPOUND, cb_notify_lock, cb_notify_lock),
PROC(CB_OFFLOAD, COMPOUND, cb_offload, cb_offload),
PROC(CB_RECALL_ANY, COMPOUND, cb_recall_any, cb_recall_any),
+ PROC(CB_GETATTR, COMPOUND, cb_getattr, cb_getattr),
};
static unsigned int nfs4_cb_counts[ARRAY_SIZE(nfs4_cb_procedures)];
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index d49d3060ed4f..92349375053a 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -117,6 +117,19 @@ struct nfs4_cpntf_state {
time64_t cpntf_time; /* last time stateid used */
};
+struct nfs4_cb_fattr {
+ struct nfsd4_callback ncf_getattr;
+ u32 ncf_cb_status;
+ u32 ncf_cb_bmap[1];
+
+ /* from CB_GETATTR reply */
+ u64 ncf_cb_cinfo;
+ u64 ncf_cb_fsize;
+};
+
+/* bits for ncf_cb_flags */
+#define CB_GETATTR_BUSY 0
+
/*
* Represents a delegation stateid. The nfs4_client holds references to these
* and they are put when it is being destroyed or when the delegation is
@@ -150,6 +163,9 @@ struct nfs4_delegation {
int dl_retries;
struct nfsd4_callback dl_recall;
bool dl_recalled;
+
+ /* for CB_GETATTR */
+ struct nfs4_cb_fattr dl_cb_fattr;
};
#define cb_to_delegation(cb) \
@@ -642,6 +658,7 @@ enum nfsd4_cb_op {
NFSPROC4_CLNT_CB_SEQUENCE,
NFSPROC4_CLNT_CB_NOTIFY_LOCK,
NFSPROC4_CLNT_CB_RECALL_ANY,
+ NFSPROC4_CLNT_CB_GETATTR,
};
/* Returns true iff a is later than b: */
diff --git a/fs/nfsd/xdr4cb.h b/fs/nfsd/xdr4cb.h
index 0d39af1b00a0..3a31bb0a3ded 100644
--- a/fs/nfsd/xdr4cb.h
+++ b/fs/nfsd/xdr4cb.h
@@ -54,3 +54,22 @@
#define NFS4_dec_cb_recall_any_sz (cb_compound_dec_hdr_sz + \
cb_sequence_dec_sz + \
op_dec_sz)
+
+/*
+ * 1: CB_GETATTR opcode (32-bit)
+ * N: file_handle
+ * 1: number of entry in attribute array (32-bit)
+ * 1: entry 0 in attribute array (32-bit)
+ */
+#define NFS4_enc_cb_getattr_sz (cb_compound_enc_hdr_sz + \
+ cb_sequence_enc_sz + \
+ 1 + enc_nfs4_fh_sz + 1 + 1)
+/*
+ * 1: attr mask (32-bit bmap)
+ * 2: length of attribute array (64-bit)
+ * 2: change attr (64-bit)
+ * 2: size (64-bit)
+ */
+#define NFS4_dec_cb_getattr_sz (cb_compound_dec_hdr_sz + \
+ cb_sequence_dec_sz + 7 + \
+ op_dec_sz)
--
2.9.5
Thank you Chuck for your review. I'll make the change in v2.
-Dai
On 5/12/23 8:30 AM, Chuck Lever III wrote:
> Hey Dai-
>
> Jeff's a little better with the state-related code, so let
> me start with a review of the new CB_GETATTR implementation.
>
>
>> On May 11, 2023, at 2:43 PM, Dai Ngo <[email protected]> wrote:
>>
>> Includes:
>> . CB_GETATTR proc for nfs4_cb_procedures[]
>>
>> . XDR encoding and decoding function for CB_GETATTR request/reply
>>
>> . add nfs4_cb_fattr to nfs4_delegation for sending CB_GETATTR
>> and store file attributes from client's reply.
>>
>> Signed-off-by: Dai Ngo <[email protected]>
>> ---
>> fs/nfsd/nfs4callback.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++++
>> fs/nfsd/state.h | 17 +++++++
>> fs/nfsd/xdr4cb.h | 19 ++++++++
>> 3 files changed, 153 insertions(+)
>>
>> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
>> index 4039ffcf90ba..ca3d72ef5fbc 100644
>> --- a/fs/nfsd/nfs4callback.c
>> +++ b/fs/nfsd/nfs4callback.c
>> @@ -87,6 +87,43 @@ static void encode_bitmap4(struct xdr_stream *xdr, const __u32 *bitmap,
>> WARN_ON_ONCE(xdr_stream_encode_uint32_array(xdr, bitmap, len) < 0);
>> }
>>
>> +static void decode_bitmap4(struct xdr_stream *xdr, __u32 *bitmap,
>> + size_t len)
>> +{
>> + WARN_ON_ONCE(xdr_stream_decode_uint32_array(xdr, bitmap, len) < 0);
>> +}
> encode_bitmap4() hides the WARN_ON_ONCE.
>
> However, for decoding, we actually want to get the result
> of the decode, so let's get rid of decode_bitmap4() and
> simply call xdr_stream_decode_uint32_array() directly from
> nfs4_xdr_dec_cb_getattr() (and, of course, check it's return
> code properly, no WARN_ON).
>
>
>> +
>> +static int decode_attr_length(struct xdr_stream *xdr, uint32_t *attrlen)
>> +{
>> + __be32 *p;
>> +
>> + p = xdr_inline_decode(xdr, 4);
>> + if (unlikely(!p))
>> + return -EIO;
>> + *attrlen = be32_to_cpup(p);
>> + return 0;
>> +}
>> +
>> +static int decode_cb_getattr(struct xdr_stream *xdr, uint32_t *bitmap,
>> + struct nfs4_cb_fattr *fattr)
>> +{
>> + __be32 *ptr;
>> +
>> + if (likely(bitmap[0] & FATTR4_WORD0_CHANGE)) {
>> + ptr = xdr_inline_decode(xdr, 8);
>> + if (unlikely(!ptr))
>> + return -EIO;
>> + xdr_decode_hyper(ptr, &fattr->ncf_cb_cinfo);
>> + }
>> + if (likely(bitmap[0] & FATTR4_WORD0_SIZE)) {
>> + ptr = xdr_inline_decode(xdr, 8);
>> + if (unlikely(!ptr))
>> + return -EIO;
>> + xdr_decode_hyper(ptr, &fattr->ncf_cb_fsize);
>> + }
> Let's use xdr_stream_decode_u64() for these.
>
> Also, I don't think the likely() is necessary -- this
> isn't performance-sensitive code.
>
>
>> + return 0;
>> +}
>> +
>> /*
>> * nfs_cb_opnum4
>> *
>> @@ -358,6 +395,26 @@ encode_cb_recallany4args(struct xdr_stream *xdr,
>> }
>>
>> /*
>> + * CB_GETATTR4args
>> + * struct CB_GETATTR4args {
>> + * nfs_fh4 fh;
>> + * bitmap4 attr_request;
>> + * };
>> + *
>> + * The size and change attributes are the only one
>> + * guaranteed to be serviced by the client.
>> + */
>> +static void
>> +encode_cb_getattr4args(struct xdr_stream *xdr, struct nfs4_cb_compound_hdr *hdr,
>> + struct knfsd_fh *fh, struct nfs4_cb_fattr *fattr)
> Nit: Can this take just a "struct nfs4_cb_fattr *" parameter
> instead of the filehandle and fattr?
>
>
>> +{
>> + encode_nfs_cb_opnum4(xdr, OP_CB_GETATTR);
>> + encode_nfs_fh4(xdr, fh);
>> + encode_bitmap4(xdr, fattr->ncf_cb_bmap, ARRAY_SIZE(fattr->ncf_cb_bmap));
>> + hdr->nops++;
>> +}
>> +
>> +/*
>> * CB_SEQUENCE4args
>> *
>> * struct CB_SEQUENCE4args {
>> @@ -493,6 +550,29 @@ static void nfs4_xdr_enc_cb_null(struct rpc_rqst *req, struct xdr_stream *xdr,
>> }
>>
>> /*
>> + * 20.1. Operation 3: CB_GETATTR - Get Attributes
>> + */
>> +static void nfs4_xdr_enc_cb_getattr(struct rpc_rqst *req,
>> + struct xdr_stream *xdr, const void *data)
>> +{
>> + const struct nfsd4_callback *cb = data;
>> + struct nfs4_cb_fattr *ncf =
>> + container_of(cb, struct nfs4_cb_fattr, ncf_getattr);
>> + struct nfs4_delegation *dp =
>> + container_of(ncf, struct nfs4_delegation, dl_cb_fattr);
>> + struct nfs4_cb_compound_hdr hdr = {
>> + .ident = cb->cb_clp->cl_cb_ident,
>> + .minorversion = cb->cb_clp->cl_minorversion,
>> + };
>> +
>> + encode_cb_compound4args(xdr, &hdr);
>> + encode_cb_sequence4args(xdr, cb, &hdr);
>> + encode_cb_getattr4args(xdr, &hdr,
>> + &dp->dl_stid.sc_file->fi_fhandle, &dp->dl_cb_fattr);
>> + encode_cb_nops(&hdr);
>> +}
>> +
>> +/*
>> * 20.2. Operation 4: CB_RECALL - Recall a Delegation
>> */
>> static void nfs4_xdr_enc_cb_recall(struct rpc_rqst *req, struct xdr_stream *xdr,
>> @@ -548,6 +628,42 @@ static int nfs4_xdr_dec_cb_null(struct rpc_rqst *req, struct xdr_stream *xdr,
>> }
>>
>> /*
>> + * 20.1. Operation 3: CB_GETATTR - Get Attributes
>> + */
>> +static int nfs4_xdr_dec_cb_getattr(struct rpc_rqst *rqstp,
>> + struct xdr_stream *xdr,
>> + void *data)
>> +{
>> + struct nfsd4_callback *cb = data;
>> + struct nfs4_cb_compound_hdr hdr;
>> + int status;
>> + u32 bitmap[3] = {0};
>> + u32 attrlen;
>> + struct nfs4_cb_fattr *ncf =
>> + container_of(cb, struct nfs4_cb_fattr, ncf_getattr);
>> +
>> + status = decode_cb_compound4res(xdr, &hdr);
>> + if (unlikely(status))
>> + return status;
>> +
>> + status = decode_cb_sequence4res(xdr, cb);
>> + if (unlikely(status || cb->cb_seq_status))
>> + return status;
>> +
>> + status = decode_cb_op_status(xdr, OP_CB_GETATTR, &cb->cb_status);
>> + if (status)
>> + return status;
>> + decode_bitmap4(xdr, bitmap, 3);
>> + status = decode_attr_length(xdr, &attrlen);
>> + if (status)
>> + return status;
> Let's use xdr_stream_decode_u32 directly here, and
> check that attrlen is a reasonable value. Say, not
> larger than the full expected array length?
>
>
>> + ncf->ncf_cb_cinfo = 0;
>> + ncf->ncf_cb_fsize = 0;
>> + status = decode_cb_getattr(xdr, bitmap, ncf);
>> + return status;
> You're actually decoding a fattr4 here, not the whole
> CB_GETATTR result. Can we call the function
> decode_cb_fattr4() ?
>
> Nit: Let's fold the initialization of cb_cinfo and
> cb_fsize into decode_cb_fattr4().
>
>
>
>> +}
>> +
>> +/*
>> * 20.2. Operation 4: CB_RECALL - Recall a Delegation
>> */
>> static int nfs4_xdr_dec_cb_recall(struct rpc_rqst *rqstp,
>> @@ -855,6 +971,7 @@ static const struct rpc_procinfo nfs4_cb_procedures[] = {
>> PROC(CB_NOTIFY_LOCK, COMPOUND, cb_notify_lock, cb_notify_lock),
>> PROC(CB_OFFLOAD, COMPOUND, cb_offload, cb_offload),
>> PROC(CB_RECALL_ANY, COMPOUND, cb_recall_any, cb_recall_any),
>> + PROC(CB_GETATTR, COMPOUND, cb_getattr, cb_getattr),
>> };
>>
>> static unsigned int nfs4_cb_counts[ARRAY_SIZE(nfs4_cb_procedures)];
>> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
>> index d49d3060ed4f..92349375053a 100644
>> --- a/fs/nfsd/state.h
>> +++ b/fs/nfsd/state.h
>> @@ -117,6 +117,19 @@ struct nfs4_cpntf_state {
>> time64_t cpntf_time; /* last time stateid used */
>> };
>>
>> +struct nfs4_cb_fattr {
>> + struct nfsd4_callback ncf_getattr;
>> + u32 ncf_cb_status;
>> + u32 ncf_cb_bmap[1];
>> +
>> + /* from CB_GETATTR reply */
>> + u64 ncf_cb_cinfo;
> In fs/nfsd/nfs4xdr.c, we use "cinfo" to mean change info:
> that's a boolean, and a pair of 64-bit change attribute values.
> IIUC, I don't think that's what this is; it's just a single
> change attribute value.
>
> To avoid overloading the name "cinfo" can you call this field
> something like ncf_cb_change ?
>
>
>> + u64 ncf_cb_fsize;
>> +};
>> +
>> +/* bits for ncf_cb_flags */
>> +#define CB_GETATTR_BUSY 0
>> +
>> /*
>> * Represents a delegation stateid. The nfs4_client holds references to these
>> * and they are put when it is being destroyed or when the delegation is
>> @@ -150,6 +163,9 @@ struct nfs4_delegation {
>> int dl_retries;
>> struct nfsd4_callback dl_recall;
>> bool dl_recalled;
>> +
>> + /* for CB_GETATTR */
>> + struct nfs4_cb_fattr dl_cb_fattr;
>> };
>>
>> #define cb_to_delegation(cb) \
>> @@ -642,6 +658,7 @@ enum nfsd4_cb_op {
>> NFSPROC4_CLNT_CB_SEQUENCE,
>> NFSPROC4_CLNT_CB_NOTIFY_LOCK,
>> NFSPROC4_CLNT_CB_RECALL_ANY,
>> + NFSPROC4_CLNT_CB_GETATTR,
>> };
>>
>> /* Returns true iff a is later than b: */
>> diff --git a/fs/nfsd/xdr4cb.h b/fs/nfsd/xdr4cb.h
>> index 0d39af1b00a0..3a31bb0a3ded 100644
>> --- a/fs/nfsd/xdr4cb.h
>> +++ b/fs/nfsd/xdr4cb.h
>> @@ -54,3 +54,22 @@
>> #define NFS4_dec_cb_recall_any_sz (cb_compound_dec_hdr_sz + \
>> cb_sequence_dec_sz + \
>> op_dec_sz)
>> +
>> +/*
>> + * 1: CB_GETATTR opcode (32-bit)
>> + * N: file_handle
>> + * 1: number of entry in attribute array (32-bit)
>> + * 1: entry 0 in attribute array (32-bit)
>> + */
>> +#define NFS4_enc_cb_getattr_sz (cb_compound_enc_hdr_sz + \
>> + cb_sequence_enc_sz + \
>> + 1 + enc_nfs4_fh_sz + 1 + 1)
>> +/*
>> + * 1: attr mask (32-bit bmap)
>> + * 2: length of attribute array (64-bit)
> Isn't the array length field 32 bits?
>
>
>> + * 2: change attr (64-bit)
>> + * 2: size (64-bit)
>> + */
>> +#define NFS4_dec_cb_getattr_sz (cb_compound_dec_hdr_sz + \
>> + cb_sequence_dec_sz + 7 + \
> Generally we list the length of each item separately
> to document the individual values, so:
>
> 1 + 1 + 2 + 2
>
> is preferred over a single integer.
>
>
>> + op_dec_sz)
>> --
>> 2.9.5
>>
> --
> Chuck Lever
>
>
Hey Dai-
Jeff's a little better with the state-related code, so let
me start with a review of the new CB_GETATTR implementation.
> On May 11, 2023, at 2:43 PM, Dai Ngo <[email protected]> wrote:
>
> Includes:
> . CB_GETATTR proc for nfs4_cb_procedures[]
>
> . XDR encoding and decoding function for CB_GETATTR request/reply
>
> . add nfs4_cb_fattr to nfs4_delegation for sending CB_GETATTR
> and store file attributes from client's reply.
>
> Signed-off-by: Dai Ngo <[email protected]>
> ---
> fs/nfsd/nfs4callback.c | 117 +++++++++++++++++++++++++++++++++++++++++++++++++
> fs/nfsd/state.h | 17 +++++++
> fs/nfsd/xdr4cb.h | 19 ++++++++
> 3 files changed, 153 insertions(+)
>
> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
> index 4039ffcf90ba..ca3d72ef5fbc 100644
> --- a/fs/nfsd/nfs4callback.c
> +++ b/fs/nfsd/nfs4callback.c
> @@ -87,6 +87,43 @@ static void encode_bitmap4(struct xdr_stream *xdr, const __u32 *bitmap,
> WARN_ON_ONCE(xdr_stream_encode_uint32_array(xdr, bitmap, len) < 0);
> }
>
> +static void decode_bitmap4(struct xdr_stream *xdr, __u32 *bitmap,
> + size_t len)
> +{
> + WARN_ON_ONCE(xdr_stream_decode_uint32_array(xdr, bitmap, len) < 0);
> +}
encode_bitmap4() hides the WARN_ON_ONCE.
However, for decoding, we actually want to get the result
of the decode, so let's get rid of decode_bitmap4() and
simply call xdr_stream_decode_uint32_array() directly from
nfs4_xdr_dec_cb_getattr() (and, of course, check it's return
code properly, no WARN_ON).
> +
> +static int decode_attr_length(struct xdr_stream *xdr, uint32_t *attrlen)
> +{
> + __be32 *p;
> +
> + p = xdr_inline_decode(xdr, 4);
> + if (unlikely(!p))
> + return -EIO;
> + *attrlen = be32_to_cpup(p);
> + return 0;
> +}
> +
> +static int decode_cb_getattr(struct xdr_stream *xdr, uint32_t *bitmap,
> + struct nfs4_cb_fattr *fattr)
> +{
> + __be32 *ptr;
> +
> + if (likely(bitmap[0] & FATTR4_WORD0_CHANGE)) {
> + ptr = xdr_inline_decode(xdr, 8);
> + if (unlikely(!ptr))
> + return -EIO;
> + xdr_decode_hyper(ptr, &fattr->ncf_cb_cinfo);
> + }
> + if (likely(bitmap[0] & FATTR4_WORD0_SIZE)) {
> + ptr = xdr_inline_decode(xdr, 8);
> + if (unlikely(!ptr))
> + return -EIO;
> + xdr_decode_hyper(ptr, &fattr->ncf_cb_fsize);
> + }
Let's use xdr_stream_decode_u64() for these.
Also, I don't think the likely() is necessary -- this
isn't performance-sensitive code.
> + return 0;
> +}
> +
> /*
> * nfs_cb_opnum4
> *
> @@ -358,6 +395,26 @@ encode_cb_recallany4args(struct xdr_stream *xdr,
> }
>
> /*
> + * CB_GETATTR4args
> + * struct CB_GETATTR4args {
> + * nfs_fh4 fh;
> + * bitmap4 attr_request;
> + * };
> + *
> + * The size and change attributes are the only one
> + * guaranteed to be serviced by the client.
> + */
> +static void
> +encode_cb_getattr4args(struct xdr_stream *xdr, struct nfs4_cb_compound_hdr *hdr,
> + struct knfsd_fh *fh, struct nfs4_cb_fattr *fattr)
Nit: Can this take just a "struct nfs4_cb_fattr *" parameter
instead of the filehandle and fattr?
> +{
> + encode_nfs_cb_opnum4(xdr, OP_CB_GETATTR);
> + encode_nfs_fh4(xdr, fh);
> + encode_bitmap4(xdr, fattr->ncf_cb_bmap, ARRAY_SIZE(fattr->ncf_cb_bmap));
> + hdr->nops++;
> +}
> +
> +/*
> * CB_SEQUENCE4args
> *
> * struct CB_SEQUENCE4args {
> @@ -493,6 +550,29 @@ static void nfs4_xdr_enc_cb_null(struct rpc_rqst *req, struct xdr_stream *xdr,
> }
>
> /*
> + * 20.1. Operation 3: CB_GETATTR - Get Attributes
> + */
> +static void nfs4_xdr_enc_cb_getattr(struct rpc_rqst *req,
> + struct xdr_stream *xdr, const void *data)
> +{
> + const struct nfsd4_callback *cb = data;
> + struct nfs4_cb_fattr *ncf =
> + container_of(cb, struct nfs4_cb_fattr, ncf_getattr);
> + struct nfs4_delegation *dp =
> + container_of(ncf, struct nfs4_delegation, dl_cb_fattr);
> + struct nfs4_cb_compound_hdr hdr = {
> + .ident = cb->cb_clp->cl_cb_ident,
> + .minorversion = cb->cb_clp->cl_minorversion,
> + };
> +
> + encode_cb_compound4args(xdr, &hdr);
> + encode_cb_sequence4args(xdr, cb, &hdr);
> + encode_cb_getattr4args(xdr, &hdr,
> + &dp->dl_stid.sc_file->fi_fhandle, &dp->dl_cb_fattr);
> + encode_cb_nops(&hdr);
> +}
> +
> +/*
> * 20.2. Operation 4: CB_RECALL - Recall a Delegation
> */
> static void nfs4_xdr_enc_cb_recall(struct rpc_rqst *req, struct xdr_stream *xdr,
> @@ -548,6 +628,42 @@ static int nfs4_xdr_dec_cb_null(struct rpc_rqst *req, struct xdr_stream *xdr,
> }
>
> /*
> + * 20.1. Operation 3: CB_GETATTR - Get Attributes
> + */
> +static int nfs4_xdr_dec_cb_getattr(struct rpc_rqst *rqstp,
> + struct xdr_stream *xdr,
> + void *data)
> +{
> + struct nfsd4_callback *cb = data;
> + struct nfs4_cb_compound_hdr hdr;
> + int status;
> + u32 bitmap[3] = {0};
> + u32 attrlen;
> + struct nfs4_cb_fattr *ncf =
> + container_of(cb, struct nfs4_cb_fattr, ncf_getattr);
> +
> + status = decode_cb_compound4res(xdr, &hdr);
> + if (unlikely(status))
> + return status;
> +
> + status = decode_cb_sequence4res(xdr, cb);
> + if (unlikely(status || cb->cb_seq_status))
> + return status;
> +
> + status = decode_cb_op_status(xdr, OP_CB_GETATTR, &cb->cb_status);
> + if (status)
> + return status;
> + decode_bitmap4(xdr, bitmap, 3);
> + status = decode_attr_length(xdr, &attrlen);
> + if (status)
> + return status;
Let's use xdr_stream_decode_u32 directly here, and
check that attrlen is a reasonable value. Say, not
larger than the full expected array length?
> + ncf->ncf_cb_cinfo = 0;
> + ncf->ncf_cb_fsize = 0;
> + status = decode_cb_getattr(xdr, bitmap, ncf);
> + return status;
You're actually decoding a fattr4 here, not the whole
CB_GETATTR result. Can we call the function
decode_cb_fattr4() ?
Nit: Let's fold the initialization of cb_cinfo and
cb_fsize into decode_cb_fattr4().
> +}
> +
> +/*
> * 20.2. Operation 4: CB_RECALL - Recall a Delegation
> */
> static int nfs4_xdr_dec_cb_recall(struct rpc_rqst *rqstp,
> @@ -855,6 +971,7 @@ static const struct rpc_procinfo nfs4_cb_procedures[] = {
> PROC(CB_NOTIFY_LOCK, COMPOUND, cb_notify_lock, cb_notify_lock),
> PROC(CB_OFFLOAD, COMPOUND, cb_offload, cb_offload),
> PROC(CB_RECALL_ANY, COMPOUND, cb_recall_any, cb_recall_any),
> + PROC(CB_GETATTR, COMPOUND, cb_getattr, cb_getattr),
> };
>
> static unsigned int nfs4_cb_counts[ARRAY_SIZE(nfs4_cb_procedures)];
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index d49d3060ed4f..92349375053a 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -117,6 +117,19 @@ struct nfs4_cpntf_state {
> time64_t cpntf_time; /* last time stateid used */
> };
>
> +struct nfs4_cb_fattr {
> + struct nfsd4_callback ncf_getattr;
> + u32 ncf_cb_status;
> + u32 ncf_cb_bmap[1];
> +
> + /* from CB_GETATTR reply */
> + u64 ncf_cb_cinfo;
In fs/nfsd/nfs4xdr.c, we use "cinfo" to mean change info:
that's a boolean, and a pair of 64-bit change attribute values.
IIUC, I don't think that's what this is; it's just a single
change attribute value.
To avoid overloading the name "cinfo" can you call this field
something like ncf_cb_change ?
> + u64 ncf_cb_fsize;
> +};
> +
> +/* bits for ncf_cb_flags */
> +#define CB_GETATTR_BUSY 0
> +
> /*
> * Represents a delegation stateid. The nfs4_client holds references to these
> * and they are put when it is being destroyed or when the delegation is
> @@ -150,6 +163,9 @@ struct nfs4_delegation {
> int dl_retries;
> struct nfsd4_callback dl_recall;
> bool dl_recalled;
> +
> + /* for CB_GETATTR */
> + struct nfs4_cb_fattr dl_cb_fattr;
> };
>
> #define cb_to_delegation(cb) \
> @@ -642,6 +658,7 @@ enum nfsd4_cb_op {
> NFSPROC4_CLNT_CB_SEQUENCE,
> NFSPROC4_CLNT_CB_NOTIFY_LOCK,
> NFSPROC4_CLNT_CB_RECALL_ANY,
> + NFSPROC4_CLNT_CB_GETATTR,
> };
>
> /* Returns true iff a is later than b: */
> diff --git a/fs/nfsd/xdr4cb.h b/fs/nfsd/xdr4cb.h
> index 0d39af1b00a0..3a31bb0a3ded 100644
> --- a/fs/nfsd/xdr4cb.h
> +++ b/fs/nfsd/xdr4cb.h
> @@ -54,3 +54,22 @@
> #define NFS4_dec_cb_recall_any_sz (cb_compound_dec_hdr_sz + \
> cb_sequence_dec_sz + \
> op_dec_sz)
> +
> +/*
> + * 1: CB_GETATTR opcode (32-bit)
> + * N: file_handle
> + * 1: number of entry in attribute array (32-bit)
> + * 1: entry 0 in attribute array (32-bit)
> + */
> +#define NFS4_enc_cb_getattr_sz (cb_compound_enc_hdr_sz + \
> + cb_sequence_enc_sz + \
> + 1 + enc_nfs4_fh_sz + 1 + 1)
> +/*
> + * 1: attr mask (32-bit bmap)
> + * 2: length of attribute array (64-bit)
Isn't the array length field 32 bits?
> + * 2: change attr (64-bit)
> + * 2: size (64-bit)
> + */
> +#define NFS4_dec_cb_getattr_sz (cb_compound_dec_hdr_sz + \
> + cb_sequence_dec_sz + 7 + \
Generally we list the length of each item separately
to document the individual values, so:
1 + 1 + 2 + 2
is preferred over a single integer.
> + op_dec_sz)
> --
> 2.9.5
>
--
Chuck Lever