|
Hello,
On Wed, 30 Jul 2014, Alex Gartrell wrote:
> Thanks for your review!
>
> I've made most of the changes that you've asked for, just a couple
> quick questions before I submit v2.
>
>
> > This feature looks good to me.
> >
> > Such change should go first in net-next tree, after the change
> > for skb->encapsulation (net tree). So, probably, after some -rc
> > we can add this patchset.
> >
>
> SGTM
>
> > So, to summarize our plan:
> >
> > - we have sockopt and netlink interface
> >
> > - IPv6 is supported only on netlink interface
> >
> > - the sockopt structs can not be changed, eg.
> > struct ip_vs_dest_user in include/uapi/linux/ip_vs.h.
> >
> > - ipvsadm should allow different family (svc and dest) only for TUN
> > method
> >
> > - we can also encode somehow the tunnel mode in sync message, eg.
> > around the STYPE_F_INET6 value. May be this octet can become
> > tunnel/header type. This can be done later in another patchset.
> >
> > Comments for your patchset:
> >
> > Patch 1:
> > - please fix the dest->af = udest.af in __ip_vs_update_dest(),
> > so that this patch can compile and as result this fix will
> > disappear from patch 2:
> > - dest->af = udest.af;
> > + dest->af = udest->af;
>
> Ah, embarrassing. I thought I checked that this all compiled. Will do.
>
> > - __ip_vs_get_dest_entries() needs to skip dests with
> > dest->af != svc->af, we can not exports such dests to
> > sockopt users
>
> Yeah seems reasonable, I'll include a short comment as well
>
> > - too long comments, use scripts/checkpatch.pl
> > --strict /tmp/p1.diff
>
> Another amateur move. Thanks for the epointer
>
> > - everything else looks ok
> >
> > Patch 2:
> > - avoid error from patch 1 in __ip_vs_update_dest
> >
> > - errors from scripts/checkpatch.pl --strict /tmp/p2.diff
> >
> > Patch 3:
> > - in the "Destination %u/%s:%u still in trash, " message
> > we should use dest->af, not dest_af.
> >
> > Patch 4:
> > - we have to introduce u16 cp->daf, just below cp->protocol
>
> yeah makes sense
>
> > - as result, cp->af is for caddr and vaddr, cp->daf is for
> > cp->daddr, this is needed because sync protocol can create connections
> > with cp->dest = NULL, cp binds to dest later
>
> Agree that we should just create daf, but if we keep heterogeneous pools
> and sync distinct can we still run into this? (Just curious)
I think, in patch 7 ip_vs_bind_xmit and ip_vs_bind_xmit_v6
will simply crash when sync is used, without heterogeneous pools,
due to cp->dest dereference. So, it happens for synced conns.
> > - ip_vs_ftp.c uses ip_vs_conn_new, now it does not compile
>
> It appears that these are all IPv4 only, so I'll just use AF_INET and add a
> comment.
Yes, ip_vs_ftp still supports v4 only.
> > - Another file net/netfilter/xt_ipvs.c depends on IPVS
> > definitions but I think this patchset does not break it
> >
>
> Yeah, this still builds
>
> > Patch 5:
> > - note that dest address can not be changed, so if you see
> > reason this patch to exist the check in __ip_vs_update_dest
> > should be: if (add && udest->af != svc->af)
> >
>
> Well that definitely simplifies things. My plan is to add a
> BUG_ON(!add && udest->af != dest->af) -- more as documentation than
> anything.
The af,addr,port are the key we use to search dest,
it is silly to duplicate them as parameters just to support
such dest change, I think it can not happen.
> > - errors from scripts/checkpatch.pl --strict /tmp/p5.diff
> >
> > Patch 6:
> > - comment and code do not match
>
> What do you mean? This just removes a temporary "return -EINVAL" to keep
> the code sane.
This patch (6) changes only ip_vs_new_dest() but talks
about "sync" and "switch statement" ...
> > Patch 7:
> > - patch looks based on skb_checksum_help patch, the plan
> > is to base it after the offload change we are about to
> > send soon
>
> Yeah, nothing significant about that, I was planning to rebase it the patch
> when it lands.
I'll wait for comments about GSO for 1-2 days and will send
official patch.
> > - ip_vs_bind_xmit*: sync protocol does not guarantee that
> > cp->dest exists in all cases, we can not use cp->dest->af
> > because cp->dest can be NULL, we have to use cp->daf (from
> > patch 4)
>
> Fixed this
>
> >
> > - for ip_vs_prepare_tunneled_skb():
> > - usage of 'version' can be changed to 'af'
>
> I'm a little bit confused by this. In this case, we aren't passing in af.
> Since the caller doesn't really know (or care) what the type of the packet
> is it made sense to figure it out from the ipheader version. Do you mean
> that there's another function I should use to grab the address family from
> the skb or is it possible to get it from somewhere else?
We can use cp->af, cp->dest can be NULL (synced conn)
> > - __ip_vs_get_out_rt_v6 and __ip_vs_get_out_rt should
> > be changed to account header from both families for MTU check,
> > these checks happen for IP_VS_RT_MODE_TUNNEL
> >
>
> Yes this looks fairly significant. Unless you'll object I'll introduce
> this as a separate patch.
But in the same patchset, right?
> > - errors from scripts/checkpatch.pl --strict /tmp/p7.diff
> >
> > - looking at cp, set_tcp_state() can not know what family is used for
> > cp->daddr, we have to use cp->daf. We have to check the
> > cp->af usage everywhere.
> >
> > - I didn't fixed ip_vs_core.c logs yet
> >
> > I'm attaching some patches for your patchset. If it is
> > difficult for you to maintain them in the patchset, we can post
> > them later after your patchset, as separate patchset. For now you
> > can use them as reference. Or may be they should be inserted
> > at some point in your patchset, so that your patchset looks
> > correct if applied up to some patch.
>
> I'll add them to my patch set. Thank you for doing them!
Ok, thanks!
> > Let me know if you plan ipvsadm changes based on the
> > ipvsadm tree:
> >
>
> I have made the appropriate changes for this. It's a relatively
> straightforward patch. I can submit it at any time.
ok
Regards
--
Julian Anastasov <ja@xxxxxx>
--
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
|
