> On Oct 27, 2022, at 10:16 PM, Dai Ngo <dai.ngo@xxxxxxxxxx> wrote:
>
> There is only one nfsd4_callback, cl_recall_any, added for each
> nfs4_client.
Is only one needed at a time, or might NFSD need to send more
than one concurrently? So far it looks like the only reason to
limit it to one at a time is the way the cb_recall_any arguments
are passed to the XDR layer.
> Access to it must be serialized. For now it's done
> by the cl_recall_any_busy flag since it's used only by the
> delegation shrinker. If there is another consumer of CB_RECALL_ANY
> then a spinlock must be used.
The usual arrangement is to add the XDR infrastructure for a new
operation in one patch, and then add consumers in subsequent
patches. Can you move the hunks that change fs/nfsd/nfs4state.c
to 2/2 and update the above description accordingly?
In a separate patch you should add a trace_nfsd_cb_recall_any and
a trace_nfsd_cb_recall_any_done tracepoint. There are already nice
examples in fs/nfsd/trace.h for the other callback operations.
A little more below.
> Signed-off-by: Dai Ngo <dai.ngo@xxxxxxxxxx>
> ---
> fs/nfsd/nfs4callback.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++
> fs/nfsd/nfs4state.c | 32 +++++++++++++++++++++++++
> fs/nfsd/state.h | 8 +++++++
> fs/nfsd/xdr4cb.h | 6 +++++
> 4 files changed, 110 insertions(+)
>
> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
> index f0e69edf5f0f..03587e1397f4 100644
> --- a/fs/nfsd/nfs4callback.c
> +++ b/fs/nfsd/nfs4callback.c
> @@ -329,6 +329,29 @@ static void encode_cb_recall4args(struct xdr_stream *xdr,
> }
>
> /*
> + * CB_RECALLANY4args
> + *
> + * struct CB_RECALLANY4args {
> + * uint32_t craa_objects_to_keep;
> + * bitmap4 craa_type_mask;
> + * };
> + */
> +static void
> +encode_cb_recallany4args(struct xdr_stream *xdr,
> + struct nfs4_cb_compound_hdr *hdr, uint32_t bmval)
> +{
> + __be32 *p;
> +
> + encode_nfs_cb_opnum4(xdr, OP_CB_RECALL_ANY);
> + p = xdr_reserve_space(xdr, 4);
> + *p++ = xdr_zero; /* craa_objects_to_keep */
Let's use xdr_stream_encode_u32() here. Would it be reasonable
for the upper layer to provide this value, or will NFSD always
want it to be zero?
> + p = xdr_reserve_space(xdr, 8);
Let's use xdr_stream_encode_uint32_array() here. encode_cb_recallany4args's
caller should pass a u32 * and a length, not just a simple u32.
> + *p++ = cpu_to_be32(1);
> + *p++ = cpu_to_be32(bmval);
> + hdr->nops++;
> +}
> +
> +/*
> * CB_SEQUENCE4args
> *
> * struct CB_SEQUENCE4args {
> @@ -482,6 +505,24 @@ static void nfs4_xdr_enc_cb_recall(struct rpc_rqst *req, struct xdr_stream *xdr,
> encode_cb_nops(&hdr);
> }
>
> +/*
> + * 20.6. Operation 8: CB_RECALL_ANY - Keep Any N Recallable Objects
> + */
> +static void
> +nfs4_xdr_enc_cb_recall_any(struct rpc_rqst *req,
> + struct xdr_stream *xdr, const void *data)
> +{
> + const struct nfsd4_callback *cb = data;
> + 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_recallany4args(xdr, &hdr, cb->cb_clp->cl_recall_any_bm);
> + encode_cb_nops(&hdr);
> +}
>
> /*
> * NFSv4.0 and NFSv4.1 XDR decode functions
> @@ -520,6 +561,28 @@ static int nfs4_xdr_dec_cb_recall(struct rpc_rqst *rqstp,
> return decode_cb_op_status(xdr, OP_CB_RECALL, &cb->cb_status);
> }
>
> +/*
> + * 20.6. Operation 8: CB_RECALL_ANY - Keep Any N Recallable Objects
> + */
> +static int
> +nfs4_xdr_dec_cb_recall_any(struct rpc_rqst *rqstp,
> + struct xdr_stream *xdr,
> + void *data)
> +{
> + struct nfsd4_callback *cb = data;
> + struct nfs4_cb_compound_hdr hdr;
> + int status;
> +
> + 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_RECALL_ANY, &cb->cb_status);
> + return status;
> +}
> +
> #ifdef CONFIG_NFSD_PNFS
> /*
> * CB_LAYOUTRECALL4args
> @@ -783,6 +846,7 @@ static const struct rpc_procinfo nfs4_cb_procedures[] = {
> #endif
> 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),
> };
>
> static unsigned int nfs4_cb_counts[ARRAY_SIZE(nfs4_cb_procedures)];
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 4e718500a00c..68d049973ce3 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -2854,6 +2854,36 @@ static const struct tree_descr client_files[] = {
> [3] = {""},
> };
>
> +static int
> +nfsd4_cb_recall_any_done(struct nfsd4_callback *cb,
> + struct rpc_task *task)
> +{
> + switch (task->tk_status) {
> + case -NFS4ERR_DELAY:
> + rpc_delay(task, 2 * HZ);
> + return 0;
> + default:
> + return 1;
> + }
> +}
> +
> +static void
> +nfsd4_cb_recall_any_release(struct nfsd4_callback *cb)
> +{
> + struct nfs4_client *clp = cb->cb_clp;
> + struct nfsd_net *nn = net_generic(clp->net, nfsd_net_id);
> +
> + spin_lock(&nn->client_lock);
> + clp->cl_recall_any_busy = false;
> + put_client_renew_locked(clp);
> + spin_unlock(&nn->client_lock);
> +}
> +
> +static const struct nfsd4_callback_ops nfsd4_cb_recall_any_ops = {
> + .done = nfsd4_cb_recall_any_done,
> + .release = nfsd4_cb_recall_any_release,
> +};
> +
> static struct nfs4_client *create_client(struct xdr_netobj name,
> struct svc_rqst *rqstp, nfs4_verifier *verf)
> {
> @@ -2891,6 +2921,8 @@ static struct nfs4_client *create_client(struct xdr_netobj name,
> free_client(clp);
> return NULL;
> }
> + nfsd4_init_cb(&clp->cl_recall_any, clp, &nfsd4_cb_recall_any_ops,
> + NFSPROC4_CLNT_CB_RECALL_ANY);
> return clp;
> }
>
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index e2daef3cc003..49ca06169642 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -411,6 +411,10 @@ struct nfs4_client {
>
> unsigned int cl_state;
> atomic_t cl_delegs_in_recall;
> +
> + bool cl_recall_any_busy;
Rather than adding a boolean field, you could add a bit to
cl_flags.
I'm not convinced you need to add the argument fields here...
I think kmalloc'ing the arguments and then freeing them in
nfsd4_cb_recall_any_release() would be sufficient.
> + uint32_t cl_recall_any_bm;
> + struct nfsd4_callback cl_recall_any;
> };
>
> /* struct nfs4_client_reset
> @@ -639,8 +643,12 @@ enum nfsd4_cb_op {
> NFSPROC4_CLNT_CB_OFFLOAD,
> NFSPROC4_CLNT_CB_SEQUENCE,
> NFSPROC4_CLNT_CB_NOTIFY_LOCK,
> + NFSPROC4_CLNT_CB_RECALL_ANY,
> };
>
> +#define RCA4_TYPE_MASK_RDATA_DLG 0
> +#define RCA4_TYPE_MASK_WDATA_DLG 1
> +
> /* Returns true iff a is later than b: */
> static inline bool nfsd4_stateid_generation_after(stateid_t *a, stateid_t *b)
> {
> diff --git a/fs/nfsd/xdr4cb.h b/fs/nfsd/xdr4cb.h
> index 547cf07cf4e0..0d39af1b00a0 100644
> --- a/fs/nfsd/xdr4cb.h
> +++ b/fs/nfsd/xdr4cb.h
> @@ -48,3 +48,9 @@
> #define NFS4_dec_cb_offload_sz (cb_compound_dec_hdr_sz + \
> cb_sequence_dec_sz + \
> op_dec_sz)
> +#define NFS4_enc_cb_recall_any_sz (cb_compound_enc_hdr_sz + \
> + cb_sequence_enc_sz + \
> + 1 + 1 + 1)
> +#define NFS4_dec_cb_recall_any_sz (cb_compound_dec_hdr_sz + \
> + cb_sequence_dec_sz + \
> + op_dec_sz)
> --
> 2.9.5
>
--
Chuck Lever
