[CRIU] Sync TODO-s for mount.c work

[Tycho Andersen]

On Thu, Apr 23, 2015 at 07:41:13PM +0300, Pavel Emelyanov wrote:
> On 04/23/2015 07:22 PM, Tycho Andersen wrote:
> > On Thu, Apr 23, 2015 at 07:15:47PM +0300, Pavel Emelyanov wrote:
> >> On 04/23/2015 07:05 PM, Tycho Andersen wrote:
> >>> On Thu, Apr 23, 2015 at 06:51:26PM +0300, Pavel Emelyanov wrote:
> >>>> On 04/23/2015 06:09 PM, Tycho Andersen wrote:
> >>>>> On Thu, Apr 23, 2015 at 06:04:35PM +0300, Pavel Emelyanov wrote:
> >>>>>> On 04/23/2015 05:31 PM, Tycho Andersen wrote:
> >>>>>>> On Thu, Apr 23, 2015 at 02:52:35PM +0300, Pavel Emelyanov wrote:
> >>>>>>>> On 04/23/2015 02:17 PM, Oleg Nesterov wrote:
> >>>>>>>>
> >>>>>>>>> Now what? Obviously we can't just add
> >>>>>>>>>
> >>>>>>>>> 	if (is_not_external_mount(m))
> >>>>>>>>> 		continue;
> >>>>>>>>
> >>>>>>>> Why not? The is_not_external_mount() is
> >>>>>>>>
> >>>>>>>> fsroot_mounted(m) || /* mount's root is visible from chroot */
> >>>>>>>
> >>>>>>> But what if the mount's root is /? e.g with pstore in lxc, the
> >>>>>>> mountinfo from inside looks like:
> >>>>>>>
> >>>>>>> 110 104 0:23 / /sys/fs/pstore rw,nosuid,nodev,noexec,relatime master:11 - pstore pstore rw
> >>>>>>>
> >>>>>>> which is a bind mount from the host's:
> >>>>>>>
> >>>>>>> 29 17 0:23 / /sys/fs/pstore rw,nosuid,nodev,noexec,relatime shared:11 - pstore pstore rw
> >>>>>>
> >>>>>> Ah, so sharing takes place. OK, then (sorry, maybe I'm replaying your
> >>>>>> conversation again, but I'm still blind about it) the check should be
> >>>>>>
> >>>>>>    (fsroot_mounted(m) || root_is_visible(m)) && master_is_external(m)
> >>>>>
> >>>>> I think not, because if we didn't have this as a shared mount it would
> >>>>> still be an external bind mount, just without sharing. An external
> >>>>> master peer is an indication that it's a bind mount, but if we don't
> >>>>> have one of those, it could still be a bind mount.
> >>>>
> >>>> Ah, sure. It should be NOT before master_is_external, like this
> >>>>
> >>>>    (fsroot_mounted(m) || root_is_visible(m)) && !master_is_external()
> >>>>
> >>>> Provided master_is_external() is false for private mounts, this should
> >>>> handle Oleg's case with mount --make-rprivate, but in suboptimal way --
> >>>> all mountpoints will be bind-mounted from host with MS_SLAVE flag.
> >>>
> >>> But this only works for mounts that have external masters. Doesn't it
> >>> fail to exclude mounts which aren't in a peer group at all (since
> >>> !master_is_external() will always be true for them)?
> >>
> >> Yes, !master_is_external() will be true and then the 2nd part of the
> >> check will take place -- fsroot || root_is_visible. If _this_ will be
> >> false then mountpoint will be still considered to be external.
> > 
> > Oh, sorry, I was off by one in my negation :). Doesn't this exclude my
> > pstore example if the mounts are all private?
> 
> The master_is_external() will be false, fsroot_mounted() will be true, so
> this mountpoint will be classified as "not external" and criu will try to
> just mount a new FS instance.
> 
> Since pstore is single-sb filesystem we'll get what we need. If it some
> day will be make multi-sb (virtualized) we will notice this by devices
> being different.
> 
> Hmm... I think I'm getting the point. Let's take your example with pstore
> but relpace pstore with some multi-sb fs.

Right :)

> E.g. the proc one. So if we have
> proc privately inherited from host and proc privately mounted inside then
> my checks will classify both as "private". While the former one should be
> bind-mounted and the latter one should be just mounted. So we also need to
> compare the super blocks, i.e. the devices. %)
> 
> So...
> 
> int is_not_external()
> {
>        if (master_is_external())
>              return false;
> 
>        if (root_is_accessible())
>              return true;
> 
>        return false;
> }
> 
> where root_is_accessible() should also care for the superblocks, i.e.
> 
> int root_is_accessible()
> {
>        if (device_is_visible())
>              return true;
> 
>        if (root_path_is_visible())

The problem is that the root path can be / (which really means "the
external /"), but we think it is visible since it's visible from the
internal / as well.

Tycho

>              return true;
> 
>        return false;
> }
> 
> -- Pavel