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.
--b.
>
> I take it back - as written, it might be risky :) Perhaps I should
> make sure that the xpt_ref = 1 before returning from bc_svc_process.
>
> -->Andy
> >
> >> The nfs client code knows when it wants to create and destroy the
> >> backchannel, so I think all you want is to create the svc_xprt when you
> >> create the backchannel and then put it once when you bring it down.
> >
> > Correct, and without the references in bc_svc_process, svc_drop == freeing.
> >
> > -->Andy
> >>
> >> --b.
> >>
> >>> + Â Â ret = svc_process_common(rqstp, argv, resv);
> >>> + Â Â if (ret <= 0)
> >>> + Â Â Â Â Â Â return ret;
> >>>
> >>> Â Â Â memcpy(&req->rq_snd_buf, &rqstp->rq_res, sizeof(req->rq_snd_buf));
> >>> - Â Â return bc_send(req);
> >>> + Â Â ret = bc_send(req);
> >>> + Â Â svc_xprt_put(serv->bc_xprt);
> >>> + Â Â return ret;
> >>> Â}
> >>> ÂEXPORT_SYMBOL(bc_svc_process);
> >>> Â#endif /* CONFIG_NFS_V4_1 */
> >>> diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
> >>> index 07919e1..5875551 100644
> >>> --- a/net/sunrpc/svcsock.c
> >>> +++ b/net/sunrpc/svcsock.c
> >>> @@ -66,6 +66,13 @@ static void        Âsvc_sock_free(struct svc_xprt *);
> >>> Âstatic struct svc_xprt *svc_create_socket(struct svc_serv *, int,
> >>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â struct net *, struct sockaddr *,
> >>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â int, int);
> >>> +#if defined(CONFIG_NFS_V4_1)
> >>> +static struct svc_xprt *svc_bc_create_socket(struct svc_serv *, int,
> >>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âstruct net *, struct sockaddr *,
> >>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âint, int);
> >>> +static void svc_bc_sock_free(struct svc_xprt *xprt);
> >>> +#endif /* CONFIG_NFS_V4_1 */
> >>> +
> >>> Â#ifdef CONFIG_DEBUG_LOCK_ALLOC
> >>> Âstatic struct lock_class_key svc_key[2];
> >>> Âstatic struct lock_class_key svc_slock_key[2];
> >>> @@ -1184,6 +1191,79 @@ static struct svc_xprt *svc_tcp_create(struct svc_serv *serv,
> >>> Â Â Â return svc_create_socket(serv, IPPROTO_TCP, net, sa, salen, flags);
> >>> Â}
> >>>
> >>> +#if defined(CONFIG_NFS_V4_1)
> >>> +static struct svc_xprt *svc_bc_create_socket(struct svc_serv *, int,
> >>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âstruct net *, struct sockaddr *,
> >>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âint, int);
> >>> +static void svc_bc_sock_free(struct svc_xprt *xprt);
> >>> +
> >>> +static struct svc_xprt *svc_bc_tcp_create(struct svc_serv *serv,
> >>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âstruct net *net,
> >>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âstruct sockaddr *sa, int salen,
> >>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âint flags)
> >>> +{
> >>> + Â Â return svc_bc_create_socket(serv, IPPROTO_TCP, net, sa, salen, flags);
> >>> +}
> >>> +
> >>> +static void svc_bc_tcp_sock_detach(struct svc_xprt *xprt)
> >>> +{
> >>> +}
> >>> +
> >>> +/* These bc ops are never called. The shared fore channel is used instead */
> >>> +static int svc_bc_tcp_recvfrom(struct svc_rqst *rqstp)
> >>> +{
> >>> + Â Â return 0;
> >>> +}
> >>> +
> >>> +static int svc_bc_tcp_sendto(struct svc_rqst *rqstp)
> >>> +{
> >>> + Â Â return 0;
> >>> +}
> >>> +
> >>> +static void svc_bc_tcp_prep_reply_hdr(struct svc_rqst *rqstp)
> >>> +{
> >>> +}
> >>> +
> >>> +static void svc_bc_release_skb(struct svc_rqst *rqstp)
> >>> +{
> >>> +}
> >>> +
> >>> +static int svc_bc_tcp_has_wspace(struct svc_xprt *xprt)
> >>> +{
> >>> + Â Â return 0;
> >>> +}
> >>> +
> >>> +static struct svc_xprt *svc_bc_tcp_accept(struct svc_xprt *xprt)
> >>> +{
> >>> + Â Â return NULL;
> >>> +}
> >>> +
> >>> +static struct svc_xprt_ops svc_tcp_bc_ops = {
> >>> + Â Â .xpo_create = svc_bc_tcp_create,
> >>> + Â Â .xpo_recvfrom = svc_bc_tcp_recvfrom,
> >>> + Â Â .xpo_sendto = svc_bc_tcp_sendto,
> >>> + Â Â .xpo_release_rqst = svc_bc_release_skb,
> >>> + Â Â .xpo_detach = svc_bc_tcp_sock_detach,
> >>> + Â Â .xpo_free = svc_bc_sock_free,
> >>> + Â Â .xpo_prep_reply_hdr = svc_bc_tcp_prep_reply_hdr,
> >>> + Â Â .xpo_has_wspace = svc_bc_tcp_has_wspace,
> >>> + Â Â .xpo_accept = svc_bc_tcp_accept,
> >>> +};
> >>> +
> >>> +static struct svc_xprt_class svc_tcp_bc_class = {
> >>> + Â Â .xcl_name = "tcp-bc",
> >>> + Â Â .xcl_owner = THIS_MODULE,
> >>> + Â Â .xcl_ops = &svc_tcp_bc_ops,
> >>> + Â Â .xcl_max_payload = RPCSVC_MAXPAYLOAD_TCP,
> >>> +};
> >>> +
> >>> +void svc_init_bc_xprt_sock(void)
> >>> +{
> >>> + Â Â svc_reg_xprt_class(&svc_tcp_bc_class);
> >>> +}
> >>> +EXPORT_SYMBOL_GPL(svc_init_bc_xprt_sock);
> >>> +#endif /* CONFIG_NFS_V4_1 */
> >>> +
> >>> Âstatic struct svc_xprt_ops svc_tcp_ops = {
> >>> Â Â Â .xpo_create = svc_tcp_create,
> >>> Â Â Â .xpo_recvfrom = svc_tcp_recvfrom,
> >>> @@ -1509,41 +1589,43 @@ static void svc_sock_free(struct svc_xprt *xprt)
> >>> Â Â Â kfree(svsk);
> >>> Â}
> >>>
> >>> +#if defined(CONFIG_NFS_V4_1)
> >>> Â/*
> >>> - * Create a svc_xprt.
> >>> - *
> >>> - * For internal use only (e.g. nfsv4.1 backchannel).
> >>> - * Callers should typically use the xpo_create() method.
> >>> + * Create a back channel svc_xprt which shares the fore channel socket.
> >>> Â */
> >>> -struct svc_xprt *svc_sock_create(struct svc_serv *serv, int prot)
> >>> +static struct svc_xprt *svc_bc_create_socket(struct svc_serv *serv,
> >>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âint protocol,
> >>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âstruct net *net,
> >>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âstruct sockaddr *sin, int len,
> >>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Âint flags)
> >>> Â{
> >>> Â Â Â struct svc_sock *svsk;
> >>> - Â Â struct svc_xprt *xprt = NULL;
> >>> + Â Â struct svc_xprt *xprt;
> >>> +
> >>> + Â Â if (protocol != IPPROTO_TCP) {
> >>> + Â Â Â Â Â Â printk(KERN_WARNING "svc: only and TCP sockets"
> >>> + Â Â Â Â Â Â Â Â Â Â " supported on shared back channel\n");
> >>> + Â Â Â Â Â Â return ERR_PTR(-EINVAL);
> >>> + Â Â }
> >>>
> >>> - Â Â dprintk("svc: %s\n", __func__);
> >>> Â Â Â svsk = kzalloc(sizeof(*svsk), GFP_KERNEL);
> >>> Â Â Â if (!svsk)
> >>> - Â Â Â Â Â Â goto out;
> >>> + Â Â Â Â Â Â return ERR_PTR(-ENOMEM);
> >>>
> >>> Â Â Â xprt = &svsk->sk_xprt;
> >>> - Â Â if (prot == IPPROTO_TCP)
> >>> - Â Â Â Â Â Â svc_xprt_init(&svc_tcp_class, xprt, serv);
> >>> - Â Â else if (prot == IPPROTO_UDP)
> >>> - Â Â Â Â Â Â svc_xprt_init(&svc_udp_class, xprt, serv);
> >>> - Â Â else
> >>> - Â Â Â Â Â Â BUG();
> >>> -out:
> >>> - Â Â dprintk("svc: %s return %p\n", __func__, xprt);
> >>> + Â Â svc_xprt_init(&svc_tcp_bc_class, xprt, serv);
> >>> +
> >>> + Â Â serv->bc_xprt = xprt;
> >>> +
> >>> Â Â Â return xprt;
> >>> Â}
> >>> -EXPORT_SYMBOL_GPL(svc_sock_create);
> >>>
> >>> Â/*
> >>> - * Destroy a svc_sock.
> >>> + * Free a back channel svc_sock.
> >>> Â */
> >>> -void svc_sock_destroy(struct svc_xprt *xprt)
> >>> +static void svc_bc_sock_free(struct svc_xprt *xprt)
> >>> Â{
> >>> Â Â Â if (xprt)
> >>> Â Â Â Â Â Â Â kfree(container_of(xprt, struct svc_sock, sk_xprt));
> >>> Â}
> >>> -EXPORT_SYMBOL_GPL(svc_sock_destroy);
> >>> +#endif /* CONFIG_NFS_V4_1 */
> >>> --
> >>> 1.6.6
> >>>
> >> --
> >> 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
> >>
> >
--
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
