[Devel] Re: [PATCH RFC] Restore task fs_root and pwd

[Serge E. Hallyn]

Quoting Oren Laadan (orenl at cs.columbia.edu):
> 
> 
> Serge E. Hallyn wrote:
> > Checkpoint and restore task->fs.  Tasks sharing task->fs will
> > share them again after restart.
> > 
> > The fs/fs_struct.c part should of course be broken out, but
> > this does the right thing for me.
> > 
> > Signed-off-by: Serge E. Hallyn <serue at us.ibm.com>
> 
> Looks good. See a few comments below.
> 
> > ---
> >  checkpoint/files.c             |  211 ++++++++++++++++++++++++++++++++++++++++
> >  checkpoint/objhash.c           |    9 ++
> >  checkpoint/process.c           |   13 +++
> >  fs/open.c                      |   53 ++++++----
> >  include/linux/checkpoint.h     |   10 ++-
> >  include/linux/checkpoint_hdr.h |   10 ++
> >  include/linux/fs.h             |    4 +
> >  7 files changed, 287 insertions(+), 23 deletions(-)
> > 
> > diff --git a/checkpoint/files.c b/checkpoint/files.c
> > index b622588..c8e8d7f 100644
> > --- a/checkpoint/files.c
> > +++ b/checkpoint/files.c
> > @@ -24,6 +24,9 @@
> >  #include <linux/checkpoint_hdr.h>
> >  #include <linux/eventpoll.h>
> >  #include <linux/eventfd.h>
> > +#include <linux/fs.h>
> > +#include <linux/fs_struct.h>
> > +#include <linux/namei.h>
> >  #include <net/sock.h>
> >  
> >  
> > @@ -449,6 +452,71 @@ int ckpt_collect_file_table(struct ckpt_ctx *ctx, struct task_struct *t)
> >  	return ret;
> >  }
> >  
> > +int checkpoint_get_task_fs(struct ckpt_ctx *ctx, struct task_struct *t)
> > +{
> > +	struct fs_struct *fs;
> > +	int fs_objref;
> > +	int kill;
> > +
> > +	task_lock(current);
> > +	fs = t->fs;
> > +	write_lock(&fs->lock);
> > +	fs->users++;
> > +	write_unlock(&fs->lock);
> 
> 3 lines above are the same in obj_task_fs_grab()...
> 
> > +	task_unlock(current);
> > +
> > +	fs_objref = checkpoint_obj(ctx, fs, CKPT_OBJ_TASK_FS);
> > +	write_lock(&fs->lock);
> > +	kill = !--fs->users;
> > +	write_unlock(&fs->lock);
> > +	if (kill)
> > +		free_fs_struct(fs);
> 
> And last 5 lines are the same in obj_task_fs_drop().
> 
> Perhaps put as helpers in fs_struct.h ?

Absolutely.  Aside from waiting on confirmation on the basics, I
was also a little intimidated about adding more generic refcount
handling to fs_struct.h, since it appears to me to be a case where
generic handlers were not-added on purpose :)  But it's not right
to have this code here.

> > +
> > +	return fs_objref;
> > +}
> > +
> > +/*
> > + * called with fs read_lock()d
> > + */
> 
> Am I right to guess that this comment is stale ?

Sure enough - read_locking it will deadlock checkpoint :)  Should
be "called with fs refcount bumped".

> > +int checkpoint_obj_task_fs(struct ckpt_ctx *ctx, struct fs_struct *fs)
> > +{
> > +	struct ckpt_hdr_task_fs *h;
> > +	int ret;
> > +	struct fs_struct *fscopy;
> > +
> 
> ...
> 
> > +/* this is the fn called by objhash when it runs into a
> > + * CKPT_OBJ_TASK_FS entry.  Creates an fs_struct and
> > + * places it in the hash. */
> > +static struct fs_struct *restore_obj_task_fs(struct ckpt_ctx *ctx)
> > +{
> > +	struct ckpt_hdr_task_fs *h;
> > +	struct fs_struct *fs;
> > +	int ret = 0;
> > +	char *root, *cwd;
> > +	int len;
> > +
> > +	h = ckpt_read_obj_type(ctx, sizeof(*h), CKPT_HDR_TASK_FS);
> > +	if (IS_ERR(h))
> > +		return ERR_PTR(PTR_ERR(h));
> > +	ckpt_hdr_put(ctx, h);
> > +
> > +	fs = copy_fs_struct(current->fs);
> > +	if (!fs)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	len = ckpt_read_payload(ctx, (void **) &root,
> > +				PATH_MAX, CKPT_HDR_FILE_NAME);
> 
> Test for len < 0 ... ?
> 
> Since this is another place where we read file-name (also in
> checkpoint/files.c), perhaps introduce a ckpt_read_fname() ?

Ok.

> > +	ret = restore_chroot(ctx, fs, root);
> > +	kfree(root);
> > +	if (ret) {
> > +		free_fs_struct(fs);
> > +		return ERR_PTR(ret);
> > +	}
> > +
> > +	len = ckpt_read_payload(ctx, (void **) &cwd,
> > +				PATH_MAX, CKPT_HDR_FILE_NAME);
> > +	ret = restore_cwd(ctx, fs, cwd);
> > +	kfree(cwd);
> > +
> > +	if (ret) {
> > +		free_fs_struct(fs);
> > +		return ERR_PTR(ret);
> > +	}
> > +	return fs;
> > +}
> 
> ...
> 
> Oren.

Laptop is rebuilding at the moment (plus I'm out) so I may not
get this out until Monday.  If you wanted to apply this now I'll
send a patch to move the refcounting to the right place etc next
week, else I'll just send a whole new patch.

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