[PATCH] overlayfs: Make dir inode always point to the underlay

[PATCH] overlayfs: Make dir inode always point to the underlay

[oc]

Since file inode points to the underlay from ovl_path_real method, the dir
inode should also point to the one from underlay. This issue introduced
problem when live migrates a container instance using criu, which need to
convert inotify inode entry to file path, because inotify point to the real
inode in underlay.

Signed-off-by: Chen Haiquan <oc@yunify.com>
---
  fs/overlayfs/dir.c | 3 ---
  1 file changed, 3 deletions(-)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 12bcd07b9e32..a06f23d325ec 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -147,9 +147,6 @@ static int ovl_dir_getattr(struct vfsmount *mnt, 
struct dentry *dentry,
         if (err)
                 return err;

-       stat->dev = dentry->d_sb->s_dev;
-       stat->ino = dentry->d_inode->i_ino;
-
         /*
          * It's probably not worth it to count subdirs to get the
          * correct link count.  nlink=1 seems to pacify 'find' and
-- 
2.7.4

Re: [PATCH] overlayfs: Make dir inode always point to the underlay

[Miklos Szeredi]

[This is *far* from being a trivial patch,  please don't treat it as such]

On Fri, Aug 12, 2016 at 03:34:51PM +0800, oc wrote:
> Since file inode points to the underlay from ovl_path_real method, the dir
> inode should also point to the one from underlay. This issue introduced
> problem when live migrates a container instance using criu, which need to
> convert inotify inode entry to file path, because inotify point to the real
> inode in underlay.

Not quite sure what the problem is here.  It is true that inotify is currently
broken.  There's a fix pending for that here:

  https://git.kernel.org/cgit/linux/kernel/git/mszeredi/vfs.git/commit/?h=overlayfs-next&id=09557932babf3462125749212bc1cde574404fbb

There's also an issue with inode numbers being unstable for directories:

  http://marc.info/?t=146914417200002&r=1&w=2

As explained in that thread the problem with always using the underlying
inode/dev is that it makes the inode number change when the directory is copied
up.

So there are multiple, somewhat conflicting requirements here and it would be
good to have a solution that everyone is happy with and is unlikely to break
existing setups.

Does the last patch in the above thread work for you?

And what about if you additionally do

-stat->dev = lowerstat.dev;
+stat->dev = dentry->d_sb->s_dev;

?

Vito, could you also please supply feedback on those patches?

Thanks,
Miklos

Re: [PATCH] overlayfs: Make dir inode always point to the underlay

[Vito Caputo]

Miklos,

Sure, I think the approach of returning the lower layer's ino/dev
combination for merged directories should be ok?

I guess the one somewhat unexpected behavior/potential down side is
st_dev spuriously being different within an overlayfs mount point,
things like `find -xdev` or `du -x` won't recurse into directories on
the upper layer.  The advantage of the copying up the directory on
getattr was it gave all the directories in the overlay unique inode
numbers within a single device.

Regards,
Vito Caputo

On Mon, Aug 15, 2016 at 2:08 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
> [This is *far* from being a trivial patch,  please don't treat it as such]
>
> On Fri, Aug 12, 2016 at 03:34:51PM +0800, oc wrote:
>> Since file inode points to the underlay from ovl_path_real method, the dir
>> inode should also point to the one from underlay. This issue introduced
>> problem when live migrates a container instance using criu, which need to
>> convert inotify inode entry to file path, because inotify point to the real
>> inode in underlay.
>
> Not quite sure what the problem is here.  It is true that inotify is currently
> broken.  There's a fix pending for that here:
>
>   https://git.kernel.org/cgit/linux/kernel/git/mszeredi/vfs.git/commit/?h=overlayfs-next&id=09557932babf3462125749212bc1cde574404fbb
>
> There's also an issue with inode numbers being unstable for directories:
>
>   http://marc.info/?t=146914417200002&r=1&w=2
>
> As explained in that thread the problem with always using the underlying
> inode/dev is that it makes the inode number change when the directory is copied
> up.
>
> So there are multiple, somewhat conflicting requirements here and it would be
> good to have a solution that everyone is happy with and is unlikely to break
> existing setups.
>
> Does the last patch in the above thread work for you?
>
> And what about if you additionally do
>
> -stat->dev = lowerstat.dev;
> +stat->dev = dentry->d_sb->s_dev;
>
> ?
>
> Vito, could you also please supply feedback on those patches?
>
> Thanks,
> Miklos

Re: [PATCH] overlayfs: Make dir inode always point to the underlay

[oc]

Hi Vito,

The approach of returning the lower layer's ino/dev combination for 
merged directories is not OK for
inotify on overlayfs in the scenario of criu dump.

I am working on solving problem when live migrating container on 
overlayfs, and it may also affect
to your case:
When live migrating container, either lxc or docker, it use criu to dump 
the process.
If the process has inotify entry (with the pending fix 0955793 in Miklos 
mail), criu need to find the
file path to inode of inofity entry from scanning files to compare inode 
in order to get the file path. It
is described in https://criu.org/Irmap. Since inofity entry's inode is 
the one for real directory, probably the
the upper layer, overlayfs need to return the real inode to get criu 
working.

It  is a conflicting requirements to you. What do you think on this?


在 2016年08月16日 08:14, Vito Caputo 写道:
> Miklos,
>
> Sure, I think the approach of returning the lower layer's ino/dev
> combination for merged directories should be ok?
>
> I guess the one somewhat unexpected behavior/potential down side is
> st_dev spuriously being different within an overlayfs mount point,
> things like `find -xdev` or `du -x` won't recurse into directories on
> the upper layer.  The advantage of the copying up the directory on
> getattr was it gave all the directories in the overlay unique inode
> numbers within a single device.
>
> Regards,
> Vito Caputo
>
> On Mon, Aug 15, 2016 at 2:08 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>> [This is *far* from being a trivial patch,  please don't treat it as such]
>>
>> On Fri, Aug 12, 2016 at 03:34:51PM +0800, oc wrote:
>>> Since file inode points to the underlay from ovl_path_real method, the dir
>>> inode should also point to the one from underlay. This issue introduced
>>> problem when live migrates a container instance using criu, which need to
>>> convert inotify inode entry to file path, because inotify point to the real
>>> inode in underlay.
>> Not quite sure what the problem is here.  It is true that inotify is currently
>> broken.  There's a fix pending for that here:
>>
>>    https://git.kernel.org/cgit/linux/kernel/git/mszeredi/vfs.git/commit/?h=overlayfs-next&id=09557932babf3462125749212bc1cde574404fbb
>>
>> There's also an issue with inode numbers being unstable for directories:
>>
>>    http://marc.info/?t=146914417200002&r=1&w=2
>>
>> As explained in that thread the problem with always using the underlying
>> inode/dev is that it makes the inode number change when the directory is copied
>> up.
>>
>> So there are multiple, somewhat conflicting requirements here and it would be
>> good to have a solution that everyone is happy with and is unlikely to break
>> existing setups.
>>
>> Does the last patch in the above thread work for you?
>>
>> And what about if you additionally do
>>
>> -stat->dev = lowerstat.dev;
>> +stat->dev = dentry->d_sb->s_dev;
>>
>> ?
>>
>> Vito, could you also please supply feedback on those patches?
>>
>> Thanks,
>> Miklos

Re: [PATCH] overlayfs: Make dir inode always point to the underlay

[oc]

Hi Vito,

The approach of returning the lower layer's ino/dev combination for 
merged directories is not OK for
inotify on overlayfs in the scenario of criu dump.

I am working on solving problem when live migrating container on 
overlayfs, and it may also affect
to your case:
When live migrating container, either lxc or docker, it use criu to dump 
the process.
If the process has inotify entry (with the pending fix 0955793 in Miklos 
mail), criu need to find the
file path to inode of inofity entry from scanning files to compare inode 
in order to get the file path. It
is described in https://criu.org/Irmap. Since inofity entry's inode is 
the one for real directory, probably the
the upper layer, overlayfs need to return the real inode to get criu 
working.

It  is a conflicting requirement to you. What do you think on this?


在 2016年08月16日 08:14, Vito Caputo 写道:
> Miklos,
>
> Sure, I think the approach of returning the lower layer's ino/dev
> combination for merged directories should be ok?
>
> I guess the one somewhat unexpected behavior/potential down side is
> st_dev spuriously being different within an overlayfs mount point,
> things like `find -xdev` or `du -x` won't recurse into directories on
> the upper layer.  The advantage of the copying up the directory on
> getattr was it gave all the directories in the overlay unique inode
> numbers within a single device.
>
> Regards,
> Vito Caputo
>
> On Mon, Aug 15, 2016 at 2:08 AM, Miklos Szeredi <miklos@szeredi.hu> wrote:
>> [This is *far* from being a trivial patch,  please don't treat it as such]
>>
>> On Fri, Aug 12, 2016 at 03:34:51PM +0800, oc wrote:
>>> Since file inode points to the underlay from ovl_path_real method, the dir
>>> inode should also point to the one from underlay. This issue introduced
>>> problem when live migrates a container instance using criu, which need to
>>> convert inotify inode entry to file path, because inotify point to the real
>>> inode in underlay.
>> Not quite sure what the problem is here.  It is true that inotify is currently
>> broken.  There's a fix pending for that here:
>>
>>    https://git.kernel.org/cgit/linux/kernel/git/mszeredi/vfs.git/commit/?h=overlayfs-next&id=09557932babf3462125749212bc1cde574404fbb
>>
>> There's also an issue with inode numbers being unstable for directories:
>>
>>    http://marc.info/?t=146914417200002&r=1&w=2
>>
>> As explained in that thread the problem with always using the underlying
>> inode/dev is that it makes the inode number change when the directory is copied
>> up.
>>
>> So there are multiple, somewhat conflicting requirements here and it would be
>> good to have a solution that everyone is happy with and is unlikely to break
>> existing setups.
>>
>> Does the last patch in the above thread work for you?
>>
>> And what about if you additionally do
>>
>> -stat->dev = lowerstat.dev;
>> +stat->dev = dentry->d_sb->s_dev;
>>
>> ?
>>
>> Vito, could you also please supply feedback on those patches?
>>
>> Thanks,
>> Miklos

Re: [PATCH] overlayfs: Make dir inode always point to the underlay

[Miklos Szeredi]

On Tue, Aug 16, 2016 at 5:43 AM, oc <oc@yunify.com> wrote:
> Hi Vito,

[Please refrain from top posting, restored]

> 在 2016年08月16日 08:14, Vito Caputo 写道:
>>
>> Miklos,
>>
>> Sure, I think the approach of returning the lower layer's ino/dev
>> combination for merged directories should be ok?
>>
>> I guess the one somewhat unexpected behavior/potential down side is
>> st_dev spuriously being different within an overlayfs mount point,
>> things like `find -xdev` or `du -x` won't recurse into directories on
>> the upper layer.

Argh, true.

>>  The advantage of the copying up the directory on
>> getattr was it gave all the directories in the overlay unique inode
>> numbers within a single device.

Yes, directory copy-up on getattr (or lookup) may be the best overall
solution.  I'd do that optionally, and leave copy-up on write the
default as it seems to be OK for most people.

> I am working on solving problem when live migrating container on overlayfs,
> and it may also affect
> to your case:
> When live migrating container, either lxc or docker, it use criu to dump the
> process.
> If the process has inotify entry (with the pending fix 0955793 in Miklos
> mail), criu need to find the
> file path to inode of inofity entry from scanning files to compare inode in
> order to get the file path. It
> is described in https://criu.org/Irmap. Since inofity entry's inode is the
> one for real directory, probably the
> the upper layer, overlayfs need to return the real inode to get criu
> working.

I think overlayfs needs sb->s_export_op filled in.  Together with the
overlayfs fsnotify fix that can ensure that fdinfo will contain a
fhandle consistent with what stat(2) returns, whathever it is.  And
AFAICS that is what lrmap needs to get from a notify handle to a path.

Thanks,
Miklos