[CRIU] OverlayFS Patch

[Pavel Emelyanov]

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

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