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

[Tycho Andersen]

On Fri, Apr 03, 2015 at 02:44:23PM +0300, Pavel Emelyanov wrote:
> 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?

If the mount point is not a slave I think this hunk should have no
effect (here we're just masking off the bits to restore MS_SLAVE
later). Without masking of MS_SLAVE, mount(2) was giving me an error
(EINVAL, I think).

The rest of the changes are good, I will make them and re-send once we
resolve this.

Tycho

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