Quoting Oren Laadan (orenl@xxxxxxxxxxxxxxx): > > > 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@xxxxxxxxxx> > > 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@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linux-foundation.org/mailman/listinfo/containers