On Sat, 2022-10-22 at 11:09 -0700, Dai Ngo wrote: > There is only one nfsd4_callback, cl_recall_any, added for each > nfs4_client. 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. > > Signed-off-by: Dai Ngo <dai.ngo@xxxxxxxxxx> > --- > fs/nfsd/nfs4callback.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++++++ > fs/nfsd/nfs4state.c | 27 +++++++++++++++++++++ > fs/nfsd/state.h | 8 +++++++ > fs/nfsd/xdr4cb.h | 6 +++++ > 4 files changed, 105 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 */ > + p = xdr_reserve_space(xdr, 8); > + *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..c60c937dece6 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -2854,6 +2854,31 @@ 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) > +{ > + cb->cb_clp->cl_recall_any_busy = false; > + atomic_dec(&cb->cb_clp->cl_rpc_users); > +} This series probably ought to be one big patch. The problem is that you're adding a bunch of code to do CB_RECALL_ANY, but there is no way to call it without patch #2. That makes it hard to judge whether there could be races and locking issues around the handling of cb_recall_any_busy, in particular. From patch #2, it looks like cb_recall_any_busy is protected by the nn->client_lock, but I don't think ->release is called with that held. Also, cl_rpc_users is a refcount (though we don't necessarily free the object when it goes to zero). I think you need to call put_client_renew_locked here instead of just decrementing the counter. > + > +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 +2916,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; > + 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) -- Jeff Layton <jlayton@xxxxxxxxxx>