[pnfs] [PATCH 11/19] nfs41: Refactor svc_process()
J. Bruce Fields
bfields at fieldses.org
Thu Jul 17 10:31:14 EDT 2008
On Thu, Jul 17, 2008 at 10:08:56AM +0300, Benny Halevy wrote:
> On Jul. 16, 2008, 19:53 +0300, "J. Bruce Fields" <bfields at fieldses.org> wrote:
> > On Wed, Jul 16, 2008 at 12:29:45PM +0300, Benny Halevy wrote:
> >> From: Ricardo Labiaga <ricardo.labiaga at netapp.com>
> >>
> >> net/sunrpc/svc.c:svc_process() is used by the NFSv4 callback service
> >> to process RPC requests arriving over connections initiated by the
> >> server. NFSv4.1 supports callbacks over the backchannel on connections
> >> initiated by the client. This patch refactors svc_process() so that
> >> common code can also be used by the backchannel.
> >>
> >> Signed-off-by: Ricardo Labiaga <ricardo.labiaga at netapp.com>
> >> Signed-off-by: Benny Halevy <bhalevy at panasas.com>
> >> ---
> >> net/sunrpc/svc.c | 80 +++++++++++++++++++++++++++++++++--------------------
> >> 1 files changed, 50 insertions(+), 30 deletions(-)
> >>
> >> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> >> index 01c7e31..ef03377 100644
> >> --- a/net/sunrpc/svc.c
> >> +++ b/net/sunrpc/svc.c
> >> @@ -824,20 +824,18 @@ svc_printk(struct svc_rqst *rqstp, const char *fmt, ...)
> >> }
> >>
> >> /*
> >> - * Process the RPC request.
> >> + * Common routine for processing the RPC request.
> >> */
> >> -int
> >> -svc_process(struct svc_rqst *rqstp)
> >> +static int
> >> +svc_process_common(struct svc_rqst *rqstp, struct kvec *argv, struct kvec *resv)
> >> {
> >> struct svc_program *progp;
> >> struct svc_version *versp = NULL; /* compiler food */
> >> struct svc_procedure *procp = NULL;
> >> - struct kvec * argv = &rqstp->rq_arg.head[0];
> >> - struct kvec * resv = &rqstp->rq_res.head[0];
> >> struct svc_serv *serv = rqstp->rq_server;
> >> kxdrproc_t xdr;
> >> __be32 *statp;
> >> - u32 dir, prog, vers, proc;
> >> + u32 prog, vers, proc;
> >> __be32 auth_stat, rpc_stat;
> >> int auth_res;
> >> __be32 *reply_statp;
> >> @@ -847,36 +845,19 @@ svc_process(struct svc_rqst *rqstp)
> >> if (argv->iov_len < 6*4)
> >> goto err_short_len;
> >>
> >> - /* setup response xdr_buf.
> >> - * Initially it has just one page
> >> - */
> >> - rqstp->rq_resused = 1;
> >> - resv->iov_base = page_address(rqstp->rq_respages[0]);
> >> - resv->iov_len = 0;
> >> - rqstp->rq_res.pages = rqstp->rq_respages + 1;
> >> - rqstp->rq_res.len = 0;
> >> - rqstp->rq_res.page_base = 0;
> >> - rqstp->rq_res.page_len = 0;
> >> - rqstp->rq_res.buflen = PAGE_SIZE;
> >> - rqstp->rq_res.tail[0].iov_base = NULL;
> >> - rqstp->rq_res.tail[0].iov_len = 0;
> >> /* Will be turned off only in gss privacy case: */
> >> rqstp->rq_splice_ok = 1;
> >>
> >> /* Setup reply header */
> >> rqstp->rq_xprt->xpt_ops->xpo_prep_reply_hdr(rqstp);
> >>
> >> - rqstp->rq_xid = svc_getu32(argv);
> >> svc_putu32(resv, rqstp->rq_xid);
> >>
> >> - dir = svc_getnl(argv);
> >> vers = svc_getnl(argv);
> >>
> >> /* First words of reply: */
> >> svc_putnl(resv, 1); /* REPLY */
> >>
> >> - if (dir != 0) /* direction != CALL */
> >> - goto err_bad_dir;
> >> if (vers != 2) /* RPC version number */
> >> goto err_bad_rpc;
> >>
> >> @@ -1000,7 +981,7 @@ svc_process(struct svc_rqst *rqstp)
> >> sendit:
> >> if (svc_authorise(rqstp))
> >> goto dropit;
> >> - return svc_send(rqstp);
> >> + return 1; /* Caller can now send it */
> >>
> >> dropit:
> >> svc_authorise(rqstp); /* doesn't hurt to call this twice */
> >> @@ -1014,12 +995,6 @@ err_short_len:
> >>
> >> goto dropit; /* drop request */
> >>
> >> -err_bad_dir:
> >> - svc_printk(rqstp, "bad direction %d, dropping request\n", dir);
> >> -
> >> - serv->sv_stats->rpcbadfmt++;
> >> - goto dropit; /* drop request */
> >> -
> >> err_bad_rpc:
> >> serv->sv_stats->rpcbadfmt++;
> >> svc_putnl(resv, 1); /* REJECT */
> >> @@ -1073,6 +1048,51 @@ err_bad:
> >> EXPORT_SYMBOL(svc_process);
> >>
> >> /*
> >> + * Process the RPC request.
> >> + */
> >> +int
> >> +svc_process(struct svc_rqst *rqstp)
> >> +{
> >> + struct kvec *argv = &rqstp->rq_arg.head[0];
> >> + struct kvec *resv = &rqstp->rq_res.head[0];
> >> + struct svc_serv *serv = rqstp->rq_server;
> >> + u32 dir;
> >> + int error;
> >> +
> >> + /*
> >> + * Setup response xdr_buf.
> >> + * Initially it has just one page
> >> + */
> >> + rqstp->rq_resused = 1;
> >> + resv->iov_base = page_address(rqstp->rq_respages[0]);
> >> + resv->iov_len = 0;
> >> + rqstp->rq_res.pages = rqstp->rq_respages + 1;
> >> + rqstp->rq_res.len = 0;
> >> + rqstp->rq_res.page_base = 0;
> >> + rqstp->rq_res.page_len = 0;
> >> + rqstp->rq_res.buflen = PAGE_SIZE;
> >> + rqstp->rq_res.tail[0].iov_base = NULL;
> >> + rqstp->rq_res.tail[0].iov_len = 0;
> >> +
> >> + rqstp->rq_xid = svc_getu32(argv);
> >> +
> >> + dir = svc_getnl(argv);
> >> + if (dir != 0) {
> >> + /* direction != CALL */
> >> + svc_printk(rqstp, "bad direction %d, dropping request\n", dir);
> >> + serv->sv_stats->rpcbadfmt++;
> >> + svc_drop(rqstp);
> >> + return 0;
> >> + }
> >> +
> >> + error = svc_process_common(rqstp, argv, resv);
> >> + if (error <= 0)
> >> + return error;
> >> +
> >> + return svc_send(rqstp);
> >> +}
> >
> > So the return value here is:
> > - -ERRNO or 0 if already dropped
> > - 1 to send
> >
> > Could we do the svc_send() inside svc_process_common()? If not, let's
>
> Not really, the need for refactoring of the code here was to allow
> svc_process to call svc_send and bc_svc_process to call bc_send,
> in both cases after calling svc_process_common.
>
> > at least add a quick sentence to the comment on svc_process_comment()
> > documenting the convention.
>
> Grepping for svc_process shows that no caller actually uses
> its return value so there's no problem in changing this undocumented
> interface and just documenting it now.
OK. Without looking at the code--if it looks like things'll stay that
way for the forseeable future, then maybe we should just declare it
boolean and stop pretending it returns error numbers?
Another random (possibly dumb) question: would it be possible to define
a new export class for this case and just handle the difference in
xpo_sendto()?
--b.
More information about the pNFS
mailing list