Re: [PATCH RFC] ipvs: add genetlink cmd to dump all services and destinations

[Cezar Sá Espinola]

Hi,

> > +     if (ctx->idx_svc == ctx->start_svc && ctx->last_svc != svc)
> > +             return 0;
> > +
> > +     if (ctx->idx_svc > ctx->start_svc) {
> > +             if (ip_vs_genl_dump_service(skb, svc, cb) < 0) {
> > +                     ctx->idx_svc--;
> > +                     return -EMSGSIZE;
> > +             }
> > +             ctx->last_svc = svc;
> > +             ctx->start_dest = 0;
> > +     }
> > +
> > +     ctx->idx_dest = 0;
> > +     list_for_each_entry(dest, &svc->destinations, n_list) {
> > +             if (++ctx->idx_dest <= ctx->start_dest)
> > +                     continue;
> > +             if (ip_vs_genl_dump_dest(skb, dest, cb) < 0) {
> > +                     ctx->idx_dest--;
>
>         At this point idx_svc is incremented and we
> stop at the middle of dest list, so we need ctx->idx_svc-- too.
>
>         And now what happens if all dests can not fit in a packet?
> We should start next packet with the same svc? And then
> user space should merge the dests when multiple packets
> start with same service?

My (maybe not so great) idea was to avoid repeating the svc on each
packet. It's possible for a packet to start with a destination and
user space must consider then as belonging to the last svc received on
the previous packet. The comparison "ctx->last_svc != svc" was
intended to ensure that a packet only starts with destinations if the
current service is the same as the svc we sent on the previous packet.

>
>         The main points are:
>
> - the virtual services are in hash table, their order is
> not important, user space can sort them
>
> - order of dests in a service is important for the schedulers
>
> - every packet should contain info for svc, so that we can
> properly add dests to the right svc

Thanks, I will rework the patch with these points in mind. It does
sound safer to ensure every packet starts with service information.

> > +nla_put_failure:
> > +     mutex_unlock(&__ip_vs_mutex);
> > +     cb->args[0] = ctx.idx_svc;
> > +     cb->args[1] = ctx.idx_dest;
> > +     cb->args[2] = (long)ctx.last_svc;
>
>         last_svc is used out of __ip_vs_mutex region,
> so it is not safe. We can get a reference count but this
> is bad if user space blocks.

I thought it would be relatively safe to store a pointer to the last
svc since I would only use it for pointer comparison and never
dereferencing it. But in retrospect it does look unsafe and fragile
and could probably lead to errors especially if services are modified
during a dump causing the stored pointer to point to a different
service.

>         But even if we use just indexes it should be ok.
> If multiple agents are used in parallel it is not our
> problem. What can happen is that we can send duplicates
> or to skip entries (both svcs and dests). It is impossible
> to keep any kind of references to current entries or even
> keys to lookup them if another agent can remove them.

Got it. I noticed this behavior while writing this patch and even
created a few crude validation scripts running parallel agents and
checking the diff in [1].

[1] - https://github.com/cezarsa/ipvsadm-validate/blob/37ebd39785b1e835c6d4b5c58aaca7be60d5e194/test.sh#L86-L87

Thanks a lot for the review,
--
Cezar Sá Espinola