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

[Pavel Emelyanov]

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

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