<div dir="ltr">Anyway, as is we can simply enable opts.overlayfs = true always, and make sure that __mntns_get_root_fd and fstatat get called only if there is an OverlayFS entry in the mountinfo table. What do you think of that?</div><div class="gmail_extra"><br><div class="gmail_quote">2015-08-05 15:06 GMT-07:00 Gabriel Guimaraes <span dir="ltr"><<a href="mailto:gabriellimaguimaraes@gmail.com" target="_blank">gabriellimaguimaraes@gmail.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Hmm, I'm using GDB to look directly at the value of p->fs_type within fill_fdlink, when p refers to a file within a Docker container mounted with Overlayfs and I get <span style="font-size:12.8000001907349px">p->fs_type = 0xef53</span></div><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><div class="gmail_quote">2015-08-05 14:36 GMT-07:00 Filipe Brandenburger <span dir="ltr"><<a href="mailto:filbranden@google.com" target="_blank">filbranden@google.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">More specifically, I'm looking at this code:<br>
<br>
/**<br>
* ovl_statfs<br>
* @sb: The overlayfs super block<br>
* @buf: The struct kstatfs to fill in with stats<br>
*<br>
* Get the filesystem statistics. As writes always target the upper layer<br>
* filesystem pass the statfs to the same filesystem.<br>
*/<br>
static int ovl_statfs(struct dentry *dentry, struct kstatfs *buf)<br>
{<br>
struct ovl_fs *ofs = dentry->d_sb->s_fs_info;<br>
struct dentry *root_dentry = dentry->d_sb->s_root;<br>
struct path path;<br>
int err;<br>
<br>
ovl_path_upper(root_dentry, &path);<br>
<br>
err = vfs_statfs(&path, buf);<br>
if (!err) {<br>
buf->f_namelen = max(buf->f_namelen, ofs->lower_namelen);<br>
buf->f_type = OVERLAYFS_SUPER_MAGIC;<br>
}<br>
<br>
return err;<br>
}<br>
<br>
<br>
So, indeed, it's forwarding the call to the upper layer (since it's<br>
where writes happen) but before returning it's overwriting f_type with<br>
OVERLAYFS_SUPER_MAGIC which is the constant with the value I posted<br>
above...<br>
<br>
Cheers,<br>
Filipe<br>
<div><div><br>
<br>
On Wed, Aug 5, 2015 at 2:34 PM, Filipe Brandenburger<br>
<<a href="mailto:filbranden@google.com" target="_blank">filbranden@google.com</a>> wrote:<br>
> Can you try a directory instead of a file?<br>
><br>
> Either a subdirectory or the root of the filesystem (where it's mounted.)<br>
><br>
><br>
> On Wed, Aug 5, 2015 at 2:31 PM, Gabriel Guimaraes<br>
> <<a href="mailto:gabriellimaguimaraes@gmail.com" target="_blank">gabriellimaguimaraes@gmail.com</a>> wrote:<br>
>> Hi Filipe,<br>
>><br>
>> I just checked the value of p->fs_type when referring to /tmp/file within a<br>
>> Docker container, using overlayFS. It reports<br>
>><br>
>> p->fs_type = 0xef53 which I believe is the magic number of ext2/3/4<br>
>><br>
>> 2015-08-05 14:25 GMT-07:00 Filipe Brandenburger <<a href="mailto:filbranden@google.com" target="_blank">filbranden@google.com</a>>:<br>
>>><br>
>>> Hi Gabriel,<br>
>>><br>
>>> Looking at the code, I believe it f_type in statfs() output will<br>
>>> return this value for overlayfs:<br>
>>><br>
>>> fs/overlayfs/super.c:#define OVERLAYFS_SUPER_MAGIC 0x794c7630<br>
>>><br>
>>> You can easily double check that with a test program running it<br>
>>> against a mounted overlayfs.<br>
>>><br>
>>> Cheers,<br>
>>> Filipe<br>
>>><br>
>>><br>
>>> On Wed, Aug 5, 2015 at 2:19 PM, Gabriel Guimaraes<br>
>>> <<a href="mailto:gabriellimaguimaraes@gmail.com" target="_blank">gabriellimaguimaraes@gmail.com</a>> wrote:<br>
>>> ><br>
>>> ><br>
>>> > 2015-08-05 13:36 GMT-07:00 Pavel Emelyanov <<a href="mailto:xemul@parallels.com" target="_blank">xemul@parallels.com</a>>:<br>
>>> >><br>
>>> >> On 08/05/2015 08:51 PM, Gabriel Guimaraes wrote:<br>
>>> >> > Hi Pavel,<br>
>>> >> ><br>
>>> >> > Thanks for the review.<br>
>>> >> ><br>
>>> >> > On Wed, Aug 5, 2015 at 4:00 AM, Pavel Emelyanov <<a href="mailto:xemul@parallels.com" target="_blank">xemul@parallels.com</a><br>
>>> >> > <mailto:<a href="mailto:xemul@parallels.com" target="_blank">xemul@parallels.com</a>>> wrote:<br>
>>> >> ><br>
>>> >> > Gabriel,<br>
>>> >> ><br>
>>> >> > Please, find my comments inline.<br>
>>> >> ><br>
>>> >> > > diff --git a/crtools.c b/crtools.c<br>
>>> >> > > index 6af6080..521d0ff 100644<br>
>>> >> > > --- a/crtools.c<br>
>>> >> > > +++ b/crtools.c<br>
>>> >> > > @@ -235,6 +235,7 @@ int main(int argc, char *argv[], char<br>
>>> >> > *envp[])<br>
>>> >> > > { "enable-fs",<br>
>>> >> > required_argument,<br>
>>> >> > 0, 1065 },<br>
>>> >> > > { "enable-external-sharing", no_argument,<br>
>>> >> > 0, 1066 },<br>
>>> >> > > { "enable-external-masters", no_argument,<br>
>>> >> > 0, 1067 },<br>
>>> >> > > + { "overlayfs", no_argument,<br>
>>> >> > 0, 1068 },<br>
>>> >> ><br>
>>> >> > With AUFS workaround we ended up with auto-detection scheme --<br>
>>> >> > when<br>
>>> >> > meeting an AUFS mount the opts.aufs is set to true. Can we do the<br>
>>> >> > same with overlayfs?<br>
>>> >> ><br>
>>> >> ><br>
>>> >> > Technically we could always enable overlayfs by default, since we<br>
>>> >> > only<br>
>>> >> > do something if there<br>
>>> >> > is an overlayfs mounted entry in the mountinfo table. Should I enable<br>
>>> >> > it<br>
>>> >> > always?<br>
>>> >><br>
>>> >> In the fill_fdlink() there's fd_parms structure with fs_type field<br>
>>> >> filled<br>
>>> >> from<br>
>>> >> statfs() syscall. If we check for this value being overlayfs magic, we<br>
>>> >> can<br>
>>> >> silently<br>
>>> >> imply the workaround is on and go calling fixup_overlayfs().<br>
>>> >><br>
>>> > Per man statfs, I don't think Overlayfs has a magic number. OverlayFS<br>
>>> > just<br>
>>> > overlays two filesystems on top of each other. From statfs' point of<br>
>>> > view, a<br>
>>> > file's filesystem type is the original one, not OverlayFS.<br>
>>> ><br>
>>> > More information here:<br>
>>> > <a href="https://www.kernel.org/doc/Documentation/filesystems/overlayfs.txt" rel="noreferrer" target="_blank">https://www.kernel.org/doc/Documentation/filesystems/overlayfs.txt</a><br>
>>> >><br>
>>> >> > > +static struct mount_info *__lookup_overlayfs(struct mount_info<br>
>>> >> > *list, char *rpath,<br>
>>> >> > > + unsigned int<br>
>>> >> > st_dev,<br>
>>> >> > unsigned int st_ino,<br>
>>> >> > > + unsigned int<br>
>>> >> > mnt_id)<br>
>>> >> > > +{<br>
>>> >> > > + /*<br>
>>> >> > > + * Goes through all entries in the mountinfo table<br>
>>> >> > > + * looking for a mount point that contains the file<br>
>>> >> > specified<br>
>>> >> > > + * in rpath. Uses the device number st_dev and the inode<br>
>>> >> > number st_ino<br>
>>> >> > > + * to make sure the file is correct.<br>
>>> >> > > + */<br>
>>> >> > > + struct mount_info *mi_ret = NULL;<br>
>>> >> > > + struct mount_info *m;<br>
>>> >> > > +<br>
>>> >> > > + for (m = list; m != NULL; m = m->next) {<br>
>>> >> > > + /* Case 1: If mnt_id is correct, don't do<br>
>>> >> > anything<br>
>>> >> > */<br>
>>> >> > > + if (st_dev == m->s_dev && mnt_id == m->mnt_id)<br>
>>> >> > > + return NULL;<br>
>>> >> ><br>
>>> >> > Shouldn't this if () be under the next one, so that everything<br>
>>> >> > that<br>
>>> >> > happens<br>
>>> >> > here applies to overlayfs only?<br>
>>> >> ><br>
>>> >> ><br>
>>> >> > This if () is basically a check to see if the bug exists. We look for<br>
>>> >> > all instances of the device<br>
>>> >> > st_dev and see if any of them match our mnt_id. If that's not the<br>
>>> >> > case,<br>
>>> >> > the bug exists and we want<br>
>>> >> > to proceed to try to fix it.<br>
>>> >><br>
>>> >> OK<br>
>>> >><br>
>>> >> > I could have left this as a separate for loop to make this more<br>
>>> >> > clear,<br>
>>> >> > but this way we only use<br>
>>> >> > one for loop for both. I believe it's better if we separate it into<br>
>>> >> > two<br>
>>> >> > for loops since we prevent<br>
>>> >> > some unnecessary stating. I can also move this part to<br>
>>> >> > lookup_overlayfs,<br>
>>> >> > before dealing with<br>
>>> >> > namespaces. What do you think?<br>
>>> >><br>
>>> >> Yup, makes sense to me.<br>
>>> >><br>
>>> ><br>
>><br>
>><br>
</div></div></blockquote></div><br></div>
</div></div></blockquote></div><br></div>