J. Bruce Fields: > On Mon, Aug 22, 2011 at 03:32:13PM -0400, Chuck Lever wrote: >> On Aug 22, 2011, at 3:26 PM, J. Bruce Fields wrote: >> >>> On Sat, Aug 20, 2011 at 06:24:29PM +0800, Mi Jinlong wrote: >>>> +/* >>>> + * Add scope id for LINKLOCAL address >>>> + */ >>>> +struct in6_addr_scopeid{ >>>> + struct in6_addr sin6_addr; >>>> + __u32 sin6_scope_id; >>>> +}; >>>> + >>>> union svc_addr_u { >>>> - struct in_addr addr; >>>> - struct in6_addr addr6; >>>> + struct in_addr addr; >>>> + struct in6_addr_scopeid addr6; >>> By the way, is there any reason why nfsd really needs its own address >>> structure? Shouldn't we use sockaddr_storage or something? I feel like >>> we've got a little too much one-off address handling in nfsd. >> That would be my only complaint about the patch. >> >> I think we chose a smaller struct here to save space, and we could do that because we didn't need a port number or scope ID. If a scope ID is indeed required, then we should consider something larger like a struct sockaddr_storage, IMO. > > If we care about the size of struct svc_rqst: rq_vec and rq_pages fields > are 4k and 2k, respectively, and the rest is lost in the noise. > > Mi Jinlong, would you have the time to try this? > > - Replace svc_addr_u by sockaddr_storage > - See of that allows us to delete some lines of code in nfsd by > using standard helper functions instead of copying things by > hand. > > You'd do that in one patch, then apply your scope-id fix on top of it as > a second patch. After replace svc_addr_u by sockaddr_storage, the scope-id fix isn't needed. thanks, Mi Jinlong >From 86bb57f2d7c29124ae73bd121bb6b97a72fe6f4d Mon Sep 17 00:00:00 2001 From: Mi Jinlong <mijinlong@xxxxxxxxxxxxxx> Date: Mon, 24 Aug 2011 13:22:39 +0800 Subject: [PATCH] SUNRPC: Replace svc_addr_u by sockaddr_storage For IPv6 local address, lockd can not callback to client for missing scope id when binding address at inet6_bind: 324 if (addr_type & IPV6_ADDR_LINKLOCAL) { 325 if (addr_len >= sizeof(struct sockaddr_in6) && 326 addr->sin6_scope_id) { 327 /* Override any existing binding, if another one 328 * is supplied by user. 329 */ 330 sk->sk_bound_dev_if = addr->sin6_scope_id; 331 } 332 333 /* Binding to link-local address requires an interface */ 334 if (!sk->sk_bound_dev_if) { 335 err = -EINVAL; 336 goto out_unlock; 337 } Replacing svc_addr_u by sockaddr_storage, let rqstp->rq_daddr contains more info besides address. Signed-off-by: Mi Jinlong <mijinlong@xxxxxxxxxxxxxx> --- fs/lockd/host.c | 14 ++------------ fs/nfsd/nfs4state.c | 16 +--------------- include/linux/sunrpc/svc.h | 30 +++++++++++++++++++++--------- net/sunrpc/svc_xprt.c | 13 ++----------- net/sunrpc/svcsock.c | 9 +++++---- 5 files changed, 31 insertions(+), 51 deletions(-) diff --git a/fs/lockd/host.c b/fs/lockd/host.c index b7c99bf..b23db05 100644 --- a/fs/lockd/host.c +++ b/fs/lockd/host.c @@ -316,14 +316,8 @@ struct nlm_host *nlmsvc_lookup_host(const struct svc_rqst *rqstp, struct hlist_node *pos; struct nlm_host *host = NULL; struct nsm_handle *nsm = NULL; - struct sockaddr_in sin = { - .sin_family = AF_INET, - }; - struct sockaddr_in6 sin6 = { - .sin6_family = AF_INET6, - }; struct sockaddr *src_sap; - size_t src_len = rqstp->rq_addrlen; + size_t src_len = rqstp->rq_daddrlen; struct nlm_lookup_host_info ni = { .server = 1, .sap = svc_addr(rqstp), @@ -342,12 +336,8 @@ struct nlm_host *nlmsvc_lookup_host(const struct svc_rqst *rqstp, switch (ni.sap->sa_family) { case AF_INET: - sin.sin_addr.s_addr = rqstp->rq_daddr.addr.s_addr; - src_sap = (struct sockaddr *)&sin; - break; case AF_INET6: - ipv6_addr_copy(&sin6.sin6_addr, &rqstp->rq_daddr.addr6); - src_sap = (struct sockaddr *)&sin6; + src_sap = svc_daddr(rqstp); break; default: dprintk("lockd: %s failed; unrecognized address family\n", diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c index 3787ec1..07a18a5 100644 --- a/fs/nfsd/nfs4state.c +++ b/fs/nfsd/nfs4state.c @@ -1173,20 +1173,6 @@ 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, struct svc_rqst *rqstp) { @@ -1218,7 +1204,7 @@ gen_callback(struct nfs4_client *clp, struct nfsd4_setclientid *se, struct svc_r 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); + memcpy(&conn->cb_saddr, &rqstp->rq_daddr, rqstp->rq_daddrlen); return; out_err: conn->cb_addr.ss_family = AF_UNSPEC; diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h index 223588a..3bc0dc9 100644 --- a/include/linux/sunrpc/svc.h +++ b/include/linux/sunrpc/svc.h @@ -212,11 +212,6 @@ static inline void svc_putu32(struct kvec *iov, __be32 val) iov->iov_len += sizeof(__be32); } -union svc_addr_u { - struct in_addr addr; - struct in6_addr addr6; -}; - /* * The context of a single thread, including the request currently being * processed. @@ -225,8 +220,12 @@ struct svc_rqst { struct list_head rq_list; /* idle list */ struct list_head rq_all; /* all threads list */ struct svc_xprt * rq_xprt; /* transport ptr */ + struct sockaddr_storage rq_addr; /* peer address */ size_t rq_addrlen; + struct sockaddr_storage rq_daddr; /* dest addr of request + * - reply from here */ + size_t rq_daddrlen; struct svc_serv * rq_server; /* RPC service definition */ struct svc_pool * rq_pool; /* thread pool */ @@ -255,9 +254,6 @@ struct svc_rqst { unsigned short rq_secure : 1; /* secure port */ - union svc_addr_u rq_daddr; /* dest addr of request - * - reply from here */ - void * rq_argp; /* decoded arguments */ void * rq_resp; /* xdr'd results */ void * rq_auth_data; /* flavor-specific data */ @@ -300,6 +296,21 @@ static inline struct sockaddr *svc_addr(const struct svc_rqst *rqst) return (struct sockaddr *) &rqst->rq_addr; } +static inline struct sockaddr_in *svc_daddr_in(const struct svc_rqst *rqst) +{ + return (struct sockaddr_in *) &rqst->rq_daddr; +} + +static inline struct sockaddr_in6 *svc_daddr_in6(const struct svc_rqst *rqst) +{ + return (struct sockaddr_in6 *) &rqst->rq_daddr; +} + +static inline struct sockaddr *svc_daddr(const struct svc_rqst *rqst) +{ + return (struct sockaddr *) &rqst->rq_daddr; +} + /* * Check buffer bounds after decoding arguments */ @@ -340,7 +351,8 @@ struct svc_deferred_req { struct svc_xprt *xprt; struct sockaddr_storage addr; /* where reply must go */ size_t addrlen; - union svc_addr_u daddr; /* where reply must come from */ + struct sockaddr_storage daddr; /* where reply must come from */ + size_t daddrlen; struct cache_deferred_req handle; size_t xprt_hlen; int argslen; diff --git a/net/sunrpc/svc_xprt.c b/net/sunrpc/svc_xprt.c index bd31208..d86bb67 100644 --- a/net/sunrpc/svc_xprt.c +++ b/net/sunrpc/svc_xprt.c @@ -254,8 +254,6 @@ EXPORT_SYMBOL_GPL(svc_create_xprt); */ void svc_xprt_copy_addrs(struct svc_rqst *rqstp, struct svc_xprt *xprt) { - struct sockaddr *sin; - memcpy(&rqstp->rq_addr, &xprt->xpt_remote, xprt->xpt_remotelen); rqstp->rq_addrlen = xprt->xpt_remotelen; @@ -263,15 +261,8 @@ void svc_xprt_copy_addrs(struct svc_rqst *rqstp, struct svc_xprt *xprt) * Destination address in request is needed for binding the * source address in RPC replies/callbacks later. */ - sin = (struct sockaddr *)&xprt->xpt_local; - switch (sin->sa_family) { - case AF_INET: - rqstp->rq_daddr.addr = ((struct sockaddr_in *)sin)->sin_addr; - break; - case AF_INET6: - rqstp->rq_daddr.addr6 = ((struct sockaddr_in6 *)sin)->sin6_addr; - break; - } + memcpy(&rqstp->rq_daddr, &xprt->xpt_local, xprt->xpt_locallen); + rqstp->rq_daddrlen = xprt->xpt_locallen; } EXPORT_SYMBOL_GPL(svc_xprt_copy_addrs); diff --git a/net/sunrpc/svcsock.c b/net/sunrpc/svcsock.c index 767d494..b9f4ad4 100644 --- a/net/sunrpc/svcsock.c +++ b/net/sunrpc/svcsock.c @@ -143,7 +143,8 @@ static void svc_set_cmsg_data(struct svc_rqst *rqstp, struct cmsghdr *cmh) cmh->cmsg_level = SOL_IP; cmh->cmsg_type = IP_PKTINFO; pki->ipi_ifindex = 0; - pki->ipi_spec_dst.s_addr = rqstp->rq_daddr.addr.s_addr; + pki->ipi_spec_dst.s_addr = + svc_daddr_in(rqstp)->sin_addr.s_addr; cmh->cmsg_len = CMSG_LEN(sizeof(*pki)); } break; @@ -155,7 +156,7 @@ static void svc_set_cmsg_data(struct svc_rqst *rqstp, struct cmsghdr *cmh) cmh->cmsg_type = IPV6_PKTINFO; pki->ipi6_ifindex = 0; ipv6_addr_copy(&pki->ipi6_addr, - &rqstp->rq_daddr.addr6); + &svc_daddr_in6(rqstp)->sin6_addr); cmh->cmsg_len = CMSG_LEN(sizeof(*pki)); } break; @@ -500,7 +501,7 @@ static int svc_udp_get_dest_address4(struct svc_rqst *rqstp, struct in_pktinfo *pki = CMSG_DATA(cmh); if (cmh->cmsg_type != IP_PKTINFO) return 0; - rqstp->rq_daddr.addr.s_addr = pki->ipi_spec_dst.s_addr; + svc_daddr_in(rqstp)->sin_addr.s_addr = pki->ipi_spec_dst.s_addr; return 1; } @@ -513,7 +514,7 @@ static int svc_udp_get_dest_address6(struct svc_rqst *rqstp, struct in6_pktinfo *pki = CMSG_DATA(cmh); if (cmh->cmsg_type != IPV6_PKTINFO) return 0; - ipv6_addr_copy(&rqstp->rq_daddr.addr6, &pki->ipi6_addr); + ipv6_addr_copy(&svc_daddr_in6(rqstp)->sin6_addr, &pki->ipi6_addr); return 1; } -- 1.7.6 -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html