Return-Path: Received: from fieldses.org ([174.143.236.118]:44751 "EHLO fieldses.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752908Ab0LQTd0 (ORCPT ); Fri, 17 Dec 2010 14:33:26 -0500 Date: Fri, 17 Dec 2010 14:33:24 -0500 From: "J. Bruce Fields" To: "William A. (Andy) Adamson" Cc: trond.myklebust@netapp.com, bfields@redhat.com, linux-nfs@vger.kernel.org Subject: Re: [PATCH_V4 1/9] SUNRPC new transport for the NFSv4.1 shared back channel Message-ID: <20101217193324.GC14510@fieldses.org> References: <1292610010-19084-1-git-send-email-andros@netapp.com> <1292610010-19084-2-git-send-email-andros@netapp.com> <20101217184047.GD11515@fieldses.org> <20101217191037.GA14510@fieldses.org> Content-Type: text/plain; charset=utf-8 In-Reply-To: Sender: linux-nfs-owner@vger.kernel.org List-ID: MIME-Version: 1.0 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 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 > >> wrote: > >> > On Fri, Dec 17, 2010 at 1:40 PM, J. Bruce Fields wrote: > >> >> On Fri, Dec 17, 2010 at 01:20:02PM -0500, andros@netapp.com wrote: > >> >>> From: Andy Adamson > >> >>> > >> >>> 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 > >> >>> --- > >> >>>  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.