* [PATCH] ipvs: Fix reuse connection if real server is dead
@ 2013-05-07 21:54 Grzegorz Lyczba
2013-05-08 6:12 ` Hans Schillstrom
2013-05-08 7:35 ` Julian Anastasov
0 siblings, 2 replies; 12+ messages in thread
From: Grzegorz Lyczba @ 2013-05-07 21:54 UTC (permalink / raw)
To: lvs-devel; +Cc: Grzegorz Lyczba, Julian Anastasov, Simon Horman
Expire cached connection for new TCP connection if real server is down
Signed-off-by: Grzegorz Lyczba <grzegorz.lyczba@gmail.com>
---
net/netfilter/ipvs/ip_vs_core.c | 20 ++++++++++++++++++++
1 files changed, 20 insertions(+), 0 deletions(-)
diff --git a/net/netfilter/ipvs/ip_vs_core.c b/net/netfilter/ipvs/ip_vs_core.c
index 47edf5a..7c5f083 100644
--- a/net/netfilter/ipvs/ip_vs_core.c
+++ b/net/netfilter/ipvs/ip_vs_core.c
@@ -1002,6 +1002,17 @@ static inline int is_tcp_reset(const struct sk_buff *skb, int nh_len)
return th->rst;
}
+static inline int is_tcp_syn(const struct sk_buff *skb, int nh_len)
+{
+ struct tcphdr _tcph, *th;
+
+ th = skb_header_pointer(skb, nh_len, sizeof(_tcph), &_tcph);
+ if (th == NULL)
+ return 0;
+ return th->syn;
+}
+
+
/* Handle response packets: rewrite addresses and send away...
*/
static unsigned int
@@ -1627,6 +1638,15 @@ ip_vs_in(unsigned int hooknum, struct sk_buff *skb, int af)
* Check if the packet belongs to an existing connection entry
*/
cp = pp->conn_in_get(af, skb, &iph, 0);
+
+ if (likely(cp) && likely(cp->dest) &&
+ unlikely(!atomic_read(&cp->dest->weight)) &&
+ iph.protocol == IPPROTO_TCP && is_tcp_syn(skb, iph.len)) {
+ ip_vs_conn_expire_now(cp);
+ __ip_vs_conn_put(cp);
+ cp = NULL;
+ }
+
if (unlikely(!cp) && !iph.fragoffs) {
/* No (second) fragments need to enter here, as nf_defrag_ipv6
* replayed fragment zero will already have created the cp
--
1.7.5.4
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] ipvs: Fix reuse connection if real server is dead
2013-05-07 21:54 [PATCH] ipvs: Fix reuse connection if real server is dead Grzegorz Lyczba
@ 2013-05-08 6:12 ` Hans Schillstrom
2013-05-08 7:35 ` Julian Anastasov
1 sibling, 0 replies; 12+ messages in thread
From: Hans Schillstrom @ 2013-05-08 6:12 UTC (permalink / raw)
To: Grzegorz Lyczba, lvs-devel, Simon Horman, Julian Anastasov
[-- Attachment #1: Type: text/plain, Size: 1891 bytes --]
Hello
On Tue, 2013-05-07 at 23:54 +0200, Grzegorz Lyczba wrote:
> Expire cached connection for new TCP connection if real server is down
This patch will break some "common" use cases,
on the other hand it also solves some issues...
I would prefer to have a sysctl to turn it on.
Should we have something similar for sctp?
Regards
Hans
> Signed-off-by: Grzegorz Lyczba <grzegorz.lyczba@gmail.com>
> ---
> net/netfilter/ipvs/ip_vs_core.c | 20 ++++++++++++++++++++
> 1 files changed, 20 insertions(+), 0 deletions(-)
>
> diff --git a/net/netfilter/ipvs/ip_vs_core.c b/net/netfilter/ipvs/ip_vs_core.c
> index 47edf5a..7c5f083 100644
> --- a/net/netfilter/ipvs/ip_vs_core.c
> +++ b/net/netfilter/ipvs/ip_vs_core.c
> @@ -1002,6 +1002,17 @@ static inline int is_tcp_reset(const struct sk_buff *skb, int nh_len)
> return th->rst;
> }
>
> +static inline int is_tcp_syn(const struct sk_buff *skb, int nh_len)
> +{
> + struct tcphdr _tcph, *th;
> +
> + th = skb_header_pointer(skb, nh_len, sizeof(_tcph), &_tcph);
> + if (th == NULL)
> + return 0;
> + return th->syn;
> +}
> +
> +
> /* Handle response packets: rewrite addresses and send away...
> */
> static unsigned int
> @@ -1627,6 +1638,15 @@ ip_vs_in(unsigned int hooknum, struct sk_buff *skb, int af)
> * Check if the packet belongs to an existing connection entry
> */
> cp = pp->conn_in_get(af, skb, &iph, 0);
> +
> + if (likely(cp) && likely(cp->dest) &&
> + unlikely(!atomic_read(&cp->dest->weight)) &&
> + iph.protocol == IPPROTO_TCP && is_tcp_syn(skb, iph.len)) {
> + ip_vs_conn_expire_now(cp);
> + __ip_vs_conn_put(cp);
> + cp = NULL;
> + }
> +
> if (unlikely(!cp) && !iph.fragoffs) {
> /* No (second) fragments need to enter here, as nf_defrag_ipv6
> * replayed fragment zero will already have created the cp
[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6177 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] ipvs: Fix reuse connection if real server is dead
2013-05-07 21:54 [PATCH] ipvs: Fix reuse connection if real server is dead Grzegorz Lyczba
2013-05-08 6:12 ` Hans Schillstrom
@ 2013-05-08 7:35 ` Julian Anastasov
2013-05-08 8:07 ` Hans Schillstrom
2013-05-11 9:32 ` Grzegorz Łyczba
1 sibling, 2 replies; 12+ messages in thread
From: Julian Anastasov @ 2013-05-08 7:35 UTC (permalink / raw)
To: Grzegorz Lyczba; +Cc: lvs-devel, Simon Horman
Hello,
On Tue, 7 May 2013, Grzegorz Lyczba wrote:
> Expire cached connection for new TCP connection if real server is down
>
> Signed-off-by: Grzegorz Lyczba <grzegorz.lyczba@gmail.com>
> ---
> net/netfilter/ipvs/ip_vs_core.c | 20 ++++++++++++++++++++
> 1 files changed, 20 insertions(+), 0 deletions(-)
>
> diff --git a/net/netfilter/ipvs/ip_vs_core.c b/net/netfilter/ipvs/ip_vs_core.c
> index 47edf5a..7c5f083 100644
> --- a/net/netfilter/ipvs/ip_vs_core.c
> +++ b/net/netfilter/ipvs/ip_vs_core.c
> @@ -1002,6 +1002,17 @@ static inline int is_tcp_reset(const struct sk_buff *skb, int nh_len)
> return th->rst;
> }
>
> +static inline int is_tcp_syn(const struct sk_buff *skb, int nh_len)
> +{
> + struct tcphdr _tcph, *th;
> +
> + th = skb_header_pointer(skb, nh_len, sizeof(_tcph), &_tcph);
As Hans notes, it is a good idea to support SCTP by
using the iph address as second argument. May be the func can
be named is_new_conn:
static inline bool is_new_conn(const struct sk_buff *skb,
struct ip_vs_iphdr *iph)
...
> + if (th == NULL)
> + return 0;
> + return th->syn;
> +}
> +
> +
> /* Handle response packets: rewrite addresses and send away...
> */
> static unsigned int
> @@ -1627,6 +1638,15 @@ ip_vs_in(unsigned int hooknum, struct sk_buff *skb, int af)
> * Check if the packet belongs to an existing connection entry
> */
> cp = pp->conn_in_get(af, skb, &iph, 0);
> +
> + if (likely(cp) && likely(cp->dest) &&
I hope it is appropriate to use the expire_nodest_conn flag:
if (unlikely(sysctl_expire_nodest_conn(ipvs) && cp && cp->dest &&
...
> + unlikely(!atomic_read(&cp->dest->weight)) &&
We should add '!iph.fragoffs &&' here.
> + iph.protocol == IPPROTO_TCP && is_tcp_syn(skb, iph.len)) {
> + ip_vs_conn_expire_now(cp);
> + __ip_vs_conn_put(cp);
> + cp = NULL;
> + }
> +
> if (unlikely(!cp) && !iph.fragoffs) {
> /* No (second) fragments need to enter here, as nf_defrag_ipv6
> * replayed fragment zero will already have created the cp
Regards
--
Julian Anastasov <ja@ssi.bg>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] ipvs: Fix reuse connection if real server is dead
2013-05-08 7:35 ` Julian Anastasov
@ 2013-05-08 8:07 ` Hans Schillstrom
2013-05-08 8:53 ` Julian Anastasov
2013-05-11 9:32 ` Grzegorz Łyczba
1 sibling, 1 reply; 12+ messages in thread
From: Hans Schillstrom @ 2013-05-08 8:07 UTC (permalink / raw)
To: Julian Anastasov; +Cc: Grzegorz Lyczba, lvs-devel, Simon Horman
[-- Attachment #1: Type: text/plain, Size: 2648 bytes --]
On Wed, 2013-05-08 at 10:35 +0300, Julian Anastasov wrote:
> Hello,
>
> On Tue, 7 May 2013, Grzegorz Lyczba wrote:
>
> > Expire cached connection for new TCP connection if real server is down
> >
> > Signed-off-by: Grzegorz Lyczba <grzegorz.lyczba@gmail.com>
> > ---
> > net/netfilter/ipvs/ip_vs_core.c | 20 ++++++++++++++++++++
> > 1 files changed, 20 insertions(+), 0 deletions(-)
> >
> > diff --git a/net/netfilter/ipvs/ip_vs_core.c b/net/netfilter/ipvs/ip_vs_core.c
> > index 47edf5a..7c5f083 100644
> > --- a/net/netfilter/ipvs/ip_vs_core.c
> > +++ b/net/netfilter/ipvs/ip_vs_core.c
> > @@ -1002,6 +1002,17 @@ static inline int is_tcp_reset(const struct sk_buff *skb, int nh_len)
> > return th->rst;
> > }
> >
> > +static inline int is_tcp_syn(const struct sk_buff *skb, int nh_len)
> > +{
> > + struct tcphdr _tcph, *th;
> > +
> > + th = skb_header_pointer(skb, nh_len, sizeof(_tcph), &_tcph);
>
> As Hans notes, it is a good idea to support SCTP by
> using the iph address as second argument. May be the func can
> be named is_new_conn:
>
> static inline bool is_new_conn(const struct sk_buff *skb,
> struct ip_vs_iphdr *iph)
> ...
>
> > + if (th == NULL)
> > + return 0;
> > + return th->syn;
> > +}
> > +
> > +
> > /* Handle response packets: rewrite addresses and send away...
> > */
> > static unsigned int
> > @@ -1627,6 +1638,15 @@ ip_vs_in(unsigned int hooknum, struct sk_buff *skb, int af)
> > * Check if the packet belongs to an existing connection entry
> > */
> > cp = pp->conn_in_get(af, skb, &iph, 0);
> > +
> > + if (likely(cp) && likely(cp->dest) &&
>
> I hope it is appropriate to use the expire_nodest_conn flag:
>
> if (unlikely(sysctl_expire_nodest_conn(ipvs) && cp && cp->dest &&
> ...
>
> > + unlikely(!atomic_read(&cp->dest->weight)) &&
>
> We should add '!iph.fragoffs &&' here.
Do we really need that?
I don't think a SYN can be fragmented.
Regards
Hans
>
> > + iph.protocol == IPPROTO_TCP && is_tcp_syn(skb, iph.len)) {
> > + ip_vs_conn_expire_now(cp);
> > + __ip_vs_conn_put(cp);
> > + cp = NULL;
> > + }
> > +
> > if (unlikely(!cp) && !iph.fragoffs) {
> > /* No (second) fragments need to enter here, as nf_defrag_ipv6
> > * replayed fragment zero will already have created the cp
>
> Regards
>
> --
> Julian Anastasov <ja@ssi.bg>
> --
> To unsubscribe from this list: send the line "unsubscribe lvs-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6177 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] ipvs: Fix reuse connection if real server is dead
2013-05-08 8:07 ` Hans Schillstrom
@ 2013-05-08 8:53 ` Julian Anastasov
2013-05-08 9:21 ` Hans Schillstrom
0 siblings, 1 reply; 12+ messages in thread
From: Julian Anastasov @ 2013-05-08 8:53 UTC (permalink / raw)
To: Hans Schillstrom; +Cc: Grzegorz Lyczba, lvs-devel, Simon Horman
Hello,
On Wed, 8 May 2013, Hans Schillstrom wrote:
> > > + unlikely(!atomic_read(&cp->dest->weight)) &&
> >
> > We should add '!iph.fragoffs &&' here.
>
> Do we really need that?
> I don't think a SYN can be fragmented.
SYNs are usually not fragmented. But we don't have the
right to search for SYN in a packet without TCP header. It can
be data fragment that contains 1 at this bit position, when
weight=0 such packets should be forwarded as part of existing
connection.
Regards
--
Julian Anastasov <ja@ssi.bg>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] ipvs: Fix reuse connection if real server is dead
2013-05-08 8:53 ` Julian Anastasov
@ 2013-05-08 9:21 ` Hans Schillstrom
0 siblings, 0 replies; 12+ messages in thread
From: Hans Schillstrom @ 2013-05-08 9:21 UTC (permalink / raw)
To: Julian Anastasov; +Cc: Grzegorz Lyczba, lvs-devel, Simon Horman
[-- Attachment #1: Type: text/plain, Size: 783 bytes --]
On Wed, 2013-05-08 at 11:53 +0300, Julian Anastasov wrote:
> Hello,
>
> On Wed, 8 May 2013, Hans Schillstrom wrote:
>
> > > > + unlikely(!atomic_read(&cp->dest->weight)) &&
> > >
> > > We should add '!iph.fragoffs &&' here.
> >
> > Do we really need that?
> > I don't think a SYN can be fragmented.
>
> SYNs are usually not fragmented. But we don't have the
> right to search for SYN in a packet without TCP header. It can
> be data fragment that contains 1 at this bit position, when
> weight=0 such packets should be forwarded as part of existing
> connection.
>
Shame on me, I should really have thought about that...
I think I know who invented the iph.fragoffs :-)
Regards
Hans
> Regards
>
> --
> Julian Anastasov <ja@ssi.bg>
[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6177 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] ipvs: Fix reuse connection if real server is dead
2013-05-08 7:35 ` Julian Anastasov
2013-05-08 8:07 ` Hans Schillstrom
@ 2013-05-11 9:32 ` Grzegorz Łyczba
2013-05-11 13:23 ` Julian Anastasov
1 sibling, 1 reply; 12+ messages in thread
From: Grzegorz Łyczba @ 2013-05-11 9:32 UTC (permalink / raw)
To: lvs-devel, Julian Anastasov; +Cc: Simon Horman, Grzegorz Lyczba
From: Grzegorz Lyczba <grzegorz.lyczba@gmail.com>
Expire cached connection for new TCP/SCTP connection if real server is down
Signed-off-by: Grzegorz Lyczba <grzegorz.lyczba@gmail.com>
---
net/netfilter/ipvs/ip_vs_core.c | 36 ++++++++++++++++++++++++++++++++++++
1 files changed, 36 insertions(+), 0 deletions(-)
diff --git a/net/netfilter/ipvs/ip_vs_core.c b/net/netfilter/ipvs/ip_vs_core.c
index 47edf5a..9ee8b70 100644
--- a/net/netfilter/ipvs/ip_vs_core.c
+++ b/net/netfilter/ipvs/ip_vs_core.c
@@ -1002,6 +1002,33 @@ static inline int is_tcp_reset(const struct sk_buff *skb, int nh_len)
return th->rst;
}
+static inline bool is_new_conn(const struct sk_buff *skb,
+ struct ip_vs_iphdr *iph)
+{
+ switch (iph->protocol) {
+ case IPPROTO_TCP: {
+ struct tcphdr _tcph, *th;
+ th = skb_header_pointer(skb, iph->len, sizeof(_tcph), &_tcph);
+ if (th == NULL) {
+ return false;
+ }
+ return th->syn;
+ }
+ case IPPROTO_SCTP: {
+ sctp_chunkhdr_t *sch, schunk;
+ sch = skb_header_pointer(skb, iph->len + sizeof(sctp_sctphdr_t),
+ sizeof(schunk), &schunk);
+ if (sch == NULL) {
+ return false;
+ }
+ return (sch->type == SCTP_CID_INIT);
+ }
+ default:
+ return false;
+ }
+}
+
+
/* Handle response packets: rewrite addresses and send away...
*/
static unsigned int
@@ -1627,6 +1654,15 @@ ip_vs_in(unsigned int hooknum, struct sk_buff *skb, int af)
* Check if the packet belongs to an existing connection entry
*/
cp = pp->conn_in_get(af, skb, &iph, 0);
+
+ if (unlikely(sysctl_expire_nodest_conn(ipvs)) && cp && cp->dest &&
+ !iph.fragoffs && unlikely(!atomic_read(&cp->dest->weight)) &&
+ is_new_conn(skb, &iph)) {
+ ip_vs_conn_expire_now(cp);
+ __ip_vs_conn_put(cp);
+ cp = NULL;
+ }
+
if (unlikely(!cp) && !iph.fragoffs) {
/* No (second) fragments need to enter here, as nf_defrag_ipv6
* replayed fragment zero will already have created the cp
--
1.7.5.4
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] ipvs: Fix reuse connection if real server is dead
2013-05-11 9:32 ` Grzegorz Łyczba
@ 2013-05-11 13:23 ` Julian Anastasov
2013-05-13 1:38 ` Simon Horman
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Julian Anastasov @ 2013-05-11 13:23 UTC (permalink / raw)
To: Grzegorz Łyczba; +Cc: lvs-devel, Simon Horman
[-- Attachment #1: Type: TEXT/PLAIN, Size: 3483 bytes --]
Hello,
On Sat, 11 May 2013, Grzegorz Łyczba wrote:
> From: Grzegorz Lyczba <grzegorz.lyczba@gmail.com>
>
> Expire cached connection for new TCP/SCTP connection if real server is down
>
> Signed-off-by: Grzegorz Lyczba <grzegorz.lyczba@gmail.com>
The change looks correct but you have to fix some
coding style issues. Otherwise, patch will be rejected.
You can also check Documentation/CodingStyle
# scripts/checkpatch.pl /tmp/patch.diff
WARNING: braces {} are not necessary for single statement blocks
#25: FILE: net/netfilter/ipvs/ip_vs_core.c:1012:
+ if (th == NULL) {
+ return false;
+ }
WARNING: braces {} are not necessary for single statement blocks
#34: FILE: net/netfilter/ipvs/ip_vs_core.c:1021:
+ if (sch == NULL) {
+ return false;
+ }
total: 0 errors, 2 warnings, 48 lines checked
> ---
> net/netfilter/ipvs/ip_vs_core.c | 36 ++++++++++++++++++++++++++++++++++++
> 1 files changed, 36 insertions(+), 0 deletions(-)
>
> diff --git a/net/netfilter/ipvs/ip_vs_core.c b/net/netfilter/ipvs/ip_vs_core.c
> index 47edf5a..9ee8b70 100644
> --- a/net/netfilter/ipvs/ip_vs_core.c
> +++ b/net/netfilter/ipvs/ip_vs_core.c
> @@ -1002,6 +1002,33 @@ static inline int is_tcp_reset(const struct sk_buff *skb, int nh_len)
> return th->rst;
> }
>
> +static inline bool is_new_conn(const struct sk_buff *skb,
> + struct ip_vs_iphdr *iph)
Above "struct" should be at the same column as "const"
in first argument, like this:
static inline bool is_new_conn(const struct sk_buff *skb,
struct ip_vs_iphdr *iph)
> +{
> + switch (iph->protocol) {
I'm still not sure if this construct is allowed:
case ...: {
}
May be it should be:
case ...:
{
}
case ...
but CodingStyle does not include rules about it.
If in doubt, you can use 'if' instead of 'switch' without
adding new indentation.
> + case IPPROTO_TCP: {
> + struct tcphdr _tcph, *th;
empty line is needed here
> + th = skb_header_pointer(skb, iph->len, sizeof(_tcph), &_tcph);
> + if (th == NULL) {
> + return false;
extra braces
> + }
> + return th->syn;
> + }
> + case IPPROTO_SCTP: {
> + sctp_chunkhdr_t *sch, schunk;
empty line is needed here
> + sch = skb_header_pointer(skb, iph->len + sizeof(sctp_sctphdr_t),
> + sizeof(schunk), &schunk);
> + if (sch == NULL) {
> + return false;
extra braces
> + }
> + return (sch->type == SCTP_CID_INIT);
> + }
> + default:
> + return false;
> + }
> +}
> +
> +
> /* Handle response packets: rewrite addresses and send away...
> */
> static unsigned int
> @@ -1627,6 +1654,15 @@ ip_vs_in(unsigned int hooknum, struct sk_buff *skb, int af)
> * Check if the packet belongs to an existing connection entry
> */
> cp = pp->conn_in_get(af, skb, &iph, 0);
> +
> + if (unlikely(sysctl_expire_nodest_conn(ipvs)) && cp && cp->dest &&
> + !iph.fragoffs && unlikely(!atomic_read(&cp->dest->weight)) &&
Lets move '!iph.fragoffs' after the
'unlikely(!atomic_read(&cp->dest->weight))' because fragoffs
is usually 0. Only IPv6 can see fragments at that point.
> + is_new_conn(skb, &iph)) {
> + ip_vs_conn_expire_now(cp);
> + __ip_vs_conn_put(cp);
> + cp = NULL;
> + }
> +
> if (unlikely(!cp) && !iph.fragoffs) {
> /* No (second) fragments need to enter here, as nf_defrag_ipv6
> * replayed fragment zero will already have created the cp
> --
> 1.7.5.4
Regards
--
Julian Anastasov <ja@ssi.bg>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] ipvs: Fix reuse connection if real server is dead
2013-05-11 13:23 ` Julian Anastasov
@ 2013-05-13 1:38 ` Simon Horman
2013-05-13 13:38 ` Jesper Dangaard Brouer
2013-05-13 21:56 ` [PATCH v3] " Grzegorz Łyczba
2 siblings, 0 replies; 12+ messages in thread
From: Simon Horman @ 2013-05-13 1:38 UTC (permalink / raw)
To: Julian Anastasov; +Cc: Grzegorz Łyczba, lvs-devel
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="windows-1252", Size: 4052 bytes --]
On Sat, May 11, 2013 at 04:23:33PM +0300, Julian Anastasov wrote:
>
> Hello,
>
> On Sat, 11 May 2013, Grzegorz Łyczba wrote:
>
> > From: Grzegorz Lyczba <grzegorz.lyczba@gmail.com>
> >
> > Expire cached connection for new TCP/SCTP connection if real server is down
> >
> > Signed-off-by: Grzegorz Lyczba <grzegorz.lyczba@gmail.com>
>
> The change looks correct but you have to fix some
> coding style issues. Otherwise, patch will be rejected.
> You can also check Documentation/CodingStyle
>
> # scripts/checkpatch.pl /tmp/patch.diff
> WARNING: braces {} are not necessary for single statement blocks
> #25: FILE: net/netfilter/ipvs/ip_vs_core.c:1012:
> + if (th == NULL) {
> + return false;
> + }
>
> WARNING: braces {} are not necessary for single statement blocks
> #34: FILE: net/netfilter/ipvs/ip_vs_core.c:1021:
> + if (sch == NULL) {
> + return false;
> + }
>
> total: 0 errors, 2 warnings, 48 lines checked
>
> > ---
> > net/netfilter/ipvs/ip_vs_core.c | 36 ++++++++++++++++++++++++++++++++++++
> > 1 files changed, 36 insertions(+), 0 deletions(-)
> >
> > diff --git a/net/netfilter/ipvs/ip_vs_core.c b/net/netfilter/ipvs/ip_vs_core.c
> > index 47edf5a..9ee8b70 100644
> > --- a/net/netfilter/ipvs/ip_vs_core.c
> > +++ b/net/netfilter/ipvs/ip_vs_core.c
> > @@ -1002,6 +1002,33 @@ static inline int is_tcp_reset(const struct sk_buff *skb, int nh_len)
> > return th->rst;
> > }
> >
> > +static inline bool is_new_conn(const struct sk_buff *skb,
> > + struct ip_vs_iphdr *iph)
>
> Above "struct" should be at the same column as "const"
> in first argument, like this:
>
> static inline bool is_new_conn(const struct sk_buff *skb,
> struct ip_vs_iphdr *iph)
>
> > +{
> > + switch (iph->protocol) {
>
> I'm still not sure if this construct is allowed:
>
> case ...: {
> }
I see some evidence of the above style in net/core/
so personally I am fine with it.
>
> May be it should be:
>
> case ...:
> {
> }
> case ...
>
> but CodingStyle does not include rules about it.
> If in doubt, you can use 'if' instead of 'switch' without
> adding new indentation.
>
> > + case IPPROTO_TCP: {
> > + struct tcphdr _tcph, *th;
>
> empty line is needed here
>
> > + th = skb_header_pointer(skb, iph->len, sizeof(_tcph), &_tcph);
> > + if (th == NULL) {
> > + return false;
>
> extra braces
>
> > + }
> > + return th->syn;
> > + }
> > + case IPPROTO_SCTP: {
> > + sctp_chunkhdr_t *sch, schunk;
>
> empty line is needed here
>
> > + sch = skb_header_pointer(skb, iph->len + sizeof(sctp_sctphdr_t),
> > + sizeof(schunk), &schunk);
> > + if (sch == NULL) {
> > + return false;
>
> extra braces
>
> > + }
> > + return (sch->type == SCTP_CID_INIT);
> > + }
> > + default:
> > + return false;
> > + }
> > +}
> > +
> > +
> > /* Handle response packets: rewrite addresses and send away...
> > */
> > static unsigned int
> > @@ -1627,6 +1654,15 @@ ip_vs_in(unsigned int hooknum, struct sk_buff *skb, int af)
> > * Check if the packet belongs to an existing connection entry
> > */
> > cp = pp->conn_in_get(af, skb, &iph, 0);
> > +
> > + if (unlikely(sysctl_expire_nodest_conn(ipvs)) && cp && cp->dest &&
> > + !iph.fragoffs && unlikely(!atomic_read(&cp->dest->weight)) &&
>
> Lets move '!iph.fragoffs' after the
> 'unlikely(!atomic_read(&cp->dest->weight))' because fragoffs
> is usually 0. Only IPv6 can see fragments at that point.
>
> > + is_new_conn(skb, &iph)) {
> > + ip_vs_conn_expire_now(cp);
> > + __ip_vs_conn_put(cp);
> > + cp = NULL;
> > + }
> > +
> > if (unlikely(!cp) && !iph.fragoffs) {
> > /* No (second) fragments need to enter here, as nf_defrag_ipv6
> > * replayed fragment zero will already have created the cp
> > --
> > 1.7.5.4
>
> Regards
>
> --
> Julian Anastasov <ja@ssi.bg>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] ipvs: Fix reuse connection if real server is dead
2013-05-11 13:23 ` Julian Anastasov
2013-05-13 1:38 ` Simon Horman
@ 2013-05-13 13:38 ` Jesper Dangaard Brouer
2013-05-13 21:56 ` [PATCH v3] " Grzegorz Łyczba
2 siblings, 0 replies; 12+ messages in thread
From: Jesper Dangaard Brouer @ 2013-05-13 13:38 UTC (permalink / raw)
To: Julian Anastasov; +Cc: Grzegorz Łyczba, lvs-devel
On Sat, 11 May 2013 16:23:33 +0300 (EEST) Julian Anastasov <ja@ssi.bg>
wrote:
> The change looks correct but you have to fix some
> coding style issues. Otherwise, patch will be rejected.
> You can also check Documentation/CodingStyle
Also remember to add a "version" tag to the subject, when you resubmit.
Subject like:
[PATCH v3] ipvs: Fix reuse connection if real server is dead
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Sr. Network Kernel Developer at Red Hat
Author of http://www.iptv-analyzer.org
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v3] ipvs: Fix reuse connection if real server is dead
2013-05-11 13:23 ` Julian Anastasov
2013-05-13 1:38 ` Simon Horman
2013-05-13 13:38 ` Jesper Dangaard Brouer
@ 2013-05-13 21:56 ` Grzegorz Łyczba
2013-05-14 5:35 ` Julian Anastasov
2 siblings, 1 reply; 12+ messages in thread
From: Grzegorz Łyczba @ 2013-05-13 21:56 UTC (permalink / raw)
To: Julian Anastasov; +Cc: lvs-devel, Simon Horman, Grzegorz Lyczba
From: Grzegorz Lyczba <grzegorz.lyczba@gmail.com>
Expire cached connection for new TCP/SCTP connection if real server is down
Signed-off-by: Grzegorz Lyczba <grzegorz.lyczba@gmail.com>
---
net/netfilter/ipvs/ip_vs_core.c | 36 ++++++++++++++++++++++++++++++++++++
1 files changed, 36 insertions(+), 0 deletions(-)
diff --git a/net/netfilter/ipvs/ip_vs_core.c b/net/netfilter/ipvs/ip_vs_core.c
index 47edf5a..2e349ca 100644
--- a/net/netfilter/ipvs/ip_vs_core.c
+++ b/net/netfilter/ipvs/ip_vs_core.c
@@ -1002,6 +1002,33 @@ static inline int is_tcp_reset(const struct sk_buff *skb, int nh_len)
return th->rst;
}
+static inline bool is_new_conn(const struct sk_buff *skb,
+ struct ip_vs_iphdr *iph)
+{
+ switch (iph->protocol) {
+ case IPPROTO_TCP: {
+ struct tcphdr _tcph, *th;
+
+ th = skb_header_pointer(skb, iph->len, sizeof(_tcph), &_tcph);
+ if (th == NULL)
+ return false;
+ return th->syn;
+ }
+ case IPPROTO_SCTP: {
+ sctp_chunkhdr_t *sch, schunk;
+
+ sch = skb_header_pointer(skb, iph->len + sizeof(sctp_sctphdr_t),
+ sizeof(schunk), &schunk);
+ if (sch == NULL)
+ return false;
+ return (sch->type == SCTP_CID_INIT);
+ }
+ default:
+ return false;
+ }
+}
+
+
/* Handle response packets: rewrite addresses and send away...
*/
static unsigned int
@@ -1627,6 +1654,15 @@ ip_vs_in(unsigned int hooknum, struct sk_buff *skb, int af)
* Check if the packet belongs to an existing connection entry
*/
cp = pp->conn_in_get(af, skb, &iph, 0);
+
+ if (unlikely(sysctl_expire_nodest_conn(ipvs)) && cp && cp->dest &&
+ unlikely(!atomic_read(&cp->dest->weight)) && !iph.fragoffs &&
+ is_new_conn(skb, &iph)) {
+ ip_vs_conn_expire_now(cp);
+ __ip_vs_conn_put(cp);
+ cp = NULL;
+ }
+
if (unlikely(!cp) && !iph.fragoffs) {
/* No (second) fragments need to enter here, as nf_defrag_ipv6
* replayed fragment zero will already have created the cp
--
1.7.5.4
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v3] ipvs: Fix reuse connection if real server is dead
2013-05-13 21:56 ` [PATCH v3] " Grzegorz Łyczba
@ 2013-05-14 5:35 ` Julian Anastasov
0 siblings, 0 replies; 12+ messages in thread
From: Julian Anastasov @ 2013-05-14 5:35 UTC (permalink / raw)
To: Grzegorz Łyczba; +Cc: lvs-devel, Simon Horman
[-- Attachment #1: Type: TEXT/PLAIN, Size: 2211 bytes --]
Hello,
On Mon, 13 May 2013, Grzegorz Łyczba wrote:
> From: Grzegorz Lyczba <grzegorz.lyczba@gmail.com>
>
> Expire cached connection for new TCP/SCTP connection if real server is down
>
> Signed-off-by: Grzegorz Lyczba <grzegorz.lyczba@gmail.com>
Looks good to me, thanks!
Acked-by: Julian Anastasov <ja@ssi.bg>
> ---
> net/netfilter/ipvs/ip_vs_core.c | 36 ++++++++++++++++++++++++++++++++++++
> 1 files changed, 36 insertions(+), 0 deletions(-)
>
> diff --git a/net/netfilter/ipvs/ip_vs_core.c b/net/netfilter/ipvs/ip_vs_core.c
> index 47edf5a..2e349ca 100644
> --- a/net/netfilter/ipvs/ip_vs_core.c
> +++ b/net/netfilter/ipvs/ip_vs_core.c
> @@ -1002,6 +1002,33 @@ static inline int is_tcp_reset(const struct sk_buff *skb, int nh_len)
> return th->rst;
> }
>
> +static inline bool is_new_conn(const struct sk_buff *skb,
> + struct ip_vs_iphdr *iph)
> +{
> + switch (iph->protocol) {
> + case IPPROTO_TCP: {
> + struct tcphdr _tcph, *th;
> +
> + th = skb_header_pointer(skb, iph->len, sizeof(_tcph), &_tcph);
> + if (th == NULL)
> + return false;
> + return th->syn;
> + }
> + case IPPROTO_SCTP: {
> + sctp_chunkhdr_t *sch, schunk;
> +
> + sch = skb_header_pointer(skb, iph->len + sizeof(sctp_sctphdr_t),
> + sizeof(schunk), &schunk);
> + if (sch == NULL)
> + return false;
> + return (sch->type == SCTP_CID_INIT);
> + }
> + default:
> + return false;
> + }
> +}
> +
> +
> /* Handle response packets: rewrite addresses and send away...
> */
> static unsigned int
> @@ -1627,6 +1654,15 @@ ip_vs_in(unsigned int hooknum, struct sk_buff *skb, int af)
> * Check if the packet belongs to an existing connection entry
> */
> cp = pp->conn_in_get(af, skb, &iph, 0);
> +
> + if (unlikely(sysctl_expire_nodest_conn(ipvs)) && cp && cp->dest &&
> + unlikely(!atomic_read(&cp->dest->weight)) && !iph.fragoffs &&
> + is_new_conn(skb, &iph)) {
> + ip_vs_conn_expire_now(cp);
> + __ip_vs_conn_put(cp);
> + cp = NULL;
> + }
> +
> if (unlikely(!cp) && !iph.fragoffs) {
> /* No (second) fragments need to enter here, as nf_defrag_ipv6
> * replayed fragment zero will already have created the cp
> --
> 1.7.5.4
Regards
--
Julian Anastasov <ja@ssi.bg>
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2013-05-14 5:35 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-05-07 21:54 [PATCH] ipvs: Fix reuse connection if real server is dead Grzegorz Lyczba
2013-05-08 6:12 ` Hans Schillstrom
2013-05-08 7:35 ` Julian Anastasov
2013-05-08 8:07 ` Hans Schillstrom
2013-05-08 8:53 ` Julian Anastasov
2013-05-08 9:21 ` Hans Schillstrom
2013-05-11 9:32 ` Grzegorz Łyczba
2013-05-11 13:23 ` Julian Anastasov
2013-05-13 1:38 ` Simon Horman
2013-05-13 13:38 ` Jesper Dangaard Brouer
2013-05-13 21:56 ` [PATCH v3] " Grzegorz Łyczba
2013-05-14 5:35 ` Julian Anastasov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.