2007-10-09 15:37:10

by Tom Tucker

[permalink] [raw]
Subject: [RFC,PATCH 0/7] svc: Incremental svc patchset cleanup

This patchset is a series of independent, incremental patches against
the previously posted svc transport switch patchset. I believe each
of these patches can be taken independently. They are presented as a
series soley because they reflect what I believe to be the collective
consensus of the svc transport switch review.

The main, non-trivial changes are as follows:

- Transport class data is no longer copied to each transport instance.

- A svc_xprt_find service has been added to allow a service to query
for transport instances based on class name, address family and port.

--
Signed-off-by: Tom Tucker <[email protected]>

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs


2007-10-10 02:40:59

by Tom Tucker

[permalink] [raw]
Subject: Re: [RFC,PATCH 0/7] svc: Incremental svc patchset cleanup

On Wed, 2007-10-10 at 12:24 +1000, Greg Banks wrote:
> G'day,
>
> Compendium reply.
>
> On Tue, Oct 09, 2007 at 10:35:39AM -0500, Tom Tucker wrote:
> > This patchset is a series of independent, incremental patches against
> > the previously posted svc transport switch patchset. I believe each
> > of these patches can be taken independently. They are presented as a
> > series soley because they reflect what I believe to be the collective
> > consensus of the svc transport switch review.
> >
> > The main, non-trivial changes are as follows:
> >
> > - Transport class data is no longer copied to each transport instance.
> >
> > - A svc_xprt_find service has been added to allow a service to query
> > for transport instances based on class name, address family and port.
> >
>
> > Subject: [RFC,PATCH 1/7] svc: Don't copy xprt_class data into svc_xprt instance
>
> ok
>
> > Subject: [RFC,PATCH 2/7] svc: Rename xpo_release to xpo_release_rqst
>
> ok
>
> > Subject: [RFC,PATCH 3/7] svc: Move setting of XPT_LISTENER bit to svc_tcp_init
>
> ok
>
> > Subject: [RFC,PATCH 4/7] svc: Add a sockaddr length argument to the xpo_create function.
>
> ok
>
> > Subject: [RFC,PATCH 5/7] svc: Remove extraneous debug svc_send printk
>
> ok
>
> > Subject: [RFC,PATCH 6/7] svc: Add svc API that queries for a transport instance
>
> > @@ -242,11 +230,11 @@ static int make_socks(struct svc_serv *s
> > int err = 0;
> >
> > if (proto == IPPROTO_UDP || nlm_udpport)
> > - if (!find_xprt(serv, "udp"))
> > + if (!svc_find_xprt(serv, "udp", 0, 0))
> > err = svc_create_xprt(serv, "udp", nlm_udpport,
> > SVC_SOCK_DEFAULTS);
> > if (err >= 0 && (proto == IPPROTO_TCP || nlm_tcpport))
> > - if (!find_xprt(serv, "tcp"))
> > + if (!svc_find_xprt(serv, "tcp", 0, 0))
> > err = svc_create_xprt(serv, "tcp", nlm_tcpport,
> > SVC_SOCK_DEFAULTS);
>
> You could use AF_UNSPEC as the "wildcard" value for address family.

Yes, this is clearly preferred. Good suggestion..

>
> > +struct svc_xprt *svc_find_xprt(struct svc_serv *serv, char *xcl_name,
> > + int af, int port)
> > +{
> > + struct svc_xprt *xprt = NULL;
> > + list_for_each_entry(xprt, &serv->sv_permsocks, xpt_list) {
>
> Why have address family and port? Do you need them?

Well, I think not based on the current user 'lockd'. But the additional
logic is trivial and it allows services to discriminate between AF_INET,
AF_INET6, port this, port that, etc.... when searching for existing
transports.

>
> Also, if this is a generic, exported API, it should be taking
> serv->sv_lock to protect traversal of the sv_permsocks list.

Good point. Thanks,
>
> > + * Specifying 0 for the address family or port is a wildcard and will
> > + * match any transport with the firt transport with the same class
> > + * name active for the service.
> > [...]
> > + struct sockaddr_in *sin;
> > + if (strcmp(xprt->xpt_class->xcl_name, xcl_name))
> > + continue;
> > + sin = (struct sockaddr_in *)&xprt->xpt_local;
> > + if (af && sin->sin_family != af)
> > + continue;
> > +
> > + if (port && (sin->sin_port != port))
> > + continue;
>
> So what are the intended semantics of svc_find_xprt(,,0,2049) ?
> When called like that, it will reach into xprt->xpt_local assuming
> it's a sockaddr_in and match the passed port number against whatever
> it finds at bytes 2-3.
>
> Perhaps this function could be pruned back to only do what it needs
> to do right now, which is match on transport name?
>
>
> > Subject: [RFC,PATCH 7/7] svc: Place type on same line for new API
>
> ok
>
> Greg.


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

2007-10-10 03:33:19

by Tom Tucker

[permalink] [raw]
Subject: Re: [RFC,PATCH 0/7] svc: Incremental svc patchset cleanup

On Wed, 2007-10-10 at 12:24 +1000, Greg Banks wrote:
> G'day,
>
> Compendium reply.
>
> On Tue, Oct 09, 2007 at 10:35:39AM -0500, Tom Tucker wrote:

> So what are the intended semantics of svc_find_xprt(,,0,2049) ?
> When called like that, it will reach into xprt->xpt_local assuming
> it's a sockaddr_in and match the passed port number against whatever
> it finds at bytes 2-3.

Sorry, I missed this question. My intent was that the filters were
hierarchical: class, af, port.

AF_UNSPEC, notwithstanding, a class of <null> never matches anything.

>
> Perhaps this function could be pruned back to only do what it needs
> to do right now, which is match on transport name?
>

Yes, it could.

>
> > Subject: [RFC,PATCH 7/7] svc: Place type on same line for new API
>
> ok
>
> Greg.


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

2007-10-10 04:17:06

by Greg Banks

[permalink] [raw]
Subject: Re: [RFC,PATCH 0/7] svc: Incremental svc patchset cleanup

On Tue, Oct 09, 2007 at 09:39:46PM -0500, Tom Tucker wrote:
> On Wed, 2007-10-10 at 12:24 +1000, Greg Banks wrote:
> >
> > Why have address family and port? Do you need them?
>
> Well, I think not based on the current user 'lockd'. But the additional
> logic is trivial and it allows services to discriminate between AF_INET,
> AF_INET6, port this, port that, etc.... when searching for existing
> transports.

Is it still trivial when implemented correctly?

Greg.
--
Greg Banks, R&D Software Engineer, SGI Australian Software Group.
Apparently, I'm Bedevere. Which MPHG character are you?
I don't speak for SGI.

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

2007-10-10 04:17:39

by Greg Banks

[permalink] [raw]
Subject: Re: [RFC,PATCH 0/7] svc: Incremental svc patchset cleanup

On Tue, Oct 09, 2007 at 10:32:05PM -0500, Tom Tucker wrote:
> On Wed, 2007-10-10 at 12:24 +1000, Greg Banks wrote:
> > G'day,
> >
> > Compendium reply.
> >
> > On Tue, Oct 09, 2007 at 10:35:39AM -0500, Tom Tucker wrote:
>
> > So what are the intended semantics of svc_find_xprt(,,0,2049) ?
> > When called like that, it will reach into xprt->xpt_local assuming
> > it's a sockaddr_in and match the passed port number against whatever
> > it finds at bytes 2-3.
>
> Sorry, I missed this question. My intent was that the filters were
> hierarchical: class, af, port.

Ah. Perhaps the comment should say that, and the code enforce it.

Greg.
--
Greg Banks, R&D Software Engineer, SGI Australian Software Group.
Apparently, I'm Bedevere. Which MPHG character are you?
I don't speak for SGI.

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

2007-10-09 15:37:15

by Tom Tucker

[permalink] [raw]
Subject: [RFC, PATCH 3/7] svc: Move setting of XPT_LISTENER bit to svc_tcp_init


Move the setting of the XPT_LISTENER bit to svc_tcp_init where
the remaining TCP transport initializiation is done.

Signed-off-by: Tom Tucker <[email protected]>
---

net/sunrpc/svcsock.c | 5 +----
1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
index f967c23..07a6f42 100644
--- a/net/sunrpc/svcsock.c
+++ b/net/sunrpc/svcsock.c
@@ -1128,6 +1128,7 @@ svc_tcp_init(struct svc_sock *svsk, stru
set_bit(XPT_CACHE_AUTH, &svsk->sk_xprt.xpt_flags);
if (sk->sk_state == TCP_LISTEN) {
dprintk("setting up TCP socket for listening\n");
+ set_bit(XPT_LISTENER, &svsk->sk_xprt.xpt_flags);
sk->sk_data_ready = svc_tcp_listen_data_ready;
set_bit(XPT_CONN, &svsk->sk_xprt.xpt_flags);
} else {
@@ -1253,8 +1254,6 @@ int svc_addsock(struct svc_serv *serv,
svc_xprt_received(&svsk->sk_xprt);
err = 0;
}
- if (so->sk->sk_protocol == IPPROTO_TCP)
- set_bit(XPT_LISTENER, &svsk->sk_xprt.xpt_flags);
clear_bit(XPT_TEMP, &svsk->sk_xprt.xpt_flags);
spin_lock_bh(&serv->sv_lock);
list_add(&svsk->sk_xprt.xpt_list, &serv->sv_permsocks);
@@ -1311,8 +1310,6 @@ svc_create_socket(struct svc_serv *serv,
}

if ((svsk = svc_setup_socket(serv, sock, &error, flags)) != NULL) {
- if (protocol == IPPROTO_TCP)
- set_bit(XPT_LISTENER, &svsk->sk_xprt.xpt_flags);
svc_xprt_received(&svsk->sk_xprt);
return (struct svc_xprt *)svsk;
}

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

2007-10-09 15:37:25

by Tom Tucker

[permalink] [raw]
Subject: [RFC, PATCH 5/7] svc: Remove extraneous debug svc_send printk


The svc_send function has a debug check and kern info printk
for a null xprt pointers in the rqstp structure. Remove the
printk.

Signed-off-by: Tom Tucker <[email protected]>
---

net/sunrpc/svc_xprt.c | 5 +----
1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
index 76036af..323977a 100644
--- a/net/sunrpc/svc_xprt.c
+++ b/net/sunrpc/svc_xprt.c
@@ -678,11 +678,8 @@ svc_send(struct svc_rqst *rqstp)
int len;
struct xdr_buf *xb;

- if ((xprt = rqstp->rq_xprt) == NULL) {
- printk(KERN_WARNING "NULL transport pointer in %s:%d\n",
- __FILE__, __LINE__);
+ if ((xprt = rqstp->rq_xprt) == NULL)
return -EFAULT;
- }

/* release the receive skb before sending the reply */
rqstp->rq_xprt->xpt_ops->xpo_release_rqst(rqstp);

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

2007-10-09 15:37:11

by Tom Tucker

[permalink] [raw]
Subject: [RFC, PATCH 1/7] svc: Don't copy xprt_class data into svc_xprt instance


Previously, data from the transport class was copied into the transport
structure. Make the xpt_xpo a ptr to the ops structure from the transport
class, and access the max_payload value through the existing xpt_class
pointer.

Signed-off-by: Tom Tucker <[email protected]>
---

include/linux/sunrpc/svc_xprt.h | 3 +--
net/sunrpc/svc.c | 4 ++--
net/sunrpc/svc_xprt.c | 19 +++++++++----------
3 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/include/linux/sunrpc/svc_xprt.h b/include/linux/sunrpc/svc_xprt.h
index 53c8891..89bd5ff 100644
--- a/include/linux/sunrpc/svc_xprt.h
+++ b/include/linux/sunrpc/svc_xprt.h
@@ -34,8 +34,7 @@ struct svc_xprt_class {

struct svc_xprt {
struct svc_xprt_class *xpt_class;
- struct svc_xprt_ops xpt_ops;
- u32 xpt_max_payload;
+ struct svc_xprt_ops *xpt_ops;
struct kref xpt_ref;
struct list_head xpt_list;
struct list_head xpt_ready;
diff --git a/net/sunrpc/svc.c b/net/sunrpc/svc.c
index 440ea59..e1d4c03 100644
--- a/net/sunrpc/svc.c
+++ b/net/sunrpc/svc.c
@@ -812,7 +812,7 @@ svc_process(struct svc_rqst *rqstp)
rqstp->rq_splice_ok = 1;

/* Setup reply header */
- rqstp->rq_xprt->xpt_ops.xpo_prep_reply_hdr(rqstp);
+ rqstp->rq_xprt->xpt_ops->xpo_prep_reply_hdr(rqstp);

rqstp->rq_xid = svc_getu32(argv);
svc_putu32(resv, rqstp->rq_xid);
@@ -1029,7 +1029,7 @@ err_bad:
*/
u32 svc_max_payload(const struct svc_rqst *rqstp)
{
- int max = rqstp->rq_xprt->xpt_max_payload;
+ int max = rqstp->rq_xprt->xpt_class->xcl_max_payload;

if (rqstp->rq_server->sv_max_payload < max)
max = rqstp->rq_server->sv_max_payload;
diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
index e55904f..16ffc73 100644
--- a/net/sunrpc/svc_xprt.c
+++ b/net/sunrpc/svc_xprt.c
@@ -127,7 +127,7 @@ static inline void svc_xprt_free(struct
container_of(kref, struct svc_xprt, xpt_ref);
struct module *owner = xprt->xpt_class->xcl_owner;
BUG_ON(atomic_read(&kref->refcount));
- xprt->xpt_ops.xpo_free(xprt);
+ xprt->xpt_ops->xpo_free(xprt);
if (test_bit(XPT_CACHE_AUTH, &xprt->xpt_flags)
&& xprt->xpt_auth_cache != NULL)
svcauth_unix_info_release(xprt->xpt_auth_cache);
@@ -148,8 +148,7 @@ void svc_xprt_init(struct svc_xprt_class
struct svc_serv *serv)
{
xpt->xpt_class = xcl;
- xpt->xpt_ops = *xcl->xcl_ops;
- xpt->xpt_max_payload = xcl->xcl_max_payload;
+ xpt->xpt_ops = xcl->xcl_ops;
kref_init(&xpt->xpt_ref);
xpt->xpt_server = serv;
INIT_LIST_HEAD(&xpt->xpt_list);
@@ -281,7 +280,7 @@ svc_xprt_enqueue(struct svc_xprt *xprt)
goto process;

/* Check if we have space to reply to a request */
- if (!xprt->xpt_ops.xpo_has_wspace(xprt)) {
+ if (!xprt->xpt_ops->xpo_has_wspace(xprt)) {
/* Don't enqueue while not enough space for reply */
dprintk("svc: no write space, transport %p not enqueued\n", xprt);
xprt->xpt_pool = NULL;
@@ -382,7 +381,7 @@ svc_xprt_release(struct svc_rqst *rqstp)
{
struct svc_xprt *xprt = rqstp->rq_xprt;

- rqstp->rq_xprt->xpt_ops.xpo_release(rqstp);
+ rqstp->rq_xprt->xpt_ops->xpo_release(rqstp);

svc_free_res_pages(rqstp);
rqstp->rq_res.page_len = 0;
@@ -604,7 +603,7 @@ svc_recv(struct svc_rqst *rqstp, long ti
svc_delete_xprt(xprt);
} else if (test_bit(XPT_LISTENER, &xprt->xpt_flags)) {
struct svc_xprt *newxpt;
- newxpt = xprt->xpt_ops.xpo_accept(xprt);
+ newxpt = xprt->xpt_ops->xpo_accept(xprt);
if (newxpt) {
svc_xprt_received(newxpt);
/*
@@ -636,7 +635,7 @@ svc_recv(struct svc_rqst *rqstp, long ti
svc_xprt_received(xprt);
len = svc_deferred_recv(rqstp);
} else
- len = xprt->xpt_ops.xpo_recvfrom(rqstp);
+ len = xprt->xpt_ops->xpo_recvfrom(rqstp);
svc_copy_addr(rqstp, xprt);
dprintk("svc: got len=%d\n", len);
}
@@ -684,7 +683,7 @@ svc_send(struct svc_rqst *rqstp)
}

/* release the receive skb before sending the reply */
- rqstp->rq_xprt->xpt_ops.xpo_release(rqstp);
+ rqstp->rq_xprt->xpt_ops->xpo_release(rqstp);

/* calculate over-all length */
xb = & rqstp->rq_res;
@@ -697,7 +696,7 @@ svc_send(struct svc_rqst *rqstp)
if (test_bit(XPT_DEAD, &xprt->xpt_flags))
len = -ENOTCONN;
else
- len = xprt->xpt_ops.xpo_sendto(rqstp);
+ len = xprt->xpt_ops->xpo_sendto(rqstp);
mutex_unlock(&xprt->xpt_mutex);
svc_xprt_release(rqstp);

@@ -772,7 +771,7 @@ svc_delete_xprt(struct svc_xprt *xprt)

serv = xprt->xpt_server;

- xprt->xpt_ops.xpo_detach(xprt);
+ xprt->xpt_ops->xpo_detach(xprt);

spin_lock_bh(&serv->sv_lock);


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

2007-10-09 15:37:21

by Tom Tucker

[permalink] [raw]
Subject: [RFC, PATCH 6/7] svc: Add svc API that queries for a transport instance


Add a new svc function that allows a service to query whether a
transport instance has already been created. This is used in lockd
to determine whether or not a transport needs to be created when
a lockd instance is brought up.

Specifying 0 for the address family or port is effectively a wild-card,
and will result in matching the first transport in the service's list
that has a matching class name.

Signed-off-by: Tom Tucker <[email protected]>
---

fs/lockd/svc.c | 16 ++--------------
include/linux/sunrpc/svc_xprt.h | 2 ++
net/sunrpc/svc_xprt.c | 30 ++++++++++++++++++++++++++++++
3 files changed, 34 insertions(+), 14 deletions(-)

diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
index a8e79a9..470af01 100644
--- a/fs/lockd/svc.c
+++ b/fs/lockd/svc.c
@@ -219,18 +219,6 @@ lockd(struct svc_rqst *rqstp)
module_put_and_exit(0);
}

-static int find_xprt(struct svc_serv *serv, char *proto)
-{
- struct svc_xprt *xprt;
- int found = 0;
- list_for_each_entry(xprt, &serv->sv_permsocks, xpt_list)
- if (strcmp(xprt->xpt_class->xcl_name, proto) == 0) {
- found = 1;
- break;
- }
- return found;
-}
-
/*
* Make any sockets that are needed but not present.
* If nlm_udpport or nlm_tcpport were set as module
@@ -242,11 +230,11 @@ static int make_socks(struct svc_serv *s
int err = 0;

if (proto == IPPROTO_UDP || nlm_udpport)
- if (!find_xprt(serv, "udp"))
+ if (!svc_find_xprt(serv, "udp", 0, 0))
err = svc_create_xprt(serv, "udp", nlm_udpport,
SVC_SOCK_DEFAULTS);
if (err >= 0 && (proto == IPPROTO_TCP || nlm_tcpport))
- if (!find_xprt(serv, "tcp"))
+ if (!svc_find_xprt(serv, "tcp", 0, 0))
err = svc_create_xprt(serv, "tcp", nlm_tcpport,
SVC_SOCK_DEFAULTS);

diff --git a/include/linux/sunrpc/svc_xprt.h b/include/linux/sunrpc/svc_xprt.h
index 2f45327..5dba9a5 100644
--- a/include/linux/sunrpc/svc_xprt.h
+++ b/include/linux/sunrpc/svc_xprt.h
@@ -82,4 +82,6 @@ static inline void svc_xprt_get(struct s
void svc_delete_xprt(struct svc_xprt *xprt);
void svc_close_xprt(struct svc_xprt *xprt);
int svc_print_xprts(char *buf, int maxlen);
+struct svc_xprt *
+svc_find_xprt(struct svc_serv *serv, char *xprt_class, int af, int port);
#endif /* SUNRPC_SVC_XPRT_H */
diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
index 323977a..4841c4b 100644
--- a/net/sunrpc/svc_xprt.c
+++ b/net/sunrpc/svc_xprt.c
@@ -950,3 +950,33 @@ static struct svc_deferred_req *svc_defe
spin_unlock(&xprt->xpt_lock);
return dr;
}
+
+/*
+ * Return the transport instance pointer for the endpoint accepting
+ * connections/peer traffic from the specified transport class,
+ * address family and port.
+ *
+ * Specifying 0 for the address family or port is a wildcard and will
+ * match any transport with the firt transport with the same class
+ * name active for the service.
+ */
+struct svc_xprt *svc_find_xprt(struct svc_serv *serv, char *xcl_name,
+ int af, int port)
+{
+ struct svc_xprt *xprt = NULL;
+ list_for_each_entry(xprt, &serv->sv_permsocks, xpt_list) {
+ struct sockaddr_in *sin;
+ if (strcmp(xprt->xpt_class->xcl_name, xcl_name))
+ continue;
+ sin = (struct sockaddr_in *)&xprt->xpt_local;
+ if (af && sin->sin_family != af)
+ continue;
+
+ if (port && (sin->sin_port != port))
+ continue;
+
+ break;
+ }
+ return xprt;
+}
+EXPORT_SYMBOL_GPL(svc_find_xprt);

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

2007-10-09 15:37:21

by Tom Tucker

[permalink] [raw]
Subject: [RFC, PATCH 4/7] svc: Add a sockaddr length argument to the xpo_create function.


The xpo_create function doesn't currently accept a sockaddr length.
Add this as a parameter to be consistent with other kernel interfaces
taking a sockaddr.

Signed-off-by: Tom Tucker <[email protected]>
---

include/linux/sunrpc/svc_xprt.h | 2 +-
net/sunrpc/svc_xprt.c | 4 +++-
net/sunrpc/svcsock.c | 10 ++++------
3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/include/linux/sunrpc/svc_xprt.h b/include/linux/sunrpc/svc_xprt.h
index a1a7d15..2f45327 100644
--- a/include/linux/sunrpc/svc_xprt.h
+++ b/include/linux/sunrpc/svc_xprt.h
@@ -12,7 +12,7 @@ #include <linux/module.h>

struct svc_xprt_ops {
struct svc_xprt *(*xpo_create)(struct svc_serv *,
- struct sockaddr *,
+ struct sockaddr *, int,
int);
struct svc_xprt *(*xpo_accept)(struct svc_xprt *);
int (*xpo_has_wspace)(struct svc_xprt *);
diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
index 807795f..76036af 100644
--- a/net/sunrpc/svc_xprt.c
+++ b/net/sunrpc/svc_xprt.c
@@ -178,7 +178,9 @@ int svc_create_xprt(struct svc_serv *ser
struct svc_xprt *newxprt;
ret = 0;
newxprt = xcl->xcl_ops->xpo_create
- (serv, (struct sockaddr *)&sin, flags);
+ (serv,
+ (struct sockaddr *)&sin, sizeof(sin),
+ flags);
if (IS_ERR(newxprt)) {
module_put(xcl->xcl_owner);
ret = PTR_ERR(newxprt);
diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
index 07a6f42..ec834a7 100644
--- a/net/sunrpc/svcsock.c
+++ b/net/sunrpc/svcsock.c
@@ -655,10 +655,9 @@ svc_udp_accept(struct svc_xprt *xprt)
}

static struct svc_xprt *
-svc_udp_create(struct svc_serv *serv, struct sockaddr *sa, int flags)
+svc_udp_create(struct svc_serv *serv, struct sockaddr *sa, int alen, int flags)
{
- return svc_create_socket(serv, IPPROTO_UDP, sa,
- sizeof(struct sockaddr_in), flags);
+ return svc_create_socket(serv, IPPROTO_UDP, sa, alen, flags);
}

static struct svc_xprt_ops svc_udp_ops = {
@@ -1081,10 +1080,9 @@ svc_tcp_has_wspace(struct svc_xprt *xprt
}

static struct svc_xprt *
-svc_tcp_create(struct svc_serv *serv, struct sockaddr *sa, int flags)
+svc_tcp_create(struct svc_serv *serv, struct sockaddr *sa, int alen, int flags)
{
- return svc_create_socket(serv, IPPROTO_TCP, sa,
- sizeof(struct sockaddr_in), flags);
+ return svc_create_socket(serv, IPPROTO_TCP, sa, alen, flags);
}

static struct svc_xprt_ops svc_tcp_ops = {

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

2007-10-09 15:37:13

by Tom Tucker

[permalink] [raw]
Subject: [RFC,PATCH 2/7] svc: Rename xpo_release to xpo_release_rqst


The xpo_release function removes resources that the transport has
associated with a particular request. To avoid confusion with the
xpo_detach and xpo_free functions, rename this function xpo_release_rqst.

Signed-off-by: Tom Tucker <[email protected]>
---

include/linux/sunrpc/svc_xprt.h | 2 +-
net/sunrpc/svc_xprt.c | 4 ++--
net/sunrpc/svcsock.c | 4 ++--
3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/linux/sunrpc/svc_xprt.h b/include/linux/sunrpc/svc_xprt.h
index 89bd5ff..a1a7d15 100644
--- a/include/linux/sunrpc/svc_xprt.h
+++ b/include/linux/sunrpc/svc_xprt.h
@@ -19,7 +19,7 @@ struct svc_xprt_ops {
int (*xpo_recvfrom)(struct svc_rqst *);
void (*xpo_prep_reply_hdr)(struct svc_rqst *);
int (*xpo_sendto)(struct svc_rqst *);
- void (*xpo_release)(struct svc_rqst *);
+ void (*xpo_release_rqst)(struct svc_rqst *);
void (*xpo_detach)(struct svc_xprt *);
void (*xpo_free)(struct svc_xprt *);
};
diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
index 16ffc73..807795f 100644
--- a/net/sunrpc/svc_xprt.c
+++ b/net/sunrpc/svc_xprt.c
@@ -381,7 +381,7 @@ svc_xprt_release(struct svc_rqst *rqstp)
{
struct svc_xprt *xprt = rqstp->rq_xprt;

- rqstp->rq_xprt->xpt_ops->xpo_release(rqstp);
+ rqstp->rq_xprt->xpt_ops->xpo_release_rqst(rqstp);

svc_free_res_pages(rqstp);
rqstp->rq_res.page_len = 0;
@@ -683,7 +683,7 @@ svc_send(struct svc_rqst *rqstp)
}

/* release the receive skb before sending the reply */
- rqstp->rq_xprt->xpt_ops->xpo_release(rqstp);
+ rqstp->rq_xprt->xpt_ops->xpo_release_rqst(rqstp);

/* calculate over-all length */
xb = & rqstp->rq_res;
diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c
index e170866..f967c23 100644
--- a/net/sunrpc/svcsock.c
+++ b/net/sunrpc/svcsock.c
@@ -665,7 +665,7 @@ 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 = svc_release_skb,
+ .xpo_release_rqst = svc_release_skb,
.xpo_detach = svc_sock_detach,
.xpo_free = svc_sock_free,
.xpo_prep_reply_hdr = svc_udp_prep_reply_hdr,
@@ -1091,7 +1091,7 @@ 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 = svc_release_skb,
+ .xpo_release_rqst = svc_release_skb,
.xpo_detach = svc_sock_detach,
.xpo_free = svc_sock_free,
.xpo_prep_reply_hdr = svc_tcp_prep_reply_hdr,

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

2007-10-09 15:37:21

by Tom Tucker

[permalink] [raw]
Subject: [RFC,PATCH 7/7] svc: Place type on same line for new API


The convention is place the type name on the same line as the function
name. The inline directive was also removed to allow the compiler to
elect whether or not to inline.

Signed-off-by: Tom Tucker <[email protected]>
---

net/sunrpc/svc_xprt.c | 52 +++++++++++++++++--------------------------------
1 files changed, 18 insertions(+), 34 deletions(-)

diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c
index 4841c4b..080ecf5 100644
--- a/net/sunrpc/svc_xprt.c
+++ b/net/sunrpc/svc_xprt.c
@@ -121,7 +121,7 @@ int svc_print_xprts(char *buf, int maxle
return len;
}

-static inline void svc_xprt_free(struct kref *kref)
+static void svc_xprt_free(struct kref *kref)
{
struct svc_xprt *xprt =
container_of(kref, struct svc_xprt, xpt_ref);
@@ -209,8 +209,7 @@ EXPORT_SYMBOL_GPL(svc_create_xprt);
* use as many different threads as we need, and the rest don't pollute
* the cache.
*/
-static inline void
-svc_thread_enqueue(struct svc_pool *pool, struct svc_rqst *rqstp)
+static void svc_thread_enqueue(struct svc_pool *pool, struct svc_rqst *rqstp)
{
list_add(&rqstp->rq_list, &pool->sp_threads);
}
@@ -218,8 +217,7 @@ svc_thread_enqueue(struct svc_pool *pool
/*
* Dequeue an nfsd thread. Must have pool->sp_lock held.
*/
-static inline void
-svc_thread_dequeue(struct svc_pool *pool, struct svc_rqst *rqstp)
+static void svc_thread_dequeue(struct svc_pool *pool, struct svc_rqst *rqstp)
{
list_del(&rqstp->rq_list);
}
@@ -229,8 +227,7 @@ svc_thread_dequeue(struct svc_pool *pool
* processes, wake 'em up.
*
*/
-void
-svc_xprt_enqueue(struct svc_xprt *xprt)
+void svc_xprt_enqueue(struct svc_xprt *xprt)
{
struct svc_serv *serv = xprt->xpt_server;
struct svc_pool *pool;
@@ -322,8 +319,7 @@ EXPORT_SYMBOL_GPL(svc_xprt_enqueue);
/*
* Dequeue the first transport. Must be called with the pool->sp_lock held.
*/
-static inline struct svc_xprt *
-svc_xprt_dequeue(struct svc_pool *pool)
+static struct svc_xprt *svc_xprt_dequeue(struct svc_pool *pool)
{
struct svc_xprt *xprt;

@@ -346,8 +342,7 @@ svc_xprt_dequeue(struct svc_pool *pool)
* Note: XPT_DATA only gets cleared when a read-attempt finds
* no (or insufficient) data.
*/
-void
-svc_xprt_received(struct svc_xprt *xprt)
+void svc_xprt_received(struct svc_xprt *xprt)
{
xprt->xpt_pool = NULL;
clear_bit(XPT_BUSY, &xprt->xpt_flags);
@@ -378,8 +373,7 @@ void svc_reserve(struct svc_rqst *rqstp,
}
}

-static void
-svc_xprt_release(struct svc_rqst *rqstp)
+static void svc_xprt_release(struct svc_rqst *rqstp)
{
struct svc_xprt *xprt = rqstp->rq_xprt;

@@ -411,8 +405,7 @@ svc_xprt_release(struct svc_rqst *rqstp)
* This really only makes sense for services like lockd
* which have exactly one thread anyway.
*/
-void
-svc_wake_up(struct svc_serv *serv)
+void svc_wake_up(struct svc_serv *serv)
{
struct svc_rqst *rqstp;
unsigned int i;
@@ -437,8 +430,7 @@ svc_wake_up(struct svc_serv *serv)
}
}

-static void
-svc_check_conn_limits(struct svc_serv *serv)
+static void svc_check_conn_limits(struct svc_serv *serv)
{
char buf[RPC_MAX_ADDRBUFLEN];

@@ -485,7 +477,7 @@ svc_check_conn_limits(struct svc_serv *s
}
}

-static inline void svc_copy_addr(struct svc_rqst *rqstp, struct svc_xprt *xprt)
+static void svc_copy_addr(struct svc_rqst *rqstp, struct svc_xprt *xprt)
{
struct sockaddr *sin;

@@ -513,8 +505,7 @@ static inline void svc_copy_addr(struct
* organised not to touch any cachelines in the shared svc_serv
* structure, only cachelines in the local svc_pool.
*/
-int
-svc_recv(struct svc_rqst *rqstp, long timeout)
+int svc_recv(struct svc_rqst *rqstp, long timeout)
{
struct svc_xprt *xprt = NULL;
struct svc_serv *serv = rqstp->rq_server;
@@ -661,8 +652,7 @@ svc_recv(struct svc_rqst *rqstp, long ti
/*
* Drop request
*/
-void
-svc_drop(struct svc_rqst *rqstp)
+void svc_drop(struct svc_rqst *rqstp)
{
dprintk("svc: xprt %p dropped request\n", rqstp->rq_xprt);
svc_xprt_release(rqstp);
@@ -671,8 +661,7 @@ svc_drop(struct svc_rqst *rqstp)
/*
* Return reply to client.
*/
-int
-svc_send(struct svc_rqst *rqstp)
+int svc_send(struct svc_rqst *rqstp)
{
struct svc_xprt *xprt;
int len;
@@ -708,8 +697,7 @@ svc_send(struct svc_rqst *rqstp)
* Timer function to close old temporary transports, using
* a mark-and-sweep algorithm.
*/
-static void
-svc_age_temp_xprts(unsigned long closure)
+static void svc_age_temp_xprts(unsigned long closure)
{
struct svc_serv *serv = (struct svc_serv *)closure;
struct svc_xprt *xprt;
@@ -761,8 +749,7 @@ svc_age_temp_xprts(unsigned long closure
/*
* Remove a dead transport
*/
-void
-svc_delete_xprt(struct svc_xprt *xprt)
+void svc_delete_xprt(struct svc_xprt *xprt)
{
struct svc_serv *serv;

@@ -871,8 +858,7 @@ static void svc_revisit(struct cache_def
* This code can only handle requests that consist of an xprt-header
* and rpc-header.
*/
-static struct cache_deferred_req *
-svc_defer(struct cache_req *req)
+static struct cache_deferred_req *svc_defer(struct cache_req *req)
{
struct svc_rqst *rqstp = container_of(req, struct svc_rqst, rq_chandle);
struct svc_deferred_req *dr;
@@ -971,12 +957,10 @@ struct svc_xprt *svc_find_xprt(struct sv
sin = (struct sockaddr_in *)&xprt->xpt_local;
if (af && sin->sin_family != af)
continue;
-
if (port && (sin->sin_port != port))
continue;
-
- break;
+ return xprt;
}
- return xprt;
+ return NULL;
}
EXPORT_SYMBOL_GPL(svc_find_xprt);

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

2007-10-09 16:14:15

by Tom Tucker

[permalink] [raw]
Subject: Re: [RFC,PATCH 0/7] svc: Incremental svc patchset cleanup

On Tue, 2007-10-09 at 10:35 -0500, Tom Tucker wrote:
> This patchset is a series of independent, incremental patches against
> the previously posted svc transport switch patchset. I believe each
> of these patches can be taken independently. They are presented as a
> series soley because they reflect what I believe to be the collective
> consensus of the svc transport switch review.

I realize the above may imply I'm done. I still have to post the rdma
transport driver and a deferral processing optimization.

Tom

>
> The main, non-trivial changes are as follows:
>
> - Transport class data is no longer copied to each transport instance.
>
> - A svc_xprt_find service has been added to allow a service to query
> for transport instances based on class name, address family and port.
>


-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

2007-10-09 16:14:34

by Aaron Wiebe

[permalink] [raw]
Subject: Re: [RFC,PATCH 7/7] svc: Place type on same line for new API

New to list here, so I'll apologize in advance if this has already
been discussed...

On 10/9/07, Tom Tucker <[email protected]> wrote:
>
> The convention is place the type name on the same line as the function
> name. The inline directive was also removed to allow the compiler to
> elect whether or not to inline.

The two versions of gcc that I checked do not attempt to decide
whether or not to inline without -O3. Has the kernel tree started
using -O3?

-Aaron

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

2007-10-10 02:15:44

by Greg Banks

[permalink] [raw]
Subject: Re: [RFC,PATCH 0/7] svc: Incremental svc patchset cleanup

G'day,

Compendium reply.

On Tue, Oct 09, 2007 at 10:35:39AM -0500, Tom Tucker wrote:
> This patchset is a series of independent, incremental patches against
> the previously posted svc transport switch patchset. I believe each
> of these patches can be taken independently. They are presented as a
> series soley because they reflect what I believe to be the collective
> consensus of the svc transport switch review.
>
> The main, non-trivial changes are as follows:
>
> - Transport class data is no longer copied to each transport instance.
>
> - A svc_xprt_find service has been added to allow a service to query
> for transport instances based on class name, address family and port.
>

> Subject: [RFC,PATCH 1/7] svc: Don't copy xprt_class data into svc_xprt instance

ok

> Subject: [RFC,PATCH 2/7] svc: Rename xpo_release to xpo_release_rqst

ok

> Subject: [RFC,PATCH 3/7] svc: Move setting of XPT_LISTENER bit to svc_tcp_init

ok

> Subject: [RFC,PATCH 4/7] svc: Add a sockaddr length argument to the xpo_create function.

ok

> Subject: [RFC,PATCH 5/7] svc: Remove extraneous debug svc_send printk

ok

> Subject: [RFC,PATCH 6/7] svc: Add svc API that queries for a transport instance

> @@ -242,11 +230,11 @@ static int make_socks(struct svc_serv *s
> int err = 0;
>
> if (proto == IPPROTO_UDP || nlm_udpport)
> - if (!find_xprt(serv, "udp"))
> + if (!svc_find_xprt(serv, "udp", 0, 0))
> err = svc_create_xprt(serv, "udp", nlm_udpport,
> SVC_SOCK_DEFAULTS);
> if (err >= 0 && (proto == IPPROTO_TCP || nlm_tcpport))
> - if (!find_xprt(serv, "tcp"))
> + if (!svc_find_xprt(serv, "tcp", 0, 0))
> err = svc_create_xprt(serv, "tcp", nlm_tcpport,
> SVC_SOCK_DEFAULTS);

You could use AF_UNSPEC as the "wildcard" value for address family.

> +struct svc_xprt *svc_find_xprt(struct svc_serv *serv, char *xcl_name,
> + int af, int port)
> +{
> + struct svc_xprt *xprt = NULL;
> + list_for_each_entry(xprt, &serv->sv_permsocks, xpt_list) {

Why have address family and port? Do you need them?

Also, if this is a generic, exported API, it should be taking
serv->sv_lock to protect traversal of the sv_permsocks list.

> + * Specifying 0 for the address family or port is a wildcard and will
> + * match any transport with the firt transport with the same class
> + * name active for the service.
> [...]
> + struct sockaddr_in *sin;
> + if (strcmp(xprt->xpt_class->xcl_name, xcl_name))
> + continue;
> + sin = (struct sockaddr_in *)&xprt->xpt_local;
> + if (af && sin->sin_family != af)
> + continue;
> +
> + if (port && (sin->sin_port != port))
> + continue;

So what are the intended semantics of svc_find_xprt(,,0,2049) ?
When called like that, it will reach into xprt->xpt_local assuming
it's a sockaddr_in and match the passed port number against whatever
it finds at bytes 2-3.

Perhaps this function could be pruned back to only do what it needs
to do right now, which is match on transport name?


> Subject: [RFC,PATCH 7/7] svc: Place type on same line for new API

ok

Greg.
--
Greg Banks, R&D Software Engineer, SGI Australian Software Group.
Apparently, I'm Bedevere. Which MPHG character are you?
I don't speak for SGI.

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs