Re: [PATCH v5 07/21] nfsd: add callback encoding and decoding linkages for CB_NOTIFY

[Jeff Layton <jlayton@kernel.org>]

On Mon, 2026-06-08 at 12:52 -0400, Chuck Lever wrote:
> 
> On Fri, May 22, 2026, at 3:42 PM, Jeff Layton wrote:
> > Add routines for encoding and decoding CB_NOTIFY messages. These call
> > into the code generated by xdrgen to do the actual encoding and
> > decoding.
> 
> The commit message needs to explain that the encoder is not yet functional.
> Something like: "The encoder is a stub; payload encoding (stateid, fh, and
> cna_changes) is deferred."
> 

Ok.

> 
> > diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
> > index 25bbf5b8814d..ea3e7deb06fa 100644
> > --- a/fs/nfsd/nfs4callback.c
> > +++ b/fs/nfsd/nfs4callback.c
> > @@ -865,6 +865,51 @@ static void encode_stateowner(struct xdr_stream 
> > *xdr, struct nfs4_stateowner *so
> >  	xdr_encode_opaque(p, so->so_owner.data, so->so_owner.len);
> >  }
> > 
> > +static void nfs4_xdr_enc_cb_notify(struct rpc_rqst *req,
> > +				   struct xdr_stream *xdr,
> > +				   const void *data)
> > +{
> > +	const struct nfsd4_callback *cb = data;
> > +	struct nfs4_cb_compound_hdr hdr = {
> > +		.ident = 0,
> > +		.minorversion = cb->cb_clp->cl_minorversion,
> > +	};
> > +	struct CB_NOTIFY4args args = { };
> > +
> > +	WARN_ON_ONCE(hdr.minorversion == 0);
> > +
> > +	encode_cb_compound4args(xdr, &hdr);
> > +	encode_cb_sequence4args(xdr, cb, &hdr);
> > +
> > +	/*
> > +	 * FIXME: get stateid and fh from delegation. Inline the cna_changes
> > +	 * buffer, and zero it.
> > +	 */
> > +	WARN_ON_ONCE(!xdrgen_encode_CB_NOTIFY4args(xdr, &args));
> > +
> > +	hdr.nops++;
> > +	encode_cb_nops(&hdr);
> > +}
> 
> There are a number of problems with this, but since there are no
> callers yet, we can let some of those issues stand.
> 
> What is problematic in the longer-term is that this is a client-side
> encoder (since this is the server's NFSv4 callback client).
> 
> xdrgen_encode_CB_NOTIFY4args() is an argument encoder, which is
> client-side functionality, but it resides in fs/nfsd/nfs4xdr_gen.c,
> which is server-side. Let's not mix these purposes.
> 
> I replaced the comment and WARN_ON with this:
> 
> +       xdr_stream_encode_u32(xdr, OP_CB_NOTIFY);
> +
> +       /* FIXME: encode stateid, fh, and cna_changes from delegation */
> 
> You can use xdrgen functions for individual data items, but for
> full argument and response structures, only server-side is supported
> at the moment. In the later patch that completes this code, I'll cover
> the other fields, which can be a mix of open code and xdrgen.
> 

The full argument encoder and decoder works just fine. When you say
"supported" what do you mean, specifically?

I'd really rather not go back to open-coding the encoders and decoders,
particularly since CB_NOTIFY has one of the most complex argument
structures in the protocol.

> 
> > diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> > index e1c40f8b5d01..790282781243 100644
> > --- a/fs/nfsd/state.h
> > +++ b/fs/nfsd/state.h
> > @@ -190,6 +190,13 @@ struct nfs4_cb_fattr {
> >  	u64 ncf_cur_fsize;
> >  };
> > 
> > +/*
> > + * FIXME: the current backchannel encoder can't handle a send buffer longer
> > + *        than a single page (see bc_alloc/bc_free).
> > + */
> 
> Nit: The allocator function name is bc_malloc
> 

Will fix.
-- 
Jeff Layton <jlayton@kernel.org>