From: Tom Tucker Subject: Re: [RFC,PATCH 12/38] svc: Add a generic transport svc_create_xprt function Date: Fri, 21 Dec 2007 12:18:37 -0600 Message-ID: <1198261117.14237.47.camel@trinity.ogc.int> References: <20071129225510.15275.82660.stgit@dell3.ogc.int> <20071129225603.15275.54556.stgit@dell3.ogc.int> <20071214230523.GI23121@fieldses.org> Mime-Version: 1.0 Content-Type: text/plain Cc: linux-nfs@vger.kernel.org To: "J. Bruce Fields" Return-path: Received: from 209-198-142-2-host.prismnet.net ([209.198.142.2]:54353 "EHLO smtp.opengridcomputing.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752086AbXLUSNH (ORCPT ); Fri, 21 Dec 2007 13:13:07 -0500 In-Reply-To: <20071214230523.GI23121@fieldses.org> Sender: linux-nfs-owner@vger.kernel.org List-ID: On Fri, 2007-12-14 at 18:05 -0500, J. Bruce Fields wrote: > On Thu, Nov 29, 2007 at 04:56:03PM -0600, Tom Tucker wrote: > > > > The svc_create_xprt function is a transport independent version > > of the svc_makesock function. > > > > Since transport instance creation contains transport dependent and > > independent components, add an xpo_create transport function. The > > transport implementation of this function allocates the memory for the > > endpoint, implements the transport dependent initialization logic, and > > calls svc_xprt_init to initialize the transport independent field (svc_xprt) > > in it's data structure. > > > > Signed-off-by: Tom Tucker > > --- > > > > include/linux/sunrpc/svc_xprt.h | 4 +++ > > net/sunrpc/svc_xprt.c | 37 ++++++++++++++++++++++++++ > > net/sunrpc/svcsock.c | 56 +++++++++++++++++++++++++++++---------- > > 3 files changed, 82 insertions(+), 15 deletions(-) > > > > diff --git a/include/linux/sunrpc/svc_xprt.h b/include/linux/sunrpc/svc_xprt.h > > index 1527ff1..3f4a1df 100644 > > --- a/include/linux/sunrpc/svc_xprt.h > > +++ b/include/linux/sunrpc/svc_xprt.h > > @@ -10,6 +10,9 @@ > > #include > > > > struct svc_xprt_ops { > > + struct svc_xprt *(*xpo_create)(struct svc_serv *, > > + struct sockaddr *, int, > > + int); > > struct svc_xprt *(*xpo_accept)(struct svc_xprt *); > > int (*xpo_has_wspace)(struct svc_xprt *); > > int (*xpo_recvfrom)(struct svc_rqst *); > > @@ -36,5 +39,6 @@ struct svc_xprt { > > int svc_reg_xprt_class(struct svc_xprt_class *); > > int svc_unreg_xprt_class(struct svc_xprt_class *); > > void svc_xprt_init(struct svc_xprt_class *, struct svc_xprt *); > > +int svc_create_xprt(struct svc_serv *, char *, unsigned short, int); > > > > #endif /* SUNRPC_SVC_XPRT_H */ > > diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c > > index 92ea85b..9136da4 100644 > > --- a/net/sunrpc/svc_xprt.c > > +++ b/net/sunrpc/svc_xprt.c > > @@ -93,3 +93,40 @@ void svc_xprt_init(struct svc_xprt_class *xcl, struct svc_xprt *xprt) > > xprt->xpt_ops = xcl->xcl_ops; > > } > > EXPORT_SYMBOL_GPL(svc_xprt_init); > > + > > +int svc_create_xprt(struct svc_serv *serv, char *xprt_name, unsigned short port, > > + int flags) > > +{ > > + struct svc_xprt_class *xcl; > > + int ret = -ENOENT; > > + struct sockaddr_in sin = { > > + .sin_family = AF_INET, > > + .sin_addr.s_addr = INADDR_ANY, > > + .sin_port = htons(port), > > + }; > > + dprintk("svc: creating transport %s[%d]\n", xprt_name, port); > > + spin_lock(&svc_xprt_class_lock); > > + list_for_each_entry(xcl, &svc_xprt_class_list, xcl_list) { > > + if (strcmp(xprt_name, xcl->xcl_name) == 0) { > > + spin_unlock(&svc_xprt_class_lock); > > + if (try_module_get(xcl->xcl_owner)) { > > > Hm. I wonder if this is right. Don't you need to do the try_module_get > while still under the svc_xprt_class_lock? > Hmm. This is an interesting question. I'm was assuming that the unregister call from the transport provider comes from the module_exit function. If it doesn't then you are correct you could have an active reference to a module that is not present in the transport class list. I didn't really want to hold a spin lock across a call like this, but looking at try_module_lock, I don't think it can sleep. > --b. > > > + struct svc_xprt *newxprt; > > + ret = 0; > > + newxprt = xcl->xcl_ops->xpo_create > > + (serv, > > + (struct sockaddr *)&sin, sizeof(sin), > > + flags); > > + if (IS_ERR(newxprt)) { > > + module_put(xcl->xcl_owner); > > + ret = PTR_ERR(newxprt); > > + } > > + } > > + goto out; > > + } > > + } > > + spin_unlock(&svc_xprt_class_lock); > > + dprintk("svc: transport %s not found\n", xprt_name); > > + out: > > + return ret; > > +} > > +EXPORT_SYMBOL_GPL(svc_create_xprt); > > diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c > > index 661162b..0bfffbc 100644 > > --- a/net/sunrpc/svcsock.c > > +++ b/net/sunrpc/svcsock.c > > @@ -91,6 +91,8 @@ static void svc_sock_free(struct svc_xprt *); > > static struct svc_deferred_req *svc_deferred_dequeue(struct svc_sock *svsk); > > static int svc_deferred_recv(struct svc_rqst *rqstp); > > static struct cache_deferred_req *svc_defer(struct cache_req *req); > > +static struct svc_xprt * > > +svc_create_socket(struct svc_serv *, int, struct sockaddr *, int, int); > > > > /* apparently the "standard" is that clients close > > * idle connections after 5 minutes, servers after > > @@ -381,6 +383,7 @@ svc_sock_put(struct svc_sock *svsk) > > { > > if (atomic_dec_and_test(&svsk->sk_inuse)) { > > BUG_ON(!test_bit(SK_DEAD, &svsk->sk_flags)); > > + module_put(svsk->sk_xprt.xpt_class->xcl_owner); > > svsk->sk_xprt.xpt_ops->xpo_free(&svsk->sk_xprt); > > } > > } > > @@ -918,7 +921,14 @@ static struct svc_xprt *svc_udp_accept(struct svc_xprt *xprt) > > return NULL; > > } > > > > +static struct svc_xprt * > > +svc_udp_create(struct svc_serv *serv, struct sockaddr *sa, int salen, int flags) > > +{ > > + return svc_create_socket(serv, IPPROTO_UDP, sa, salen, flags); > > +} > > + > > static struct svc_xprt_ops svc_udp_ops = { > > + .xpo_create = svc_udp_create, > > .xpo_recvfrom = svc_udp_recvfrom, > > .xpo_sendto = svc_udp_sendto, > > .xpo_release_rqst = svc_release_skb, > > @@ -931,6 +941,7 @@ static struct svc_xprt_ops svc_udp_ops = { > > > > static struct svc_xprt_class svc_udp_class = { > > .xcl_name = "udp", > > + .xcl_owner = THIS_MODULE, > > .xcl_ops = &svc_udp_ops, > > .xcl_max_payload = RPCSVC_MAXPAYLOAD_UDP, > > }; > > @@ -1351,7 +1362,14 @@ static int svc_tcp_has_wspace(struct svc_xprt *xprt) > > return 1; > > } > > > > +static struct svc_xprt * > > +svc_tcp_create(struct svc_serv *serv, struct sockaddr *sa, int salen, int flags) > > +{ > > + return svc_create_socket(serv, IPPROTO_TCP, sa, salen, flags); > > +} > > + > > static struct svc_xprt_ops svc_tcp_ops = { > > + .xpo_create = svc_tcp_create, > > .xpo_recvfrom = svc_tcp_recvfrom, > > .xpo_sendto = svc_tcp_sendto, > > .xpo_release_rqst = svc_release_skb, > > @@ -1364,6 +1382,7 @@ static struct svc_xprt_ops svc_tcp_ops = { > > > > static struct svc_xprt_class svc_tcp_class = { > > .xcl_name = "tcp", > > + .xcl_owner = THIS_MODULE, > > .xcl_ops = &svc_tcp_ops, > > .xcl_max_payload = RPCSVC_MAXPAYLOAD_TCP, > > }; > > @@ -1589,8 +1608,14 @@ svc_recv(struct svc_rqst *rqstp, long timeout) > > } else if (test_bit(SK_LISTENER, &svsk->sk_flags)) { > > struct svc_xprt *newxpt; > > newxpt = svsk->sk_xprt.xpt_ops->xpo_accept(&svsk->sk_xprt); > > - if (newxpt) > > + if (newxpt) { > > + /* > > + * We know this module_get will succeed because the > > + * listener holds a reference too > > + */ > > + __module_get(newxpt->xpt_class->xcl_owner); > > svc_check_conn_limits(svsk->sk_server); > > + } > > svc_sock_received(svsk); > > } else { > > dprintk("svc: server %p, pool %u, socket %p, inuse=%d\n", > > @@ -1830,8 +1855,9 @@ EXPORT_SYMBOL_GPL(svc_addsock); > > /* > > * Create socket for RPC service. > > */ > > -static int svc_create_socket(struct svc_serv *serv, int protocol, > > - struct sockaddr *sin, int len, int flags) > > +static struct svc_xprt * > > +svc_create_socket(struct svc_serv *serv, int protocol, > > + struct sockaddr *sin, int len, int flags) > > { > > struct svc_sock *svsk; > > struct socket *sock; > > @@ -1846,13 +1872,13 @@ static int svc_create_socket(struct svc_serv *serv, int protocol, > > if (protocol != IPPROTO_UDP && protocol != IPPROTO_TCP) { > > printk(KERN_WARNING "svc: only UDP and TCP " > > "sockets supported\n"); > > - return -EINVAL; > > + return ERR_PTR(-EINVAL); > > } > > type = (protocol == IPPROTO_UDP)? SOCK_DGRAM : SOCK_STREAM; > > > > error = sock_create_kern(sin->sa_family, type, protocol, &sock); > > if (error < 0) > > - return error; > > + return ERR_PTR(error); > > > > svc_reclassify_socket(sock); > > > > @@ -1869,13 +1895,13 @@ static int svc_create_socket(struct svc_serv *serv, int protocol, > > > > if ((svsk = svc_setup_socket(serv, sock, &error, flags)) != NULL) { > > svc_sock_received(svsk); > > - return ntohs(inet_sk(svsk->sk_sk)->sport); > > + return (struct svc_xprt *)svsk; > > } > > > > bummer: > > dprintk("svc: svc_create_socket error = %d\n", -error); > > sock_release(sock); > > - return error; > > + return ERR_PTR(error); > > } > > > > /* > > @@ -1986,15 +2012,15 @@ void svc_force_close_socket(struct svc_sock *svsk) > > int svc_makesock(struct svc_serv *serv, int protocol, unsigned short port, > > int flags) > > { > > - struct sockaddr_in sin = { > > - .sin_family = AF_INET, > > - .sin_addr.s_addr = INADDR_ANY, > > - .sin_port = htons(port), > > - }; > > - > > dprintk("svc: creating socket proto = %d\n", protocol); > > - return svc_create_socket(serv, protocol, (struct sockaddr *) &sin, > > - sizeof(sin), flags); > > + switch (protocol) { > > + case IPPROTO_TCP: > > + return svc_create_xprt(serv, "tcp", port, flags); > > + case IPPROTO_UDP: > > + return svc_create_xprt(serv, "udp", port, flags); > > + default: > > + return -EINVAL; > > + } > > } > > > > /* > > - > > To unsubscribe from this list: send the line "unsubscribe linux-nfs" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html