[v2 PATCH 0/4] IPVS: Backup Adding Ipv6 and Persistence support

[v2 PATCH 0/4] IPVS: Backup Adding Ipv6 and Persistence support

[Hans Schillstrom]

This patch series adds/(updates) the following functionality
in the synchronization between master and backup daemons.

 - Ipv6
 - Persistence Engine
 - Firewall marks transfered
 - Timeouts transfered.
 - Flag field increased to 32 bits.

A new message format is also introduced, not understood by old backup daemons.
For compatibility reasons receiving the old version (version 0) is still possible.
Old (version 0) backups will just drop new (Version 1) messages.

Update scenario:
 Update the Machine with Bakup daemon first.

Message structure:
A new 32 bit word is added to the header,
where the old count_Conns is set to 0 so old Backup daemons just will drop the packet.
Added is version (1) and there is also a spare 16 bit field at the end.
This is a reworked version based upon Simon Hormans work.

 Version 1:
       0                   1                   2                   3
       0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |      0        |    Syncid     |            Size               |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |  Count Conns  |   Version     | Spare set to Zero             |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |                                                               |
      |                    IPVS Sync Connection (1)                   |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |                            .                                  |
      ~                            .                                  ~
      |                            .                                  |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |                                                               |
      |                    IPVS Sync Connection (n)                   |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

 Version 0 Header
       0                   1                   2                   3
       0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |  Count Conns  |    SyncID     |            Size               |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |                    IPVS Sync Connection (1)                   |


The Sync. Conn. message format is also changed to allow future enhancements
and versioning.

     Connection Message format

       0                   1                   2                   3
       0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |    Type       |    Protocol   | Ver.  |        Size           |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |                         Flags                                 |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |            State              |         cport                 |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |            vport              |         dport                 |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |                             fwmark                            |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |                             timeout                           |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |                              ...                              |
      |                        IP-Addresses  (IPv4 or IPv6)           |
      |                              ...                              |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
Options
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      | Option Type   | Option Length |   Option data                 |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+                               |
      |                              ...                              |
      |               +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
      |               | Option Type   | Option Length | Option data   |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+               +
      |                           Option data                         |
      |                             ...                             - |
      |          Last Opt. data should be padded for 32 bit alignment |
      +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

New fields that might need some explanation,

Type:
  Bit    7        6        . . .      2         1           0
    +-------------------------------------+-------------+-------+
    |                     Spare           | Packed IPv6 |  IPv6 |
    +-------------------------------------+-------------+-------+

#define STYPE_INET6	 1
#define STYPE_INET6_PACK 2

Ver.  Version of specified type right now it's 0

fwmark:  Firewall mark from skb.

timeout: from ip_vs_conn struct, converted to sec.


Optional Data will be added at the end without any kind of alignment
each Param data will have a header of two bytes

Option header
 Bit    7        6        . . .    0    7                      0
   +----------+----------------------+---------------------------+
   | Optional |      Option type     |    Option length          |
   +----------+----------------------+---------------------------+
   |  Option data ...                                            |
   |  ...                                                        |
   +----------+----------------------+---------------------------+
Bit 7 (msb) Optional Param, conn. entry could be keept
Bit 6-0 Option Type defined below

#define IPVS_OPT_SEQ_DATA 	1
#define IPVS_OPT_PE_DATA	2
#define IPVS_OPT_PE_NAME	3

Option Length, 1-255 bytes 0 is an illegal value.

PATCH STATUS:
 - Persistence data is not tested.


THANKS
The Persistence part is based on Simon Hormans Backup RFC
Julian for the comments and the Review of the RFC

*v2
 Simlified fwmark handling patch 1/4
 New handling of optional data patch 2/4
 Seconds in timeout
 Basically all changes is based on Julians and Simons comments on the RFC
 For details see individual patches.


SUMMARY
 include/linux/ip_vs.h           |    8 +
 include/net/ip_vs.h             |    6 +-
 net/netfilter/ipvs/ip_vs_conn.c |    5 +-
 net/netfilter/ipvs/ip_vs_core.c |    8 +-
 net/netfilter/ipvs/ip_vs_ctl.c  |    4 +-
 net/netfilter/ipvs/ip_vs_ftp.c  |    5 +-
 net/netfilter/ipvs/ip_vs_sync.c |  612 +++++++++++++++++++++++++++++++--------
 7 files changed, 508 insertions(+), 140 deletions(-)

--
Regards
Hans Schillstrom <hans.schillstrom@ericsson.com>

Re: [v2 PATCH 0/4] IPVS: Backup Adding Ipv6 and Persistence support

[Julian Anastasov]

 	Hello,

On Fri, 29 Oct 2010, Hans Schillstrom wrote:

> PATCH STATUS:
> - Persistence data is not tested.
>
>
> THANKS
> The Persistence part is based on Simon Hormans Backup RFC
> Julian for the comments and the Review of the RFC
>
> *v2
> Simlified fwmark handling patch 1/4
> New handling of optional data patch 2/4
> Seconds in timeout
> Basically all changes is based on Julians and Simons comments on the RFC
> For details see individual patches.

v2 PATCH 1/4
 	OK

v2 PATCH 2/4
 	- Do we change the name from "Option" to "Parameter" ?
 	Also, 'Option Length" will be part of the DATA, i.e.
 	it should be present depending on type, in some cases
 	it will be implicit, eg. for IPVS_OPT_SEQ_DATA:

 	IPVS_OPT_SEQ_DATA:
 		- 1 octet IPVS_OPT_SEQ_DATA followed by
 		struct ip_vs_sync_conn_options

 	IPVS_OPT_PE_DATA:
 		- 1 octet IPVS_OPT_PE_DATA, 1 or 2 octets
 		for Parameter Length (pe_data_len) followed by
 		pe_data. If the decision is to support large
 		data please use 2 octets and get_unaligned_be16
 		to read it. Other PEs may need it, they can decide
 		to add many things in PE data, they will not
 		allocate other cp fields for their data.

 	IPVS_OPT_PE_NAME:
 		- 1 octet for IPVS_OPT_PE_NAME, 1 octet
 		for Parameter Length (pe_name_len) followed by
 		pe_name

 	- struct ip_vs_sync_mesg_v2 with spaces instead of tabs?

 	- can we add check in backup for timeout not to exceed
 		(MAX_SCHEDULE_TIMEOUT / HZ)

 	For example:

 	if (timeout) {
 		if (timeout > MAX_SCHEDULE_TIMEOUT / HZ)
 			timeout = MAX_SCHEDULE_TIMEOUT / HZ;
 		cp->timeout = timeout * HZ;
 	} else ...

 	- extra space in protocol:
 	ip_vs_proc_conn(&param, flags, state, s-> protocol, AF_INET,

 	- lets drop the optional parameters from ip_vs_process_message()?

 	- ip_vs_process_message: do not use mask: m2->size & SVER_MASK
 	because the message size has no version.

 	- What is the right thing to do in ip_vs_conn_fill_param_sync
 	when ip_vs_pe_get() does not find PE? May be to drop
 	connection? May be in ip_vs_process_message() we should
 	use one pointer for end of message (it was 'p' before now),
 	so that we can skip connections, for example, when some
 	mandatory parameter as PE is not supported. We should not
 	drop the whole sync message. When message is invalid it
 	should be dropped but lack of support should not hurt
 	other connections. In the case for PE may be
 	ip_vs_conn_fill_param_sync() should return 1 if
 	PE is unknown (ip_vs_pe_get). Then caller will continue
 	with next connection if result > 0 or will drop message
 	if result < 0 as in current patch. May be the pe_name
 	presence should be the first check in
 	ip_vs_conn_fill_param_sync. Also note that p->pe is
 	pointer without reference while called in ip_vs_sched_persist.
 	In our case with ip_vs_conn_fill_param_sync we should
 	put this reference after calling ip_vs_proc_conn().
 	May be ip_vs_find_dest should check that p->pe matches
 	svc->pe. ip_vs_try_bind_dest should provide p->pe too
 	for this check. But we must somehow ignore new conns
 	with PE if they don't have cp->dest (service) because
 	it is risky to attach PE that is not held by svc.
 	It is bad that the check for p->pe is before
 	ip_vs_find_dest.

 	Example for parsing:

 	- rename msgEnd/p to 'char *endp;' and move it before loop.
 	It will have the 'p' role as before: to point to end
 	of connection.

 	before loop:
 	endp = buffer + sizeof(struct ip_vs_sync_mesg_v2);

 	In loop:

+		p = endp;
+		s = (union ip_vs_sync_conn *) endp;
+		size = ntohs(s->v4.ver_size) & SVER_MASK;
+		endp = p + size; <--- we are ready for 'continue'

 	By this way endp should point to next connection in case
 	we want to ignore current one at any time. 'p' will walk
 	parameters as you do now. After that all checks for p should be
 	with endp, I mean use 'if (p > endp)' here:

+		if (p > buffer+buflen) {
+			IP_VS_ERR_RL("bogus conn in sync message\n");
+			return;
+		}

 	- Note that pe_data is leaked when ip_vs_proc_conn() fails
 	to create connection. May be PE info for non-templates
 	should be ignored? And we need a way to know if
 	ip_vs_proc_conn called ip_vs_conn_new at all and that
 	it succeeded so that we can safely free pe_data.

 	Simon should tell what happens if some PE updates
 	ct->pe_data, may be we should replace it too for the
 	case when ip_vs_proc_conn works with existing template?

v2 PATCH 4/4
 	May be we should add configuration option if sync is enabled
 	to default to version 0 because how we solve the problem if
 	backup can not be upgraded?

 	For IPVS_OPT_PE_NAME: because we have length better not to
 	add zero terminator in sync message. It complicates the
 	checks in backup. Or backup prefers to check with zero
 	terminated string directly from message? In this case we
 	should check for existing terminator.

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [v2 PATCH 0/4] IPVS: Backup Adding Ipv6 and Persistence support

[Simon Horman]

On Sat, Oct 30, 2010 at 05:55:32PM +0300, Julian Anastasov wrote:
> 
> 	Hello,
> 
> On Fri, 29 Oct 2010, Hans Schillstrom wrote:
> 
> >PATCH STATUS:
> >- Persistence data is not tested.
> >
> >
> >THANKS
> >The Persistence part is based on Simon Hormans Backup RFC
> >Julian for the comments and the Review of the RFC
> >
> >*v2
> >Simlified fwmark handling patch 1/4
> >New handling of optional data patch 2/4
> >Seconds in timeout
> >Basically all changes is based on Julians and Simons comments on the RFC
> >For details see individual patches.
> 
> v2 PATCH 1/4
> 	OK
> 
> v2 PATCH 2/4
> 	- Do we change the name from "Option" to "Parameter" ?
> 	Also, 'Option Length" will be part of the DATA, i.e.
> 	it should be present depending on type, in some cases
> 	it will be implicit, eg. for IPVS_OPT_SEQ_DATA:
> 
> 	IPVS_OPT_SEQ_DATA:
> 		- 1 octet IPVS_OPT_SEQ_DATA followed by
> 		struct ip_vs_sync_conn_options
> 
> 	IPVS_OPT_PE_DATA:
> 		- 1 octet IPVS_OPT_PE_DATA, 1 or 2 octets
> 		for Parameter Length (pe_data_len) followed by
> 		pe_data. If the decision is to support large
> 		data please use 2 octets and get_unaligned_be16
> 		to read it. Other PEs may need it, they can decide
> 		to add many things in PE data, they will not
> 		allocate other cp fields for their data.
> 
> 	IPVS_OPT_PE_NAME:
> 		- 1 octet for IPVS_OPT_PE_NAME, 1 octet
> 		for Parameter Length (pe_name_len) followed by
> 		pe_name
> 
> 	- struct ip_vs_sync_mesg_v2 with spaces instead of tabs?
> 
> 	- can we add check in backup for timeout not to exceed
> 		(MAX_SCHEDULE_TIMEOUT / HZ)
> 
> 	For example:
> 
> 	if (timeout) {
> 		if (timeout > MAX_SCHEDULE_TIMEOUT / HZ)
> 			timeout = MAX_SCHEDULE_TIMEOUT / HZ;
> 		cp->timeout = timeout * HZ;
> 	} else ...
> 
> 	- extra space in protocol:
> 	ip_vs_proc_conn(&param, flags, state, s-> protocol, AF_INET,
> 
> 	- lets drop the optional parameters from ip_vs_process_message()?
> 
> 	- ip_vs_process_message: do not use mask: m2->size & SVER_MASK
> 	because the message size has no version.
> 
> 	- What is the right thing to do in ip_vs_conn_fill_param_sync
> 	when ip_vs_pe_get() does not find PE? May be to drop
> 	connection? May be in ip_vs_process_message() we should
> 	use one pointer for end of message (it was 'p' before now),
> 	so that we can skip connections, for example, when some
> 	mandatory parameter as PE is not supported. We should not
> 	drop the whole sync message. When message is invalid it
> 	should be dropped but lack of support should not hurt
> 	other connections. In the case for PE may be
> 	ip_vs_conn_fill_param_sync() should return 1 if
> 	PE is unknown (ip_vs_pe_get). Then caller will continue
> 	with next connection if result > 0 or will drop message
> 	if result < 0 as in current patch. May be the pe_name
> 	presence should be the first check in
> 	ip_vs_conn_fill_param_sync.

	Yes, I agree with this.

>	Also note that p->pe is
> 	pointer without reference while called in ip_vs_sched_persist.
> 	In our case with ip_vs_conn_fill_param_sync we should
> 	put this reference after calling ip_vs_proc_conn().
> 	May be ip_vs_find_dest should check that p->pe matches
> 	svc->pe. ip_vs_try_bind_dest should provide p->pe too
> 	for this check. But we must somehow ignore new conns
> 	with PE if they don't have cp->dest (service) because
> 	it is risky to attach PE that is not held by svc.
> 	It is bad that the check for p->pe is before
> 	ip_vs_find_dest.

	I have a patch, "IPVS: Add persistence engine to connection entry"
	that attaches the pe to the connection rather than the dest.
	In this case, if pe is NULL, then the connection doesn't use
	pe, which is fine. If its non-NULL, its there so there is no
	problem in that case either.

	I think this resolves the issue that you raise.

> 
> 	Example for parsing:
> 
> 	- rename msgEnd/p to 'char *endp;' and move it before loop.
> 	It will have the 'p' role as before: to point to end
> 	of connection.
> 
> 	before loop:
> 	endp = buffer + sizeof(struct ip_vs_sync_mesg_v2);
> 
> 	In loop:
> 
> +		p = endp;
> +		s = (union ip_vs_sync_conn *) endp;
> +		size = ntohs(s->v4.ver_size) & SVER_MASK;
> +		endp = p + size; <--- we are ready for 'continue'
> 
> 	By this way endp should point to next connection in case
> 	we want to ignore current one at any time. 'p' will walk
> 	parameters as you do now. After that all checks for p should be
> 	with endp, I mean use 'if (p > endp)' here:
> 
> +		if (p > buffer+buflen) {
> +			IP_VS_ERR_RL("bogus conn in sync message\n");
> +			return;
> +		}
> 
> 	- Note that pe_data is leaked when ip_vs_proc_conn() fails
> 	to create connection. May be PE info for non-templates
> 	should be ignored?

	Yes I think it should be ignored in that case.

> 	And we need a way to know if
> 	ip_vs_proc_conn called ip_vs_conn_new at all and that
> 	it succeeded so that we can safely free pe_data.

	Perhaps ip_vs_proc_conn() could just free pe_data if
	it is present but ip_vs_conn_new() is not called.

> 
> 	Simon should tell what happens if some PE updates
> 	ct->pe_data, may be we should replace it too for the
> 	case when ip_vs_proc_conn works with existing template?

	At this stage there is no facility for updating PE data.
	But I think replacing it should be fine, so long as
	the appropriate locking is in place.

> v2 PATCH 4/4
> 	May be we should add configuration option if sync is enabled
> 	to default to version 0 because how we solve the problem if
> 	backup can not be upgraded?

	I think that defaulting to v1 and having an option
	to move back to v0 would make more sense. Initially
	I would expect people to need to use v0 for the reason
	that you describe. But once the backup has been upgraded then,
	bugs not withstanding, there shouldn't be a reason not to use v1
	moving forward.

> 	For IPVS_OPT_PE_NAME: because we have length better not to
> 	add zero terminator in sync message. It complicates the
> 	checks in backup. Or backup prefers to check with zero
> 	terminated string directly from message? In this case we
> 	should check for existing terminator.

	I have no preference with regards to this.

Re: [v2 PATCH 0/4] IPVS: Backup Adding Ipv6 and Persistence support

[Hans Schillstrom]

Hello,
On Saturday 30 October 2010 16:55:32 Julian Anastasov wrote:
>
>  	Hello,
>
> On Fri, 29 Oct 2010, Hans Schillstrom wrote:
>
> > PATCH STATUS:
> > - Persistence data is not tested.
> >
> >
> > THANKS
> > The Persistence part is based on Simon Hormans Backup RFC
> > Julian for the comments and the Review of the RFC
> >
> > *v2
> > Simlified fwmark handling patch 1/4
> > New handling of optional data patch 2/4
> > Seconds in timeout
> > Basically all changes is based on Julians and Simons comments on the RFC
> > For details see individual patches.
>
> v2 PATCH 1/4
>  	OK
>
> v2 PATCH 2/4
>  	- Do we change the name from "Option" to "Parameter" ?
Sure, why not Optional Parameter

>  	Also, 'Option Length" will be part of the DATA, i.e.
>  	it should be present depending on type, in some cases
>  	it will be implicit, eg. for IPVS_OPT_SEQ_DATA:

No,
I think this is a bad idea.
Commonication protocols/formats should be simple an clean without exceptions.
So in our case Param options should consits of (without exceptions):
 - TYPE and a LENGTH field, followed by data
Where TYPE is 1 byte
and   LENGTH is 1 byte, (value 1-255, 0 is illegal)

>
>  	IPVS_OPT_SEQ_DATA:
>  		- 1 octet IPVS_OPT_SEQ_DATA followed by
>  		struct ip_vs_sync_conn_options
>

No, This breaks the rules.

>  	IPVS_OPT_PE_DATA:
>  		- 1 octet IPVS_OPT_PE_DATA, 1 or 2 octets
>  		for Parameter Length (pe_data_len) followed by
>  		pe_data. If the decision is to support large
>  		data please use 2 octets and get_unaligned_be16
>  		to read it. Other PEs may need it, they can decide
>  		to add many things in PE data, they will not
>  		allocate other cp fields for their data.
>
No,
If you need to send more than 255 bytes of data to identify a call,
then you should go back to the drawing board again.
Remember that we are using "tiny" datagrams.
If we later on needs changes there is a place for that,
 - the version.

>  	IPVS_OPT_PE_NAME:
>  		- 1 octet for IPVS_OPT_PE_NAME, 1 octet
>  		for Parameter Length (pe_name_len) followed by
>  		pe_name

Yes, It is like that now, including a terminating Zero...

>
>  	- struct ip_vs_sync_mesg_v2 with spaces instead of tabs?
>
>  	- can we add check in backup for timeout not to exceed
>  		(MAX_SCHEDULE_TIMEOUT / HZ)
>
>  	For example:
>
>  	if (timeout) {
>  		if (timeout > MAX_SCHEDULE_TIMEOUT / HZ)
>  			timeout = MAX_SCHEDULE_TIMEOUT / HZ;
>  		cp->timeout = timeout * HZ;
>  	} else ...

Yes, Sounds like a good Idea

>
>  	- extra space in protocol:
>  	ip_vs_proc_conn(&param, flags, state, s-> protocol, AF_INET,
>
Ooops, I'll remove that

>  	- lets drop the optional parameters from ip_vs_process_message()?

I guess that you mean ip_vs_sync_conn_options,
in that case I do agree.

>
>  	- ip_vs_process_message: do not use mask: m2->size & SVER_MASK
>  	because the message size has no version.

Ooops again :-)

>
>  	- What is the right thing to do in ip_vs_conn_fill_param_sync
>  	when ip_vs_pe_get() does not find PE? May be to drop
>  	connection? May be in ip_vs_process_message() we should
>  	use one pointer for end of message (it was 'p' before now),
>  	so that we can skip connections, for example, when some
>  	mandatory parameter as PE is not supported. We should not
>  	drop the whole sync message. When message is invalid it
>  	should be dropped but lack of support should not hurt
>  	other connections. In the case for PE may be
>  	ip_vs_conn_fill_param_sync() should return 1 if
>  	PE is unknown (ip_vs_pe_get). Then caller will continue
>  	with next connection if result > 0 or will drop message
>  	if result < 0 as in current patch. May be the pe_name
>  	presence should be the first check in
>  	ip_vs_conn_fill_param_sync.

Yes I'll fix that.

>       Also note that p->pe is
>  	pointer without reference while called in ip_vs_sched_persist.
>  	In our case with ip_vs_conn_fill_param_sync we should
>  	put this reference after calling ip_vs_proc_conn().
>  	May be ip_vs_find_dest should check that p->pe matches
>  	svc->pe. ip_vs_try_bind_dest should provide p->pe too
>  	for this check. But we must somehow ignore new conns
>  	with PE if they don't have cp->dest (service) because
>  	it is risky to attach PE that is not held by svc.
>  	It is bad that the check for p->pe is before
>  	ip_vs_find_dest.

Can we agree upon that Simons patch "IPVS: Add persistence engine to connection entry"
solves this issue ?

The basic principle for param decoding is;
 - Unknown param: skip this entry and continue with next
 - Invalid param length: drop the buffer.
 - Invalid param data: skip this entry and continue with next
   (If possible to check)

There is a violation to this that I will correct:
 Checking for a terminating Zero in pe_name string causes a drop of entire buffer, not just the conn. entry.


>  	Example for parsing:
>
>  	- rename msgEnd/p to 'char *endp;' and move it before loop.
>  	It will have the 'p' role as before: to point to end
>  	of connection.
>
>  	before loop:
>  	endp = buffer + sizeof(struct ip_vs_sync_mesg_v2);
>
>  	In loop:
>
> +		p = endp;
> +		s = (union ip_vs_sync_conn *) endp;
> +		size = ntohs(s->v4.ver_size) & SVER_MASK;
> +		endp = p + size; <--- we are ready for 'continue'
>
>  	By this way endp should point to next connection in case
>  	we want to ignore current one at any time. 'p' will walk
>  	parameters as you do now. After that all checks for p should be
>  	with endp, I mean use 'if (p > endp)' here:
>
> +		if (p > buffer+buflen) {
> +			IP_VS_ERR_RL("bogus conn in sync message\n");
> +			return;
> +		}
>
No problem, a minor adj.

>  	- Note that pe_data is leaked when ip_vs_proc_conn() fails
>  	to create connection. May be PE info for non-templates
>  	should be ignored? And we need a way to know if
>  	ip_vs_proc_conn called ip_vs_conn_new at all and that
>  	it succeeded so that we can safely free pe_data.
>
>  	Simon should tell what happens if some PE updates
>  	ct->pe_data, may be we should replace it too for the
>  	case when ip_vs_proc_conn works with existing template?
>

I will have a look at Simons answer.

> v2 PATCH 4/4
>  	May be we should add configuration option if sync is enabled
>  	to default to version 0 because how we solve the problem if
>  	backup can not be upgraded?

That means that we need to have a "version 0 sending functions" as well.
I think that version 1 should be default, if you ran into problems activate ver 0.

>  	For IPVS_OPT_PE_NAME: because we have length better not to
>  	add zero terminator in sync message. It complicates the
>  	checks in backup. Or backup prefers to check with zero
>  	terminated string directly from message? In this case we
>  	should check for existing terminator.

My hart says, don't use terminating Zeroes when there is a length but:
"Since there is no space to add a trailing Zero in the buffer
 some changes has to be done. (I'm not going to make temp copy!)
 Length param needs to be added to ip_vs_pe_get() and ip_vs_pe_getbyname()
 which affects ip_vs_add_service() and  ip_vs_edit_service()"

Leave it as is or remove the trailing Zero thats the question ?

>
> Regards
>
> --
> Julian Anastasov <ja@ssi.bg>


--
Regards
Hans Schillstrom <hans.schillstrom@ericsson.com>

Re: [v2 PATCH 0/4] IPVS: Backup Adding Ipv6 and Persistence support

[Hans Schillstrom]

On Sunday 31 October 2010 01:16:02 Simon Horman wrote:
> On Sat, Oct 30, 2010 at 05:55:32PM +0300, Julian Anastasov wrote:
> >
[ snip ]
> >
> > 	- What is the right thing to do in ip_vs_conn_fill_param_sync
> > 	when ip_vs_pe_get() does not find PE? May be to drop
> > 	connection? May be in ip_vs_process_message() we should
> > 	use one pointer for end of message (it was 'p' before now),
> > 	so that we can skip connections, for example, when some
> > 	mandatory parameter as PE is not supported. We should not
> > 	drop the whole sync message. When message is invalid it
> > 	should be dropped but lack of support should not hurt
> > 	other connections. In the case for PE may be
> > 	ip_vs_conn_fill_param_sync() should return 1 if
> > 	PE is unknown (ip_vs_pe_get). Then caller will continue
> > 	with next connection if result > 0 or will drop message
> > 	if result < 0 as in current patch. May be the pe_name
> > 	presence should be the first check in
> > 	ip_vs_conn_fill_param_sync.
>
> 	Yes, I agree with this.

I will fix that.

>
> >	Also note that p->pe is
> > 	pointer without reference while called in ip_vs_sched_persist.
> > 	In our case with ip_vs_conn_fill_param_sync we should
> > 	put this reference after calling ip_vs_proc_conn().
> > 	May be ip_vs_find_dest should check that p->pe matches
> > 	svc->pe. ip_vs_try_bind_dest should provide p->pe too
> > 	for this check. But we must somehow ignore new conns
> > 	with PE if they don't have cp->dest (service) because
> > 	it is risky to attach PE that is not held by svc.
> > 	It is bad that the check for p->pe is before
> > 	ip_vs_find_dest.
>
> 	I have a patch, "IPVS: Add persistence engine to connection entry"
> 	that attaches the pe to the connection rather than the dest.
> 	In this case, if pe is NULL, then the connection doesn't use
> 	pe, which is fine. If its non-NULL, its there so there is no
> 	problem in that case either.
>
> 	I think this resolves the issue that you raise.

As mention before I will include that patch.

[ snip ]

> >
> > 	- Note that pe_data is leaked when ip_vs_proc_conn() fails
> > 	to create connection. May be PE info for non-templates
> > 	should be ignored?
>
> 	Yes I think it should be ignored in that case.

OK

>
> > 	And we need a way to know if
> > 	ip_vs_proc_conn called ip_vs_conn_new at all and that
> > 	it succeeded so that we can safely free pe_data.
>
> 	Perhaps ip_vs_proc_conn() could just free pe_data if
> 	it is present but ip_vs_conn_new() is not called.
>
Hmmm, I missed that one....

> >
> > 	Simon should tell what happens if some PE updates
> > 	ct->pe_data, may be we should replace it too for the
> > 	case when ip_vs_proc_conn works with existing template?
>
> 	At this stage there is no facility for updating PE data.
> 	But I think replacing it should be fine, so long as
> 	the appropriate locking is in place.

OK

>
> > v2 PATCH 4/4
> > 	May be we should add configuration option if sync is enabled
> > 	to default to version 0 because how we solve the problem if
> > 	backup can not be upgraded?
>
> 	I think that defaulting to v1 and having an option
> 	to move back to v0 would make more sense. Initially
> 	I would expect people to need to use v0 for the reason
> 	that you describe. But once the backup has been upgraded then,
> 	bugs not withstanding, there shouldn't be a reason not to use v1
> 	moving forward.
OK, but then I need to add sending of version_0

[ snip ]

I will start cooking a v3 soon

--
Regards
Hans Schillstrom <hans.schillstrom@ericsson.com>

Re: [v2 PATCH 0/4] IPVS: Backup Adding Ipv6 and Persistence support -> option_param skip ?

[Hans Schillstrom]

On Saturday 30 October 2010 16:55:32 Julian Anastasov wrote:
[ snip ]
>
>  	- lets drop the optional parameters from ip_vs_process_message()?
>
In case we receive optional_param from an old master,
 I can't see that we can handle it in a proper way with contrack mangling the packet.

In that case:
  if we drop it in the receive path.
  we should skip sending of it as well...
  (since noboy will listen for it)


Regards
Hans Schillstrom <hans.schillstrom@ericsson.com>

Re: [v2 PATCH 0/4] IPVS: Backup Adding Ipv6 and Persistence support

[Hans Schillstrom]

On Sunday 31 October 2010 01:16:02 Simon Horman wrote:
> On Sat, Oct 30, 2010 at 05:55:32PM +0300, Julian Anastasov wrote:
> >
> > 	Hello,
> >
> > On Fri, 29 Oct 2010, Hans Schillstrom wrote:
[ snip ]
> >
> > 	Simon should tell what happens if some PE updates
> > 	ct->pe_data, may be we should replace it too for the
> > 	case when ip_vs_proc_conn works with existing template?
>
> 	At this stage there is no facility for updating PE data.
> 	But I think replacing it should be fine, so long as
> 	the appropriate locking is in place.

Julian,
I can't see where this could happen,
 but I might be out of sync :-)

IF this an issue that we must take care of,
  (Then I'm a little bit lost)
  could you please explain it for me or pass a code example of how to do it.
ENDIF
--
Regards
Hans Schillstrom <hans.schillstrom@ericsson.com>

Re: [v2 PATCH 0/4] IPVS: Backup Adding Ipv6 and Persistence support

[Julian Anastasov]

 	Hello,

On Mon, 1 Nov 2010, Hans Schillstrom wrote:

>> v2 PATCH 2/4
>>  	- Do we change the name from "Option" to "Parameter" ?
> Sure, why not Optional Parameter

 	Initially, we wanted parameters to be mandatory and
optional, more correctly the backup's treatment for them.
I.e. as a way to handle unsupported parameters in backup.
Now we decided that backup must support all parameters,
if some parameter is unknown the connection entry is
ignored. Of course, when sending parameters their
presence in message is optional and they are added only
when the connection has that property: PE DATA, etc.

>>  	Also, 'Option Length" will be part of the DATA, i.e.
>>  	it should be present depending on type, in some cases
>>  	it will be implicit, eg. for IPVS_OPT_SEQ_DATA:
>
> No,
> I think this is a bad idea.
> Commonication protocols/formats should be simple an clean without exceptions.
> So in our case Param options should consits of (without exceptions):
> - TYPE and a LENGTH field, followed by data
> Where TYPE is 1 byte
> and   LENGTH is 1 byte, (value 1-255, 0 is illegal)

 	ok, for me this method was required to accept
connection entries with unknown parameters and is not
needed if message is accepted only with known parameters.

>>  	- lets drop the optional parameters from ip_vs_process_message()?
>
> I guess that you mean ip_vs_sync_conn_options,
> in that case I do agree.

 	Not ip_vs_sync_conn_options but the support for
accepting connection entries with unknown parameters,
i.e. unknownOpt code. As for ip_vs_sync_conn_options
there is no app in mainline kernel 2.6.36+ that will trigger
this parameter. And we do not know (yet) how to sync SEQs with
Netfilter in the case for ip_vs_ftp.

>>       Also note that p->pe is
>>  	pointer without reference while called in ip_vs_sched_persist.
>>  	In our case with ip_vs_conn_fill_param_sync we should
>>  	put this reference after calling ip_vs_proc_conn().
>>  	May be ip_vs_find_dest should check that p->pe matches
>>  	svc->pe. ip_vs_try_bind_dest should provide p->pe too
>>  	for this check. But we must somehow ignore new conns
>>  	with PE if they don't have cp->dest (service) because
>>  	it is risky to attach PE that is not held by svc.
>>  	It is bad that the check for p->pe is before
>>  	ip_vs_find_dest.
>
> Can we agree upon that Simons patch "IPVS: Add persistence engine to connection entry"
> solves this issue ?

 	Yes, agreed

> The basic principle for param decoding is;
> - Unknown param: skip this entry and continue with next
> - Invalid param length: drop the buffer.
> - Invalid param data: skip this entry and continue with next
>   (If possible to check)

 	Yes, that is fine

> There is a violation to this that I will correct:
> Checking for a terminating Zero in pe_name string causes a drop of entire buffer, not just the conn. entry.
>
>> v2 PATCH 4/4
>>  	May be we should add configuration option if sync is enabled
>>  	to default to version 0 because how we solve the problem if
>>  	backup can not be upgraded?
>
> That means that we need to have a "version 0 sending functions" as well.
> I think that version 1 should be default, if you ran into problems activate ver 0.

 	I'm not sure how to solve this problem. I worry to
leave users without option to sync to some old backups that can
not be upgraded to new kernel. And if we have such config
option it is not fatal if it defaults to 1. At least, it
can be changed.

>>  	For IPVS_OPT_PE_NAME: because we have length better not to
>>  	add zero terminator in sync message. It complicates the
>>  	checks in backup. Or backup prefers to check with zero
>>  	terminated string directly from message? In this case we
>>  	should check for existing terminator.
>
> My hart says, don't use terminating Zeroes when there is a length but:
> "Since there is no space to add a trailing Zero in the buffer
> some changes has to be done. (I'm not going to make temp copy!)
> Length param needs to be added to ip_vs_pe_get() and ip_vs_pe_getbyname()
> which affects ip_vs_add_service() and  ip_vs_edit_service()"

 	May be adding pe_name_len argument to ip_vs_pe_getbyname
is a better way.

> Leave it as is or remove the trailing Zero thats the question ?

 	With above method terminator is not needed.

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [v2 PATCH 0/4] IPVS: Backup Adding Ipv6 and Persistence support

[Julian Anastasov]

 	Hello,

On Mon, 1 Nov 2010, Hans Schillstrom wrote:

>> 	At this stage there is no facility for updating PE data.
>> 	But I think replacing it should be fine, so long as
>> 	the appropriate locking is in place.
>
> Julian,
> I can't see where this could happen,
> but I might be out of sync :-)
>
> IF this an issue that we must take care of,
>  (Then I'm a little bit lost)
>  could you please explain it for me or pass a code example of how to do it.
> ENDIF

 	Hm, lets leave this update for now. We can think
for this problem when some PE needs to update its data.

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [v2 PATCH 0/4] IPVS: Backup Adding Ipv6 and Persistence support

[Hans Schillstrom]

On Monday, November 01, 2010 22:53:38 Julian Anastasov wrote:
> 
>  	Hello,
[ snip ]

> >>  	- lets drop the optional parameters from ip_vs_process_message()?
> >
> > I guess that you mean ip_vs_sync_conn_options,
> > in that case I do agree.
> 
>  	Not ip_vs_sync_conn_options but the support for
> accepting connection entries with unknown parameters,
> i.e. unknownOpt code. 

Yes, I will do that

> As for ip_vs_sync_conn_options
> there is no app in mainline kernel 2.6.36+ that will trigger
> this parameter. And we do not know (yet) how to sync SEQs with
> Netfilter in the case for ip_vs_ftp.

If we receive packets with ip_vs_sync_conn_options what shoud be done 
 - dropt that single conn msg ? (My vote)
or
 - ignore ip_vs_sync_conn_options and try to process the message ?

I leave the "dead code"  in case it's needed later on.
(except for Version 1 receive)

[ snip ]
> >> v2 PATCH 4/4
> >>  	May be we should add configuration option if sync is enabled
> >>  	to default to version 0 because how we solve the problem if
> >>  	backup can not be upgraded?
> >
> > That means that we need to have a "version 0 sending functions" as well.
> > I think that version 1 should be default, if you ran into problems activate ver 0.
> 
>  	I'm not sure how to solve this problem. I worry to
> leave users without option to sync to some old backups that can
> not be upgraded to new kernel. And if we have such config
> option it is not fatal if it defaults to 1. At least, it
> can be changed.

I have made the implementation already, at it did not become so many extra bytes.
It will be added as the last patch in the serie.

> 
> >>  	For IPVS_OPT_PE_NAME: because we have length better not to
> >>  	add zero terminator in sync message. It complicates the
> >>  	checks in backup. Or backup prefers to check with zero
> >>  	terminated string directly from message? In this case we
> >>  	should check for existing terminator.
> >
> > My hart says, don't use terminating Zeroes when there is a length but:
> > "Since there is no space to add a trailing Zero in the buffer
> > some changes has to be done. (I'm not going to make temp copy!)
> > Length param needs to be added to ip_vs_pe_get() and ip_vs_pe_getbyname()
> > which affects ip_vs_add_service() and  ip_vs_edit_service()"
> 
>  	May be adding pe_name_len argument to ip_vs_pe_getbyname
> is a better way.

Yes, I do agree.

> 
> > Leave it as is or remove the trailing Zero thats the question ?
> 
>  	With above method terminator is not needed.
> 
> Regards
> 
> --
> Julian Anastasov <ja@ssi.bg>

Regards
 Hans Schillstrom <hans@schillstrom.com>

Re: [v2 PATCH 0/4] IPVS: Backup Adding Ipv6 and Persistence support

[Julian Anastasov]

 	Hello,

On Mon, 1 Nov 2010, Hans Schillstrom wrote:

>> As for ip_vs_sync_conn_options
>> there is no app in mainline kernel 2.6.36+ that will trigger
>> this parameter. And we do not know (yet) how to sync SEQs with
>> Netfilter in the case for ip_vs_ftp.
>
> If we receive packets with ip_vs_sync_conn_options what shoud be done
> - dropt that single conn msg ? (My vote)
> or
> - ignore ip_vs_sync_conn_options and try to process the message ?
>
> I leave the "dead code"  in case it's needed later on.
> (except for Version 1 receive)

 	Why? Just copy data to cp->in_seq and cp->out_seq
as before but for version 1 make sure the values in
struct ip_vs_seq are sent in network order. If some APP
provides diff != 0 these SEQs will be sent.

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [v2 PATCH 0/4] IPVS: Backup Adding Ipv6 and Persistence support

[Hans Schillstrom]

On Tuesday 02 November 2010 01:17:39 Julian Anastasov wrote:
>
>  	Hello,
>
> On Mon, 1 Nov 2010, Hans Schillstrom wrote:
>
> >> As for ip_vs_sync_conn_options
> >> there is no app in mainline kernel 2.6.36+ that will trigger
> >> this parameter. And we do not know (yet) how to sync SEQs with
> >> Netfilter in the case for ip_vs_ftp.
> >
> > If we receive packets with ip_vs_sync_conn_options what shoud be done
> > - dropt that single conn msg ? (My vote)
> > or
> > - ignore ip_vs_sync_conn_options and try to process the message ?
> >
> > I leave the "dead code"  in case it's needed later on.
> > (except for Version 1 receive)
>
>  	Why? Just copy data to cp->in_seq and cp->out_seq
> as before but for version 1 make sure the values in
> struct ip_vs_seq are sent in network order. If some APP
> provides diff != 0 these SEQs will be sent.
>
OK, I'll do that
--
Regards
Hans Schillstrom <hans.schillstrom@ericsson.com>

Re: [v2 PATCH 0/4] IPVS: Backup Adding Ipv6 and Persistence support

[Hans Schillstrom]

Hello again
On Sunday 31 October 2010 01:16:02 Simon Horman wrote:
> On Sat, Oct 30, 2010 at 05:55:32PM +0300, Julian Anastasov wrote:
[ snip ]
> >
> > 	- What is the right thing to do in ip_vs_conn_fill_param_sync
> > 	when ip_vs_pe_get() does not find PE? May be to drop
> > 	connection? May be in ip_vs_process_message() we should
> > 	use one pointer for end of message (it was 'p' before now),
> > 	so that we can skip connections, for example, when some
> > 	mandatory parameter as PE is not supported. We should not
> > 	drop the whole sync message. When message is invalid it
> > 	should be dropped but lack of support should not hurt
> > 	other connections. In the case for PE may be
> > 	ip_vs_conn_fill_param_sync() should return 1 if
> > 	PE is unknown (ip_vs_pe_get). Then caller will continue
> > 	with next connection if result > 0 or will drop message
> > 	if result < 0 as in current patch. May be the pe_name
> > 	presence should be the first check in
> > 	ip_vs_conn_fill_param_sync.
>
> 	Yes, I agree with this.
>
> >	Also note that p->pe is
> > 	pointer without reference while called in ip_vs_sched_persist.
> > 	In our case with ip_vs_conn_fill_param_sync we should
> > 	put this reference after calling ip_vs_proc_conn().
> > 	May be ip_vs_find_dest should check that p->pe matches
> > 	svc->pe. ip_vs_try_bind_dest should provide p->pe too
> > 	for this check. But we must somehow ignore new conns
> > 	with PE if they don't have cp->dest (service) because
> > 	it is risky to attach PE that is not held by svc.
> > 	It is bad that the check for p->pe is before
> > 	ip_vs_find_dest.
>
> 	I have a patch, "IPVS: Add persistence engine to connection entry"
> 	that attaches the pe to the connection rather than the dest.
> 	In this case, if pe is NULL, then the connection doesn't use
> 	pe, which is fine. If its non-NULL, its there so there is no
> 	problem in that case either.
>
> 	I think this resolves the issue that you raise.

There is a little Ooops with this, a locking problem.
We are in a local_bh_disable() when trying to load a module...

in the sync_thread()
 ...
   local_bh_disable();
   ip_vs_process_message(tinfo->buf, len);
   local_bh_enable();

When ip_vs_process_message() get pe_data it calls
 -> ip_vs_conn_fill_param_sync() and it calls
 --> ip_vs_pe_getbyname()  and it might call
 ----> request_module(..)

My suggestion is to avoid modul loading by calling "__ip_vs_pe_getbyname()" instead,
and if it fails just drop that single sync_conn.

Any other ideas ?

--
Regards
Hans Schillstrom <hans.schillstrom@ericsson.com>

Re: [v2 PATCH 0/4] IPVS: Backup Adding Ipv6 and Persistence support

[Simon Horman]

On Wed, Nov 03, 2010 at 09:08:06PM +0100, Hans Schillstrom wrote:
> Hello again
> On Sunday 31 October 2010 01:16:02 Simon Horman wrote:
> > On Sat, Oct 30, 2010 at 05:55:32PM +0300, Julian Anastasov wrote:
> [ snip ]
> > >
> > > 	- What is the right thing to do in ip_vs_conn_fill_param_sync
> > > 	when ip_vs_pe_get() does not find PE? May be to drop
> > > 	connection? May be in ip_vs_process_message() we should
> > > 	use one pointer for end of message (it was 'p' before now),
> > > 	so that we can skip connections, for example, when some
> > > 	mandatory parameter as PE is not supported. We should not
> > > 	drop the whole sync message. When message is invalid it
> > > 	should be dropped but lack of support should not hurt
> > > 	other connections. In the case for PE may be
> > > 	ip_vs_conn_fill_param_sync() should return 1 if
> > > 	PE is unknown (ip_vs_pe_get). Then caller will continue
> > > 	with next connection if result > 0 or will drop message
> > > 	if result < 0 as in current patch. May be the pe_name
> > > 	presence should be the first check in
> > > 	ip_vs_conn_fill_param_sync.
> >
> > 	Yes, I agree with this.
> >
> > >	Also note that p->pe is
> > > 	pointer without reference while called in ip_vs_sched_persist.
> > > 	In our case with ip_vs_conn_fill_param_sync we should
> > > 	put this reference after calling ip_vs_proc_conn().
> > > 	May be ip_vs_find_dest should check that p->pe matches
> > > 	svc->pe. ip_vs_try_bind_dest should provide p->pe too
> > > 	for this check. But we must somehow ignore new conns
> > > 	with PE if they don't have cp->dest (service) because
> > > 	it is risky to attach PE that is not held by svc.
> > > 	It is bad that the check for p->pe is before
> > > 	ip_vs_find_dest.
> >
> > 	I have a patch, "IPVS: Add persistence engine to connection entry"
> > 	that attaches the pe to the connection rather than the dest.
> > 	In this case, if pe is NULL, then the connection doesn't use
> > 	pe, which is fine. If its non-NULL, its there so there is no
> > 	problem in that case either.
> >
> > 	I think this resolves the issue that you raise.
> 
> There is a little Ooops with this, a locking problem.
> We are in a local_bh_disable() when trying to load a module...
> 
> in the sync_thread()
>  ...
>    local_bh_disable();
>    ip_vs_process_message(tinfo->buf, len);
>    local_bh_enable();
> 
> When ip_vs_process_message() get pe_data it calls
>  -> ip_vs_conn_fill_param_sync() and it calls
>  --> ip_vs_pe_getbyname()  and it might call
>  ----> request_module(..)
> 
> My suggestion is to avoid modul loading by calling "__ip_vs_pe_getbyname()" instead,
> and if it fails just drop that single sync_conn.

I wonder if we could just remove the modular aspect of persistence engines
as there is currently only one module and no plans on the drawing board
for any more at this time. That is, compile ip_vs_pe_sip directly
into ip_vs.ko

I'm happy to make a patch for that.

Re: [v2 PATCH 0/4] IPVS: Backup Adding Ipv6 and Persistence support

[Hans Schillstrom]

On Saturday, November 06, 2010 01:56:14 Simon Horman wrote:
> On Wed, Nov 03, 2010 at 09:08:06PM +0100, Hans Schillstrom wrote:
> > Hello again
> > On Sunday 31 October 2010 01:16:02 Simon Horman wrote:
> > > On Sat, Oct 30, 2010 at 05:55:32PM +0300, Julian Anastasov wrote:
> > [ snip ]
> > > >
> > > > 	- What is the right thing to do in ip_vs_conn_fill_param_sync
> > > > 	when ip_vs_pe_get() does not find PE? May be to drop
> > > > 	connection? May be in ip_vs_process_message() we should
> > > > 	use one pointer for end of message (it was 'p' before now),
> > > > 	so that we can skip connections, for example, when some
> > > > 	mandatory parameter as PE is not supported. We should not
> > > > 	drop the whole sync message. When message is invalid it
> > > > 	should be dropped but lack of support should not hurt
> > > > 	other connections. In the case for PE may be
> > > > 	ip_vs_conn_fill_param_sync() should return 1 if
> > > > 	PE is unknown (ip_vs_pe_get). Then caller will continue
> > > > 	with next connection if result > 0 or will drop message
> > > > 	if result < 0 as in current patch. May be the pe_name
> > > > 	presence should be the first check in
> > > > 	ip_vs_conn_fill_param_sync.
> > >
> > > 	Yes, I agree with this.
> > >
> > > >	Also note that p->pe is
> > > > 	pointer without reference while called in ip_vs_sched_persist.
> > > > 	In our case with ip_vs_conn_fill_param_sync we should
> > > > 	put this reference after calling ip_vs_proc_conn().
> > > > 	May be ip_vs_find_dest should check that p->pe matches
> > > > 	svc->pe. ip_vs_try_bind_dest should provide p->pe too
> > > > 	for this check. But we must somehow ignore new conns
> > > > 	with PE if they don't have cp->dest (service) because
> > > > 	it is risky to attach PE that is not held by svc.
> > > > 	It is bad that the check for p->pe is before
> > > > 	ip_vs_find_dest.
> > >
> > > 	I have a patch, "IPVS: Add persistence engine to connection entry"
> > > 	that attaches the pe to the connection rather than the dest.
> > > 	In this case, if pe is NULL, then the connection doesn't use
> > > 	pe, which is fine. If its non-NULL, its there so there is no
> > > 	problem in that case either.
> > >
> > > 	I think this resolves the issue that you raise.
> > 
> > There is a little Ooops with this, a locking problem.
> > We are in a local_bh_disable() when trying to load a module...
> > 
> > in the sync_thread()
> >  ...
> >    local_bh_disable();
> >    ip_vs_process_message(tinfo->buf, len);
> >    local_bh_enable();
> > 
> > When ip_vs_process_message() get pe_data it calls
> >  -> ip_vs_conn_fill_param_sync() and it calls
> >  --> ip_vs_pe_getbyname()  and it might call
> >  ----> request_module(..)
> > 
> > My suggestion is to avoid modul loading by calling "__ip_vs_pe_getbyname()" instead,
> > and if it fails just drop that single sync_conn.
> 
> I wonder if we could just remove the modular aspect of persistence engines
> as there is currently only one module and no plans on the drawing board
> for any more at this time. That is, compile ip_vs_pe_sip directly
> into ip_vs.ko

It's OK for me, and in the long run try to break apart the big lock.
(or use netlink for backup instead)

> 
> I'm happy to make a patch for that.
Thanks, 
can you send it to me since the backup will depend upon this  patch ?



Another silly question,
Have any of you guys seen incompleet packets in ip_vs_in() ?
When I was testing SIP there was no hit for Call-Id and the reason was only a part (88 bytes) came in to  ip_vs..
skb->len shows 513 bytes but 397 bytes was missing....

The packet was comming in on eth0 and out to the RS on eth1 
Using tcpdump on the sending machines and the Receiving RS shows that 
- eth0 all 513 bytes was sent
- eth1 all 513 bytes was received at the RS.... :-?

and then the big Boss was calling and I had to go home :-(

I guess that I have to dig into this on Monday
Btw with 2.6.32  it works


Thanks
Hans 

Re: [v2 PATCH 0/4] IPVS: Backup Adding Ipv6 and Persistence support

[Simon Horman]

On Sat, Nov 06, 2010 at 11:02:50AM +0100, Hans Schillstrom wrote:
> 
> On Saturday, November 06, 2010 01:56:14 Simon Horman wrote:
> > On Wed, Nov 03, 2010 at 09:08:06PM +0100, Hans Schillstrom wrote:
> > > Hello again
> > > On Sunday 31 October 2010 01:16:02 Simon Horman wrote:
> > > > On Sat, Oct 30, 2010 at 05:55:32PM +0300, Julian Anastasov wrote:
> > > [ snip ]
> > > > >
> > > > > 	- What is the right thing to do in ip_vs_conn_fill_param_sync
> > > > > 	when ip_vs_pe_get() does not find PE? May be to drop
> > > > > 	connection? May be in ip_vs_process_message() we should
> > > > > 	use one pointer for end of message (it was 'p' before now),
> > > > > 	so that we can skip connections, for example, when some
> > > > > 	mandatory parameter as PE is not supported. We should not
> > > > > 	drop the whole sync message. When message is invalid it
> > > > > 	should be dropped but lack of support should not hurt
> > > > > 	other connections. In the case for PE may be
> > > > > 	ip_vs_conn_fill_param_sync() should return 1 if
> > > > > 	PE is unknown (ip_vs_pe_get). Then caller will continue
> > > > > 	with next connection if result > 0 or will drop message
> > > > > 	if result < 0 as in current patch. May be the pe_name
> > > > > 	presence should be the first check in
> > > > > 	ip_vs_conn_fill_param_sync.
> > > >
> > > > 	Yes, I agree with this.
> > > >
> > > > >	Also note that p->pe is
> > > > > 	pointer without reference while called in ip_vs_sched_persist.
> > > > > 	In our case with ip_vs_conn_fill_param_sync we should
> > > > > 	put this reference after calling ip_vs_proc_conn().
> > > > > 	May be ip_vs_find_dest should check that p->pe matches
> > > > > 	svc->pe. ip_vs_try_bind_dest should provide p->pe too
> > > > > 	for this check. But we must somehow ignore new conns
> > > > > 	with PE if they don't have cp->dest (service) because
> > > > > 	it is risky to attach PE that is not held by svc.
> > > > > 	It is bad that the check for p->pe is before
> > > > > 	ip_vs_find_dest.
> > > >
> > > > 	I have a patch, "IPVS: Add persistence engine to connection entry"
> > > > 	that attaches the pe to the connection rather than the dest.
> > > > 	In this case, if pe is NULL, then the connection doesn't use
> > > > 	pe, which is fine. If its non-NULL, its there so there is no
> > > > 	problem in that case either.
> > > >
> > > > 	I think this resolves the issue that you raise.
> > > 
> > > There is a little Ooops with this, a locking problem.
> > > We are in a local_bh_disable() when trying to load a module...
> > > 
> > > in the sync_thread()
> > >  ...
> > >    local_bh_disable();
> > >    ip_vs_process_message(tinfo->buf, len);
> > >    local_bh_enable();
> > > 
> > > When ip_vs_process_message() get pe_data it calls
> > >  -> ip_vs_conn_fill_param_sync() and it calls
> > >  --> ip_vs_pe_getbyname()  and it might call
> > >  ----> request_module(..)
> > > 
> > > My suggestion is to avoid modul loading by calling "__ip_vs_pe_getbyname()" instead,
> > > and if it fails just drop that single sync_conn.
> > 
> > I wonder if we could just remove the modular aspect of persistence engines
> > as there is currently only one module and no plans on the drawing board
> > for any more at this time. That is, compile ip_vs_pe_sip directly
> > into ip_vs.ko
> 
> It's OK for me, and in the long run try to break apart the big lock.
> (or use netlink for backup instead)
> 
> > 
> > I'm happy to make a patch for that.
> Thanks, 
> can you send it to me since the backup will depend upon this  patch ?

Sure, I'll get a patch together.

> Another silly question,
> Have any of you guys seen incompleet packets in ip_vs_in() ?
> When I was testing SIP there was no hit for Call-Id and the reason was only a part (88 bytes) came in to  ip_vs..
> skb->len shows 513 bytes but 397 bytes was missing....
> 
> The packet was comming in on eth0 and out to the RS on eth1 
> Using tcpdump on the sending machines and the Receiving RS shows that 
> - eth0 all 513 bytes was sent
> - eth1 all 513 bytes was received at the RS.... :-?
> 
> and then the big Boss was calling and I had to go home :-(
> 
> I guess that I have to dig into this on Monday
> Btw with 2.6.32  it works

I haven't observed that behaviour
but it would most certainly break ip_vs_pe_sip.

Re: [v2 PATCH 0/4] IPVS: Backup Adding Ipv6 and Persistence support

[Julian Anastasov]

 	Hello,

On Sat, 6 Nov 2010, Simon Horman wrote:

> I wonder if we could just remove the modular aspect of persistence engines
> as there is currently only one module and no plans on the drawing board
> for any more at this time. That is, compile ip_vs_pe_sip directly
> into ip_vs.ko

 	May be it is better svc to hold module refcnt for
PE as currently implemented. If in backup the svc and dest
are not found when creating connection with PE data then just
ignore the connection. The PE name must match the PE attached
to svc (ip_vs_find_dest). This check must exist. The benefit
comes from the fact that svc is freed after all its connections
are freed, cp->dest->svc is always valid. Then there is no
need for cp->pe. ip_vs_conn_hashkey_conn() has checks for
cp->dest, so there is no point to try to create synced
connections in backup with PE but without cp->dest.

 	The benefits:

- no cp->pe, it is not needed, the persistent scheduler
gets PE from svc, i.e. PEs are needed only during
scheduling, so svc, PE and dest are present

- PEs can be modular

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [v2 PATCH 0/4] IPVS: Backup Adding Ipv6 and Persistence support

[Simon Horman]

On Sat, Nov 06, 2010 at 04:07:22PM +0200, Julian Anastasov wrote:
> 
> 	Hello,
> 
> On Sat, 6 Nov 2010, Simon Horman wrote:
> 
> >I wonder if we could just remove the modular aspect of persistence engines
> >as there is currently only one module and no plans on the drawing board
> >for any more at this time. That is, compile ip_vs_pe_sip directly
> >into ip_vs.ko
> 
> 	May be it is better svc to hold module refcnt for
> PE as currently implemented. If in backup the svc and dest
> are not found when creating connection with PE data then just
> ignore the connection.

As far as I understand, the svc and dest existing hasn't
really been a requirement for syncrhonisation, except in corner cases.
Personally I think thats a good thing. But making it a requirement
would certainly simplify things.

> The PE name must match the PE attached
> to svc (ip_vs_find_dest). This check must exist. The benefit
> comes from the fact that svc is freed after all its connections
> are freed, cp->dest->svc is always valid. Then there is no
> need for cp->pe. ip_vs_conn_hashkey_conn() has checks for
> cp->dest, so there is no point to try to create synced
> connections in backup with PE but without cp->dest.

But dest could be created as part of failover and thus
exist by the time any packets need to be forwarded, right?

There are cases, such as where the backup is also a real-server
that its rather inconvenient for svc and dst to exist while
synchronisation information is being received.

> 	The benefits:
> 
> - no cp->pe, it is not needed, the persistent scheduler
> gets PE from svc, i.e. PEs are needed only during
> scheduling, so svc, PE and dest are present
> 
> - PEs can be modular

These are clear wins. But I think that we need to think
carefully when deciding that svc and dest must exist
on the backup for successful synchronisation.

Re: [v2 PATCH 0/4] IPVS: Backup Adding Ipv6 and Persistence support

[Julian Anastasov]

 	Hello,

On Sat, 6 Nov 2010, Simon Horman wrote:

>> 	May be it is better svc to hold module refcnt for
>> PE as currently implemented. If in backup the svc and dest
>> are not found when creating connection with PE data then just
>> ignore the connection.
>
> As far as I understand, the svc and dest existing hasn't
> really been a requirement for syncrhonisation, except in corner cases.
> Personally I think thats a good thing. But making it a requirement
> would certainly simplify things.

 	I mean, requirement only for connections with PE data.
But lets leave it this way, so that we can support deferred
binding to dest.

>> The PE name must match the PE attached
>> to svc (ip_vs_find_dest). This check must exist. The benefit
>> comes from the fact that svc is freed after all its connections
>> are freed, cp->dest->svc is always valid. Then there is no
>> need for cp->pe. ip_vs_conn_hashkey_conn() has checks for
>> cp->dest, so there is no point to try to create synced
>> connections in backup with PE but without cp->dest.
>
> But dest could be created as part of failover and thus
> exist by the time any packets need to be forwarded, right?
>
> There are cases, such as where the backup is also a real-server
> that its rather inconvenient for svc and dst to exist while
> synchronisation information is being received.

 	OK, then we should not reach request_module,
new arg to ip_vs_pe_get() can specify that we call it
from interrupt, so the PE must be already loaded as module.
Then cp->pe can hold the reference to PE until
we bind the template to svc and dest where svc->pe
should be compared to ct->pe. ct->pe is needed only
for this purpose because later it can be determined
from svc. But I see another problem which is not backup
specific: how ip_vs_sip_ct_match knows that ct->pe_data
is created by ip_vs_sip_fill_param and not by another PE?
We need to compare p->pe with cp->pe in ip_vs_ct_in_get
before calling ct_match.

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [v2 PATCH 0/4] IPVS: Backup Adding Ipv6 and Persistence support

[Simon Horman]

On Sat, Nov 06, 2010 at 08:57:11PM +0200, Julian Anastasov wrote:
> 
> 	Hello,
> 
> On Sat, 6 Nov 2010, Simon Horman wrote:
> 
> >>	May be it is better svc to hold module refcnt for
> >>PE as currently implemented. If in backup the svc and dest
> >>are not found when creating connection with PE data then just
> >>ignore the connection.
> >
> >As far as I understand, the svc and dest existing hasn't
> >really been a requirement for syncrhonisation, except in corner cases.
> >Personally I think thats a good thing. But making it a requirement
> >would certainly simplify things.
> 
> 	I mean, requirement only for connections with PE data.
> But lets leave it this way, so that we can support deferred
> binding to dest.
> 
> >>The PE name must match the PE attached
> >>to svc (ip_vs_find_dest). This check must exist. The benefit
> >>comes from the fact that svc is freed after all its connections
> >>are freed, cp->dest->svc is always valid. Then there is no
> >>need for cp->pe. ip_vs_conn_hashkey_conn() has checks for
> >>cp->dest, so there is no point to try to create synced
> >>connections in backup with PE but without cp->dest.
> >
> >But dest could be created as part of failover and thus
> >exist by the time any packets need to be forwarded, right?
> >
> >There are cases, such as where the backup is also a real-server
> >that its rather inconvenient for svc and dst to exist while
> >synchronisation information is being received.
> 
> 	OK, then we should not reach request_module,
> new arg to ip_vs_pe_get() can specify that we call it
> from interrupt, so the PE must be already loaded as module.
> Then cp->pe can hold the reference to PE until
> we bind the template to svc and dest where svc->pe
> should be compared to ct->pe. ct->pe is needed only
> for this purpose because later it can be determined
> from svc.

Do you have a preference for this approach
over making ip_vs_pe_sip non-modular?

> But I see another problem which is not backup
> specific: how ip_vs_sip_ct_match knows that ct->pe_data
> is created by ip_vs_sip_fill_param and not by another PE?
> We need to compare p->pe with cp->pe in ip_vs_ct_in_get
> before calling ct_match.

Yes, I agree that is a problem.

In practice it won't be affecting anyone at this time
as there is only one pe.

How about this, which applies on top of
"IPVS: Add persistence engine to connection entry".

From: Simon Horman <horms@verge.net.au>
Subject: IPVS: Only match pe_data created by the same pe

Only match persistence engine data if it was
created by the same persistence engine.

Reported-by: Julian Anastasov <ja@ssi.bg>
Signed-off-by: Simon Horman <horms@verge.net.au>

Index: lvs-test-2.6/net/netfilter/ipvs/ip_vs_conn.c
===================================================================
--- lvs-test-2.6.orig/net/netfilter/ipvs/ip_vs_conn.c	2010-11-08 15:18:57.000000000 +0900
+++ lvs-test-2.6/net/netfilter/ipvs/ip_vs_conn.c	2010-11-08 15:19:02.000000000 +0900
@@ -354,7 +354,7 @@ struct ip_vs_conn *ip_vs_ct_in_get(const
 
 	list_for_each_entry(cp, &ip_vs_conn_tab[hash], c_list) {
 		if (p->pe_data && p->pe->ct_match) {
-			if (p->pe->ct_match(p, cp))
+			if (p->pe == cp->pe && p->pe->ct_match(p, cp))
 				goto out;
 			continue;
 		}

Re: [v2 PATCH 0/4] IPVS: Backup Adding Ipv6 and Persistence support

[Julian Anastasov]

 	Hello,

On Mon, 8 Nov 2010, Simon Horman wrote:

>>> But dest could be created as part of failover and thus
>>> exist by the time any packets need to be forwarded, right?
>>>
>>> There are cases, such as where the backup is also a real-server
>>> that its rather inconvenient for svc and dst to exist while
>>> synchronisation information is being received.
>>
>> 	OK, then we should not reach request_module,
>> new arg to ip_vs_pe_get() can specify that we call it
>> from interrupt, so the PE must be already loaded as module.
>> Then cp->pe can hold the reference to PE until
>> we bind the template to svc and dest where svc->pe
>> should be compared to ct->pe. ct->pe is needed only
>> for this purpose because later it can be determined
>> from svc.
>
> Do you have a preference for this approach
> over making ip_vs_pe_sip non-modular?

 	We already decided about "IPVS: Add persistence engine to 
connection entry", so cp->pe should be attached to backup
and I hope Hans will add checks for same PE in ip_vs_find_dest
and ip_vs_try_bind_dest. Then the only problem remains
to change code so that request_module is not called by
softirq. If the svc is not created yet in backup to
load the PE module, it must be loaded manually to
allow connections with PE to be created. If you still
prefer to see some code I have to create fresh tree later
today. May be if Hans uses ip_vs_pe_getbyname instead of
ip_vs_pe_get that should solve the request_module problem.

 	What should we do if PE module is not loaded
while we are creating connection in backup? We can not
load modules, may be when connection is bound to
dest+svc we should inherit the PE from svc->pe ?
May be using request_module_nowait is not an option
because we risk to try forever if module is not
present.

>> But I see another problem which is not backup
>> specific: how ip_vs_sip_ct_match knows that ct->pe_data
>> is created by ip_vs_sip_fill_param and not by another PE?
>> We need to compare p->pe with cp->pe in ip_vs_ct_in_get
>> before calling ct_match.
>
> Yes, I agree that is a problem.
>
> In practice it won't be affecting anyone at this time
> as there is only one pe.
>
> How about this, which applies on top of
> "IPVS: Add persistence engine to connection entry".

 	Yes, it is fine

Signed-off-by: Julian Anastasov <ja@ssi.bg>

> From: Simon Horman <horms@verge.net.au>
> Subject: IPVS: Only match pe_data created by the same pe
>
> Only match persistence engine data if it was
> created by the same persistence engine.
>
> Reported-by: Julian Anastasov <ja@ssi.bg>
> Signed-off-by: Simon Horman <horms@verge.net.au>
>
> Index: lvs-test-2.6/net/netfilter/ipvs/ip_vs_conn.c
> ===================================================================
> --- lvs-test-2.6.orig/net/netfilter/ipvs/ip_vs_conn.c	2010-11-08 15:18:57.000000000 +0900
> +++ lvs-test-2.6/net/netfilter/ipvs/ip_vs_conn.c	2010-11-08 15:19:02.000000000 +0900
> @@ -354,7 +354,7 @@ struct ip_vs_conn *ip_vs_ct_in_get(const
>
> 	list_for_each_entry(cp, &ip_vs_conn_tab[hash], c_list) {
> 		if (p->pe_data && p->pe->ct_match) {
> -			if (p->pe->ct_match(p, cp))
> +			if (p->pe == cp->pe && p->pe->ct_match(p, cp))
> 				goto out;
> 			continue;
> 		}

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [v2 PATCH 0/4] IPVS: Backup Adding Ipv6 and Persistence support

[Simon Horman]

On Mon, Nov 08, 2010 at 10:51:58AM +0200, Julian Anastasov wrote:
> 
> 	Hello,
> 
> On Mon, 8 Nov 2010, Simon Horman wrote:
> 
> >>>But dest could be created as part of failover and thus
> >>>exist by the time any packets need to be forwarded, right?
> >>>
> >>>There are cases, such as where the backup is also a real-server
> >>>that its rather inconvenient for svc and dst to exist while
> >>>synchronisation information is being received.
> >>
> >>	OK, then we should not reach request_module,
> >>new arg to ip_vs_pe_get() can specify that we call it
> >>from interrupt, so the PE must be already loaded as module.
> >>Then cp->pe can hold the reference to PE until
> >>we bind the template to svc and dest where svc->pe
> >>should be compared to ct->pe. ct->pe is needed only
> >>for this purpose because later it can be determined
> >>from svc.
> >
> >Do you have a preference for this approach
> >over making ip_vs_pe_sip non-modular?
> 
> 	We already decided about "IPVS: Add persistence engine to
> connection entry", so cp->pe should be attached to backup
> and I hope Hans will add checks for same PE in ip_vs_find_dest
> and ip_vs_try_bind_dest. Then the only problem remains
> to change code so that request_module is not called by
> softirq. If the svc is not created yet in backup to
> load the PE module, it must be loaded manually to
> allow connections with PE to be created. If you still
> prefer to see some code I have to create fresh tree later
> today. May be if Hans uses ip_vs_pe_getbyname instead of
> ip_vs_pe_get that should solve the request_module problem.

I changed things around a bit in "IPVS: Add persistence engine to
connection entry".

	ip_vs_pe_getbyname() became __ip_vs_pe_getbyname()
	ip_vs_pe_get() became ip_vs_pe_getbyname()
	And ip_vs_pe_get() now just takes a reference on the module if its
	loaded.

So yes I agree, except that __ip_vs_pe_getbyname() is the name
of the function that should be called, which needs to be made
un-static and possibly renamed (again).

Also, to __ip_vs_pe_getbyname() calls try_module_get().
Is that safe from interrupt context?

> 	What should we do if PE module is not loaded
> while we are creating connection in backup? We can not
> load modules, may be when connection is bound to
> dest+svc we should inherit the PE from svc->pe ?

If the modules isn't loaded, then svc->pe can't be non-NULL, right?

> May be using request_module_nowait is not an option
> because we risk to try forever if module is not
> present.

That does not sound desirable.

> >> But I see another problem which is not backup
> >> specific: how ip_vs_sip_ct_match knows that ct->pe_data
> >> is created by ip_vs_sip_fill_param and not by another PE?
> >> We need to compare p->pe with cp->pe in ip_vs_ct_in_get
> >> before calling ct_match.
> >
> > Yes, I agree that is a problem.
> >
> > In practice it won't be affecting anyone at this time
> > as there is only one pe.
> >
> > How about this, which applies on top of
> > "IPVS: Add persistence engine to connection entry".
>
>	Yes, it is fine

Thanks. I have pushed "IPVS: Add persistence engine to connection entry",
the change below, and a few other (unrelated) changes that I have been
sitting on into a staging branch of lvs-test-2.6.

I may rebase the staging branch - by which I mean its intended
to be a transient branch - but I figure its better than nothing.

> Signed-off-by: Julian Anastasov <ja@ssi.bg>
> 
> > From: Simon Horman <horms@verge.net.au>
> > Subject: IPVS: Only match pe_data created by the same pe
> >
> > Only match persistence engine data if it was
> > created by the same persistence engine.
> >
> > Reported-by: Julian Anastasov <ja@ssi.bg>
> > Signed-off-by: Simon Horman <horms@verge.net.au>
> >
> > Index: lvs-test-2.6/net/netfilter/ipvs/ip_vs_conn.c
> > ===================================================================
> > --- lvs-test-2.6.orig/net/netfilter/ipvs/ip_vs_conn.c	2010-11-08 15:18:57.000000000 +0900
> > +++ lvs-test-2.6/net/netfilter/ipvs/ip_vs_conn.c	2010-11-08 15:19:02.000000000 +0900
> > @@ -354,7 +354,7 @@ struct ip_vs_conn *ip_vs_ct_in_get(const
> >
> > 	list_for_each_entry(cp, &ip_vs_conn_tab[hash], c_list) {
> > 		if (p->pe_data && p->pe->ct_match) {
> > -			if (p->pe->ct_match(p, cp))
> > +			if (p->pe == cp->pe && p->pe->ct_match(p, cp))
> > 				goto out;
> > 			continue;
> > 		}

Re: [v2 PATCH 0/4] IPVS: Backup Adding Ipv6 and Persistence support

[Hans Schillstrom]

On Monday 08 November 2010 12:16:02 Simon Horman wrote:
> On Mon, Nov 08, 2010 at 10:51:58AM +0200, Julian Anastasov wrote:
> >
> > 	Hello,
> >
> > On Mon, 8 Nov 2010, Simon Horman wrote:
> >
> > >>>But dest could be created as part of failover and thus
> > >>>exist by the time any packets need to be forwarded, right?
> > >>>
> > >>>There are cases, such as where the backup is also a real-server
> > >>>that its rather inconvenient for svc and dst to exist while
> > >>>synchronisation information is being received.
> > >>
> > >>	OK, then we should not reach request_module,
> > >>new arg to ip_vs_pe_get() can specify that we call it
> > >>from interrupt, so the PE must be already loaded as module.
> > >>Then cp->pe can hold the reference to PE until
> > >>we bind the template to svc and dest where svc->pe
> > >>should be compared to ct->pe. ct->pe is needed only
> > >>for this purpose because later it can be determined
> > >>from svc.
> > >
> > >Do you have a preference for this approach
> > >over making ip_vs_pe_sip non-modular?
> >
> > 	We already decided about "IPVS: Add persistence engine to
> > connection entry", so cp->pe should be attached to backup
> > and I hope Hans will add checks for same PE in ip_vs_find_dest
> > and ip_vs_try_bind_dest.

Sure.

> > Then the only problem remains
> > to change code so that request_module is not called by
> > softirq. If the svc is not created yet in backup to
> > load the PE module, it must be loaded manually to
> > allow connections with PE to be created. If you still
> > prefer to see some code I have to create fresh tree later
> > today. May be if Hans uses ip_vs_pe_getbyname instead of
> > ip_vs_pe_get that should solve the request_module problem.

From my point of view it's a configuration error,
if the pe modules aint loaded in the backup machine.

>
> I changed things around a bit in "IPVS: Add persistence engine to
> connection entry".
>
> 	ip_vs_pe_getbyname() became __ip_vs_pe_getbyname()
> 	ip_vs_pe_get() became ip_vs_pe_getbyname()
> 	And ip_vs_pe_get() now just takes a reference on the module if its
> 	loaded.
>
> So yes I agree, except that __ip_vs_pe_getbyname() is the name
> of the function that should be called, which needs to be made
> un-static and possibly renamed (again).
>
> Also, to __ip_vs_pe_getbyname() calls try_module_get().
> Is that safe from interrupt context?

I think so, or at least it seems to work :-)

> > 	What should we do if PE module is not loaded
> > while we are creating connection in backup? We can not
> > load modules, may be when connection is bound to
> > dest+svc we should inherit the PE from svc->pe ?

I think we shoud drop that conn.
(Still I think it's a configuration error)

>
> If the modules isn't loaded, then svc->pe can't be non-NULL, right?
>
> > May be using request_module_nowait is not an option
> > because we risk to try forever if module is not
> > present.
>
> That does not sound desirable.

I tried before and got a dump...

>
> > >> But I see another problem which is not backup
> > >> specific: how ip_vs_sip_ct_match knows that ct->pe_data
> > >> is created by ip_vs_sip_fill_param and not by another PE?
> > >> We need to compare p->pe with cp->pe in ip_vs_ct_in_get
> > >> before calling ct_match.
> > >
> > > Yes, I agree that is a problem.
> > >
> > > In practice it won't be affecting anyone at this time
> > > as there is only one pe.
> > >
> > > How about this, which applies on top of
> > > "IPVS: Add persistence engine to connection entry".
> >
> >	Yes, it is fine
>
> Thanks. I have pushed "IPVS: Add persistence engine to connection entry",
> the change below, and a few other (unrelated) changes that I have been
> sitting on into a staging branch of lvs-test-2.6.
>
> I may rebase the staging branch - by which I mean its intended
> to be a transient branch - but I figure its better than nothing.
>
Thanks

> > Signed-off-by: Julian Anastasov <ja@ssi.bg>
> >
> > > From: Simon Horman <horms@verge.net.au>
> > > Subject: IPVS: Only match pe_data created by the same pe
> > >
> > > Only match persistence engine data if it was
> > > created by the same persistence engine.
> > >
> > > Reported-by: Julian Anastasov <ja@ssi.bg>
> > > Signed-off-by: Simon Horman <horms@verge.net.au>
> > >
> > > Index: lvs-test-2.6/net/netfilter/ipvs/ip_vs_conn.c
> > > ===================================================================
> > > --- lvs-test-2.6.orig/net/netfilter/ipvs/ip_vs_conn.c	2010-11-08 15:18:57.000000000 +0900
> > > +++ lvs-test-2.6/net/netfilter/ipvs/ip_vs_conn.c	2010-11-08 15:19:02.000000000 +0900
> > > @@ -354,7 +354,7 @@ struct ip_vs_conn *ip_vs_ct_in_get(const
> > >
> > > 	list_for_each_entry(cp, &ip_vs_conn_tab[hash], c_list) {
> > > 		if (p->pe_data && p->pe->ct_match) {
> > > -			if (p->pe->ct_match(p, cp))
> > > +			if (p->pe == cp->pe && p->pe->ct_match(p, cp))
> > > 				goto out;
> > > 			continue;
> > > 		}
> --
> 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
>

--
Regards
Hans Schillstrom <hans.schillstrom@ericsson.com>

Re: [v2 PATCH 0/4] IPVS: Backup Adding Ipv6 and Persistence support

[Hans Schillstrom]

On Monday 08 November 2010 07:21:23 Simon Horman wrote:
[ snip ]

> How about this, which applies on top of
> "IPVS: Add persistence engine to connection entry".
>
> From: Simon Horman <horms@verge.net.au>
> Subject: IPVS: Only match pe_data created by the same pe
>
> Only match persistence engine data if it was
> created by the same persistence engine.
>
> Reported-by: Julian Anastasov <ja@ssi.bg>
> Signed-off-by: Simon Horman <horms@verge.net.au>

Thanks I'll will use that one,
I think it's time to cook a new backup patch,
or do any one have more patches in the pipe ?

>
> Index: lvs-test-2.6/net/netfilter/ipvs/ip_vs_conn.c
> ===================================================================
> --- lvs-test-2.6.orig/net/netfilter/ipvs/ip_vs_conn.c	2010-11-08 15:18:57.000000000 +0900
> +++ lvs-test-2.6/net/netfilter/ipvs/ip_vs_conn.c	2010-11-08 15:19:02.000000000 +0900
> @@ -354,7 +354,7 @@ struct ip_vs_conn *ip_vs_ct_in_get(const
>
>  	list_for_each_entry(cp, &ip_vs_conn_tab[hash], c_list) {
>  		if (p->pe_data && p->pe->ct_match) {
> -			if (p->pe->ct_match(p, cp))
> +			if (p->pe == cp->pe && p->pe->ct_match(p, cp))
>  				goto out;
>  			continue;
>  		}
> --
> 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
>

--
Regards
Hans Schillstrom <hans.schillstrom@ericsson.com>

Re: [v2 PATCH 0/4] IPVS: Backup Adding Ipv6 and Persistence support

[Julian Anastasov]

 	Hello,

On Mon, 8 Nov 2010, Simon Horman wrote:

>> today. May be if Hans uses ip_vs_pe_getbyname instead of
>> ip_vs_pe_get that should solve the request_module problem.
>
> I changed things around a bit in "IPVS: Add persistence engine to
> connection entry".
>
> 	ip_vs_pe_getbyname() became __ip_vs_pe_getbyname()
> 	ip_vs_pe_get() became ip_vs_pe_getbyname()
> 	And ip_vs_pe_get() now just takes a reference on the module if its
> 	loaded.
>
> So yes I agree, except that __ip_vs_pe_getbyname() is the name
> of the function that should be called, which needs to be made
> un-static and possibly renamed (again).
>
> Also, to __ip_vs_pe_getbyname() calls try_module_get().
> Is that safe from interrupt context?

 	Yes, we already use it for cp->app: ip_vs_bind_app ->
tcp_app_conn_bind -> ip_vs_app_inc_get -> ip_vs_app_get

>> 	What should we do if PE module is not loaded
>> while we are creating connection in backup? We can not
>> load modules, may be when connection is bound to
>> dest+svc we should inherit the PE from svc->pe ?
>
> If the modules isn't loaded, then svc->pe can't be non-NULL, right?

 	Adding svc will load the PE module, for example:

- backup creates template but there is no PE => cp->pe remains NULL if
 	we want to keep conn entry. Option 2 is that we can ignore
 	the conn entry if the PE module is not loaded before this step

- svc is added => PE module is loaded (request_module)

- next sync message comes and we bind cp->dest => if cp->pe
is NULL we should set cp->pe to svc->pe. The problem here is
that svc can be added without PE, then this template will not
work as expected.

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [v2 PATCH 0/4] IPVS: Backup Adding Ipv6 and Persistence support

[Simon Horman]

On Mon, Nov 08, 2010 at 04:07:00PM +0100, Hans Schillstrom wrote:
> On Monday 08 November 2010 12:16:02 Simon Horman wrote:
> > On Mon, Nov 08, 2010 at 10:51:58AM +0200, Julian Anastasov wrote:
> > >
> > > 	Hello,
> > >
> > > On Mon, 8 Nov 2010, Simon Horman wrote:
> > >
> > > >>>But dest could be created as part of failover and thus
> > > >>>exist by the time any packets need to be forwarded, right?
> > > >>>
> > > >>>There are cases, such as where the backup is also a real-server
> > > >>>that its rather inconvenient for svc and dst to exist while
> > > >>>synchronisation information is being received.
> > > >>
> > > >>	OK, then we should not reach request_module,
> > > >>new arg to ip_vs_pe_get() can specify that we call it
> > > >>from interrupt, so the PE must be already loaded as module.
> > > >>Then cp->pe can hold the reference to PE until
> > > >>we bind the template to svc and dest where svc->pe
> > > >>should be compared to ct->pe. ct->pe is needed only
> > > >>for this purpose because later it can be determined
> > > >>from svc.
> > > >
> > > >Do you have a preference for this approach
> > > >over making ip_vs_pe_sip non-modular?
> > >
> > > 	We already decided about "IPVS: Add persistence engine to
> > > connection entry", so cp->pe should be attached to backup
> > > and I hope Hans will add checks for same PE in ip_vs_find_dest
> > > and ip_vs_try_bind_dest.
> 
> Sure.
> 
> > > Then the only problem remains
> > > to change code so that request_module is not called by
> > > softirq. If the svc is not created yet in backup to
> > > load the PE module, it must be loaded manually to
> > > allow connections with PE to be created. If you still
> > > prefer to see some code I have to create fresh tree later
> > > today. May be if Hans uses ip_vs_pe_getbyname instead of
> > > ip_vs_pe_get that should solve the request_module problem.
> 
> From my point of view it's a configuration error,
> if the pe modules aint loaded in the backup machine.
> 
> >
> > I changed things around a bit in "IPVS: Add persistence engine to
> > connection entry".
> >
> > 	ip_vs_pe_getbyname() became __ip_vs_pe_getbyname()
> > 	ip_vs_pe_get() became ip_vs_pe_getbyname()
> > 	And ip_vs_pe_get() now just takes a reference on the module if its
> > 	loaded.
> >
> > So yes I agree, except that __ip_vs_pe_getbyname() is the name
> > of the function that should be called, which needs to be made
> > un-static and possibly renamed (again).
> >
> > Also, to __ip_vs_pe_getbyname() calls try_module_get().
> > Is that safe from interrupt context?
> 
> I think so, or at least it seems to work :-)
> 
> > > 	What should we do if PE module is not loaded
> > > while we are creating connection in backup? We can not
> > > load modules, may be when connection is bound to
> > > dest+svc we should inherit the PE from svc->pe ?
> 
> I think we shoud drop that conn.
> (Still I think it's a configuration error)

Yes, I agree (now).

> > If the modules isn't loaded, then svc->pe can't be non-NULL, right?
> >
> > > May be using request_module_nowait is not an option
> > > because we risk to try forever if module is not
> > > present.
> >
> > That does not sound desirable.
> 
> I tried before and got a dump...
> 
> >
> > > >> But I see another problem which is not backup
> > > >> specific: how ip_vs_sip_ct_match knows that ct->pe_data
> > > >> is created by ip_vs_sip_fill_param and not by another PE?
> > > >> We need to compare p->pe with cp->pe in ip_vs_ct_in_get
> > > >> before calling ct_match.
> > > >
> > > > Yes, I agree that is a problem.
> > > >
> > > > In practice it won't be affecting anyone at this time
> > > > as there is only one pe.
> > > >
> > > > How about this, which applies on top of
> > > > "IPVS: Add persistence engine to connection entry".
> > >
> > >	Yes, it is fine
> >
> > Thanks. I have pushed "IPVS: Add persistence engine to connection entry",
> > the change below, and a few other (unrelated) changes that I have been
> > sitting on into a staging branch of lvs-test-2.6.
> >
> > I may rebase the staging branch - by which I mean its intended
> > to be a transient branch - but I figure its better than nothing.
> >
> Thanks
> 
> > > Signed-off-by: Julian Anastasov <ja@ssi.bg>
> > >
> > > > From: Simon Horman <horms@verge.net.au>
> > > > Subject: IPVS: Only match pe_data created by the same pe
> > > >
> > > > Only match persistence engine data if it was
> > > > created by the same persistence engine.
> > > >
> > > > Reported-by: Julian Anastasov <ja@ssi.bg>
> > > > Signed-off-by: Simon Horman <horms@verge.net.au>
> > > >
> > > > Index: lvs-test-2.6/net/netfilter/ipvs/ip_vs_conn.c
> > > > ===================================================================
> > > > --- lvs-test-2.6.orig/net/netfilter/ipvs/ip_vs_conn.c	2010-11-08 15:18:57.000000000 +0900
> > > > +++ lvs-test-2.6/net/netfilter/ipvs/ip_vs_conn.c	2010-11-08 15:19:02.000000000 +0900
> > > > @@ -354,7 +354,7 @@ struct ip_vs_conn *ip_vs_ct_in_get(const
> > > >
> > > > 	list_for_each_entry(cp, &ip_vs_conn_tab[hash], c_list) {
> > > > 		if (p->pe_data && p->pe->ct_match) {
> > > > -			if (p->pe->ct_match(p, cp))
> > > > +			if (p->pe == cp->pe && p->pe->ct_match(p, cp))
> > > > 				goto out;
> > > > 			continue;
> > > > 		}
> > --
> > 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
> >
> 
> --
> Regards
> Hans Schillstrom <hans.schillstrom@ericsson.com>
> 

Re: [v2 PATCH 0/4] IPVS: Backup Adding Ipv6 and Persistence support

[Simon Horman]

On Mon, Nov 08, 2010 at 10:59:39PM +0200, Julian Anastasov wrote:
> 
> 	Hello,
> 
> On Mon, 8 Nov 2010, Simon Horman wrote:
> 
> >>today. May be if Hans uses ip_vs_pe_getbyname instead of
> >>ip_vs_pe_get that should solve the request_module problem.
> >
> >I changed things around a bit in "IPVS: Add persistence engine to
> >connection entry".
> >
> >	ip_vs_pe_getbyname() became __ip_vs_pe_getbyname()
> >	ip_vs_pe_get() became ip_vs_pe_getbyname()
> >	And ip_vs_pe_get() now just takes a reference on the module if its
> >	loaded.
> >
> >So yes I agree, except that __ip_vs_pe_getbyname() is the name
> >of the function that should be called, which needs to be made
> >un-static and possibly renamed (again).
> >
> >Also, to __ip_vs_pe_getbyname() calls try_module_get().
> >Is that safe from interrupt context?
> 
> 	Yes, we already use it for cp->app: ip_vs_bind_app ->
> tcp_app_conn_bind -> ip_vs_app_inc_get -> ip_vs_app_get
> 
> >>	What should we do if PE module is not loaded
> >>while we are creating connection in backup? We can not
> >>load modules, may be when connection is bound to
> >>dest+svc we should inherit the PE from svc->pe ?
> >
> >If the modules isn't loaded, then svc->pe can't be non-NULL, right?
> 
> 	Adding svc will load the PE module, for example:
> 
> - backup creates template but there is no PE => cp->pe remains NULL if
> 	we want to keep conn entry. Option 2 is that we can ignore
> 	the conn entry if the PE module is not loaded before this step

I think that is better to drop the entry as (at least the way PE SIP works)
its entirely likely that it could be used by the scheduler to use
the wont real server if the pe_data is missing. Or in other words,
it is no better than the entry being missing all together.

If later we have PE engines that behave differently, and creating
templates without pe_data makes sense, we can revisit this.

> - svc is added => PE module is loaded (request_module)
> 
> - next sync message comes and we bind cp->dest => if cp->pe
> is NULL we should set cp->pe to svc->pe. The problem here is
> that svc can be added without PE, then this template will not
> work as expected.

I'm unsure what you mean by "svc can be added without PE"

Re: [v2 PATCH 0/4] IPVS: Backup Adding Ipv6 and Persistence support

[Simon Horman]

On Mon, Nov 08, 2010 at 04:15:10PM +0100, Hans Schillstrom wrote:
> On Monday 08 November 2010 07:21:23 Simon Horman wrote:
> [ snip ]
> 
> > How about this, which applies on top of
> > "IPVS: Add persistence engine to connection entry".
> >
> > From: Simon Horman <horms@verge.net.au>
> > Subject: IPVS: Only match pe_data created by the same pe
> >
> > Only match persistence engine data if it was
> > created by the same persistence engine.
> >
> > Reported-by: Julian Anastasov <ja@ssi.bg>
> > Signed-off-by: Simon Horman <horms@verge.net.au>
> 
> Thanks I'll will use that one,
> I think it's time to cook a new backup patch,
> or do any one have more patches in the pipe ?

I have added this and the other patches that I am completely happy with
to the staging branch of lvs-test-2.6.

Specifically the patches in there are:

* IPVS: Only match pe_data created by the same pe
* IPVS: Add persistence engine to connection entry
* IPVS: Backup, Adding structs for new sync format
* IPVS: Prepare for transferring firewall marks (fwmark) to the backup daemon.
* IPVS: ip_vs_pe.c, use strncmp to be safe.

If you are unhappy with any of those changes let me know,
I'm happy to revert and rebase the staging branch
(its mainly for your benefit at this stage).

I would also like to add the following clean-up changes,
could I get an Ack or Nack from you on each of them?

* IPVS: Make the cp argument to ip_vs_sync_conn() static
  http://www.spinics.net/lists/lvs-devel/msg01375.html
* IPVS: Remove useless { } block from ip_vs_process_message()
  http://www.spinics.net/lists/lvs-devel/msg01369.html
* IPVS: buffer argument to ip_vs_process_message() should not be const
  http://www.spinics.net/lists/lvs-devel/msg01377.html

Re: [v2 PATCH 0/4] IPVS: Backup Adding Ipv6 and Persistence support

[Hans Schillstrom]

On Monday, November 08, 2010 23:00:29 Simon Horman wrote:
> On Mon, Nov 08, 2010 at 04:15:10PM +0100, Hans Schillstrom wrote:
> > On Monday 08 November 2010 07:21:23 Simon Horman wrote:
> > [ snip ]
> > 
> > > How about this, which applies on top of
> > > "IPVS: Add persistence engine to connection entry".
> > >
> > > From: Simon Horman <horms@verge.net.au>
> > > Subject: IPVS: Only match pe_data created by the same pe
> > >
> > > Only match persistence engine data if it was
> > > created by the same persistence engine.
> > >
> > > Reported-by: Julian Anastasov <ja@ssi.bg>
> > > Signed-off-by: Simon Horman <horms@verge.net.au>
> > 
> > Thanks I'll will use that one,
> > I think it's time to cook a new backup patch,
> > or do any one have more patches in the pipe ?
> 
> I have added this and the other patches that I am completely happy with
> to the staging branch of lvs-test-2.6.
> 
> Specifically the patches in there are:
> 
> * IPVS: Only match pe_data created by the same pe
> * IPVS: Add persistence engine to connection entry
> * IPVS: Backup, Adding structs for new sync format
> * IPVS: Prepare for transferring firewall marks (fwmark) to the backup daemon.
> * IPVS: ip_vs_pe.c, use strncmp to be safe.
> 
> If you are unhappy with any of those changes let me know,
> I'm happy to revert and rebase the staging branch
> (its mainly for your benefit at this stage).

Thanks a lot,
I will start with the cooking tomorrow, with lvs-test-2.6 as my base

> 
> I would also like to add the following clean-up changes,
> could I get an Ack or Nack from you on each of them?
> 
> * IPVS: Make the cp argument to ip_vs_sync_conn() static
>   http://www.spinics.net/lists/lvs-devel/msg01375.html
ACK,
I'll do the same in the last patch for _v0/_v1 sending

> * IPVS: Remove useless { } block from ip_vs_process_message()
>   http://www.spinics.net/lists/lvs-devel/msg01369.html
ACK,

> * IPVS: buffer argument to ip_vs_process_message() should not be const
>   http://www.spinics.net/lists/lvs-devel/msg01377.html
> 
ACK

Thanks,
 Hans

Re: [v2 PATCH 0/4] IPVS: Backup Adding Ipv6 and Persistence support

[Julian Anastasov]

 	Hello,

On Tue, 9 Nov 2010, Simon Horman wrote:

>> - backup creates template but there is no PE => cp->pe remains NULL if
>> 	we want to keep conn entry. Option 2 is that we can ignore
>> 	the conn entry if the PE module is not loaded before this step
>
> I think that is better to drop the entry as (at least the way PE SIP works)
> its entirely likely that it could be used by the scheduler to use
> the wont real server if the pe_data is missing. Or in other words,
> it is no better than the entry being missing all together.

 	Agreed.

> If later we have PE engines that behave differently, and creating
> templates without pe_data makes sense, we can revisit this.
>
>> - svc is added => PE module is loaded (request_module)
>>
>> - next sync message comes and we bind cp->dest => if cp->pe
>> is NULL we should set cp->pe to svc->pe. The problem here is
>> that svc can be added without PE, then this template will not
>> work as expected.
>
> I'm unsure what you mean by "svc can be added without PE"

 	-A without --pe [engine] option in backup.

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [v2 PATCH 0/4] IPVS: Backup Adding Ipv6 and Persistence support

[Julian Anastasov]

 	Hello,

On Tue, 9 Nov 2010, Simon Horman wrote:

> I have added this and the other patches that I am completely happy with
> to the staging branch of lvs-test-2.6.
>
> Specifically the patches in there are:
>
> * IPVS: Only match pe_data created by the same pe
> * IPVS: Add persistence engine to connection entry
> * IPVS: Backup, Adding structs for new sync format
> * IPVS: Prepare for transferring firewall marks (fwmark) to the backup daemon.
> * IPVS: ip_vs_pe.c, use strncmp to be safe.

 	Using if (strncmp(pe_name, pe->name, IP_VS_PENAME_MAXLEN )==0
does not look useful. If the goal is to match data from
sync message I expect to see the already discussed check
for pe_name_len, for example:

if (!strncmp(pe_name, pe->name, pe_name_len) &&
     !pe->name[pe_name_len])

 	and somewhere check for pe_name_len > 0.

> If you are unhappy with any of those changes let me know,
> I'm happy to revert and rebase the staging branch
> (its mainly for your benefit at this stage).
>
> I would also like to add the following clean-up changes,
> could I get an Ack or Nack from you on each of them?

 	I don't see problem here:

> * IPVS: Make the cp argument to ip_vs_sync_conn() static
>  http://www.spinics.net/lists/lvs-devel/msg01375.html
> * IPVS: Remove useless { } block from ip_vs_process_message()
>  http://www.spinics.net/lists/lvs-devel/msg01369.html
> * IPVS: buffer argument to ip_vs_process_message() should not be const
>  http://www.spinics.net/lists/lvs-devel/msg01377.html

Regards

--
Julian Anastasov <ja@ssi.bg>

Re: [v2 PATCH 0/4] IPVS: Backup Adding Ipv6 and Persistence support

[Simon Horman]

On Mon, Nov 08, 2010 at 11:23:25PM +0100, Hans Schillstrom wrote:
> 
> On Monday, November 08, 2010 23:00:29 Simon Horman wrote:
> > On Mon, Nov 08, 2010 at 04:15:10PM +0100, Hans Schillstrom wrote:
> > > On Monday 08 November 2010 07:21:23 Simon Horman wrote:
> > > [ snip ]
> > > 
> > > > How about this, which applies on top of
> > > > "IPVS: Add persistence engine to connection entry".
> > > >
> > > > From: Simon Horman <horms@verge.net.au>
> > > > Subject: IPVS: Only match pe_data created by the same pe
> > > >
> > > > Only match persistence engine data if it was
> > > > created by the same persistence engine.
> > > >
> > > > Reported-by: Julian Anastasov <ja@ssi.bg>
> > > > Signed-off-by: Simon Horman <horms@verge.net.au>
> > > 
> > > Thanks I'll will use that one,
> > > I think it's time to cook a new backup patch,
> > > or do any one have more patches in the pipe ?
> > 
> > I have added this and the other patches that I am completely happy with
> > to the staging branch of lvs-test-2.6.
> > 
> > Specifically the patches in there are:
> > 
> > * IPVS: Only match pe_data created by the same pe
> > * IPVS: Add persistence engine to connection entry
> > * IPVS: Backup, Adding structs for new sync format
> > * IPVS: Prepare for transferring firewall marks (fwmark) to the backup daemon.
> > * IPVS: ip_vs_pe.c, use strncmp to be safe.
> > 
> > If you are unhappy with any of those changes let me know,
> > I'm happy to revert and rebase the staging branch
> > (its mainly for your benefit at this stage).
> 
> Thanks a lot,
> I will start with the cooking tomorrow, with lvs-test-2.6 as my base

Be sure to use the staging branch :-)

> > I would also like to add the following clean-up changes,
> > could I get an Ack or Nack from you on each of them?
> > 
> > * IPVS: Make the cp argument to ip_vs_sync_conn() static
> >   http://www.spinics.net/lists/lvs-devel/msg01375.html
> ACK,
> I'll do the same in the last patch for _v0/_v1 sending
> 
> > * IPVS: Remove useless { } block from ip_vs_process_message()
> >   http://www.spinics.net/lists/lvs-devel/msg01369.html
> ACK,
> 
> > * IPVS: buffer argument to ip_vs_process_message() should not be const
> >   http://www.spinics.net/lists/lvs-devel/msg01377.html
> > 
> ACK

Thanks.

Re: [v2 PATCH 0/4] IPVS: Backup Adding Ipv6 and Persistence support

[Simon Horman]

On Tue, Nov 09, 2010 at 01:01:46AM +0200, Julian Anastasov wrote:
> 
> 	Hello,
> 
> On Tue, 9 Nov 2010, Simon Horman wrote:
> 
> >>- backup creates template but there is no PE => cp->pe remains NULL if
> >>	we want to keep conn entry. Option 2 is that we can ignore
> >>	the conn entry if the PE module is not loaded before this step
> >
> >I think that is better to drop the entry as (at least the way PE SIP works)
> >its entirely likely that it could be used by the scheduler to use
> >the wont real server if the pe_data is missing. Or in other words,
> >it is no better than the entry being missing all together.
> 
> 	Agreed.
> 
> >If later we have PE engines that behave differently, and creating
> >templates without pe_data makes sense, we can revisit this.
> >
> >>- svc is added => PE module is loaded (request_module)
> >>
> >>- next sync message comes and we bind cp->dest => if cp->pe
> >>is NULL we should set cp->pe to svc->pe. The problem here is
> >>that svc can be added without PE, then this template will not
> >>work as expected.
> >
> >I'm unsure what you mean by "svc can be added without PE"
> 
> 	-A without --pe [engine] option in backup.

I think that is ok because of the change made
by "IPVS: Only match pe_data created by the same pe".
That is, the template will never match.

From 12933de34a16c6585bd2a388ac0d48ef5c5599fa Mon Sep 17 00:00:00 2001
From: Simon Horman <horms@verge.net.au>
Date: Mon, 8 Nov 2010 20:06:30 +0900
Subject: [PATCH] IPVS: Only match pe_data created by the same pe

Only match persistence engine data if it was
created by the same persistence engine.

Reported-by: Julian Anastasov <ja@ssi.bg>
Signed-off-by: Simon Horman <horms@verge.net.au>
---
 net/netfilter/ipvs/ip_vs_conn.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/net/netfilter/ipvs/ip_vs_conn.c b/net/netfilter/ipvs/ip_vs_conn.c
index 44df5f0..0e0604c 100644
--- a/net/netfilter/ipvs/ip_vs_conn.c
+++ b/net/netfilter/ipvs/ip_vs_conn.c
@@ -354,7 +354,7 @@ struct ip_vs_conn *ip_vs_ct_in_get(const struct ip_vs_conn_param *p)
 
 	list_for_each_entry(cp, &ip_vs_conn_tab[hash], c_list) {
 		if (p->pe_data && p->pe->ct_match) {
-			if (p->pe->ct_match(p, cp))
+			if (p->pe == cp->pe && p->pe->ct_match(p, cp))
 				goto out;
 			continue;
 		}
-- 
1.7.1

Re: [v2 PATCH 0/4] IPVS: Backup Adding Ipv6 and Persistence support

[Simon Horman]

On Tue, Nov 09, 2010 at 01:19:54AM +0200, Julian Anastasov wrote:
> 
> 	Hello,
> 
> On Tue, 9 Nov 2010, Simon Horman wrote:
> 
> >I have added this and the other patches that I am completely happy with
> >to the staging branch of lvs-test-2.6.
> >
> >Specifically the patches in there are:
> >
> >* IPVS: Only match pe_data created by the same pe
> >* IPVS: Add persistence engine to connection entry
> >* IPVS: Backup, Adding structs for new sync format
> >* IPVS: Prepare for transferring firewall marks (fwmark) to the backup daemon.
> >* IPVS: ip_vs_pe.c, use strncmp to be safe.
> 
> 	Using if (strncmp(pe_name, pe->name, IP_VS_PENAME_MAXLEN )==0
> does not look useful. If the goal is to match data from
> sync message I expect to see the already discussed check
> for pe_name_len, for example:
> 
> if (!strncmp(pe_name, pe->name, pe_name_len) &&
>     !pe->name[pe_name_len])
> 
> 	and somewhere check for pe_name_len > 0.

To be honest, I am a bit dubious about the need for strncmp() at all.
I think it all depends on how/if pe_name can be trusted.
And I think that will depend on how Hans codes up the synchronisation code.