[CRIU] OverlayFS Patch

[Filipe Brandenburger]

Can you try a directory instead of a file?

Either a subdirectory or the root of the filesystem (where it's mounted.)


On Wed, Aug 5, 2015 at 2:31 PM, Gabriel Guimaraes
<gabriellimaguimaraes at gmail.com> wrote:
> Hi Filipe,
>
> I just checked the value of p->fs_type when referring to /tmp/file within a
> Docker container, using overlayFS. It reports
>
> p->fs_type = 0xef53 which I believe is the magic number of ext2/3/4
>
> 2015-08-05 14:25 GMT-07:00 Filipe Brandenburger <filbranden at google.com>:
>>
>> Hi Gabriel,
>>
>> Looking at the code, I believe it f_type in statfs() output will
>> return this value for overlayfs:
>>
>> fs/overlayfs/super.c:#define OVERLAYFS_SUPER_MAGIC 0x794c7630
>>
>> You can easily double check that with a test program running it
>> against a mounted overlayfs.
>>
>> Cheers,
>> Filipe
>>
>>
>> On Wed, Aug 5, 2015 at 2:19 PM, Gabriel Guimaraes
>> <gabriellimaguimaraes at gmail.com> wrote:
>> >
>> >
>> > 2015-08-05 13:36 GMT-07:00 Pavel Emelyanov <xemul at parallels.com>:
>> >>
>> >> On 08/05/2015 08:51 PM, Gabriel Guimaraes wrote:
>> >> > Hi Pavel,
>> >> >
>> >> > Thanks for the review.
>> >> >
>> >> > On Wed, Aug 5, 2015 at 4:00 AM, Pavel Emelyanov <xemul at parallels.com
>> >> > <mailto:xemul at parallels.com>> wrote:
>> >> >
>> >> >     Gabriel,
>> >> >
>> >> >     Please, find my comments inline.
>> >> >
>> >> >     > diff --git a/crtools.c b/crtools.c
>> >> >     > index 6af6080..521d0ff 100644
>> >> >     > --- a/crtools.c
>> >> >     > +++ b/crtools.c
>> >> >     > @@ -235,6 +235,7 @@ int main(int argc, char *argv[], char
>> >> > *envp[])
>> >> >     >               { "enable-fs",
>> >> > required_argument,
>> >> > 0, 1065 },
>> >> >     >               { "enable-external-sharing",    no_argument,
>> >> > 0, 1066 },
>> >> >     >               { "enable-external-masters",    no_argument,
>> >> > 0, 1067 },
>> >> >     > +             { "overlayfs",                  no_argument,
>> >> > 0, 1068 },
>> >> >
>> >> >     With AUFS workaround we ended up with auto-detection scheme --
>> >> > when
>> >> >     meeting an AUFS mount the opts.aufs is set to true. Can we do the
>> >> >     same with overlayfs?
>> >> >
>> >> >
>> >> > Technically we could always enable overlayfs by default, since we
>> >> > only
>> >> > do something if there
>> >> > is an overlayfs mounted entry in the mountinfo table. Should I enable
>> >> > it
>> >> > always?
>> >>
>> >> In the fill_fdlink() there's fd_parms structure with fs_type field
>> >> filled
>> >> from
>> >> statfs() syscall. If we check for this value being overlayfs magic, we
>> >> can
>> >> silently
>> >> imply the workaround is on and go calling fixup_overlayfs().
>> >>
>> > Per man statfs, I don't think Overlayfs has a magic number. OverlayFS
>> > just
>> > overlays two filesystems on top of each other. From statfs' point of
>> > view, a
>> > file's filesystem type is the original one, not OverlayFS.
>> >
>> > More information here:
>> > https://www.kernel.org/doc/Documentation/filesystems/overlayfs.txt
>> >>
>> >> >     > +static struct mount_info *__lookup_overlayfs(struct mount_info
>> >> > *list, char *rpath,
>> >> >     > +                                             unsigned int
>> >> > st_dev,
>> >> > unsigned int st_ino,
>> >> >     > +                                             unsigned int
>> >> > mnt_id)
>> >> >     > +{
>> >> >     > +     /*
>> >> >     > +      * Goes through all entries in the mountinfo table
>> >> >     > +      * looking for a mount point that contains the file
>> >> > specified
>> >> >     > +      * in rpath. Uses the device number st_dev and the inode
>> >> > number st_ino
>> >> >     > +      * to make sure the file is correct.
>> >> >     > +      */
>> >> >     > +     struct mount_info *mi_ret = NULL;
>> >> >     > +     struct mount_info *m;
>> >> >     > +
>> >> >     > +     for (m = list; m != NULL; m = m->next) {
>> >> >     > +             /* Case 1: If mnt_id is correct, don't do
>> >> > anything
>> >> > */
>> >> >     > +             if (st_dev == m->s_dev && mnt_id == m->mnt_id)
>> >> >     > +                     return NULL;
>> >> >
>> >> >     Shouldn't this if () be under the next one, so that everything
>> >> > that
>> >> > happens
>> >> >     here applies to overlayfs only?
>> >> >
>> >> >
>> >> > This if () is basically a check to see if the bug exists. We look for
>> >> > all instances of the device
>> >> > st_dev and see if any of them match our mnt_id. If that's not the
>> >> > case,
>> >> > the bug exists and we want
>> >> > to proceed to try to fix it.
>> >>
>> >> OK
>> >>
>> >> > I could have left this as a separate for loop to make this more
>> >> > clear,
>> >> > but this way we only use
>> >> > one for loop for both. I believe it's better if we separate it into
>> >> > two
>> >> > for loops since we prevent
>> >> > some unnecessary stating. I can also move this part to
>> >> > lookup_overlayfs,
>> >> > before dealing with
>> >> > namespaces. What do you think?
>> >>
>> >> Yup, makes sense to me.
>> >>
>> >
>
>