* [PATCH] nfs4: set source address when callback is generated @ 2010-12-15 5:09 Takuma Umeya 2010-12-16 3:49 ` J. Bruce Fields 0 siblings, 1 reply; 8+ messages in thread From: Takuma Umeya @ 2010-12-15 5:09 UTC (permalink / raw) To: linux-nfs; +Cc: linux-kernel when callback is generated in NFSv4 server, it doesn't set the source address. When an alias IP is utilized on NFSv4 server and suppose the client is accessing via that alias IP (e.g. eth0:0), the client invokes the callback to the IP address that is set on the original device (e.g. eth0). This behavior results in timeout of xprt. The patch sets the IP address that the client should invoke callback to. Signed-off-by: Takuma Umeya <tumeya@redhat.com> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c index 143da2e..bb5de7f 100644 --- a/fs/nfsd/nfs4callback.c +++ b/fs/nfsd/nfs4callback.c @@ -483,6 +483,7 @@ int setup_callback_client(struct nfs4_client *clp, struct nfs4_cb_conn *conn) .net = &init_net, .address = (struct sockaddr *) &conn->cb_addr, .addrsize = conn->cb_addrlen, + .saddress = (struct sockaddr *) &conn->cb_saddr, .timeout = &timeparms, .program = &cb_program, .version = 0, diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 116cab9..c7d3171 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -1177,9 +1177,11 @@ find_unconfirmed_client_by_str(const char *dname, unsigned int hashval, } static void -gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, u32 scopeid) +gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, + u32 scopeid, struct svc_rqst *rqstp) { struct nfs4_cb_conn *conn = &clp->cl_cb_conn; + struct sockaddr s_sa; unsigned short expected_family; /* Currently, we only support tcp and tcp6 for the callback channel */ @@ -1205,6 +1207,19 @@ gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, u32 scopeid) conn->cb_prog = se->se_callback_prog; conn->cb_ident = se->se_callback_ident; + switch(expected_family){ + case AF_INET: + ((struct sockaddr_in *)&s_sa)->sin_family = AF_INET; + ((struct sockaddr_in *)&s_sa)->sin_addr = rqstp-> + rq_daddr.addr; + break; + case AF_INET6: + ((struct sockaddr_in6 *)&s_sa)->sin6_family = AF_INET6; + ((struct sockaddr_in6 *)&s_sa)->sin6_addr = rqstp-> + rq_daddr.addr6; + break; + } + rpc_copy_addr((struct sockaddr *)&conn->cb_saddr, &s_sa); return; out_err: conn->cb_addr.ss_family = AF_UNSPEC; @@ -1876,7 +1891,7 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, * for consistent minorversion use throughout: */ new->cl_minorversion = 0; - gen_callback(new, setclid, rpc_get_scope_id(sa)); + gen_callback(new, setclid, rpc_get_scope_id(sa), rqstp); add_to_unconfirmed(new, strhashval); setclid->se_clientid.cl_boot = new->cl_clientid.cl_boot; setclid->se_clientid.cl_id = new->cl_clientid.cl_id; diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h index 39adc27..0cee03d 100644 --- a/fs/nfsd/state.h +++ b/fs/nfsd/state.h @@ -95,6 +95,7 @@ struct nfs4_delegation { struct nfs4_cb_conn { /* SETCLIENTID info */ struct sockaddr_storage cb_addr; + struct sockaddr_storage cb_saddr; size_t cb_addrlen; u32 cb_prog; /* used only in 4.0 case; per-session otherwise */ ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] nfs4: set source address when callback is generated 2010-12-15 5:09 [PATCH] nfs4: set source address when callback is generated Takuma Umeya @ 2010-12-16 3:49 ` J. Bruce Fields 2010-12-16 15:54 ` Chuck Lever 0 siblings, 1 reply; 8+ messages in thread From: J. Bruce Fields @ 2010-12-16 3:49 UTC (permalink / raw) To: Takuma Umeya; +Cc: linux-nfs, linux-kernel, chuck.lever On Wed, Dec 15, 2010 at 02:09:01PM +0900, Takuma Umeya wrote: > when callback is generated in NFSv4 server, it doesn't set the source > address. When an alias IP is utilized on NFSv4 server and suppose the > client is accessing via that alias IP (e.g. eth0:0), the client invokes > the callback to the IP address that is set on the original device (e.g. > eth0). This behavior results in timeout of xprt. > The patch sets the IP address that the client should invoke callback to. > > Signed-off-by: Takuma Umeya <tumeya@redhat.com> > > > diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c > index 143da2e..bb5de7f 100644 > --- a/fs/nfsd/nfs4callback.c > +++ b/fs/nfsd/nfs4callback.c > @@ -483,6 +483,7 @@ int setup_callback_client(struct nfs4_client *clp, struct nfs4_cb_conn *conn) > .net = &init_net, > .address = (struct sockaddr *) &conn->cb_addr, > .addrsize = conn->cb_addrlen, > + .saddress = (struct sockaddr *) &conn->cb_saddr, > .timeout = &timeparms, > .program = &cb_program, > .version = 0, > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index 116cab9..c7d3171 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -1177,9 +1177,11 @@ find_unconfirmed_client_by_str(const char *dname, unsigned int hashval, > } > > static void > -gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, u32 scopeid) > +gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, > + u32 scopeid, struct svc_rqst *rqstp) > { > struct nfs4_cb_conn *conn = &clp->cl_cb_conn; > + struct sockaddr s_sa; > unsigned short expected_family; > > /* Currently, we only support tcp and tcp6 for the callback channel */ > @@ -1205,6 +1207,19 @@ gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, u32 scopeid) > > conn->cb_prog = se->se_callback_prog; > conn->cb_ident = se->se_callback_ident; > + switch(expected_family){ > + case AF_INET: > + ((struct sockaddr_in *)&s_sa)->sin_family = AF_INET; > + ((struct sockaddr_in *)&s_sa)->sin_addr = rqstp-> > + rq_daddr.addr; > + break; > + case AF_INET6: > + ((struct sockaddr_in6 *)&s_sa)->sin6_family = AF_INET6; > + ((struct sockaddr_in6 *)&s_sa)->sin6_addr = rqstp-> > + rq_daddr.addr6; > + break; > + } > + rpc_copy_addr((struct sockaddr *)&conn->cb_saddr, &s_sa); Chuck, is there any reason we use this special svc_addr_u union and not a struct sockaddr_storage? This seems more verbose than necessary.... --b. > return; > out_err: > conn->cb_addr.ss_family = AF_UNSPEC; > @@ -1876,7 +1891,7 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, > * for consistent minorversion use throughout: > */ > new->cl_minorversion = 0; > - gen_callback(new, setclid, rpc_get_scope_id(sa)); > + gen_callback(new, setclid, rpc_get_scope_id(sa), rqstp); > add_to_unconfirmed(new, strhashval); > setclid->se_clientid.cl_boot = new->cl_clientid.cl_boot; > setclid->se_clientid.cl_id = new->cl_clientid.cl_id; > diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h > index 39adc27..0cee03d 100644 > --- a/fs/nfsd/state.h > +++ b/fs/nfsd/state.h > @@ -95,6 +95,7 @@ struct nfs4_delegation { > struct nfs4_cb_conn { > /* SETCLIENTID info */ > struct sockaddr_storage cb_addr; > + struct sockaddr_storage cb_saddr; > size_t cb_addrlen; > u32 cb_prog; /* used only in 4.0 case; > per-session otherwise */ > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] nfs4: set source address when callback is generated 2010-12-16 3:49 ` J. Bruce Fields @ 2010-12-16 15:54 ` Chuck Lever 2011-01-05 0:58 ` J. Bruce Fields 0 siblings, 1 reply; 8+ messages in thread From: Chuck Lever @ 2010-12-16 15:54 UTC (permalink / raw) To: J. Bruce Fields; +Cc: Takuma Umeya, linux-nfs, linux-kernel On Dec 15, 2010, at 10:49 PM, J. Bruce Fields wrote: > On Wed, Dec 15, 2010 at 02:09:01PM +0900, Takuma Umeya wrote: >> when callback is generated in NFSv4 server, it doesn't set the source >> address. When an alias IP is utilized on NFSv4 server and suppose the >> client is accessing via that alias IP (e.g. eth0:0), the client invokes >> the callback to the IP address that is set on the original device (e.g. >> eth0). This behavior results in timeout of xprt. >> The patch sets the IP address that the client should invoke callback to. >> >> Signed-off-by: Takuma Umeya <tumeya@redhat.com> >> >> >> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c >> index 143da2e..bb5de7f 100644 >> --- a/fs/nfsd/nfs4callback.c >> +++ b/fs/nfsd/nfs4callback.c >> @@ -483,6 +483,7 @@ int setup_callback_client(struct nfs4_client *clp, struct nfs4_cb_conn *conn) >> .net = &init_net, >> .address = (struct sockaddr *) &conn->cb_addr, >> .addrsize = conn->cb_addrlen, >> + .saddress = (struct sockaddr *) &conn->cb_saddr, >> .timeout = &timeparms, >> .program = &cb_program, >> .version = 0, >> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c >> index 116cab9..c7d3171 100644 >> --- a/fs/nfsd/nfs4state.c >> +++ b/fs/nfsd/nfs4state.c >> @@ -1177,9 +1177,11 @@ find_unconfirmed_client_by_str(const char *dname, unsigned int hashval, >> } >> >> static void >> -gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, u32 scopeid) >> +gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, >> + u32 scopeid, struct svc_rqst *rqstp) >> { >> struct nfs4_cb_conn *conn = &clp->cl_cb_conn; >> + struct sockaddr s_sa; >> unsigned short expected_family; >> >> /* Currently, we only support tcp and tcp6 for the callback channel */ >> @@ -1205,6 +1207,19 @@ gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, u32 scopeid) >> >> conn->cb_prog = se->se_callback_prog; >> conn->cb_ident = se->se_callback_ident; >> + switch(expected_family){ >> + case AF_INET: >> + ((struct sockaddr_in *)&s_sa)->sin_family = AF_INET; >> + ((struct sockaddr_in *)&s_sa)->sin_addr = rqstp-> >> + rq_daddr.addr; >> + break; >> + case AF_INET6: >> + ((struct sockaddr_in6 *)&s_sa)->sin6_family = AF_INET6; >> + ((struct sockaddr_in6 *)&s_sa)->sin6_addr = rqstp-> >> + rq_daddr.addr6; >> + break; >> + } >> + rpc_copy_addr((struct sockaddr *)&conn->cb_saddr, &s_sa); > > Chuck, is there any reason we use this special svc_addr_u union and not > a struct sockaddr_storage? This seems more verbose than necessary.... I don't recall creating svc_addr_u, but I'll take a stab at a guess. It looks like someone thought that we should retain the idea of storing just the address part of the socket address, and not the other stuff (like the family and port, since this code doesn't appear to need that additional information). It greatly reduces the size of the field. A full sockaddr_storage is more than 128 bytes, since it has to be able to store an AF_UNIX pathname. Doing this, there is a lot less data to keep around, but an IPv6 socket address has other items outside of in6_addr that can be used to form a full address. We decided at some point we could copy this information from the other address storage field in the rqstp. But the result of this space savings means we must construct a full socket address when needed, using logic such as the above. > > --b. > >> return; >> out_err: >> conn->cb_addr.ss_family = AF_UNSPEC; >> @@ -1876,7 +1891,7 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, >> * for consistent minorversion use throughout: >> */ >> new->cl_minorversion = 0; >> - gen_callback(new, setclid, rpc_get_scope_id(sa)); >> + gen_callback(new, setclid, rpc_get_scope_id(sa), rqstp); >> add_to_unconfirmed(new, strhashval); >> setclid->se_clientid.cl_boot = new->cl_clientid.cl_boot; >> setclid->se_clientid.cl_id = new->cl_clientid.cl_id; >> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h >> index 39adc27..0cee03d 100644 >> --- a/fs/nfsd/state.h >> +++ b/fs/nfsd/state.h >> @@ -95,6 +95,7 @@ struct nfs4_delegation { >> struct nfs4_cb_conn { >> /* SETCLIENTID info */ >> struct sockaddr_storage cb_addr; >> + struct sockaddr_storage cb_saddr; >> size_t cb_addrlen; >> u32 cb_prog; /* used only in 4.0 case; >> per-session otherwise */ >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> Please read the FAQ at http://www.tux.org/lkml/ -- Chuck Lever chuck[dot]lever[at]oracle[dot]com ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] nfs4: set source address when callback is generated 2010-12-16 15:54 ` Chuck Lever @ 2011-01-05 0:58 ` J. Bruce Fields 2011-01-05 17:06 ` Chuck Lever 0 siblings, 1 reply; 8+ messages in thread From: J. Bruce Fields @ 2011-01-05 0:58 UTC (permalink / raw) To: Chuck Lever; +Cc: Takuma Umeya, linux-nfs, linux-kernel On Thu, Dec 16, 2010 at 10:54:00AM -0500, Chuck Lever wrote: > I don't recall creating svc_addr_u, but I'll take a stab at a guess. > > It looks like someone thought that we should retain the idea of storing just the address part of the socket address, and not the other stuff (like the family and port, since this code doesn't appear to need that additional information). It greatly reduces the size of the field. A full sockaddr_storage is more than 128 bytes, since it has to be able to store an AF_UNIX pathname. > > Doing this, there is a lot less data to keep around, but an IPv6 socket address has other items outside of in6_addr that can be used to form a full address. We decided at some point we could copy this information from the other address storage field in the rqstp. > > But the result of this space savings means we must construct a full socket address when needed, using logic such as the above. Seems to me we should either just waste the extra 100 bytes or define something that would be useful elsewhere as well.... But if we do it this way we can at least simplify a little. --b. commit 6f3d772fb8a039de8f21d725f5e38c252b4c0efd Author: Takuma Umeya <tumeya@redhat.com> Date: Wed Dec 15 14:09:01 2010 +0900 nfs4: set source address when callback is generated when callback is generated in NFSv4 server, it doesn't set the source address. When an alias IP is utilized on NFSv4 server and suppose the client is accessing via that alias IP (e.g. eth0:0), the client invokes the callback to the IP address that is set on the original device (e.g. eth0). This behavior results in timeout of xprt. The patch sets the IP address that the client should invoke callback to. Signed-off-by: Takuma Umeya <tumeya@redhat.com> [bfields@redhat.com: Simplify gen_callback arguments, use helper function] Signed-off-by: J. Bruce Fields <bfields@redhat.com> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c index a085805..dd183af 100644 --- a/fs/nfsd/nfs4callback.c +++ b/fs/nfsd/nfs4callback.c @@ -484,6 +484,7 @@ static int setup_callback_client(struct nfs4_client *clp, .net = &init_net, .address = (struct sockaddr *) &conn->cb_addr, .addrsize = conn->cb_addrlen, + .saddress = (struct sockaddr *) &conn->cb_saddr, .timeout = &timeparms, .program = &cb_program, .version = 0, diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 87d4c48..b583e4e 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -1163,10 +1163,26 @@ find_unconfirmed_client_by_str(const char *dname, unsigned int hashval) return NULL; } +static void rpc_svcaddr2sockaddr(struct sockaddr *sa, unsigned short family, union svc_addr_u *svcaddr) +{ + switch (family) { + case AF_INET: + ((struct sockaddr_in *)sa)->sin_family = AF_INET; + ((struct sockaddr_in *)sa)->sin_addr = svcaddr->addr; + return; + case AF_INET6: + ((struct sockaddr_in6 *)sa)->sin6_family = AF_INET6; + ((struct sockaddr_in6 *)sa)->sin6_addr = svcaddr->addr6; + return; + } +} + static void -gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, u32 scopeid) +gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, struct svc_rqst *rqstp) { struct nfs4_cb_conn *conn = &clp->cl_cb_conn; + struct sockaddr *sa = svc_addr(rqstp); + u32 scopeid = rpc_get_scope_id(sa); unsigned short expected_family; /* Currently, we only support tcp and tcp6 for the callback channel */ @@ -1192,6 +1208,7 @@ gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, u32 scopeid) conn->cb_prog = se->se_callback_prog; conn->cb_ident = se->se_callback_ident; + rpc_svcaddr2sockaddr((struct sockaddr *)&conn->cb_saddr, expected_family, &rqstp->rq_daddr); return; out_err: conn->cb_addr.ss_family = AF_UNSPEC; @@ -1768,7 +1785,6 @@ __be32 nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, struct nfsd4_setclientid *setclid) { - struct sockaddr *sa = svc_addr(rqstp); struct xdr_netobj clname = { .len = setclid->se_namelen, .data = setclid->se_name, @@ -1871,7 +1887,7 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, * for consistent minorversion use throughout: */ new->cl_minorversion = 0; - gen_callback(new, setclid, rpc_get_scope_id(sa)); + gen_callback(new, setclid, rqstp); add_to_unconfirmed(new, strhashval); setclid->se_clientid.cl_boot = new->cl_clientid.cl_boot; setclid->se_clientid.cl_id = new->cl_clientid.cl_id; diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h index 84b2302..cf6dc83 100644 --- a/fs/nfsd/state.h +++ b/fs/nfsd/state.h @@ -96,6 +96,7 @@ struct nfs4_delegation { struct nfs4_cb_conn { /* SETCLIENTID info */ struct sockaddr_storage cb_addr; + struct sockaddr_storage cb_saddr; size_t cb_addrlen; u32 cb_prog; /* used only in 4.0 case; per-session otherwise */ ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] nfs4: set source address when callback is generated 2011-01-05 0:58 ` J. Bruce Fields @ 2011-01-05 17:06 ` Chuck Lever 2011-01-05 17:11 ` J. Bruce Fields 0 siblings, 1 reply; 8+ messages in thread From: Chuck Lever @ 2011-01-05 17:06 UTC (permalink / raw) To: J. Bruce Fields; +Cc: Takuma Umeya, linux-nfs, linux-kernel On Jan 4, 2011, at 7:58 PM, J. Bruce Fields wrote: > On Thu, Dec 16, 2010 at 10:54:00AM -0500, Chuck Lever wrote: >> I don't recall creating svc_addr_u, but I'll take a stab at a guess. >> >> It looks like someone thought that we should retain the idea of storing just the address part of the socket address, and not the other stuff (like the family and port, since this code doesn't appear to need that additional information). It greatly reduces the size of the field. A full sockaddr_storage is more than 128 bytes, since it has to be able to store an AF_UNIX pathname. >> >> Doing this, there is a lot less data to keep around, but an IPv6 socket address has other items outside of in6_addr that can be used to form a full address. We decided at some point we could copy this information from the other address storage field in the rqstp. >> >> But the result of this space savings means we must construct a full socket address when needed, using logic such as the above. > > Seems to me we should either just waste the extra 100 bytes or define > something that would be useful elsewhere as well.... In nfs-utils, we define: union nfs_sockaddr { struct sockaddr_in s4; struct sockaddr_in6 s6; struct sockaddr sa; }; A variable of this type is large enough to hold a full IPv6 sockaddr, but is significantly smaller than a sockaddr_storage. The addition of the "struct sockaddr" element is to enable access to such variables via a "struct sockaddr *" without type punning. This seems to be preferred by gcc over type casting in order to handle optimizations involving address aliasing. It also allows more precise type checking. A full conversion to use such a construct in kernel RPC and NFS components is, I fear, too late for 2.6.38, but might be considered for a future release if there is consensus on this approach. > But if we do it this way we can at least simplify a little. > > --b. > > commit 6f3d772fb8a039de8f21d725f5e38c252b4c0efd > Author: Takuma Umeya <tumeya@redhat.com> > Date: Wed Dec 15 14:09:01 2010 +0900 > > nfs4: set source address when callback is generated > > when callback is generated in NFSv4 server, it doesn't set the source > address. When an alias IP is utilized on NFSv4 server and suppose the > client is accessing via that alias IP (e.g. eth0:0), the client invokes > the callback to the IP address that is set on the original device (e.g. > eth0). This behavior results in timeout of xprt. > The patch sets the IP address that the client should invoke callback to. > > Signed-off-by: Takuma Umeya <tumeya@redhat.com> > [bfields@redhat.com: Simplify gen_callback arguments, use helper function] > Signed-off-by: J. Bruce Fields <bfields@redhat.com> > > diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c > index a085805..dd183af 100644 > --- a/fs/nfsd/nfs4callback.c > +++ b/fs/nfsd/nfs4callback.c > @@ -484,6 +484,7 @@ static int setup_callback_client(struct nfs4_client *clp, > .net = &init_net, > .address = (struct sockaddr *) &conn->cb_addr, > .addrsize = conn->cb_addrlen, > + .saddress = (struct sockaddr *) &conn->cb_saddr, > .timeout = &timeparms, > .program = &cb_program, > .version = 0, > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > index 87d4c48..b583e4e 100644 > --- a/fs/nfsd/nfs4state.c > +++ b/fs/nfsd/nfs4state.c > @@ -1163,10 +1163,26 @@ find_unconfirmed_client_by_str(const char *dname, unsigned int hashval) > return NULL; > } > > +static void rpc_svcaddr2sockaddr(struct sockaddr *sa, unsigned short family, union svc_addr_u *svcaddr) > +{ > + switch (family) { > + case AF_INET: > + ((struct sockaddr_in *)sa)->sin_family = AF_INET; > + ((struct sockaddr_in *)sa)->sin_addr = svcaddr->addr; > + return; > + case AF_INET6: > + ((struct sockaddr_in6 *)sa)->sin6_family = AF_INET6; > + ((struct sockaddr_in6 *)sa)->sin6_addr = svcaddr->addr6; > + return; > + } > +} > + > static void > -gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, u32 scopeid) > +gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, struct svc_rqst *rqstp) > { > struct nfs4_cb_conn *conn = &clp->cl_cb_conn; > + struct sockaddr *sa = svc_addr(rqstp); > + u32 scopeid = rpc_get_scope_id(sa); > unsigned short expected_family; > > /* Currently, we only support tcp and tcp6 for the callback channel */ > @@ -1192,6 +1208,7 @@ gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, u32 scopeid) > > conn->cb_prog = se->se_callback_prog; > conn->cb_ident = se->se_callback_ident; > + rpc_svcaddr2sockaddr((struct sockaddr *)&conn->cb_saddr, expected_family, &rqstp->rq_daddr); > return; > out_err: > conn->cb_addr.ss_family = AF_UNSPEC; > @@ -1768,7 +1785,6 @@ __be32 > nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, > struct nfsd4_setclientid *setclid) > { > - struct sockaddr *sa = svc_addr(rqstp); > struct xdr_netobj clname = { > .len = setclid->se_namelen, > .data = setclid->se_name, > @@ -1871,7 +1887,7 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, > * for consistent minorversion use throughout: > */ > new->cl_minorversion = 0; > - gen_callback(new, setclid, rpc_get_scope_id(sa)); > + gen_callback(new, setclid, rqstp); > add_to_unconfirmed(new, strhashval); > setclid->se_clientid.cl_boot = new->cl_clientid.cl_boot; > setclid->se_clientid.cl_id = new->cl_clientid.cl_id; > diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h > index 84b2302..cf6dc83 100644 > --- a/fs/nfsd/state.h > +++ b/fs/nfsd/state.h > @@ -96,6 +96,7 @@ struct nfs4_delegation { > struct nfs4_cb_conn { > /* SETCLIENTID info */ > struct sockaddr_storage cb_addr; > + struct sockaddr_storage cb_saddr; > size_t cb_addrlen; > u32 cb_prog; /* used only in 4.0 case; > per-session otherwise */ -- Chuck Lever chuck[dot]lever[at]oracle[dot]com ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] nfs4: set source address when callback is generated 2011-01-05 17:06 ` Chuck Lever @ 2011-01-05 17:11 ` J. Bruce Fields 2011-01-05 17:18 ` Chuck Lever 0 siblings, 1 reply; 8+ messages in thread From: J. Bruce Fields @ 2011-01-05 17:11 UTC (permalink / raw) To: Chuck Lever; +Cc: Takuma Umeya, linux-nfs, linux-kernel On Wed, Jan 05, 2011 at 12:06:04PM -0500, Chuck Lever wrote: > > On Jan 4, 2011, at 7:58 PM, J. Bruce Fields wrote: > > > On Thu, Dec 16, 2010 at 10:54:00AM -0500, Chuck Lever wrote: > >> I don't recall creating svc_addr_u, but I'll take a stab at a guess. > >> > >> It looks like someone thought that we should retain the idea of storing just the address part of the socket address, and not the other stuff (like the family and port, since this code doesn't appear to need that additional information). It greatly reduces the size of the field. A full sockaddr_storage is more than 128 bytes, since it has to be able to store an AF_UNIX pathname. > >> > >> Doing this, there is a lot less data to keep around, but an IPv6 socket address has other items outside of in6_addr that can be used to form a full address. We decided at some point we could copy this information from the other address storage field in the rqstp. > >> > >> But the result of this space savings means we must construct a full socket address when needed, using logic such as the above. > > > > Seems to me we should either just waste the extra 100 bytes or define > > something that would be useful elsewhere as well.... > > In nfs-utils, we define: > > union nfs_sockaddr { > struct sockaddr_in s4; > struct sockaddr_in6 s6; > struct sockaddr sa; > }; > > A variable of this type is large enough to hold a full IPv6 sockaddr, but is significantly smaller than a sockaddr_storage. > > The addition of the "struct sockaddr" element is to enable access to such variables via a "struct sockaddr *" without type punning. This seems to be preferred by gcc over type casting in order to handle optimizations involving address aliasing. It also allows more precise type checking. Sounds reasonable to me. > > A full conversion to use such a construct in kernel RPC and NFS components is, I fear, too late for 2.6.38, but might be considered for a future release if there is consensus on this approach. OK; I suppose for now I'll apply my revision of Takuma Umeya's patch below (if I didn't screw it up). --b. > > > But if we do it this way we can at least simplify a little. > > > > --b. > > > > commit 6f3d772fb8a039de8f21d725f5e38c252b4c0efd > > Author: Takuma Umeya <tumeya@redhat.com> > > Date: Wed Dec 15 14:09:01 2010 +0900 > > > > nfs4: set source address when callback is generated > > > > when callback is generated in NFSv4 server, it doesn't set the source > > address. When an alias IP is utilized on NFSv4 server and suppose the > > client is accessing via that alias IP (e.g. eth0:0), the client invokes > > the callback to the IP address that is set on the original device (e.g. > > eth0). This behavior results in timeout of xprt. > > The patch sets the IP address that the client should invoke callback to. > > > > Signed-off-by: Takuma Umeya <tumeya@redhat.com> > > [bfields@redhat.com: Simplify gen_callback arguments, use helper function] > > Signed-off-by: J. Bruce Fields <bfields@redhat.com> > > > > diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c > > index a085805..dd183af 100644 > > --- a/fs/nfsd/nfs4callback.c > > +++ b/fs/nfsd/nfs4callback.c > > @@ -484,6 +484,7 @@ static int setup_callback_client(struct nfs4_client *clp, > > .net = &init_net, > > .address = (struct sockaddr *) &conn->cb_addr, > > .addrsize = conn->cb_addrlen, > > + .saddress = (struct sockaddr *) &conn->cb_saddr, > > .timeout = &timeparms, > > .program = &cb_program, > > .version = 0, > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c > > index 87d4c48..b583e4e 100644 > > --- a/fs/nfsd/nfs4state.c > > +++ b/fs/nfsd/nfs4state.c > > @@ -1163,10 +1163,26 @@ find_unconfirmed_client_by_str(const char *dname, unsigned int hashval) > > return NULL; > > } > > > > +static void rpc_svcaddr2sockaddr(struct sockaddr *sa, unsigned short family, union svc_addr_u *svcaddr) > > +{ > > + switch (family) { > > + case AF_INET: > > + ((struct sockaddr_in *)sa)->sin_family = AF_INET; > > + ((struct sockaddr_in *)sa)->sin_addr = svcaddr->addr; > > + return; > > + case AF_INET6: > > + ((struct sockaddr_in6 *)sa)->sin6_family = AF_INET6; > > + ((struct sockaddr_in6 *)sa)->sin6_addr = svcaddr->addr6; > > + return; > > + } > > +} > > + > > static void > > -gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, u32 scopeid) > > +gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, struct svc_rqst *rqstp) > > { > > struct nfs4_cb_conn *conn = &clp->cl_cb_conn; > > + struct sockaddr *sa = svc_addr(rqstp); > > + u32 scopeid = rpc_get_scope_id(sa); > > unsigned short expected_family; > > > > /* Currently, we only support tcp and tcp6 for the callback channel */ > > @@ -1192,6 +1208,7 @@ gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, u32 scopeid) > > > > conn->cb_prog = se->se_callback_prog; > > conn->cb_ident = se->se_callback_ident; > > + rpc_svcaddr2sockaddr((struct sockaddr *)&conn->cb_saddr, expected_family, &rqstp->rq_daddr); > > return; > > out_err: > > conn->cb_addr.ss_family = AF_UNSPEC; > > @@ -1768,7 +1785,6 @@ __be32 > > nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, > > struct nfsd4_setclientid *setclid) > > { > > - struct sockaddr *sa = svc_addr(rqstp); > > struct xdr_netobj clname = { > > .len = setclid->se_namelen, > > .data = setclid->se_name, > > @@ -1871,7 +1887,7 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, > > * for consistent minorversion use throughout: > > */ > > new->cl_minorversion = 0; > > - gen_callback(new, setclid, rpc_get_scope_id(sa)); > > + gen_callback(new, setclid, rqstp); > > add_to_unconfirmed(new, strhashval); > > setclid->se_clientid.cl_boot = new->cl_clientid.cl_boot; > > setclid->se_clientid.cl_id = new->cl_clientid.cl_id; > > diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h > > index 84b2302..cf6dc83 100644 > > --- a/fs/nfsd/state.h > > +++ b/fs/nfsd/state.h > > @@ -96,6 +96,7 @@ struct nfs4_delegation { > > struct nfs4_cb_conn { > > /* SETCLIENTID info */ > > struct sockaddr_storage cb_addr; > > + struct sockaddr_storage cb_saddr; > > size_t cb_addrlen; > > u32 cb_prog; /* used only in 4.0 case; > > per-session otherwise */ > > -- > Chuck Lever > chuck[dot]lever[at]oracle[dot]com > > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] nfs4: set source address when callback is generated 2011-01-05 17:11 ` J. Bruce Fields @ 2011-01-05 17:18 ` Chuck Lever 2011-01-11 15:09 ` J. Bruce Fields 0 siblings, 1 reply; 8+ messages in thread From: Chuck Lever @ 2011-01-05 17:18 UTC (permalink / raw) To: J. Bruce Fields; +Cc: Takuma Umeya, linux-nfs, linux-kernel On Jan 5, 2011, at 12:11 PM, J. Bruce Fields wrote: > On Wed, Jan 05, 2011 at 12:06:04PM -0500, Chuck Lever wrote: >> >> On Jan 4, 2011, at 7:58 PM, J. Bruce Fields wrote: >> >>> On Thu, Dec 16, 2010 at 10:54:00AM -0500, Chuck Lever wrote: >>>> I don't recall creating svc_addr_u, but I'll take a stab at a guess. >>>> >>>> It looks like someone thought that we should retain the idea of storing just the address part of the socket address, and not the other stuff (like the family and port, since this code doesn't appear to need that additional information). It greatly reduces the size of the field. A full sockaddr_storage is more than 128 bytes, since it has to be able to store an AF_UNIX pathname. >>>> >>>> Doing this, there is a lot less data to keep around, but an IPv6 socket address has other items outside of in6_addr that can be used to form a full address. We decided at some point we could copy this information from the other address storage field in the rqstp. >>>> >>>> But the result of this space savings means we must construct a full socket address when needed, using logic such as the above. >>> >>> Seems to me we should either just waste the extra 100 bytes or define >>> something that would be useful elsewhere as well.... >> >> In nfs-utils, we define: >> >> union nfs_sockaddr { >> struct sockaddr_in s4; >> struct sockaddr_in6 s6; >> struct sockaddr sa; >> }; >> >> A variable of this type is large enough to hold a full IPv6 sockaddr, but is significantly smaller than a sockaddr_storage. >> >> The addition of the "struct sockaddr" element is to enable access to such variables via a "struct sockaddr *" without type punning. This seems to be preferred by gcc over type casting in order to handle optimizations involving address aliasing. It also allows more precise type checking. > > Sounds reasonable to me. > >> >> A full conversion to use such a construct in kernel RPC and NFS components is, I fear, too late for 2.6.38, but might be considered for a future release if there is consensus on this approach. > > OK; I suppose for now I'll apply my revision of Takuma Umeya's patch > below (if I didn't screw it up). My thinking cap is still on vacation. I don't see anything immediately wrong with this as a temporary fix. Has anyone tested this with a multi-homed IPv6 server? How about link-local IPv6 addresses? > --b. > >> >>> But if we do it this way we can at least simplify a little. >>> >>> --b. >>> >>> commit 6f3d772fb8a039de8f21d725f5e38c252b4c0efd >>> Author: Takuma Umeya <tumeya@redhat.com> >>> Date: Wed Dec 15 14:09:01 2010 +0900 >>> >>> nfs4: set source address when callback is generated >>> >>> when callback is generated in NFSv4 server, it doesn't set the source >>> address. When an alias IP is utilized on NFSv4 server and suppose the >>> client is accessing via that alias IP (e.g. eth0:0), the client invokes >>> the callback to the IP address that is set on the original device (e.g. >>> eth0). This behavior results in timeout of xprt. >>> The patch sets the IP address that the client should invoke callback to. >>> >>> Signed-off-by: Takuma Umeya <tumeya@redhat.com> >>> [bfields@redhat.com: Simplify gen_callback arguments, use helper function] >>> Signed-off-by: J. Bruce Fields <bfields@redhat.com> >>> >>> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c >>> index a085805..dd183af 100644 >>> --- a/fs/nfsd/nfs4callback.c >>> +++ b/fs/nfsd/nfs4callback.c >>> @@ -484,6 +484,7 @@ static int setup_callback_client(struct nfs4_client *clp, >>> .net = &init_net, >>> .address = (struct sockaddr *) &conn->cb_addr, >>> .addrsize = conn->cb_addrlen, >>> + .saddress = (struct sockaddr *) &conn->cb_saddr, >>> .timeout = &timeparms, >>> .program = &cb_program, >>> .version = 0, >>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c >>> index 87d4c48..b583e4e 100644 >>> --- a/fs/nfsd/nfs4state.c >>> +++ b/fs/nfsd/nfs4state.c >>> @@ -1163,10 +1163,26 @@ find_unconfirmed_client_by_str(const char *dname, unsigned int hashval) >>> return NULL; >>> } >>> >>> +static void rpc_svcaddr2sockaddr(struct sockaddr *sa, unsigned short family, union svc_addr_u *svcaddr) >>> +{ >>> + switch (family) { >>> + case AF_INET: >>> + ((struct sockaddr_in *)sa)->sin_family = AF_INET; >>> + ((struct sockaddr_in *)sa)->sin_addr = svcaddr->addr; >>> + return; >>> + case AF_INET6: >>> + ((struct sockaddr_in6 *)sa)->sin6_family = AF_INET6; >>> + ((struct sockaddr_in6 *)sa)->sin6_addr = svcaddr->addr6; >>> + return; >>> + } >>> +} >>> + >>> static void >>> -gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, u32 scopeid) >>> +gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, struct svc_rqst *rqstp) >>> { >>> struct nfs4_cb_conn *conn = &clp->cl_cb_conn; >>> + struct sockaddr *sa = svc_addr(rqstp); >>> + u32 scopeid = rpc_get_scope_id(sa); >>> unsigned short expected_family; >>> >>> /* Currently, we only support tcp and tcp6 for the callback channel */ >>> @@ -1192,6 +1208,7 @@ gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, u32 scopeid) >>> >>> conn->cb_prog = se->se_callback_prog; >>> conn->cb_ident = se->se_callback_ident; >>> + rpc_svcaddr2sockaddr((struct sockaddr *)&conn->cb_saddr, expected_family, &rqstp->rq_daddr); >>> return; >>> out_err: >>> conn->cb_addr.ss_family = AF_UNSPEC; >>> @@ -1768,7 +1785,6 @@ __be32 >>> nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, >>> struct nfsd4_setclientid *setclid) >>> { >>> - struct sockaddr *sa = svc_addr(rqstp); >>> struct xdr_netobj clname = { >>> .len = setclid->se_namelen, >>> .data = setclid->se_name, >>> @@ -1871,7 +1887,7 @@ nfsd4_setclientid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, >>> * for consistent minorversion use throughout: >>> */ >>> new->cl_minorversion = 0; >>> - gen_callback(new, setclid, rpc_get_scope_id(sa)); >>> + gen_callback(new, setclid, rqstp); >>> add_to_unconfirmed(new, strhashval); >>> setclid->se_clientid.cl_boot = new->cl_clientid.cl_boot; >>> setclid->se_clientid.cl_id = new->cl_clientid.cl_id; >>> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h >>> index 84b2302..cf6dc83 100644 >>> --- a/fs/nfsd/state.h >>> +++ b/fs/nfsd/state.h >>> @@ -96,6 +96,7 @@ struct nfs4_delegation { >>> struct nfs4_cb_conn { >>> /* SETCLIENTID info */ >>> struct sockaddr_storage cb_addr; >>> + struct sockaddr_storage cb_saddr; >>> size_t cb_addrlen; >>> u32 cb_prog; /* used only in 4.0 case; >>> per-session otherwise */ >> >> -- >> Chuck Lever >> chuck[dot]lever[at]oracle[dot]com >> >> >> >> -- Chuck Lever chuck[dot]lever[at]oracle[dot]com ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] nfs4: set source address when callback is generated 2011-01-05 17:18 ` Chuck Lever @ 2011-01-11 15:09 ` J. Bruce Fields 0 siblings, 0 replies; 8+ messages in thread From: J. Bruce Fields @ 2011-01-11 15:09 UTC (permalink / raw) To: Chuck Lever; +Cc: Takuma Umeya, linux-nfs, linux-kernel On Wed, Jan 05, 2011 at 12:18:37PM -0500, Chuck Lever wrote: > > On Jan 5, 2011, at 12:11 PM, J. Bruce Fields wrote: > > > On Wed, Jan 05, 2011 at 12:06:04PM -0500, Chuck Lever wrote: > >> > >> On Jan 4, 2011, at 7:58 PM, J. Bruce Fields wrote: > >> > >>> On Thu, Dec 16, 2010 at 10:54:00AM -0500, Chuck Lever wrote: > >>>> I don't recall creating svc_addr_u, but I'll take a stab at a > >>>> guess. > >>>> > >>>> It looks like someone thought that we should retain the idea of > >>>> storing just the address part of the socket address, and not the > >>>> other stuff (like the family and port, since this code doesn't > >>>> appear to need that additional information). It greatly reduces > >>>> the size of the field. A full sockaddr_storage is more than 128 > >>>> bytes, since it has to be able to store an AF_UNIX pathname. > >>>> > >>>> Doing this, there is a lot less data to keep around, but an IPv6 > >>>> socket address has other items outside of in6_addr that can be > >>>> used to form a full address. We decided at some point we could > >>>> copy this information from the other address storage field in the > >>>> rqstp. > >>>> > >>>> But the result of this space savings means we must construct a > >>>> full socket address when needed, using logic such as the above. > >>> > >>> Seems to me we should either just waste the extra 100 bytes or > >>> define something that would be useful elsewhere as well.... > >> > >> In nfs-utils, we define: > >> > >> union nfs_sockaddr { struct sockaddr_in s4; struct sockaddr_in6 > >> s6; struct sockaddr sa; }; > >> > >> A variable of this type is large enough to hold a full IPv6 > >> sockaddr, but is significantly smaller than a sockaddr_storage. > >> > >> The addition of the "struct sockaddr" element is to enable access > >> to such variables via a "struct sockaddr *" without type punning. > >> This seems to be preferred by gcc over type casting in order to > >> handle optimizations involving address aliasing. It also allows > >> more precise type checking. > > > > Sounds reasonable to me. > > > >> > >> A full conversion to use such a construct in kernel RPC and NFS > >> components is, I fear, too late for 2.6.38, but might be considered > >> for a future release if there is consensus on this approach. > > > > OK; I suppose for now I'll apply my revision of Takuma Umeya's patch > > below (if I didn't screw it up). > > My thinking cap is still on vacation. I don't see anything > immediately wrong with this as a temporary fix. Has anyone tested > this with a multi-homed IPv6 server? How about link-local IPv6 > addresses? I believe the original patch was tested with a multi-homed server, but probably just IPv4. It would be useful to retest to make sure I didn't introduce a typo on cleanup (my version is pushed to git://linux-nfs.org/~bfields/linux.git for-2.6.38 now). And, yes, the IPv6 cases would be good to test as well. --b. ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2011-01-11 15:09 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-12-15 5:09 [PATCH] nfs4: set source address when callback is generated Takuma Umeya 2010-12-16 3:49 ` J. Bruce Fields 2010-12-16 15:54 ` Chuck Lever 2011-01-05 0:58 ` J. Bruce Fields 2011-01-05 17:06 ` Chuck Lever 2011-01-05 17:11 ` J. Bruce Fields 2011-01-05 17:18 ` Chuck Lever 2011-01-11 15:09 ` J. Bruce Fields
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).