On Fri, Dec 17, 2010 at 02:16:35PM -0500, William A. (Andy) Adamson wrote:
> On Fri, Dec 17, 2010 at 2:10 PM, J. Bruce Fields <bfields@xxxxxxxxxxxx> wrote:
> > On Fri, Dec 17, 2010 at 01:57:50PM -0500, William A. (Andy) Adamson wrote:
> >> On Fri, Dec 17, 2010 at 1:54 PM, William A. (Andy) Adamson
> >> <androsadamson@xxxxxxxxx> wrote:
> >> > On Fri, Dec 17, 2010 at 1:40 PM, J. Bruce Fields <bfields@xxxxxxxxxxxx> wrote:
> >> >> On Fri, Dec 17, 2010 at 01:20:02PM -0500, andros@xxxxxxxxxx wrote:
> >> >>> From: Andy Adamson <andros@xxxxxxxxxx>
> >> >>>
> >> >>> Create a new transport for the shared back channel.l Move the current sock
> >> >>> create and destroy routines into the new transport ops.
> >> >>>
> >> >>> Reference the back channel transport across message processing (recv/send).
> >> >>>
> >> >>> Signed-off-by: Andy Adamson <andros@xxxxxxxxxx>
> >> >>> ---
> >> >>> Âinclude/linux/sunrpc/svcsock.h | Â Â1 +
> >> >>> Ânet/sunrpc/svc.c        |  18 +++---
> >> >>> Ânet/sunrpc/svcsock.c      | Â122 +++++++++++++++++++++++++++++++++-------
> >> >>> Â3 files changed, 112 insertions(+), 29 deletions(-)
> >> >>>
> >> >>> diff --git a/include/linux/sunrpc/svcsock.h b/include/linux/sunrpc/svcsock.h
> >> >>> index 1b353a7..3a45a80 100644
> >> >>> --- a/include/linux/sunrpc/svcsock.h
> >> >>> +++ b/include/linux/sunrpc/svcsock.h
> >> >>> @@ -45,6 +45,7 @@ int     svc_sock_names(struct svc_serv *serv, char *buf,
> >> >>> Âint     Âsvc_addsock(struct svc_serv *serv, const int fd,
> >> >>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â char *name_return, const size_t len);
> >> >>> Âvoid     svc_init_xprt_sock(void);
> >> >>> +void     svc_init_bc_xprt_sock(void);
> >> >>> Âvoid     svc_cleanup_xprt_sock(void);
> >> >>> Âstruct svc_xprt *svc_sock_create(struct svc_serv *serv, int prot);
> >> >>> Âvoid     svc_sock_destroy(struct svc_xprt *);
> >> >>> diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
> >> >>> index 6359c42..3520cb3 100644
> >> >>> --- a/net/sunrpc/svc.c
> >> >>> +++ b/net/sunrpc/svc.c
> >> >>> @@ -488,10 +488,6 @@ svc_destroy(struct svc_serv *serv)
> >> >>> Â Â Â if (svc_serv_is_pooled(serv))
> >> >>> Â Â Â Â Â Â Â svc_pool_map_put();
> >> >>>
> >> >>> -#if defined(CONFIG_NFS_V4_1)
> >> >>> - Â Â svc_sock_destroy(serv->bc_xprt);
> >> >>> -#endif /* CONFIG_NFS_V4_1 */
> >> >>> -
> >> >>> Â Â Â svc_unregister(serv);
> >> >>> Â Â Â kfree(serv->sv_pools);
> >> >>> Â Â Â kfree(serv);
> >> >>> @@ -1264,7 +1260,7 @@ bc_svc_process(struct svc_serv *serv, struct rpc_rqst *req,
> >> >>> Â{
> >> >>>    struct kvec   *argv = &rqstp->rq_arg.head[0];
> >> >>>    struct kvec   *resv = &rqstp->rq_res.head[0];
> >> >>> -   int       error;
> >> >>> +   int       ret;
> >> >>>
> >> >>> Â Â Â /* Build the svc_rqst used by the common processing routine */
> >> >>> Â Â Â rqstp->rq_xprt = serv->bc_xprt;
> >> >>> @@ -1292,12 +1288,16 @@ bc_svc_process(struct svc_serv *serv, struct rpc_rqst *req,
> >> >>> Â Â Â svc_getu32(argv); Â Â Â /* XID */
> >> >>> Â Â Â svc_getnl(argv); Â Â Â Â/* CALLDIR */
> >> >>>
> >> >>> - Â Â error = svc_process_common(rqstp, argv, resv);
> >> >>> - Â Â if (error <= 0)
> >> >>> - Â Â Â Â Â Â return error;
> >> >>> + Â Â /* Hold a reference to the back channel socket across the call */
> >> >>> + Â Â svc_xprt_get(serv->bc_xprt);
> >> >>
> >> >> Either we already have a reference, in which case this is unnecessary,
> >> >> or we don't, in which case this is risky.
> >> >
> >> > This is done so that when svc_xprt_put is called due to an svc_drop
> >> > (svc_xprt_release, svc_xprt_put) it is not freed. ÂIt's not risky
> >> > because AFAICS it's the way the rpc server code is intended to work.
> >> > Note that svc_recv calls svc_xprt_get, and svc_send calls svc_xprt_put
> >> > for the same reason.
> >
> > The svc_xprt_put()'s in svc_send/svc_drop balance the get in svc_recv().
> > They're needed because on a normal server connections can come and go
> > (because clients come and go) during the lifetime of the server.
> >
> > As I understand it, the client is just creating a new server for each
> > backchannel. ÂSo the server will never outlive the one connection it
> > uses. ÂSo you don't need that stuff. ÂIt's harmless--just leave it
> > alone--but definitely don't try to add additional reference counting
> > during the processing.
>
> If I don't add an svc_xprt_get prior to the svc_process_call, the
> svc_xprt_put called in the svc_drop case will free the bc_xprt, which
> is not what I want.
Looking at the code some more.... Oh, right, because the backchannel
doesn't call svc_recv, it calls its own bc_send, which doesn't do a put.
Oh, yuch, I see: svc_process_common returns "1" to mean send, "0" to
mean drop, and leaves it up to the caller to do the put in the send
case. That's confusing.
Maybe it would be simpler to have the caller be made responsible for
both cases? So:
if (svc_process_common(rqstp))
send it
else
drop it
?
--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
