[CRIU] OverlayFS Patch
Gabriel Guimaraes
gabriellimaguimaraes at gmail.com
Wed Aug 5 14:19:45 PDT 2015
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.
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openvz.org/pipermail/criu/attachments/20150805/814f862e/attachment.html>
More information about the CRIU
mailing list