[CRIU] [PATCH v1 05/12] gc: implement unlocking of tcp connections

[Tycho Andersen]

On Thu, Aug 04, 2016 at 06:42:06PM +0300, Eugene Batalov wrote:
> 2016-08-04 18:08 GMT+03:00 Tycho Andersen <tycho.andersen at canonical.com>:
> 
> > Hi Eugene,
> >
> > On Thu, Aug 04, 2016 at 02:49:23PM +0300, Eugene Batalov wrote:
> > > Hi Tycho,
> > >
> > > > > +int gc_network_unlock(void)
> > > > > +{
> > > > > +     /*
> > > > > +      * Unshared ps tree net ns is destroyed after successful dump.
> > > > > +      * No need to call network_unlock_internal.
> > > > > +      * Also don't call ACT_NET_UNLOCK script because we don't
> > > > > +      * resume/restore ps tree - this call would break
> > > > > +      * ACT_NET_UNLOCK semantics.
> > > > > +      */
> > > > > +     return rst_unlock_tcp_connections();
> > > >
> > > > What about cpt_unlock_tcp_connections()? IIUC this list is not
> > > > persisted, and so if we leave around the network lock stuff, we would
> > > > never turn of TCP repair mode.
> > > >
> > > > Tycho
> > > >
> > > Let's consider the moment when we start criu gc or criu restore. At
> > > this moment dumpee ps tree doesn't exist and ps tree sockets don't
> > > exist.
> >
> > Hmm. Perhaps I misunderstood then. I thought the point was to be able
> > to use --leave-stopped (so the ps tree would still exist), and then we
> > could at a later time run `criu gc` to unlock things again and clean
> > this up.
> >
> Looks like this is the use case you need to support. Let's look at current
> implementation. Does it satisfies your needs?

I worked around my issue in another way, so I think we can ignore my
needs for the purpose of `criu gc`. If we wanted to clean up after a
dump that left the network locked, I don't think this set would do it
completely, but it sounds like we may not care about that.

> We can kill whole ps tree with any standard signal based logic so it looks
> like there is no problem here.
> But is it possible for you to have dump of this killed ps tree. criu gc
> needs this dump in order to do its job.
> Also criu gc requires only subset of the dump (memory is not needed).
> 
> For now I prefer current criu gc implementation that works with dumps
> rather then with live ps trees.
> In general If we want to c/r we always create dump from live ps tree but we
> don't always restore ps tree from dump.
> So dump is more basic thing that criu gc relies on. The only question is
> about availability of such a dump for gc.

Ok, but I still don't understand, what's the use case for something this?

> >
> > > That's why we don't need to turn any tcp repair mode off - it is per
> > socket
> > > thing.
> > > Also cpt_unlock_tcp_connections called from network_unlock() that is
> > called
> > > during criu restore
> > > does nothing because cpt_tcp_repair_sockets list is empty. So for me it
> > > looks like criu gc works
> > > the same as criu restore but without calling of network unlock action
> > > script.
> >
> > I understand; it may be worth noting somewhere in the help output or
> > patch notes what exactly the use case is. Right now I don't undersand
> > what it is :)
> >
>  Yep. I'll add more info into criu help message in the next patch set
> version. Thank you for suggestion.

OK :)

Tycho

> >
> > Tycho
> >
> > > >
> > > > > +}
> > > > > +
> > > > >  int veth_pair_add(char *in, char *out)
> > > > >  {
> > > > >       char *aux;
> > > > > diff --git a/criu/sk-tcp.c b/criu/sk-tcp.c
> > > > > index 13d175a..c4aa58f 100644
> > > > > --- a/criu/sk-tcp.c
> > > > > +++ b/criu/sk-tcp.c
> > > > > @@ -713,16 +713,19 @@ void tcp_locked_conn_add(struct inet_sk_info
> > *ii)
> > > > >       ii->sk_fd = -1;
> > > > >  }
> > > > >
> > > > > -void rst_unlock_tcp_connections(void)
> > > > > +int rst_unlock_tcp_connections(void)
> > > > >  {
> > > > >       struct inet_sk_info *ii;
> > > > >
> > > > >       /* Network will be unlocked by network-unlock scripts */
> > > > >       if (root_ns_mask & CLONE_NEWNET)
> > > > > -             return;
> > > > > +             return 0;
> > > > >
> > > > >       list_for_each_entry(ii, &rst_tcp_repair_sockets, rlist)
> > > > > -             nf_unlock_connection_info(ii);
> > > > > +             if (nf_unlock_connection_info(ii))
> > > > > +                     return -1;
> > > > > +
> > > > > +     return 0;
> > > > >  }
> > > > >
> > > > >  int check_tcp(void)
> > > > > --
> > > > > 1.9.1
> > > > >
> > > > > _______________________________________________
> > > > > CRIU mailing list
> > > > > CRIU at openvz.org
> > > > > https://lists.openvz.org/mailman/listinfo/criu
> > > >
> > >
> > >
> > >
> > > --
> > > Best regards,
> > > Eugene Batalov.
> >
> 
> 
> 
> -- 
> Best regards,
> Eugene Batalov.