[CRIU] [PATCH] mnt: add support for external shared mounts

Andrew Vagin avagin at odin.com
Fri Apr 3 09:00:43 PDT 2015


On Thu, Apr 02, 2015 at 08:22:33PM +0000, Tycho Andersen wrote:
> Add support for external shared mounts via the --enable-external-masters flag.
> This flag assumes that the mount is visible to criu via /proc/self/mountinfo
> and that it will be present at the same location on restore. No attempt to dump
> or restore the content is made; the external master is bind mounted into the
> mount ns and whatever is there is what the container sees.

I need time to think about the idea. At the first glance it should work.

But we (I and Pavel) are not sure that we understand what you do:).

Could you show /proc/PID/mountinfo for a container? Why can you not use
external mounts?

A few comments are inline.

Thanks.

> 
> Signed-off-by: Tycho Andersen <tycho.andersen at canonical.com>
> ---
>  crtools.c            |   6 +++
>  include/cr_options.h |   1 +
>  include/proc_parse.h |   1 +
>  mount.c              | 115 +++++++++++++++++++++++++++++++++++++++++++++------
>  protobuf/mnt.proto   |   2 +
>  5 files changed, 112 insertions(+), 13 deletions(-)
> 
> diff --git a/crtools.c b/crtools.c
> index 0b3c497..7986beb 100644
> --- a/crtools.c
> +++ b/crtools.c
> @@ -203,6 +203,7 @@ int main(int argc, char *argv[], char *envp[])
>  		{ "cgroup-root", required_argument, 0, 1061},
>  		{ "inherit-fd", required_argument, 0, 1062},
>  		{ "feature", required_argument, 0, 1063},
> +		{ "enable-external-masters", no_argument, 0, 1064},
>  		{ },
>  	};
>  
> @@ -416,6 +417,9 @@ int main(int argc, char *argv[], char *envp[])
>  			if (check_add_feature(optarg) < 0)
>  				return 1;
>  			break;
> +		case 1064:
> +			opts.enable_external_masters = true;
> +			break;
>  		case 'M':
>  			{
>  				char *aux;
> @@ -637,6 +641,8 @@ usage:
>  "  --force-irmap         force resolving names for inotify/fsnotify watches\n"
>  "  -M|--ext-mount-map KEY:VALUE\n"
>  "                        add external mount mapping\n"
> +"  --enable-external-masters\n"
> +"                        Allow mounts with external masters\n"

Could you add a full description for this option into
Documentation/criu.txt

>  "  --manage-cgroups      dump or restore cgroups the process is in\n"
>  "  --cgroup-root [controller:]/newroot\n"
>  "                        change the root cgroup the controller will be\n"
> diff --git a/include/cr_options.h b/include/cr_options.h
> index d6ce2ae..846f03c 100644
> --- a/include/cr_options.h
> +++ b/include/cr_options.h
> @@ -59,6 +59,7 @@ struct cr_options {
>  	bool			manage_cgroups;
>  	char			*new_global_cg_root;
>  	struct list_head	new_cgroup_roots;
> +	bool			enable_external_masters;
>  	bool			aufs;		/* auto-deteced, not via cli */
>  };
>  
> diff --git a/include/proc_parse.h b/include/proc_parse.h
> index 792cf06..46ae393 100644
> --- a/include/proc_parse.h
> +++ b/include/proc_parse.h
> @@ -130,6 +130,7 @@ struct mount_info {
>  	struct ns_id	*nsid;
>  
>  	struct ext_mount *external;
> +	bool		external_master;
>  
>  	/* tree linkage */
>  	struct mount_info *parent;
> diff --git a/mount.c b/mount.c
> index 021c6e3..cbdad87 100644
> --- a/mount.c
> +++ b/mount.c
> @@ -566,6 +566,9 @@ static struct mount_info *find_fsroot_mount_for(struct mount_info *bm)
>  	return NULL;
>  }
>  
> +struct mount_info *external_mounts = NULL;
> +static int try_resolve_external_sharing(struct mount_info *m);
> +
>  static int validate_mounts(struct mount_info *info, bool for_dump)
>  {
>  	struct mount_info *m, *t;
> @@ -578,6 +581,11 @@ static int validate_mounts(struct mount_info *info, bool for_dump)
>  		if (m->shared_id && validate_shared(m))
>  			return -1;
>  
> +		if (m->master_id && opts.enable_external_masters) {
> +			if (try_resolve_external_sharing(m) == 0)
> +				continue;
> +		}
> +
>  		/*
>  		 * Mountpoint can point to / of an FS. In that case this FS
>  		 * should be of some known type so that we can just mount one.
> @@ -646,10 +654,11 @@ static int collect_shared(struct mount_info *info)
>  	struct mount_info *m, *t;
>  
>  	/*
> -	 * If we have a shared mounts, both master
> -	 * slave targets are to be present in mount
> -	 * list, otherwise we can't be sure if we can
> -	 * recreate the scheme later on restore.
> +	 * If we have a shared mounts, both master slave targets are to be
> +	 * present in mount list, otherwise we can't be sure if we can
> +	 * recreate the scheme later on restore. If --enable-external-masters
> +	 * is supplied, we assume that the scheme will be present on restore
> +	 * and allow the mounts to be dumped.
>  	 */
>  	for (m = info; m; m = m->next) {
>  		bool need_share, need_master;
> @@ -676,10 +685,28 @@ static int collect_shared(struct mount_info *info)
>  		}
>  
>  		if (need_master && m->parent) {
> -			pr_err("Mount %d (master_id: %d shared_id: %d) "
> -			       "has unreachable sharing\n", m->mnt_id,
> -				m->master_id, m->shared_id);
> -			return -1;
> +			if (opts.enable_external_masters) {
> +				struct mount_info *pm;
> +				bool found = false;
> +
> +				for (pm = external_mounts; pm->next != NULL; pm = pm->next) {
> +					if (pm->shared_id == m->master_id) {
> +						found = true;
> +						break;
> +					}
> +				}

                if (collect_shared(mntinfo))
                        goto err;
                if (validate_mounts(mntinfo, true))
                        goto err;

try_resolve_external_sharing() is called from validate_mounts(), so we
enumerate external mounts twice. Can we do this only once?

> +
> +				if (!found) {
> +					pr_err("couldn't find sharing for %d "
> +						"(master_id: %d shared_id: %d)",
> +						m->mnt_id, m->master_id, m->shared_id);
> +				}
> +			} else {
> +				pr_err("Mount %d (master_id: %d shared_id: %d) "
> +				       "has unreachable sharing. Try --enable-external-masters.\n", m->mnt_id,
> +					m->master_id, m->shared_id);
> +				return -1;
> +			}
>  		}
>  
>  		/* Search bind-mounts */
> @@ -1159,6 +1186,12 @@ static int dump_one_mountpoint(struct mount_info *pm, struct cr_img *img)
>  	me.has_shared_id	= true;
>  	me.master_id		= pm->master_id;
>  	me.has_master_id	= true;
> +
> +	if (pm->external_master) {
> +		me.external_master	= true;
> +		me.has_external_master  = true;
> +	}
> +
>  	if (pm->need_plugin) {
>  		me.has_with_plugin = true;
>  		me.with_plugin = true;
> @@ -1302,7 +1335,7 @@ again:
>  	if (!progress) {
>  		struct mount_info *m;
>  
> -		pr_err("A few mount points can't be mounted");
> +		pr_err("A few mount points can't be mounted\n");
>  		list_for_each_entry(m, &postpone2, postpone) {
>  			pr_err("%d:%d %s %s %s\n", m->mnt_id,
>  				m->parent_mnt_id, m->root,
> @@ -1475,17 +1508,19 @@ static int do_new_mount(struct mount_info *mi)
>  		return -1;
>  
>  	if (mount(src, mi->mountpoint, tp->name,
> -			mi->flags & (~MS_SHARED), mi->options) < 0) {
> +			mi->flags & ~(MS_SLAVE | MS_SHARED), mi->options) < 0) {

Maybe it's better to mount such mounts in do_bind_mount?

>  		pr_perror("Can't mount at %s", mi->mountpoint);
>  		return -1;
>  	}
>  
> -	if (restore_shared_options(mi, 0, mi->shared_id, 0))
> +	if (restore_shared_options(mi, 0, mi->shared_id, mi->flags & MS_SLAVE || mi->external_master))
>  		return -1;
>  
>  	mi->mounted = true;
>  
> -	if (tp->restore && tp->restore(mi))
> +	// If there is an external master, the content of the mount comes from
> +	// there; otherwise, we restore it as usual.
> +	if (!mi->external_master && tp->restore && tp->restore(mi))
>  		return -1;
>  
>  	return 0;
> @@ -1617,7 +1652,7 @@ static int do_mount_one(struct mount_info *mi)
>  	if (mi->mounted)
>  		return 0;
>  
> -	if (!can_mount_now(mi)) {
> +	if (!mi->external_master && !can_mount_now(mi)) {
>  		pr_debug("Postpone slave %s\n", mi->mountpoint);
>  		return 1;
>  	}
> @@ -1858,6 +1893,11 @@ static int collect_mnt_from_image(struct mount_info **pms, struct ns_id *nsid)
>  		pm->need_plugin		= me->with_plugin;
>  		pm->is_ns_root		= is_root(me->mountpoint);
>  
> +		if (me->has_external_master)
> +			pm->external_master = pm->external_master;
> +		else
> +			pm->external_master = false;
> +
>  		/* FIXME: abort unsupported early */
>  		pm->fstype		= decode_fstype(me->fstype);
>  
> @@ -2065,6 +2105,8 @@ static int prepare_roots_yard(void)
>  	return 0;
>  }
>  
> +static int collect_external_mounts();
> +
>  static int populate_mnt_ns(struct mount_info *mis)
>  {
>  	struct mount_info *pms;
> @@ -2077,6 +2119,9 @@ static int populate_mnt_ns(struct mount_info *mis)
>  	if (!pms)
>  		return -1;
>  
> +	if (collect_external_mounts() < 0)
> +		return -1;
> +
>  	if (collect_shared(mis))
>  		return -1;
>  
> @@ -2312,6 +2357,48 @@ int mntns_get_root_by_mnt_id(int mnt_id)
>  	return mntns_get_root_fd(mntns);
>  }
>  
> +// We collect the mounts in /proc/self in order to find the master node for
> +// shared mounts.

We usually use this style for comments:
/*
 *
 */

> +static int collect_external_mounts()
> +{
> +	external_mounts = parse_mountinfo(getpid(), NULL);
> +	if (!external_mounts)
> +		return -1;
> +
> +	return 0;
> +}
> +
> +static int try_resolve_external_sharing(struct mount_info *m)
> +{
> +	struct mount_info *pm;
> +
> +	for (pm = external_mounts; pm->next != NULL; pm = pm->next) {
> +		if (pm->shared_id == m->master_id) {

if (pm->shared_id != m->master_id)
	continue

> +			int size, ret;
> +			char *p;
> +
> +			size = strlen(pm->mountpoint + 1) + strlen(m->root) + 1;
> +			p = xmalloc(sizeof(char) * size);
> +			if (!p)
> +				return -1;
> +
> +			ret = snprintf(p, size+1, "%s%s", pm->mountpoint + 1, m->root);
> +			if (ret < 0 || ret >= size) {
> +				free(p);
> +				return -1;
> +			}
> +
> +			xfree(m->source);
> +			m->source = p;
> +			m->external_master = true;
> +			m->flags |= MS_BIND;
> +			return 0;
> +		}
> +	}
> +
> +	return -1;
> +}
> +
>  static int collect_mntns(struct ns_id *ns, void *oarg)
>  {
>  	struct mount_info *pms;
> @@ -2338,6 +2425,8 @@ int collect_mnt_namespaces(bool for_dump)
>  	if (for_dump && need_to_validate) {
>  		ret = -1;
>  
> +		if (collect_external_mounts())
> +			goto err;
>  		if (collect_shared(mntinfo))
>  			goto err;
>  		if (validate_mounts(mntinfo, true))
> diff --git a/protobuf/mnt.proto b/protobuf/mnt.proto
> index 343bd6d..6f52010 100644
> --- a/protobuf/mnt.proto
> +++ b/protobuf/mnt.proto
> @@ -35,4 +35,6 @@ message mnt_entry {
>  
>  	optional bool		with_plugin		= 12;
>  	optional bool		ext_mount		= 13;
> +
> +	optional bool		external_master = 14;
>  }
> -- 
> 2.1.4
> 
> _______________________________________________
> CRIU mailing list
> CRIU at openvz.org
> https://lists.openvz.org/mailman/listinfo/criu


More information about the CRIU mailing list