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

[Pavel Emelyanov]

On 04/02/2015 11:22 PM, 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.
> 
> Signed-off-by: Tycho Andersen <tycho.andersen at canonical.com>

Thanks :) Please, find my comments and questions inline.

> ---
>  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(-)
> 

> @@ -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"

Nitpick -- help text starts with small letter :)

>  "  --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/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;

AFAIU the external_mounts is regular mounts list but collected for criu's
mount namespace. We already have ns_id object for criu and it has the
mnt.mntinfo_list/_tree fields. One thing is that in collect_mnt_namespaces()
we don't collect criu's one when we dump container.

Can fix the existing collection code to have the criu's ns_id collected
in case the --enable-external-masters is specified?

> +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)

Shouldn't this be in collect_shared()? Validation is just checking that
the tree is in dumpable state, while any interconnections are be set in
other places.

> +				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.

> @@ -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;
> +					}

I mean this place -- once we foung shared_id == master_id we do the body
of try_resolve_external_mount().

> +				}
> +
> +				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 */

> @@ -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");

I'd appreciate fixes and cleanups to be sent separately :)

>  		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) {

What if mountpoint is not slave? Doesn't this hunk break the existing restore?

>  		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.

We try to follow kernel coding style which encourages usage of /**/-style
comments. Please :)

> +	if (!mi->external_master && tp->restore && tp->restore(mi))
>  		return -1;
>  
>  	return 0;

-- Pavel