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

[Hans Schillstrom <hans.schillstrom@ericsson.com>]

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>