|
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@xxxxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe lvs-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
|
