[CRIU] [PATCH 5/6] mnt: try to split a mount tree to restore over-mounted mounts

Andrei Vagin avagin at virtuozzo.com
Wed Sep 21 12:56:51 PDT 2016


On Wed, Sep 21, 2016 at 10:37:10AM +0300, Pavel Emelyanov wrote:
> On 09/13/2016 07:19 AM, Andrei Vagin wrote:
> > From: Andrei Vagin <avagin at virtuozzo.com>
> > 
> > If a mount overmounts something else, we can try to restore it
> > separetly and then move it to the right places after restoring
> > all mounts.
> 
> Hm... Why not simply tune can_mount_new() to refuse mounting the
> top-mounts until sub-mounts are?

because top-mounts can be from shared groups which are required to
restore sub-mounts.

> 
> > In this patch if we see that a mount is overmounts something,
> > we create a new directory in the root yard and restore this
> > mount and its sub-tree in this directory.
> > 
> > https://bugs.openvz.org/browse/OVZ-6778
> > Signed-off-by: Andrei Vagin <avagin at virtuozzo.com>
> > ---
> >  criu/include/mount.h |   1 +
> >  criu/mount.c         | 160 +++++++++++++++++++++++++++++++++++++++++++++++++--
> >  2 files changed, 157 insertions(+), 4 deletions(-)
> > 
> > diff --git a/criu/include/mount.h b/criu/include/mount.h
> > index a266cd0..d9b1521 100644
> > --- a/criu/include/mount.h
> > +++ b/criu/include/mount.h
> > @@ -65,6 +65,7 @@ struct mount_info {
> >  	bool			need_plugin;
> >  	bool			is_ns_root;
> >  	bool			deleted;
> > +	bool			remap;
> 
> Unused.
> 
> >  	struct mount_info	*next;
> >  	struct ns_id		*nsid;
> >  
> > diff --git a/criu/mount.c b/criu/mount.c
> > index e568249..6d61219 100644
> > --- a/criu/mount.c
> > +++ b/criu/mount.c
> > @@ -2718,7 +2718,146 @@ static int do_umount_one(struct mount_info *mi)
> >  	return 0;
> >  }
> >  
> > -static int cr_pivot_root(char *root)
> > +static struct mount_info *roots_mp = NULL;
> 
> This thing was moved from populate_mnt_ns(). This name is OK for the
> on-stack variable, but for the file-wide one it's no longer nice.
> 
> > +
> > +static inline int print_ns_root(struct ns_id *ns, int remap_id, char *buf, int bs);
> > +static int get_mp_mountpoint(char *mountpoint, struct mount_info *mi, char *root, int root_len);
> > +
> > +static LIST_HEAD(mnt_remap_list);
> > +static int remap_id;
> > +
> > +struct mnt_remap_entry {
> > +	struct mount_info *parent, *child;
> > +	struct list_head node;
> 
> Please, document the parent and child fields of this struct.
> 
> > +};
> > +
> > +/*
> > + * If a mount overmounts other mounts, it is restored
> > + * separetly and then moved to the right place.
> > + * All these mounts are moved into the root yard.
> > + */
> > +static int do_mnt_remap(struct mount_info *m)
> > +{
> > +	int len;
> > +
> > +	if (m->nsid->type == NS_OTHER) {
> > +		/* A path in root_yard has a fixed size, so it can be replaced. */

^^^^^^^^^^^^^ the answer for the next question is here
> > +		len = print_ns_root(m->nsid, remap_id, m->mountpoint, PATH_MAX);
> > +		m->mountpoint[len] = '/';
> > +	} else {
> 
> Else what? type == NS_ROOT? And why is root's mounts are __that__ special?

Because the length of m->mountpoint is changed in this case
and we need to reallocate it.
> 
> > +		char root[PATH_MAX], *mp, *ns_mp;
> > +		int len, ret;
> > +
> > +		/* Add a root_yard path */
> 
> Please, write a MORE descriptive comment showing what is changed into what.
> 
> > +		mp = m->mountpoint;
> > +		ns_mp = m->ns_mountpoint;
> > +
> > +		len = print_ns_root(m->nsid, remap_id, root, PATH_MAX);
> > +
> > +		ret = get_mp_mountpoint(ns_mp, m, root, len);
> > +		if (ret < 0)
> > +			return ret;
> > +		xfree(mp);
> > +	}
> > +	return 0;
> > +}
> > +
> > +static int remap_mnt(struct mount_info *m)
> > +{
> > +	struct mnt_remap_entry *r;
> > +
> > +	if (!does_mnt_overmount(m))
> > +		return 0;
> > +
> > +	BUG_ON(!m->parent || !list_empty(&m->parent->mnt_share));
> > +
> > +	r = xmalloc(sizeof(struct mnt_remap_entry));
> > +	if (!r)
> > +		return -1;
> > +
> > +	r->child = m;
> > +	list_add(&r->node, &mnt_remap_list);
> > +
> > +	return 0;
> > +}
> > +
> > +static int handle_overmounts(struct mount_info *root)
> > +{
> > +	struct mnt_remap_entry *r;
> > +	struct mount_info *m;
> > +
> > +	/*
> > +	 * Mark mounts which have to be restored separetly,
> > +	 * because it's imposiable to remove them from
> > +	 * a tree without interrupting enumeration.
> > +	 */

This is an answer on your next question.

> > +	if (mnt_tree_for_each(root, remap_mnt))
> > +		return -1;
> > +
> > +	/* Move remapped mounts to root_yard */
> > +	list_for_each_entry(r, &mnt_remap_list, node) {
> > +		m = r->child;
> > +		r->parent = m->parent;
> > +		m->parent = roots_mp;
> > +		list_del(&m->siblings);
> > +		list_add(&m->siblings, &roots_mp->children);
> 
> list_move()?

ok

> 
> Why not do all of the above in remap_mnt()?

I wrote a comment to answer on this question.
> 
> > +
> > +		remap_id++;
> > +		mnt_tree_for_each(m, do_mnt_remap);
> > +		pr_debug("Restore the %d mount in %s\n", m->mnt_id, m->mountpoint);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +/* Move remapped mounts to places where they have to be */
> > +static int handle_mnt_remaps(int nsid, char *put_root)
> 
> Two handle_ routines in one patch :) Too generic names, can we come up with better ones?
> 
> > +{
> > +	struct mnt_remap_entry *r;
> > +	int cwd, exit_code = -1;
> > +
> > +	cwd = open(".", O_PATH);
> > +	if (cwd < 0) {
> > +		pr_perror("Unable to open \".\"\n");
> > +		return -1;
> > +	}
> > +
> > +	if (nsid != root_item->ids->mnt_ns_id && chdir(put_root)) {
> 
> What for?

Because the root yard is there. I had to write a comment here, but
I reworked this code for the next series.
> 
> > +		pr_perror("Unable to change working directory");
> > +		return -1;
> > +	}
> > +
> > +	list_for_each_entry(r, &mnt_remap_list, node) {
> > +		struct mount_info *m = r->child;
> > +
> > +		if (r->child->nsid->id != nsid)
> > +			continue;
> 
> Where's the guarantee that the mnt_remap_list() is empty at the end?

We enumirate all namespaces, so it has to be clean.

In the next version this code will be reworked too.

> 
> > +
> > +		pr_debug("Move mount %s -> %s\n", m->mountpoint, m->ns_mountpoint);
> > +		if (mount(m->mountpoint, m->ns_mountpoint, NULL, MS_MOVE, NULL)) {
> > +			pr_perror("Unable to move mount %s -> %s", m->mountpoint, m->ns_mountpoint);
> > +			goto err;
> > +		}
> > +
> > +		list_del(&r->child->siblings);
> > +		list_add(&r->child->siblings, &r->parent->children);
> > +		r->child->parent = r->parent;
> 
> Worth having 'reparent_mount()' helper? The same code is in handle_overmounts().

It isn't the same. There we move mount to the remap list and here we
remove the mount from the remap list.
> 
> > +	}
> > +
> > +	exit_code = 0;
> > +err:
> > +	if (fchdir(cwd)) {
> > +		pr_perror("Unable to change working directory");
> > +		close(cwd);
> > +		return -1;
> > +	}
> > +	close(cwd);
> > +
> > +	return exit_code;
> > +}
> > +
> > +
> > +static int cr_pivot_root(char *root, int nsid)
> >  {
> >  	char tmp_dir_tmpl[] = "crtools-put-root.XXXXXX";
> >  	bool tmp_dir = false;
> > @@ -2759,6 +2898,9 @@ static int cr_pivot_root(char *root)
> >  		goto err_tmpfs;
> >  	}
> >  
> > +	if (nsid > 0 && handle_mnt_remaps(nsid, put_root))
> > +		return -1;
> > +
> >  	if (mount("none", put_root, "none", MS_REC|MS_SLAVE, NULL)) {
> >  		pr_perror("Can't remount root with MS_PRIVATE");
> >  		return -1;
> > @@ -3196,6 +3338,7 @@ void fini_restore_mntns(void)
> >   */
> >  static int populate_roots_yard(void)
> >  {
> > +	struct mnt_remap_entry *r;
> >  	char path[PATH_MAX];
> >  	struct ns_id *nsid;
> >  
> > @@ -3216,6 +3359,13 @@ static int populate_roots_yard(void)
> >  		}
> >  	}
> >  
> > +	list_for_each_entry(r, &mnt_remap_list, node) {
> > +		if (mkdirpat(AT_FDCWD, r->child->mountpoint)) {
> 
> You mkdir() with names from image for all the namespaces found. What if names conflict?

We add remap_id into a name, which is uniq

> 
> > +			pr_perror("Unable to create %s", r->child->mountpoint);
> > +			return -1;
> > +		}
> > +	}
> > +
> >  	return 0;
> >  }
> >  
> > @@ -3223,7 +3373,6 @@ static int populate_mnt_ns(void)
> >  {
> >  	struct mount_info *pms;
> >  	struct ns_id *nsid;
> > -	struct mount_info *roots_mp = NULL;
> >  	int ret;
> >  
> >  	if (mnt_roots) {
> > @@ -3258,6 +3407,9 @@ static int populate_mnt_ns(void)
> >  	if (validate_mounts(mntinfo, false))
> >  		return -1;
> >  
> > +	if (handle_overmounts(pms))
> > +		return -1;
> > +
> >  	/*
> >  	 * Set properties for the root before mounting a root yard,
> >  	 * otherwise the root yard can be propagated into the host
> > @@ -3462,7 +3614,7 @@ int prepare_mnt_ns(void)
> >  
> >  	ret = populate_mnt_ns();
> >  	if (!ret && opts.root)
> > -		ret = cr_pivot_root(NULL);
> > +		ret = cr_pivot_root(NULL, root_item->ids->mnt_ns_id);
> >  	if (ret)
> >  		return -1;
> >  
> > @@ -3495,7 +3647,7 @@ int prepare_mnt_ns(void)
> >  		/* Set its root */
> >  		path[0] = '/';
> >  		print_ns_root(nsid, 0, path + 1, sizeof(path) - 1);
> > -		if (cr_pivot_root(path))
> > +		if (cr_pivot_root(path, nsid->id))
> >  			goto err;
> >  
> >  		/* Pin one with a file descriptor */
> > 
> 


More information about the CRIU mailing list