[CRIU] [PATCH 3/6] mnt: add --enable-external-sharing flag

[Tycho Andersen]

On Wed, Apr 08, 2015 at 06:23:10PM +0300, Andrew Vagin wrote:
> On Wed, Apr 08, 2015 at 09:05:01AM -0600, Tycho Andersen wrote:
> > On Wed, Apr 08, 2015 at 05:47:56PM +0300, Andrew Vagin wrote:
> > > On Wed, Apr 08, 2015 at 08:36:37AM -0600, Tycho Andersen wrote:
> > > > On Wed, Apr 08, 2015 at 04:14:27PM +0300, Andrew Vagin wrote:
> > > > > On Tue, Apr 07, 2015 at 11:37:18PM +0000, Tycho Andersen wrote:
> > > > > > With this flag, external shared bind mounts are attempted to be resolved
> > > > > > automatically.
> > > > > > 
> > > > > > Signed-off-by: Tycho Andersen <tycho.andersen at canonical.com>
> > > > > > ---
> > > > > >  crtools.c            | 6 ++++++
> > > > > >  include/cr_options.h | 1 +
> > > > > >  mount.c              | 3 +++
> > > > > >  3 files changed, 10 insertions(+)
> > > > > > 
> > > > > > diff --git a/crtools.c b/crtools.c
> > > > > > index a6ee2d7..fe77f39 100644
> > > > > > --- a/crtools.c
> > > > > > +++ b/crtools.c
> > > > > > @@ -204,6 +204,7 @@ int main(int argc, char *argv[], char *envp[])
> > > > > >  		{ "inherit-fd",		required_argument,	0, 1062	},
> > > > > >  		{ "feature",		required_argument,	0, 1063	},
> > > > > >  		{ "skip-mnt",		required_argument,	0, 1064},
> > > > > > +		{ "enable-external-sharing", no_argument, 	0, 1065 },
> > > > > >  		{ },
> > > > > >  	};
> > > > > >  
> > > > > > @@ -421,6 +422,9 @@ int main(int argc, char *argv[], char *envp[])
> > > > > >  			if (!add_skip_mount(optarg))
> > > > > >  				return 1;
> > > > > >  			break;
> > > > > > +		case 1065:
> > > > > > +			opts.enable_external_sharing = true;
> > > > > > +			break;
> > > > > >  		case 'M':
> > > > > >  			{
> > > > > >  				char *aux;
> > > > > > @@ -649,6 +653,8 @@ usage:
> > > > > >  "                        add external mount mapping\n"
> > > > > >  "  -M|--ext-mount-map auto\n"
> > > > > >  "                        attempt to autodetect external mount mapings\n"
> > > > > > +"  --enable-external-sharing\n"
> > > > > > +"                        allow autoresolving mounts with external sharing\n"
> > > > > >  "  --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 f1cfc84..29be2c8 100644
> > > > > > --- a/include/cr_options.h
> > > > > > +++ b/include/cr_options.h
> > > > > > @@ -63,6 +63,7 @@ struct cr_options {
> > > > > >  	char			*new_global_cg_root;
> > > > > >  	struct list_head	new_cgroup_roots;
> > > > > >  	bool			autodetect_ext_mounts;
> > > > > > +	bool			enable_external_sharing;
> > > > > >  	bool			aufs;		/* auto-deteced, not via cli */
> > > > > >  };
> > > > > >  
> > > > > > diff --git a/mount.c b/mount.c
> > > > > > index 49a07dd..8835a81 100644
> > > > > > --- a/mount.c
> > > > > > +++ b/mount.c
> > > > > > @@ -688,6 +688,9 @@ static int resolve_external_mounts(struct mount_info *info)
> > > > > >  			if (mounts_equal(pm, m, true))
> > > > > >  				match = true;
> > > > > >  
> > > > > > +			if (opts.enable_external_sharing && pm->shared_id == m->shared_id)
> > > > > > +				match = true;
> > > > > 
> > > > > Are you sure that we need this check?
> > > > > 
> > > > > I think here is an opposite problem, when we found equal mounts and their
> > > > > shared are different. In this case we need to determin where this shared
> > > > > group is external or not.
> > > > 
> > > > Here we're only looking in the criu ns's mounts (i.e. pm is only
> > > > external mounts), so every match here should be external. As for
> > > > whether we need this check, I thought we did but re-reading
> > > > mounts_equal, maybe we don't. Are people allowed to re-mount shared
> > > > mounts with different options without having it propagate? That's the
> > > > only case I can think of where we would need this.
> > > 
> > > I don't think that two mounts from one group can have different options.
> > > 
> > > But here are two other cases:
> > > 
> > > 1.
> > > 
> > > m->shared_id = 53;
> > > pm->shared_id = 89;
> > > mounts_equal(pm, m, true) = true
> > > 
> > > What is the right value of "match" in this cases?
> > 
> > If I understand correctly, the way someone would get to this case is:
> > 
> > 1. bind mount a shared mount into the namespace
> > 2. inside the namespace, remount MS_PRIVATE
> > 3. inside the namespace, remount MS_SHARED
> 
> or
> 
> mount /xxx
> mount --bind /xxx /yyy
> mount --make-shared /yyy
> mount --bind /xxx /zzz
> mount --make-shared /zzz
> bind mount a shared mount into the namespace
> 
> So the host mount namespaces contains three mounts and we need to choose
> the right one.
> 
> > 
> > So I think the right answer is that we need another flag to indicate
> > that the mount is external, but the sharing is not?
> 
> I think you are right.

Ok, I will rework the patch and send it ASAP.

Tycho

> > 
> > Tycho
> > 
> > > I think we need to check all mounts in the host mntns and only if we
> > > don't find this shared_id, we can declare that this group isn't
> > > external.
> > > 
> > > 2.
> > > 	m->shared_id = 5
> > > 	pm->shared_id = 5;
> > > 	m->root = /
> > > 	pm->root = /xxx
> > > 
> > > What is the right value of "match" in this cases?
> > > 
> > > I think match is false, because we can't restore m from pm.
> > > 
> > > > 
> > > > Tycho
> > > > 
> > > > > > +
> > > > > >  			if (!match)
> > > > > >  				continue;
> > > > > >  
> > > > > > -- 
> > > > > > 2.1.4
> > > > > > 
> > > > > > _______________________________________________
> > > > > > CRIU mailing list
> > > > > > CRIU at openvz.org
> > > > > > https://lists.openvz.org/mailman/listinfo/criu