[PATCH RFC linux-cr] add rpid to ckpt_pids struct

[PATCH RFC linux-cr] add rpid to ckpt_pids struct

[Serge E. Hallyn]

When we restart an application, we won't try to restore the original
pid in the parent pid_ns, so we don't checkpoint that pid.

However, if we are going to dump mount info from userspace using
/proc/pid/mountinfo, then it will be easiest for a restart wrapper
to use the pids from the checkpoint image.  So add rpid to the
pids info we dump.

The rpid is taken as the vpid in the checkpointer's pidnamespace.
The container may not have valid pids in the checkpointer's
pid namespace, in which case the rpid will be 0, and the checkpoint
wrapper will not be able to access the mountinfo.

Note that eventually, in order to restore nested pid namespaces,
we will need to change our approach again, so as to print out
the pid in every ancestor pid_ns.

Signed-off-by: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 checkpoint/checkpoint.c        |    6 ++++--
 include/linux/checkpoint_hdr.h |    1 +
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/checkpoint/checkpoint.c b/checkpoint/checkpoint.c
index b3c1c4f..c9b5966 100644
--- a/checkpoint/checkpoint.c
+++ b/checkpoint/checkpoint.c
@@ -329,13 +329,15 @@ static int checkpoint_pids(struct ckpt_ctx *ctx)
 		for (n = 0; n < min(nr_tasks, CKPT_HDR_PIDS_CHUNK); n++) {
 			task = tasks_arr[pos];
 
+			h[n].rpid = task_pid_vnr(task);
 			h[n].vpid = task_pid_nr_ns(task, ns);
 			h[n].vtgid = task_tgid_nr_ns(task, ns);
 			h[n].vpgid = task_pgrp_nr_ns(task, ns);
 			h[n].vsid = task_session_nr_ns(task, ns);
 			h[n].vppid = task_tgid_nr_ns(task->real_parent, ns);
-			ckpt_debug("task[%d]: vpid %d vtgid %d parent %d\n",
-				   pos, h[n].vpid, h[n].vtgid, h[n].vppid);
+			ckpt_debug("task[%d]: rpid %d vpid %d vtgid %d "
+				"parent %d\n", pos, h[n].rpid, h[n].vpid,
+				h[n].vtgid, h[n].vppid);
 			pos++;
 		}
 		rcu_read_unlock();
diff --git a/include/linux/checkpoint_hdr.h b/include/linux/checkpoint_hdr.h
index 41412d1..d3fff0b 100644
--- a/include/linux/checkpoint_hdr.h
+++ b/include/linux/checkpoint_hdr.h
@@ -327,6 +327,7 @@ struct ckpt_hdr_tree {
 } __attribute__((aligned(8)));
 
 struct ckpt_pids {
+	__s32 rpid; /* pid in checkpointer's pidns */
 	__s32 vpid;
 	__s32 vppid;
 	__s32 vtgid;
-- 
1.6.0.6

Re: [PATCH RFC linux-cr] add rpid to ckpt_pids struct

[Matt Helsley]

On Tue, Mar 09, 2010 at 03:04:51PM -0600, Serge E. Hallyn wrote:
> When we restart an application, we won't try to restore the original
> pid in the parent pid_ns, so we don't checkpoint that pid.
> 
> However, if we are going to dump mount info from userspace using
> /proc/pid/mountinfo, then it will be easiest for a restart wrapper
> to use the pids from the checkpoint image.  So add rpid to the
> pids info we dump.
> 
> The rpid is taken as the vpid in the checkpointer's pidnamespace.
> The container may not have valid pids in the checkpointer's
> pid namespace, in which case the rpid will be 0, and the checkpoint
> wrapper will not be able to access the mountinfo.

This seems like a good idea to me.

Acked-by: Matt Helsley <matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

> 
> Note that eventually, in order to restore nested pid namespaces,
> we will need to change our approach again, so as to print out
> the pid in every ancestor pid_ns.

Perhaps we should think about what will be needed so we can avoid
extra checkpoint image format changes? Or perhaps there's something
we can do right now to ease any future transition at least?

> Signed-off-by: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> ---
>  checkpoint/checkpoint.c        |    6 ++++--
>  include/linux/checkpoint_hdr.h |    1 +
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/checkpoint/checkpoint.c b/checkpoint/checkpoint.c
> index b3c1c4f..c9b5966 100644
> --- a/checkpoint/checkpoint.c
> +++ b/checkpoint/checkpoint.c
> @@ -329,13 +329,15 @@ static int checkpoint_pids(struct ckpt_ctx *ctx)
>  		for (n = 0; n < min(nr_tasks, CKPT_HDR_PIDS_CHUNK); n++) {
>  			task = tasks_arr[pos];
> 
> +			h[n].rpid = task_pid_vnr(task);
>  			h[n].vpid = task_pid_nr_ns(task, ns);
>  			h[n].vtgid = task_tgid_nr_ns(task, ns);
>  			h[n].vpgid = task_pgrp_nr_ns(task, ns);
>  			h[n].vsid = task_session_nr_ns(task, ns);
>  			h[n].vppid = task_tgid_nr_ns(task->real_parent, ns);
> -			ckpt_debug("task[%d]: vpid %d vtgid %d parent %d\n",
> -				   pos, h[n].vpid, h[n].vtgid, h[n].vppid);
> +			ckpt_debug("task[%d]: rpid %d vpid %d vtgid %d "
> +				"parent %d\n", pos, h[n].rpid, h[n].vpid,
> +				h[n].vtgid, h[n].vppid);
>  			pos++;
>  		}
>  		rcu_read_unlock();
> diff --git a/include/linux/checkpoint_hdr.h b/include/linux/checkpoint_hdr.h
> index 41412d1..d3fff0b 100644
> --- a/include/linux/checkpoint_hdr.h
> +++ b/include/linux/checkpoint_hdr.h
> @@ -327,6 +327,7 @@ struct ckpt_hdr_tree {
>  } __attribute__((aligned(8)));
> 
>  struct ckpt_pids {
> +	__s32 rpid; /* pid in checkpointer's pidns */

e.g. Perhaps stick it at the end of the previously-defined pids.

>  	__s32 vpid;
>  	__s32 vppid;
>  	__s32 vtgid;

Cheers,
	-Matt Helsley

Re: [PATCH RFC linux-cr] add rpid to ckpt_pids struct

[Serge E. Hallyn]

Quoting Matt Helsley (matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
> On Tue, Mar 09, 2010 at 03:04:51PM -0600, Serge E. Hallyn wrote:
> > When we restart an application, we won't try to restore the original
> > pid in the parent pid_ns, so we don't checkpoint that pid.
> > 
> > However, if we are going to dump mount info from userspace using
> > /proc/pid/mountinfo, then it will be easiest for a restart wrapper
> > to use the pids from the checkpoint image.  So add rpid to the
> > pids info we dump.
> > 
> > The rpid is taken as the vpid in the checkpointer's pidnamespace.
> > The container may not have valid pids in the checkpointer's
> > pid namespace, in which case the rpid will be 0, and the checkpoint
> > wrapper will not be able to access the mountinfo.
> 
> This seems like a good idea to me.
> 
> Acked-by: Matt Helsley <matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> 
> > 
> > Note that eventually, in order to restore nested pid namespaces,
> > we will need to change our approach again, so as to print out
> > the pid in every ancestor pid_ns.
> 
> Perhaps we should think about what will be needed so we can avoid
> extra checkpoint image format changes? Or perhaps there's something
> we can do right now to ease any future transition at least?
> 
> > Signed-off-by: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> > ---
> >  checkpoint/checkpoint.c        |    6 ++++--
> >  include/linux/checkpoint_hdr.h |    1 +
> >  2 files changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/checkpoint/checkpoint.c b/checkpoint/checkpoint.c
> > index b3c1c4f..c9b5966 100644
> > --- a/checkpoint/checkpoint.c
> > +++ b/checkpoint/checkpoint.c
> > @@ -329,13 +329,15 @@ static int checkpoint_pids(struct ckpt_ctx *ctx)
> >  		for (n = 0; n < min(nr_tasks, CKPT_HDR_PIDS_CHUNK); n++) {
> >  			task = tasks_arr[pos];
> > 
> > +			h[n].rpid = task_pid_vnr(task);
> >  			h[n].vpid = task_pid_nr_ns(task, ns);
> >  			h[n].vtgid = task_tgid_nr_ns(task, ns);
> >  			h[n].vpgid = task_pgrp_nr_ns(task, ns);
> >  			h[n].vsid = task_session_nr_ns(task, ns);
> >  			h[n].vppid = task_tgid_nr_ns(task->real_parent, ns);
> > -			ckpt_debug("task[%d]: vpid %d vtgid %d parent %d\n",
> > -				   pos, h[n].vpid, h[n].vtgid, h[n].vppid);
> > +			ckpt_debug("task[%d]: rpid %d vpid %d vtgid %d "
> > +				"parent %d\n", pos, h[n].rpid, h[n].vpid,
> > +				h[n].vtgid, h[n].vppid);
> >  			pos++;
> >  		}
> >  		rcu_read_unlock();
> > diff --git a/include/linux/checkpoint_hdr.h b/include/linux/checkpoint_hdr.h
> > index 41412d1..d3fff0b 100644
> > --- a/include/linux/checkpoint_hdr.h
> > +++ b/include/linux/checkpoint_hdr.h
> > @@ -327,6 +327,7 @@ struct ckpt_hdr_tree {
> >  } __attribute__((aligned(8)));
> > 
> >  struct ckpt_pids {
> > +	__s32 rpid; /* pid in checkpointer's pidns */
> 
> e.g. Perhaps stick it at the end of the previously-defined pids.

It turns ckpt_pids into a variable-length field, so one way to go
about it is, after ckpt_hdr_tree, instead of one ckpt_hdr_buffer
with all ckpt_hdr_tree->nr_pids ckpt_pids in it, we have
ckpt_hdr_tree->nr_pids distinct ckpt_hdr_buffer's, each with one
variable-length size ckpt_pids?  The cpit_pids could then contain:

	__s32 parent_objref; /* objref of ckpt_hdr_buffer for parent pid */
	__s32 vtgid;
	__s32 vpid;
	/* from here on out we list the pids in ever-higher pid namespaces
	 * up to ctx->root_nsproxy->pid_ns one */
	__s32 vpid[];
	/* And finally, the pid_nr which is valid in checkpointer's
	 * pid_ns, or 0 if none exists */
	__s32 rpid;

Since we have the ckpt_hdr_buffer->ckpt_hdr->len we can deduce
the number of entries.

> >  	__s32 vpid;
> >  	__s32 vppid;
> >  	__s32 vtgid;
> 
> Cheers,
> 	-Matt Helsley