> On Nov 23, 2020, at 1:49 PM, bfields@xxxxxxxxxxxx wrote:
>
> On Fri, Nov 20, 2020 at 03:34:02PM -0500, Chuck Lever wrote:
>> A "permanent" struct xdr_stream is allocated in struct svc_rqst so
>> that it is usable by all server-side decoders. A per-rqst scratch
>> buffer is also allocated to handle decoding XDR data items that
>> cross page boundaries.
>>
>> To demonstrate how it will be used, add the first call site for the
>> new svcxdr_init_decode() API.
>>
>> As an additional part of the overall conversion, add symbolic
>> constants for successful and failed XDR operations. Returning "0" is
>> overloaded. Sometimes it means something failed, but sometimes it
>> means success. To make it more clear when XDR decoding functions
>> succeed or fail, introduce symbolic constants.
>
> I'm not sure how I feel about that part. Do you plan to change this
> everywhere?
Yes. But, I agree it is a little wordy.
> Maybe it'd be simpler or clearer to make pc_decode return bool?
That was my first thought. But true/false is just as overloaded
and meaningless as "1" and "0".
However, I'm open to opinions and better ideas.
> --b.
>
>>
>> Signed-off-by: Chuck Lever <chuck.lever@xxxxxxxxxx>
>> ---
>> fs/nfsd/nfs4xdr.c | 3 ++-
>> fs/nfsd/nfssvc.c | 4 +++-
>> include/linux/sunrpc/svc.h | 16 ++++++++++++++++
>> include/linux/sunrpc/xdr.h | 5 +++++
>> net/sunrpc/svc.c | 5 +++++
>> 5 files changed, 31 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
>> index e3c6bea83bd6..66274ad05a9c 100644
>> --- a/fs/nfsd/nfs4xdr.c
>> +++ b/fs/nfsd/nfs4xdr.c
>> @@ -5321,7 +5321,8 @@ nfs4svc_decode_compoundargs(struct svc_rqst *rqstp, __be32 *p)
>> args->ops = args->iops;
>> args->rqstp = rqstp;
>>
>> - return !nfsd4_decode_compound(args);
>> + return nfsd4_decode_compound(args) == nfs_ok ? XDR_DECODE_DONE :
>> + XDR_DECODE_FAILED;
>> }
>>
>> int
>> diff --git a/fs/nfsd/nfssvc.c b/fs/nfsd/nfssvc.c
>> index 27b1ad136150..daeab72975a3 100644
>> --- a/fs/nfsd/nfssvc.c
>> +++ b/fs/nfsd/nfssvc.c
>> @@ -1020,7 +1020,9 @@ int nfsd_dispatch(struct svc_rqst *rqstp, __be32 *statp)
>> * (necessary in the NFSv4.0 compound case)
>> */
>> rqstp->rq_cachetype = proc->pc_cachetype;
>> - if (!proc->pc_decode(rqstp, argv->iov_base))
>> +
>> + svcxdr_init_decode(rqstp, argv->iov_base);
>> + if (proc->pc_decode(rqstp, argv->iov_base) == XDR_DECODE_FAILED)
>> goto out_decode_err;
>>
>> switch (nfsd_cache_lookup(rqstp)) {
>> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
>> index c220b734fa69..bb6c93283697 100644
>> --- a/include/linux/sunrpc/svc.h
>> +++ b/include/linux/sunrpc/svc.h
>> @@ -248,6 +248,8 @@ struct svc_rqst {
>> size_t rq_xprt_hlen; /* xprt header len */
>> struct xdr_buf rq_arg;
>> struct xdr_buf rq_res;
>> + struct xdr_stream rq_xdr_stream;
>> + struct page *rq_scratch_page;
>> struct page *rq_pages[RPCSVC_MAXPAGES + 1];
>> struct page * *rq_respages; /* points into rq_pages */
>> struct page * *rq_next_page; /* next reply page to use */
>> @@ -557,4 +559,18 @@ static inline void svc_reserve_auth(struct svc_rqst *rqstp, int space)
>> svc_reserve(rqstp, space + rqstp->rq_auth_slack);
>> }
>>
>> +/**
>> + * svcxdr_init_decode - Prepare an xdr_stream for svc Call decoding
>> + * @rqstp: controlling server RPC transaction context
>> + * @p: Starting position
>> + *
>> + */
>> +static inline void svcxdr_init_decode(struct svc_rqst *rqstp, __be32 *p)
>> +{
>> + struct xdr_stream *xdr = &rqstp->rq_xdr_stream;
>> +
>> + xdr_init_decode(xdr, &rqstp->rq_arg, p, NULL);
>> + xdr_set_scratch_page(xdr, rqstp->rq_scratch_page);
>> +}
>> +
>> #endif /* SUNRPC_SVC_H */
>> diff --git a/include/linux/sunrpc/xdr.h b/include/linux/sunrpc/xdr.h
>> index 2729d2d6efce..abbb032de4e8 100644
>> --- a/include/linux/sunrpc/xdr.h
>> +++ b/include/linux/sunrpc/xdr.h
>> @@ -19,6 +19,11 @@
>> struct bio_vec;
>> struct rpc_rqst;
>>
>> +enum xdr_decode_result {
>> + XDR_DECODE_FAILED = 0,
>> + XDR_DECODE_DONE = 1,
>> +};
>> +
>> /*
>> * Buffer adjustment
>> */
>> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
>> index b41500645c3f..4187745887f0 100644
>> --- a/net/sunrpc/svc.c
>> +++ b/net/sunrpc/svc.c
>> @@ -614,6 +614,10 @@ svc_rqst_alloc(struct svc_serv *serv, struct svc_pool *pool, int node)
>> rqstp->rq_server = serv;
>> rqstp->rq_pool = pool;
>>
>> + rqstp->rq_scratch_page = alloc_pages_node(node, GFP_KERNEL, 0);
>> + if (!rqstp->rq_scratch_page)
>> + goto out_enomem;
>> +
>> rqstp->rq_argp = kmalloc_node(serv->sv_xdrsize, GFP_KERNEL, node);
>> if (!rqstp->rq_argp)
>> goto out_enomem;
>> @@ -842,6 +846,7 @@ void
>> svc_rqst_free(struct svc_rqst *rqstp)
>> {
>> svc_release_buffer(rqstp);
>> + put_page(rqstp->rq_scratch_page);
>> kfree(rqstp->rq_resp);
>> kfree(rqstp->rq_argp);
>> kfree(rqstp->rq_auth_data);
--
Chuck Lever
