When an RPC request needs to make a subordinate RPC request to portmap
to find the port number to communicate with, the 'soft' and 'intr'
flags should be copied from the original request to the subordinate request
to ensure consistent handling.
Currently the pmap client defaults to soft,nointr, so while it is
certain to timeout, it could still be 30 seconds without being
interruptible.
Note that copying the 'hard' flag down is not essential as if the pmap
request aborts, the parent request - being hard - will retry it
indefinitely. However copying it is more obviously right.
Signed-off-by: Neil Brown <[email protected]>
### Diffstat output
./net/sunrpc/pmap_clnt.c | 22 +++++++++++++++-------
1 file changed, 15 insertions(+), 7 deletions(-)
diff .prev/net/sunrpc/pmap_clnt.c ./net/sunrpc/pmap_clnt.c
--- .prev/net/sunrpc/pmap_clnt.c 2006-10-24 09:23:13.000000000 +1000
+++ ./net/sunrpc/pmap_clnt.c 2006-10-24 09:33:08.000000000 +1000
@@ -34,7 +34,8 @@ struct portmap_args {
};
static struct rpc_procinfo pmap_procedures[];
-static struct rpc_clnt * pmap_create(char *, struct sockaddr_in *, int, int);
+static struct rpc_clnt * pmap_create(char *, struct sockaddr_in *,
+ int, unsigned long);
static void pmap_getport_done(struct rpc_task *, void *);
static struct rpc_program pmap_program;
@@ -93,6 +94,7 @@ void rpc_getport(struct rpc_task *task)
struct rpc_clnt *pmap_clnt;
struct rpc_task *child;
int status;
+ unsigned long create_flags;
dprintk("RPC: %4d rpc_getport(%s, %u, %u, %d)\n",
task->tk_pid, clnt->cl_server,
@@ -125,7 +127,13 @@ void rpc_getport(struct rpc_task *task)
map->pm_xprt = xprt_get(xprt);
rpc_peeraddr(clnt, (struct sockaddr *) &addr, sizeof(addr));
- pmap_clnt = pmap_create(clnt->cl_server, &addr, map->pm_prot, 0);
+ create_flags = RPC_CLNT_CREATE_NONPRIVPORT;
+ if ( ! RPC_IS_SOFT(task))
+ create_flags |= RPC_CLNT_CREATE_HARDRTRY;
+ if ( ! RPC_TASK_UNINTERRUPTIBLE(task))
+ create_flags |= RPC_CLNT_CREATE_INTR;
+ pmap_clnt = pmap_create(clnt->cl_server, &addr, map->pm_prot,
+ create_flags);
status = PTR_ERR(pmap_clnt);
if (IS_ERR(pmap_clnt))
goto bailout;
@@ -178,7 +186,7 @@ int rpc_getport_external(struct sockaddr
NIPQUAD(sin->sin_addr.s_addr), prog, vers, prot);
sprintf(hostname, "%u.%u.%u.%u", NIPQUAD(sin->sin_addr.s_addr));
- pmap_clnt = pmap_create(hostname, sin, prot, 0);
+ pmap_clnt = pmap_create(hostname, sin, prot, RPC_CLNT_CREATE_NONPRIVPORT);
if (IS_ERR(pmap_clnt))
return PTR_ERR(pmap_clnt);
@@ -257,7 +265,7 @@ int rpc_register(u32 prog, u32 vers, int
dprintk("RPC: registering (%u, %u, %d, %u) with portmapper.\n",
prog, vers, prot, port);
- pmap_clnt = pmap_create("localhost", &sin, IPPROTO_UDP, 1);
+ pmap_clnt = pmap_create("localhost", &sin, IPPROTO_UDP, 0);
if (IS_ERR(pmap_clnt)) {
error = PTR_ERR(pmap_clnt);
dprintk("RPC: couldn't create pmap client. Error = %d\n", error);
@@ -277,7 +285,8 @@ int rpc_register(u32 prog, u32 vers, int
return error;
}
-static struct rpc_clnt *pmap_create(char *hostname, struct sockaddr_in *srvaddr, int proto, int privileged)
+static struct rpc_clnt *pmap_create(char *hostname, struct sockaddr_in *srvaddr,
+ int proto, unsigned long create_flags)
{
struct rpc_create_args args = {
.protocol = proto,
@@ -292,8 +301,7 @@ static struct rpc_clnt *pmap_create(char
};
srvaddr->sin_port = htons(RPC_PMAP_PORT);
- if (!privileged)
- args.flags |= RPC_CLNT_CREATE_NONPRIVPORT;
+ args.flags |= create_flags;
return rpc_create(&args);
}
-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
Looking at this has forced me to actually think about it ;-)
Since portmapper is always a child task, I think it is safe and
reasonable to use a "hard,intr" RPC client all the time. The parent
task will timeout automatically if it is a soft mount, right? The RPC
client should terminate the child if the parent dies.
In any event, I think the NONPRIVPORT and NO_PING flags should be
managed entirely within pmap_create(). No need to clutter the callers
with that.
On 10/23/06, NeilBrown <[email protected]> wrote:
>
> When an RPC request needs to make a subordinate RPC request to portmap
> to find the port number to communicate with, the 'soft' and 'intr'
> flags should be copied from the original request to the subordinate request
> to ensure consistent handling.
>
> Currently the pmap client defaults to soft,nointr, so while it is
> certain to timeout, it could still be 30 seconds without being
> interruptible.
>
> Note that copying the 'hard' flag down is not essential as if the pmap
> request aborts, the parent request - being hard - will retry it
> indefinitely. However copying it is more obviously right.
>
> Signed-off-by: Neil Brown <[email protected]>
>
> ### Diffstat output
> ./net/sunrpc/pmap_clnt.c | 22 +++++++++++++++-------
> 1 file changed, 15 insertions(+), 7 deletions(-)
>
> diff .prev/net/sunrpc/pmap_clnt.c ./net/sunrpc/pmap_clnt.c
> --- .prev/net/sunrpc/pmap_clnt.c 2006-10-24 09:23:13.000000000 +1000
> +++ ./net/sunrpc/pmap_clnt.c 2006-10-24 09:33:08.000000000 +1000
> @@ -34,7 +34,8 @@ struct portmap_args {
> };
>
> static struct rpc_procinfo pmap_procedures[];
> -static struct rpc_clnt * pmap_create(char *, struct sockaddr_in *, int, int);
> +static struct rpc_clnt * pmap_create(char *, struct sockaddr_in *,
> + int, unsigned long);
> static void pmap_getport_done(struct rpc_task *, void *);
> static struct rpc_program pmap_program;
>
> @@ -93,6 +94,7 @@ void rpc_getport(struct rpc_task *task)
> struct rpc_clnt *pmap_clnt;
> struct rpc_task *child;
> int status;
> + unsigned long create_flags;
>
> dprintk("RPC: %4d rpc_getport(%s, %u, %u, %d)\n",
> task->tk_pid, clnt->cl_server,
> @@ -125,7 +127,13 @@ void rpc_getport(struct rpc_task *task)
> map->pm_xprt = xprt_get(xprt);
>
> rpc_peeraddr(clnt, (struct sockaddr *) &addr, sizeof(addr));
> - pmap_clnt = pmap_create(clnt->cl_server, &addr, map->pm_prot, 0);
> + create_flags = RPC_CLNT_CREATE_NONPRIVPORT;
> + if ( ! RPC_IS_SOFT(task))
> + create_flags |= RPC_CLNT_CREATE_HARDRTRY;
> + if ( ! RPC_TASK_UNINTERRUPTIBLE(task))
> + create_flags |= RPC_CLNT_CREATE_INTR;
> + pmap_clnt = pmap_create(clnt->cl_server, &addr, map->pm_prot,
> + create_flags);
> status = PTR_ERR(pmap_clnt);
> if (IS_ERR(pmap_clnt))
> goto bailout;
> @@ -178,7 +186,7 @@ int rpc_getport_external(struct sockaddr
> NIPQUAD(sin->sin_addr.s_addr), prog, vers, prot);
>
> sprintf(hostname, "%u.%u.%u.%u", NIPQUAD(sin->sin_addr.s_addr));
> - pmap_clnt = pmap_create(hostname, sin, prot, 0);
> + pmap_clnt = pmap_create(hostname, sin, prot, RPC_CLNT_CREATE_NONPRIVPORT);
> if (IS_ERR(pmap_clnt))
> return PTR_ERR(pmap_clnt);
>
> @@ -257,7 +265,7 @@ int rpc_register(u32 prog, u32 vers, int
> dprintk("RPC: registering (%u, %u, %d, %u) with portmapper.\n",
> prog, vers, prot, port);
>
> - pmap_clnt = pmap_create("localhost", &sin, IPPROTO_UDP, 1);
> + pmap_clnt = pmap_create("localhost", &sin, IPPROTO_UDP, 0);
> if (IS_ERR(pmap_clnt)) {
> error = PTR_ERR(pmap_clnt);
> dprintk("RPC: couldn't create pmap client. Error = %d\n", error);
> @@ -277,7 +285,8 @@ int rpc_register(u32 prog, u32 vers, int
> return error;
> }
>
> -static struct rpc_clnt *pmap_create(char *hostname, struct sockaddr_in *srvaddr, int proto, int privileged)
> +static struct rpc_clnt *pmap_create(char *hostname, struct sockaddr_in *srvaddr,
> + int proto, unsigned long create_flags)
> {
> struct rpc_create_args args = {
> .protocol = proto,
> @@ -292,8 +301,7 @@ static struct rpc_clnt *pmap_create(char
> };
>
> srvaddr->sin_port = htons(RPC_PMAP_PORT);
> - if (!privileged)
> - args.flags |= RPC_CLNT_CREATE_NONPRIVPORT;
> + args.flags |= create_flags;
> return rpc_create(&args);
> }
>
>
> -------------------------------------------------------------------------
> Using Tomcat but need to do more? Need to support web services, security?
> Get stuff done quickly with pre-integrated technology to make your job easier
> Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
> http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
> _______________________________________________
> NFS maillist - [email protected]
> https://lists.sourceforge.net/lists/listinfo/nfs
>
--
"We who cut mere stones must always be envisioning cathedrals"
-- Quarry worker's creed
-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
On Tue, 2006-10-24 at 16:05 -0700, Chuck Lever wrote:
> Looking at this has forced me to actually think about it ;-)
>
> Since portmapper is always a child task, I think it is safe and
> reasonable to use a "hard,intr" RPC client all the time. The parent
> task will timeout automatically if it is a soft mount, right? The RPC
> client should terminate the child if the parent dies.
...or to set it to always be soft. The main problem with that might be
nfsroot.
Hmm... Looking at that code, though there is something that bothers me.
Why are we calling pmap_wake_portmap_waiters every time the call to
xprt_test_and_set_binding(xprt)!=0? That looks like a nasty bug.
Trond
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
On 11/15/06, Trond Myklebust <[email protected]> wrote:
> On Tue, 2006-10-24 at 16:05 -0700, Chuck Lever wrote:
> > Looking at this has forced me to actually think about it ;-)
> >
> > Since portmapper is always a child task, I think it is safe and
> > reasonable to use a "hard,intr" RPC client all the time. The parent
> > task will timeout automatically if it is a soft mount, right? The RPC
> > client should terminate the child if the parent dies.
>
> ...or to set it to always be soft.
I suppose that would be OK, since call_bind_status will retry
timed-out rpcbind requests for hard mounts.
> Hmm... Looking at that code, though there is something that bothers me.
> Why are we calling pmap_wake_portmap_waiters every time the call to
> xprt_test_and_set_binding(xprt)!=0? That looks like a nasty bug.
Well the idea is to make sure the task isn't left on the binding queue
if pmap_getport can't actually queue an rpcbind request. I don't
think it should be waking up all waiters, though -- that part is
probably wrong.
--
"We who cut mere stones must always be envisioning cathedrals"
-- Quarry worker's creed
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
On Wed, 2006-11-15 at 14:38 -0500, Chuck Lever wrote:
> > Hmm... Looking at that code, though there is something that bothers me.
> > Why are we calling pmap_wake_portmap_waiters every time the call to
> > xprt_test_and_set_binding(xprt)!=0? That looks like a nasty bug.
>
> Well the idea is to make sure the task isn't left on the binding queue
> if pmap_getport can't actually queue an rpcbind request. I don't
> think it should be waking up all waiters, though -- that part is
> probably wrong.
It shouldn't be waking anybody up if it just failed because a portmapper
request has already been created. In that case, it should just sleep and
wait for the request to finish.
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
On 11/15/06, Trond Myklebust <[email protected]> wrote:
> On Wed, 2006-11-15 at 14:38 -0500, Chuck Lever wrote:
> > > Hmm... Looking at that code, though there is something that bothers me.
> > > Why are we calling pmap_wake_portmap_waiters every time the call to
> > > xprt_test_and_set_binding(xprt)!=0? That looks like a nasty bug.
> >
> > Well the idea is to make sure the task isn't left on the binding queue
> > if pmap_getport can't actually queue an rpcbind request. I don't
> > think it should be waking up all waiters, though -- that part is
> > probably wrong.
>
> It shouldn't be waking anybody up if it just failed because a portmapper
> request has already been created. In that case, it should just sleep and
> wait for the request to finish.
OK, agreed. That logic should just set task->tk_status to -EACESS and return.
Waking up all waiters after bailout_nofree: is still excessive, though.
--
"We who cut mere stones must always be envisioning cathedrals"
-- Quarry worker's creed
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs
On Tuesday October 24, [email protected] wrote:
> Looking at this has forced me to actually think about it ;-)
Good :-)
>
> Since portmapper is always a child task, I think it is safe and
> reasonable to use a "hard,intr" RPC client all the time. The parent
> task will timeout automatically if it is a soft mount, right? The RPC
> client should terminate the child if the parent dies.
Maybe. Can one task timeout while a child task is active? If it can
then 'hard' would make sense.
I guess 'intr' makes sense to and the parent would just retry, but I
would need to pour over the code again to be sure.
>
> In any event, I think the NONPRIVPORT and NO_PING flags should be
> managed entirely within pmap_create(). No need to clutter the callers
> with that.
pmap needs a privport when registering and not when doing a lookup, so
I don't think we can get rid of all the flags.
NeilBrown
-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys - and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs