diff mbox

[v2,03/12,03/12] IPVS: compact ip_vs_sched_persist()

Message ID 20101002022050.GA16593@verge.net.au
State Not Applicable, archived
Delegated to: David Miller
Headers show

Commit Message

Simon Horman Oct. 2, 2010, 2:20 a.m. UTC 
On Sat, Oct 02, 2010 at 01:35:28AM +0300, Julian Anastasov wrote:
> 
> 	Hello,
> 
> On Fri, 1 Oct 2010, Simon Horman wrote:
> 
> >Compact ip_vs_sched_persist() by setting up parameters
> >and calling functions once.
> >
> >Signed-off-by: Simon Horman <horms@verge.net.au>
> >---
> >
> >v2
> >* Make "union nf_inet_addr fwmark" const
> >* Don't remove the comment next to the declaration of dport
> >* Add a comment to the declaration of vport
> >
> >Index: lvs-test-2.6/net/netfilter/ipvs/ip_vs_core.c
> >===================================================================
> >--- lvs-test-2.6.orig/net/netfilter/ipvs/ip_vs_core.c	2010-10-01 21:56:39.000000000 +0900
> >+++ lvs-test-2.6/net/netfilter/ipvs/ip_vs_core.c	2010-10-01 22:02:41.000000000 +0900
> >@@ -193,10 +193,14 @@ ip_vs_sched_persist(struct ip_vs_service
> >	struct ip_vs_iphdr iph;
> >	struct ip_vs_dest *dest;
> >	struct ip_vs_conn *ct;
> >-	__be16  dport;			/* destination port to forward */
> >+	int protocol = iph.protocol;
> >+	__be16 dport = 0;		/* destination port to forward */
> >+	__be16 vport = 0;		/* virtual service port */
> >	unsigned int flags;
> >	union nf_inet_addr snet;	/* source network of the client,
> >					   after masking */
> >+	const union nf_inet_addr fwmark = { .ip = htonl(svc->fwmark) };
> >+	const union nf_inet_addr *vaddr = &iph.daddr;
> >
> >	ip_vs_fill_iphdr(svc->af, skb_network_header(skb), &iph);
> >
> >@@ -227,119 +231,58 @@ ip_vs_sched_persist(struct ip_vs_service
> >	 * service, and a template like <caddr, 0, vaddr, vport, daddr, dport>
> >	 * is created for other persistent services.
> >	 */
> >-	if (ports[1] == svc->port) {
> >-		/* Check if a template already exists */
> >-		if (svc->port != FTPPORT)
> >-			ct = ip_vs_ct_in_get(svc->af, iph.protocol, &snet, 0,
> >-					     &iph.daddr, ports[1]);
> >-		else
> >-			ct = ip_vs_ct_in_get(svc->af, iph.protocol, &snet, 0,
> >-					     &iph.daddr, 0);
> >-
> >-		if (!ct || !ip_vs_check_template(ct)) {
> >-			/*
> >-			 * No template found or the dest of the connection
> >-			 * template is not available.
> >-			 */
> >-			dest = svc->scheduler->schedule(svc, skb);
> >-			if (dest == NULL) {
> >-				IP_VS_DBG(1, "p-schedule: no dest found.\n");
> >-				return NULL;
> >-			}
> >-
> >-			/*
> >-			 * Create a template like <protocol,caddr,0,
> >-			 * vaddr,vport,daddr,dport> for non-ftp service,
> >-			 * and <protocol,caddr,0,vaddr,0,daddr,0>
> >-			 * for ftp service.
> >+	{
> >+		if (ports[1] == svc->port) {
> >+			/* non-FTP template:
> >+			 * <protocol, caddr, 0, vaddr, vport, daddr, dport>
> >+			 * FTP template:
> >+			 * <protocol, caddr, 0, vaddr, 0, daddr, 0>
> >			 */
> >			if (svc->port != FTPPORT)
> >-				ct = ip_vs_conn_new(svc->af, iph.protocol,
> >-						    &snet, 0,
> >-						    &iph.daddr,
> >-						    ports[1],
> >-						    &dest->addr, dest->port,
> >-						    IP_VS_CONN_F_TEMPLATE,
> >-						    dest);
> >-			else
> >-				ct = ip_vs_conn_new(svc->af, iph.protocol,
> >-						    &snet, 0,
> >-						    &iph.daddr, 0,
> >-						    &dest->addr, 0,
> >-						    IP_VS_CONN_F_TEMPLATE,
> >-						    dest);
> >-			if (ct == NULL)
> >-				return NULL;
> >-
> >-			ct->timeout = svc->timeout;
> >+				vport = ports[1];
> >		} else {
> >-			/* set destination with the found template */
> >-			dest = ct->dest;
> >-		}
> >-		dport = dest->port;
> >-	} else {
> >-		/*
> >-		 * Note: persistent fwmark-based services and persistent
> >-		 * port zero service are handled here.
> >-		 * fwmark template: <IPPROTO_IP,caddr,0,fwmark,0,daddr,0>
> >-		 * port zero template: <protocol,caddr,0,vaddr,0,daddr,0>
> >-		 */
> >-		if (svc->fwmark) {
> >-			union nf_inet_addr fwmark = {
> >-				.ip = htonl(svc->fwmark)
> >-			};
> >-
> >-			ct = ip_vs_ct_in_get(svc->af, IPPROTO_IP, &snet, 0,
> >-					     &fwmark, 0);
> >-		} else
> >-			ct = ip_vs_ct_in_get(svc->af, iph.protocol, &snet, 0,
> >-					     &iph.daddr, 0);
> >-
> >-		if (!ct || !ip_vs_check_template(ct)) {
> >-			/*
> >-			 * If it is not persistent port zero, return NULL,
> >-			 * otherwise create a connection template.
> >+			/* Note: persistent fwmark-based services and
> >+			 * persistent port zero service are handled here.
> >+			 * fwmark template:
> >+			 * <IPPROTO_IP,caddr,0,fwmark,0,daddr,0>
> >+			 * port zero template:
> >+			 * <protocol,caddr,0,vaddr,0,daddr,0>
> >			 */
> >-			if (svc->port)
> >-				return NULL;
> >-
> >-			dest = svc->scheduler->schedule(svc, skb);
> >-			if (dest == NULL) {
> >-				IP_VS_DBG(1, "p-schedule: no dest found.\n");
> >-				return NULL;
> >+			if (svc->fwmark) {
> >+				protocol = IPPROTO_IP;
> >+				vaddr = &fwmark;
> >			}
> >+		}
> >+	}
> >
> >-			/*
> >-			 * Create a template according to the service
> >-			 */
> >-			if (svc->fwmark) {
> >-				union nf_inet_addr fwmark = {
> >-					.ip = htonl(svc->fwmark)
> >-				};
> >-
> >-				ct = ip_vs_conn_new(svc->af, IPPROTO_IP,
> >-						    &snet, 0,
> >-						    &fwmark, 0,
> >-						    &dest->addr, 0,
> >-						    IP_VS_CONN_F_TEMPLATE,
> >-						    dest);
> >-			} else
> >-				ct = ip_vs_conn_new(svc->af, iph.protocol,
> >-						    &snet, 0,
> >-						    &iph.daddr, 0,
> >-						    &dest->addr, 0,
> >-						    IP_VS_CONN_F_TEMPLATE,
> >-						    dest);
> >-			if (ct == NULL)
> >-				return NULL;
> >+	/* Check if a template already exists */
> >+	ct = ip_vs_ct_in_get(svc->af, protocol, &snet, 0, vaddr, vport);
> >
> >-			ct->timeout = svc->timeout;
> >-		} else {
> >-			/* set destination with the found template */
> >-			dest = ct->dest;
> >+	if (!ct || !ip_vs_check_template(ct)) {
> >+		/* No template found or the dest of the connection
> >+		 * template is not available.
> >+		 */
> >+		dest = svc->scheduler->schedule(svc, skb);
> >+		if (!dest) {
> >+			IP_VS_DBG(1, "p-schedule: no dest found.\n");
> >+			return NULL;
> >		}
> >-		dport = ports[1];
> >-	}
> >+
> >+		if (ports[1] == svc->port && svc->port != FTPPORT)
> >+			dport = dest->port;
> >+
> >+		/* Create a template */
> >+		ct = ip_vs_conn_new(svc->af, protocol, &snet, 0,vaddr, vport,
> >+				    &dest->addr, dport,
> >+				    IP_VS_CONN_F_TEMPLATE, dest);
> >+		if (ct == NULL)
> >+			return NULL;
> >+
> >+		ct->timeout = svc->timeout;
> >+	} else
> >+		/* set destination with the found template */
> >+		dest = ct->dest;
> 
> 	Here dport:
> 
> >+	dport = dest->port;
> 
> 	should be:
> 
> 	dport = ports[1];
> 	if (dport == svc->port && dest->port)
> 		dport = dest->port;

Thanks, fixed.

> >	flags = (svc->flags & IP_VS_SVC_F_ONEPACKET
> >		 && iph.protocol == IPPROTO_UDP)?

I will repost the entire series a little later.
For reference, here is the updated version of this patch.


From a6310d1a8f21bdf15fa797ed748651679c0197e2 Mon Sep 17 00:00:00 2001
From: Simon Horman <horms@verge.net.au>
Date: Sun, 22 Aug 2010 21:37:51 +0900
Subject: [PATCH 03/12] IPVS: compact ip_vs_sched_persist()

Compact ip_vs_sched_persist() by setting up parameters
and calling functions once.

Signed-off-by: Simon Horman <horms@verge.net.au>
---

v2
* Make "union nf_inet_addr fwmark" const
* Don't remove the comment next to the declaration of dport
* Add a comment to the declaration of vport

v3
* As suggested by Julian Anastasov
  - Correct dport logic

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Julian Anastasov Oct. 2, 2010, 7:56 a.m. UTC | #1 
Hello,

On Sat, 2 Oct 2010, Simon Horman wrote:

>> 	Here dport:
>>
>>> +	dport = dest->port;
>>
>> 	should be:
>>
>> 	dport = ports[1];
>> 	if (dport == svc->port && dest->port)
>> 		dport = dest->port;
>
> Thanks, fixed.

 	I'm still wondering, may be it needs separate patch
but we do not support NAT to different dest->port in the
case for fwmark. May be the above logic can be changed to
support it. By this way web to different VIPs and VPORTs
in a single virtual service (fwmark) can use single NAT
real server for name-based virtual hosting. But such change
can create compatibility problems for setups that used
different vports for the fwmark service and still expect
it in that way (vport to same dport).

Regards

--
Julian Anastasov <ja@ssi.bg>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Simon Horman Oct. 2, 2010, 8:08 a.m. UTC | #2 
On Sat, Oct 02, 2010 at 10:56:19AM +0300, Julian Anastasov wrote:
> 
> 	Hello,
> 
> On Sat, 2 Oct 2010, Simon Horman wrote:
> 
> >>	Here dport:
> >>
> >>>+	dport = dest->port;
> >>
> >>	should be:
> >>
> >>	dport = ports[1];
> >>	if (dport == svc->port && dest->port)
> >>		dport = dest->port;
> >
> >Thanks, fixed.
> 
> 	I'm still wondering, may be it needs separate patch
> but we do not support NAT to different dest->port in the
> case for fwmark. May be the above logic can be changed to
> support it. By this way web to different VIPs and VPORTs
> in a single virtual service (fwmark) can use single NAT
> real server for name-based virtual hosting. But such change
> can create compatibility problems for setups that used
> different vports for the fwmark service and still expect
> it in that way (vport to same dport).

Hi Julian,

I think that this sounds line a new flavour of fwmark virtual services to me.
Perhaps yet another flag is in order?

To be clear, what you have in mind is essentially to nat *:* (as matched by
a fwmark) to x:y, where as at this time *:y may be natted to x:y.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Julian Anastasov Oct. 4, 2010, 10:36 p.m. UTC | #3 
Hello,

On Sat, 2 Oct 2010, Simon Horman wrote:

>> 	I'm still wondering, may be it needs separate patch
>> but we do not support NAT to different dest->port in the
>> case for fwmark. May be the above logic can be changed to
>> support it. By this way web to different VIPs and VPORTs
>> in a single virtual service (fwmark) can use single NAT
>> real server for name-based virtual hosting. But such change
>> can create compatibility problems for setups that used
>> different vports for the fwmark service and still expect
>> it in that way (vport to same dport).
>
> Hi Julian,
>
> I think that this sounds line a new flavour of fwmark virtual services to me.
> Perhaps yet another flag is in order?
>
> To be clear, what you have in mind is essentially to nat *:* (as matched by
> a fwmark) to x:y, where as at this time *:y may be natted to x:y.

 	Yes, the difference is here, persistent services with fwmark 
ignore dest->port while non-persistent services with fwmark prefer
dest->port:

ip_vs_sched_persist:
         dport = ports[1];
         if (dport == svc->port && dest->port)
                 dport = dest->port;
 	- before the PE changes even dest->port=0 was selected
 	for non-fwmark

ip_vs_schedule:
 	dport set as: dest->port ? dest->port : pptr[1]

 	It is inconsistent. If we add new option for dest
it should have 3 states:

0: default (current, compatible)
1: prefer real server port
2: keep dport in packet

 	But the same can be done with rport or that was the
goal before:

- rport=0 (zero port) - keep dport from packet
- rport>0 - use this real server port

 	It works perfectly for ip_vs_schedule but not for
ip_vs_sched_persist, rport>0 is ignored for persistent
fwmark and this is wrong for NAT. Persistence should just
select dest but now it changes the rport semantic. The
right thing is ip_vs_sched_persist to work as ip_vs_schedule
but it can break some setups with persistence and fwmark.

Regards

--
Julian Anastasov <ja@ssi.bg>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

Index: lvs-test-2.6/net/netfilter/ipvs/ip_vs_core.c
===================================================================
--- lvs-test-2.6.orig/net/netfilter/ipvs/ip_vs_core.c	2010-10-02 10:54:57.000000000 +0900
+++ lvs-test-2.6/net/netfilter/ipvs/ip_vs_core.c	2010-10-02 11:04:04.000000000 +0900
@@ -193,10 +193,14 @@  ip_vs_sched_persist(struct ip_vs_service
 	struct ip_vs_iphdr iph;
 	struct ip_vs_dest *dest;
 	struct ip_vs_conn *ct;
-	__be16  dport;			/* destination port to forward */
+	int protocol = iph.protocol;
+	__be16 dport = 0;		/* destination port to forward */
+	__be16 vport = 0;		/* virtual service port */
 	unsigned int flags;
 	union nf_inet_addr snet;	/* source network of the client,
 					   after masking */
+	const union nf_inet_addr fwmark = { .ip = htonl(svc->fwmark) };
+	const union nf_inet_addr *vaddr = &iph.daddr;
 
 	ip_vs_fill_iphdr(svc->af, skb_network_header(skb), &iph);
 
@@ -227,119 +231,61 @@  ip_vs_sched_persist(struct ip_vs_service
 	 * service, and a template like <caddr, 0, vaddr, vport, daddr, dport>
 	 * is created for other persistent services.
 	 */
-	if (ports[1] == svc->port) {
-		/* Check if a template already exists */
-		if (svc->port != FTPPORT)
-			ct = ip_vs_ct_in_get(svc->af, iph.protocol, &snet, 0,
-					     &iph.daddr, ports[1]);
-		else
-			ct = ip_vs_ct_in_get(svc->af, iph.protocol, &snet, 0,
-					     &iph.daddr, 0);
-
-		if (!ct || !ip_vs_check_template(ct)) {
-			/*
-			 * No template found or the dest of the connection
-			 * template is not available.
-			 */
-			dest = svc->scheduler->schedule(svc, skb);
-			if (dest == NULL) {
-				IP_VS_DBG(1, "p-schedule: no dest found.\n");
-				return NULL;
-			}
-
-			/*
-			 * Create a template like <protocol,caddr,0,
-			 * vaddr,vport,daddr,dport> for non-ftp service,
-			 * and <protocol,caddr,0,vaddr,0,daddr,0>
-			 * for ftp service.
+	{
+		if (ports[1] == svc->port) {
+			/* non-FTP template:
+			 * <protocol, caddr, 0, vaddr, vport, daddr, dport>
+			 * FTP template:
+			 * <protocol, caddr, 0, vaddr, 0, daddr, 0>
 			 */
 			if (svc->port != FTPPORT)
-				ct = ip_vs_conn_new(svc->af, iph.protocol,
-						    &snet, 0,
-						    &iph.daddr,
-						    ports[1],
-						    &dest->addr, dest->port,
-						    IP_VS_CONN_F_TEMPLATE,
-						    dest);
-			else
-				ct = ip_vs_conn_new(svc->af, iph.protocol,
-						    &snet, 0,
-						    &iph.daddr, 0,
-						    &dest->addr, 0,
-						    IP_VS_CONN_F_TEMPLATE,
-						    dest);
-			if (ct == NULL)
-				return NULL;
-
-			ct->timeout = svc->timeout;
+				vport = ports[1];
 		} else {
-			/* set destination with the found template */
-			dest = ct->dest;
-		}
-		dport = dest->port;
-	} else {
-		/*
-		 * Note: persistent fwmark-based services and persistent
-		 * port zero service are handled here.
-		 * fwmark template: <IPPROTO_IP,caddr,0,fwmark,0,daddr,0>
-		 * port zero template: <protocol,caddr,0,vaddr,0,daddr,0>
-		 */
-		if (svc->fwmark) {
-			union nf_inet_addr fwmark = {
-				.ip = htonl(svc->fwmark)
-			};
-
-			ct = ip_vs_ct_in_get(svc->af, IPPROTO_IP, &snet, 0,
-					     &fwmark, 0);
-		} else
-			ct = ip_vs_ct_in_get(svc->af, iph.protocol, &snet, 0,
-					     &iph.daddr, 0);
-
-		if (!ct || !ip_vs_check_template(ct)) {
-			/*
-			 * If it is not persistent port zero, return NULL,
-			 * otherwise create a connection template.
+			/* Note: persistent fwmark-based services and
+			 * persistent port zero service are handled here.
+			 * fwmark template:
+			 * <IPPROTO_IP,caddr,0,fwmark,0,daddr,0>
+			 * port zero template:
+			 * <protocol,caddr,0,vaddr,0,daddr,0>
 			 */
-			if (svc->port)
-				return NULL;
-
-			dest = svc->scheduler->schedule(svc, skb);
-			if (dest == NULL) {
-				IP_VS_DBG(1, "p-schedule: no dest found.\n");
-				return NULL;
+			if (svc->fwmark) {
+				protocol = IPPROTO_IP;
+				vaddr = &fwmark;
 			}
+		}
+	}
 
-			/*
-			 * Create a template according to the service
-			 */
-			if (svc->fwmark) {
-				union nf_inet_addr fwmark = {
-					.ip = htonl(svc->fwmark)
-				};
-
-				ct = ip_vs_conn_new(svc->af, IPPROTO_IP,
-						    &snet, 0,
-						    &fwmark, 0,
-						    &dest->addr, 0,
-						    IP_VS_CONN_F_TEMPLATE,
-						    dest);
-			} else
-				ct = ip_vs_conn_new(svc->af, iph.protocol,
-						    &snet, 0,
-						    &iph.daddr, 0,
-						    &dest->addr, 0,
-						    IP_VS_CONN_F_TEMPLATE,
-						    dest);
-			if (ct == NULL)
-				return NULL;
+	/* Check if a template already exists */
+	ct = ip_vs_ct_in_get(svc->af, protocol, &snet, 0, vaddr, vport);
 
-			ct->timeout = svc->timeout;
-		} else {
-			/* set destination with the found template */
-			dest = ct->dest;
+	if (!ct || !ip_vs_check_template(ct)) {
+		/* No template found or the dest of the connection
+		 * template is not available.
+		 */
+		dest = svc->scheduler->schedule(svc, skb);
+		if (!dest) {
+			IP_VS_DBG(1, "p-schedule: no dest found.\n");
+			return NULL;
 		}
-		dport = ports[1];
-	}
+
+		if (ports[1] == svc->port && svc->port != FTPPORT)
+			dport = dest->port;
+
+		/* Create a template */
+		ct = ip_vs_conn_new(svc->af, protocol, &snet, 0,vaddr, vport,
+				    &dest->addr, dport,
+				    IP_VS_CONN_F_TEMPLATE, dest);
+		if (ct == NULL)
+			return NULL;
+
+		ct->timeout = svc->timeout;
+	} else
+		/* set destination with the found template */
+		dest = ct->dest;
+
+	dport = ports[1];
+	if (dport == svc->port && dest->port)
+		dport = dest->port;
 
 	flags = (svc->flags & IP_VS_SVC_F_ONEPACKET
 		 && iph.protocol == IPPROTO_UDP)?