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@xxxxxx>
> > >
> > > > From: Simon Horman <horms@xxxxxxxxxxxx>
> > > > 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@xxxxxx>
> > > > Signed-off-by: Simon Horman <horms@xxxxxxxxxxxx>
> > > >
> > > > 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@xxxxxxxxxxxxxxx
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> >
>
> --
> 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
