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

[Tycho Andersen]

On Mon, Apr 06, 2015 at 10:34:33PM +0300, Pavel Emelyanov wrote:
> On 04/06/2015 06:40 PM, Tycho Andersen wrote:
> > On Mon, Apr 06, 2015 at 06:19:48PM +0300, Pavel Emelyanov wrote:
> >> On 04/06/2015 06:06 PM, Tycho Andersen wrote:
> >>> Hi Pavel,
> >>>
> >>> On Mon, Apr 06, 2015 at 05:56:41PM +0300, Pavel Emelyanov wrote:
> >>>> On 04/06/2015 05:48 PM, Tycho Andersen wrote:
> >>>>> On Mon, Apr 06, 2015 at 11:26:41AM +0300, Pavel Emelyanov wrote:
> >>>>>> On 04/03/2015 07:20 PM, Tycho Andersen wrote:
> >>>>>>> Hi Andrey,
> >>>>>>>
> >>>>>>> On Fri, Apr 03, 2015 at 07:00:43PM +0300, Andrew Vagin wrote:
> >>>>>>>> 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?
> >>>>>>>
> >>>>>>> Because criu dies with "couldn't find sharing" before the external
> >>>>>>> mount map is consulted. I could use ext-mount-map, but we'd still need
> >>>>>>> some patch to skip this check, and since we can resolve these
> >>>>>>> automatically anyway it seems like we should just do that instead.
> >>>>>>
> >>>>>> Ouch, so the slavery is set up for external mount points? I've missed that, sorry :(
> >>>>>> But taking into account my new insight :) doesn't the patch falls logically into
> >>>>>> two pieces -- the first is auto-assignments for ext-mount-map-s and the second --
> >>>>>> enabling for slave external bind mounts?
> >>>>>
> >>>>> I guess it depends on how you want the implementation to work.
> >>>>
> >>>> :) I want it to suit all the parties' requirements, though not duplicate
> >>>> the functionality and API.
> >>>>
> >>>>> Currently it doesn't use the ext-mount-map piece, although I think it
> >>>>> could.
> >>>>
> >>>> I was under impression that the whole effort is to support internal
> >>>> mountpoints that happened to be slave to something from the outside.
> >>>> Now it seems that the idea is to support external slave mounts only.
> >>>
> >>> Sorry, I don't understand the distinction you're making here.
> >>
> >> In the validate_mounts() we have call for try_resolve_ext_mount() (it's
> >> the wrong place to call, but nonetheless). Any mountpoint whose root
> >> cannot be found withing the container's root gets there and we try to
> >> treat this mountpoint as "external".
> >>
> >> AFAIU you try to fix only mountpoints that are such.
> > 
> > Ah, I understand. You want to handle all external mounts (so nobody
> > would have to pass anything besides --ext-mount-map auto) here? 
> 
> Well, not that I want to do this :) But I see this happens in your patch,
> so I'm asking whether you're dealing with external bind mounts or not.
> 
> I see that the patch you suggest works, please let me explain why I'm trying
> to get more out of it :)
> 
> What you do with external masters looks so similar to the existing external
> bind mounts code we have that I'm trying to re-use API and semantics as much
> as possible :) And code if possible too. In particular -- your code that 
> merges the host_pm->mountpoint with ct_m->root is exactly what people usually 
> do to get the argument for the --ext-mount-map option on restore. When we 
> were doing the ext-mount-map, Saied proposed to declare the external bind 
> mount API in a bit different way than it is now -- he suggested instead of 
> mountpoint:ID on dump plus ID:host-root on restore do only the
> mountpoint:host-root on dump thus avoiding ID and option for restore. We
> agreed then that the options pair with an ID would be more generic as it would 
> allow to change the host-root on restore according to the restore needs.
> 
> The single option --enable-external-masters first seemed to me to do the
> Saied's proposal, but with auto-detection of the host-root path plus the
> slavery setup. So I suggested to make the auto-detection be a separate
> feature, and the slavery setup to be the separate feature too.
> 
> But you're right asking the right question:
> 
> > How do we detect where the bind mount came from without the sharing numbers?
> 
> I now see that for slave mounts we can find the "master" by ID comparison.
> We can probably do the same for "shared" mounts (can we?).

Yes, just using the shared_id instead of master_id; this patch should
probably handle that as well.

> The question is really what to do with private mounts. Currently we
> off-load this to caller asking one to specify the path as the
> argument to --ext-mount-map.
>
> I think that we can do this the same way as it's done in collect_shared() at
> the very end under the "Search bind-mounts" comment -- if we scan criu's list
> of mounts and check for mounts_equal() we find the source.
> 
> 
> So after all of the above the feature in my head looks like this.
> 
> The --ext-mount-map option still exists and is used for manual mountpoints
> assignment. We can additionally say e.g. --enable-external-masters to let
> the external mounts be marked as slave. Probably it also makes sense to
> introduce the --enable-external-shares to do the same with fully shared
> mounts. And, at the end, we can say --ext-mount-map auto to turn on the
> automatic detection of host-root paths during dump. For slave (and shared)
> mountpoints your code that compares IDs is used, for private mounts --
> the mounts_equal() is.
> 
> Does this sound OK, or /me is too stupid?

Yes, this all sounds good. I'll see about adding these things this
week.

Tycho

> -- Pavel
> 
> >>>> Am I right with this? If yes, then we should definitely re-use the
> >>>> external mounts code we have.
> >>>>
> >>>>> I think we'd still need an extra flag in the protobufs to
> >>>>> indicate that the mount was an external sharing and would not be
> >>>>> provided by the user from the CLI on restore, so I'm not sure using
> >>>>> ext-mount-map really buys us much, but I can make those changes if you
> >>>>> want.
> >>>>
> >>>> We already have the ext_mount bit in the mnt_entry. If I get it right,
> >>>> you will need a) sign that the mount point in question is slave and
> >>>> b) the path to mountpoint on dump (to use it on restore).
> >>>>
> >>>> While "a" seem to be auto-assigned on restore, the "b" can be achieved
> >>>> by introducing the "--ext-mount-map auto" mode. In this mode all the
> >>>> mountpoints that are thought to be "external" would be treated as if
> >>>> the called has specifies --ext-mount-map $mountpoint:$root. I.e. -- the
> >>>> root path will be written in the image just as you suggested in the
> >>>> your patch.
> >>>>
> >>>> Does the above work to you?
> >>>
> >>> It works, but how do I tell the difference between something where the
> >>> ext-mount-map was provided by the user and where it was autogenerated?
> >>
> >> You mean on restore? All "external" mount entries are marked. And, for
> >> such entries, the --ext-mount-map's VALUE is written into the image too.
> >>
> >> How typically --ext-mount-map is used is like this.
> >>
> >>    On dump:     --ext-mount-map mount-point:some-value
> >>
> >>    On restore:  --ext-mount-map some-value:path-to-root-on-host
> >>
> >> If instead of some-value we use some path-to-root-on-host from the
> >> very beginning, and this path is calculated the way you propose:
> >>
> >>              snprintf(p, size+1, "%s%s", pm->mountpoint + 1, m->root);
> >>
> >> then we get the "auto" mode. As you told, for standard C/R this would
> >> work, so we save many --ext-mount-map options and just use one -- the
> >> "--ext-mount-map auto".
> >>
> >>> That's the part I think we need the extra bit for. We could do
> >>> something like "if the mount name isn't found in the ext-mount-map on
> >>> restore and the user provided auto as one of the options, then assume
> >>> the mount had external sharing", but it seems much cleaner to be to be
> >>> explicit.
> >>
> >> Sure, but my point is that we have the external-mount bit in the image
> >> already. So we effectively do "if the mountpoint is external and the
> >> --ext-mount-map auto is specified (and we haven't found the explicit
> >> --ext-mount-map KEY:VAL mapping), then use the source path from image".
> > 
> > I see, ok. Take a look at the patches I just sent and see if you have
> > any more comments, and I will re-work them again to have the flags
> > look like this. I'm not sure how to do the bind mount detection
> > without sharing as above, though.
> > 
> >> -- Pavel
> >>
> > .
> > 
>