Re: [RFC][PATCH 00/11] track files for checkpointability

[Dave Hansen <dave@linux.vnet.ibm.com>]

On Fri, 2009-03-06 at 00:08 +0300, Alexey Dobriyan wrote:
> On Thu, Mar 05, 2009 at 11:16:07AM -0800, Dave Hansen wrote:
> > On Thu, 2009-03-05 at 20:40 +0300, Alexey Dobriyan wrote:
> > > * without recalculating "checkpointable" property on fs_struct
> > >   on every C/R=y kernel.
> > 
> > Yeah, this is certainly less than ideal.  Although, I haven't seen your
> > proposal for where to tie your code into the kernel.  Do you suggest
> > that we do nothing during normal kernel runtime and all the checking at
> > sys_checkpoint() time?
> 
> Of course!
> 
> C/R won't be used by majority of users, so it shouldn't bring any
> overhead. ->f_op->checkpoint (not ->checkpointable!) is probably
> acceptable. Recalculating flags is not, sorry.

Yeah, what I'm doing in dup_fd() is certainly suboptimal.  It introduces
extra overhead in fork() (with the config option turned on) which sucks
big time.  But, I'm *sure* we can optimize it, especially if we can push
it out to only occurring at "container fork()" time.  Whatever container
fork ends up being.  

> Imagine, unsupported file is opened between userspace checks
> for /proc/*/checkpointable and /proc/*/fdinfo/*/checkpointable
> and whatever, you stil have to do all the checks inside checkpoint(2).

Alexey, we have two problems here.  I completely agree that we have to
do complete and thorough checks of each file descriptor at
sys_checkpoint().  Any checks made at other times should not be trusted.

The other side is what Ingo has been asking for.  How do we *know* when
we are checkpointable *before* we call (and without calling)
sys_checkpoint()?  You are yet to acknowledge that this is a valid use
case, but it is exactly what Ingo is asking for, I believe.

If nice printk()s are sufficient to cover what Ingo wants, I'm quite
happy to remove the /proc files.  

> > > It may lack some printk, but printks are trivial to insert including
> > > using d_path for precise info.
> > 
> > This is definitely workable approach.  However, could you show how you
> > would support /dev/null and, say, /proc/$$/stat?  I've shown what it
> > takes to do that in my patches, and I think it would show a lot about
> > your approach.  
> 
> I haven't yet written code for /dev/null, but it would be:
> * at checkpoint(2)
> 	** see it's block device
> 	** see it's 1:3 => supported
> 	** dump "1:3", dump "/dev/null" as filename

Can we see code, please?  With my approach, it is a single line added to
a structure definition.  Your approach sounds like it may be more than a
single line of code.  It sounds like you would like to have some kind of
device number to c/r mapping.  I'm curious what form that would take.

> * at restore(2)
> 	** read CR_OBJ_FILE
> 	** open filename or -E
> 	** if not block device return -E
> 	** if not 1:3 return -E
> 	** save "struct file *" where needed
> 
> (all of this is modulo unlinked case)

/dev/null is a character device, btw. :)

This sanity checking on the sys_restore() side is also definitely a good
idea.  But, in the interests of keeping our patch size down, I think it
is safe to say that we require userspace to get the fs back into a state
consistent with sys_checkpoint() time.  

-- Dave