Re: [PATCH 1/2] NFSD: add support for sending CB_RECALL_ANY

[dai.ngo@xxxxxxxxxx]

On 10/18/22 6:25 AM, Tom Talpey wrote:
> On 10/18/2022 1:15 AM, 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.
>
> I'm curious if clients have shown any quirks with the operation in
> your testing. If the (Linux) server hasn't ever been sending it,
> then I'd expect some possible issues/quirks in the client.
>
> For example, do they really start handing back a significant number
> of useful delegations? Enough to satisfy the server's need without
> going to specific resource-based recalls?

In my testing with Linux client that have active delegations, the client
just replies with NFS4_OK without returning any of the active delegations.
I guess to see the client returning the delegation, that delegation must
be not in used but so far I have not been able to force that condition:
not is use but have not returned it to their server yet.

-Dai

> Tom.
>
> > 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);
> > +}
> > +
> > +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)