[CRIU] [PATCH v2 3/3] mount: handle tracefs more gracefully

[Pavel Emelyanov]

On 05/11/2016 06:21 PM, Tycho Andersen wrote:
> On Wed, May 11, 2016 at 08:49:19AM -0600, Tycho Andersen wrote:
>> On Wed, May 11, 2016 at 03:49:07PM +0300, Pavel Emelyanov wrote:
>>> On 05/11/2016 12:15 AM, Tycho Andersen wrote:
>>>> See the comment for details, but basically tracefs is automounted by the
>>>> kernel, so we can just mount debugfs with MS_REC and get the right result.
>>>>
>>>> v2: rebase on criu-dev
>>>>
>>>> Signed-off-by: Tycho Andersen <tycho.andersen at canonical.com>
>>>> ---
>>>> This is kind of similarly ugly to to a cgroup patch I sent earlier, where
>>>> we needed some filesystem specific code and checks in various parts of
>>>> mount.c. I somehow can't figure out a nicer way to do this, but I am
>>>> definitely open to suggestions.
>>>
>>> A suggestion :)
>>>
>>>> @@ -2301,6 +2310,11 @@ static int do_new_mount(struct mount_info *mi)
>>>>  	if (remount_ro)
>>>>  		sflags &= ~MS_RDONLY;
>>>>  
>>>> +	if (tp->flags) {
>>>> +		pr_info("forcing flags: %d\n", tp->flags);
>>>> +		sflags |= tp->flags;
>>>> +	}
>>>> +
>>>>  	if (do_mount(mi, src, mnt_fsname(mi), sflags) < 0) {
>>>
>>> This do_mount can call mi->fs->mount() callback. What if we define one
>>> for debugfs that would just call do_simple_mount() with updated flags?
>>
>> Yes, this sounds a lot better :). I'll resend, thanks for the
>> suggestion.
> 
> Ugh, actually this won't work for us, because liblxc by default bind
> mounts debugfs into the container, rather than letting the container
> mount it itself, so the hunk we actually care about for this patch is
> the bit in do_bind:
> 
> @@ -2474,12 +2488,16 @@ do_bind:
>                 }
>         }
> 
> -       if (mount(root, mi->mountpoint, NULL, MS_BIND, NULL) < 0) {
> +       if (mount(root, mi->mountpoint, NULL, MS_BIND | mi->fstype->flags, NULL) < 0) {
>                 pr_perror("Can't mount at %s", mi->mountpoint);
>                 goto err;
>         }
> 
>         mflags = mi->flags & (~MS_PROPAGATE);
> +       if (mi->fstype->flags) {
> +               pr_info("forcing flags: 0x%x\n", mi->fstype->flags);
> +               mflags |= mi->fstype->flags;
> +       }
>         if (!mi->bind || mflags != (mi->bind->flags & (~MS_PROPAGATE)))
>                 if (mount(NULL, mi->mountpoint, NULL, MS_BIND | MS_REMOUNT | mflags, NULL)) {
>                         pr_perror("Can't mount at %s", mi->mountpoint);

Ouch :( And we we need MS_REC on __both__ mount-s -- initial and remount?

> So we need some way to hook that. Maybe adding another fstype->bind hook, so
> it's more general than just flags, similar to the ->mount hook above?

At this point I think the fstype->flags would be better. Though I've found one
more place in the patch that looks weird:

> @@ -1380,6 +1380,13 @@ struct mount_info *parse_mountinfo(pid_t pid, struct ns_id *nsid, bool for_dump)
>  			goto end;
>  		}
>  
> +		if (new->fstype->code == FSTYPE__TRACEFS) {
> +			pr_info("\tskipping tracefs mounted at %s\n", new->mountpoint + 1);
> +			mnt_entry_free(new);
> +			new = NULL;
> +			goto end;
> +		}
> +
>  		pr_info("\ttype %s source %s mnt_id %d s_dev %#x %s @ %s flags %#x options %s\n",
>  				fsname, new->source,
>  				new->mnt_id, new->s_dev, new->root, new->mountpoint,

Why do we need to drop this entry? AFAIU this is only required to make
restore skip mounting one and let 'mount -t debugfs' do the thing, right?
If yes, then we should keep this entry parsed and skip one on restore.
I would say -- the debugfs' -> parse callback should walk all tracefs'
MI's and set mounted on them to true.

-- Pavel