[Devel] Re: [PATCH 1/1] cr: define CHECKPOINT_SUBTREE flag and sysctl

[Serge E. Hallyn]

Quoting Matt Helsley (matthltc at us.ibm.com):
> On Fri, Apr 24, 2009 at 04:06:08PM -0500, Serge E. Hallyn wrote:

Thanks for taking a look, Matt.  Oren has done some nice work to it
in his ckpt-v14, please take a look there.

> > +	cnt = ref->users + 1;
> 
> Perhaps this switch is another candidate for an fops-style function pointer.

Yup, Oren did that :)

> > +static int cr_check_leaks(struct cr_ctx *ctx)
> 
> What are "leaks" ? ;)
> 
> How about cr_find_ref_leaks ? I suggest "find" because "check" confuses with
> "checkpoint". "ref" because we're not looking for memory leaks.

Oren renamed it to ckpt_obj_contained()

...

> >  	mm = get_task_mm(t);
> > 
> 
> Might add a check for exe_file == NULL here too (see below).

Hmm, I think it's ok to store a NULL pointer in the objhash...

...

> > +	if (!exe_file) {
> > +		/* really it can't be the case that it NOT be NULL... */
> 
> This comment isn't correct. Yes, *most* of the time exe_file should be
> non-NULL. 

You misread the comment, though you're right about the code.

The comment says it can't be the case that it *not* be NULL here.
That's because the object can't be in the objhash yet, because it always
comes after the main mm entry in the layout.

> Some tools avoid pinning the filesystem with a reference to its executable file
> by copying the executable into private, anonymous, executable pages,
> and then unmaping the originals. This drops the last VMA reference to the
> file which causes the exe_file reference to be dropped as well.
> 
> It may provide an interesting testcase for checkpoint/restart since it
> would mean the executable couldn't be mapped from a checkpointed file --
> we'd have to rely solely on VMA reconstruction for these.

Yeah, taking another look I don't handle the restart case where exe_file
is NULL.  (It won't harm the kernel, just return an error from
checkpoint I believe)

How does a userspace tool do that though?  Once the file has been
exec()d, userspace doesn't have an open fd to the executable anymore...
Can you explain how it's done, and/or send me a testcase?

> >  /* cr_ctx: flags */
> > -#define CR_CTX_CKPT	0x1
> > -#define CR_CTX_RSTR	0x2
> > +#define CR_CTX_CKPT		0x1
> > +#define CR_CTX_RSTR		0x2
> > +#define CHECKPOINT_SUBTREE	0x4
> 
> The whitespace change somewhat obscures the introduction of the flag here.

True.  Guess that could've been a separate tiny patch.

thanks,
-serge
_______________________________________________
Containers mailing list
Containers at lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers