overlayfs: Delayed copy up of data

This patch modifies ovl_lookup() and friends to lookup metacopy dentries. It also allows for presence of metacopy dentries in lower layer.

During lookup, check for presence of OVL_XATTR_METACOPY and if not present, set OVL_UPPERDATA bit in flags.

We don't support metacopy feature with nfs_export. So in nfs_export code, we set OVL_UPPERDATA flag set unconditionally if upper inode exists.

Do not follow metacopy origin if we find a metacopy only inode and metacopy feature is not enabled for that mount. Like redirect, this can have security implications where an attacker could hand craft upper and try to gain access to file on lower which it should not have to begin with.

Signed-off-by Vivek Goyal <[email protected]>

---
 fs/overlayfs/export.c    |   3 ++
 fs/overlayfs/inode.c     |  11 ++++-
 fs/overlayfs/namei.c     | 102 ++++++++++++++++++++++++++++++++++++++++-------
 fs/overlayfs/overlayfs.h |   1 +
 fs/overlayfs/util.c      |  22 ++++++++++
 5 files changed, 124 insertions(+), 15 deletions(-)

fs/overlayfs/export.c +3 -0

index 5b7fee1a81ec..e1f6546b6c84 100644
@@ -311,6 +311,9 @@ static struct dentry *ovl_obtain_alias(struct super_block *sb,
                return ERR_CAST(inode);
        }
 
+       if (upper)
+               ovl_set_flag(OVL_UPPERDATA, inode);
+
        dentry = d_find_any_alias(inode);
        if (!dentry) {
                dentry = d_alloc_anon(inode->i_sb);

fs/overlayfs/inode.c +1 -1

index 59b1c84076ec..40509b7a50d2 100644
@@ -776,7 +776,7 @@ struct inode *ovl_get_inode(struct super_block *sb, struct dentry *upperdentry,
        struct dentry *lowerdentry = lowerpath ? lowerpath->dentry : NULL;
        bool bylower = ovl_hash_bylower(sb, upperdentry, lowerdentry, index);
        int fsid = bylower ? lowerpath->layer->fsid : 0;
-       bool is_dir;
+       bool is_dir, metacopy = false;
        unsigned long ino = 0;
        int err = -ENOMEM;
 

@@ -836,6 +836,15 @@ struct inode *ovl_get_inode(struct super_block *sb, struct dentry *upperdentry,
        if (index)
                ovl_set_flag(OVL_INDEX, inode);

Quoted 9 lines · Vivek Goyal

This patch modifies ovl_lookup() and friends to lookup metacopy dentries. It also allows for presence of metacopy dentries in lower layer.

During lookup, check for presence of OVL_XATTR_METACOPY and if not present, set OVL_UPPERDATA bit in flags.

We don't support metacopy feature with nfs_export. So in nfs_export code, we set OVL_UPPERDATA flag set unconditionally if upper inode exists.

Quoted 6 lines

Do not follow metacopy origin if we find a metacopy only inode and metacopy feature is not enabled for that mount. Like redirect, this can have security implications where an attacker could hand craft upper and try to gain access to file on lower which it should not have to begin with.

Signed-off-by Vivek Goyal <[email protected]>

Reviewed-by Amir Goldstein <[email protected]>

Except for one comment below...

Quoted 39 lines

---
 fs/overlayfs/export.c    |   3 ++
 fs/overlayfs/inode.c     |  11 ++++-
 fs/overlayfs/namei.c     | 102 ++++++++++++++++++++++++++++++++++++++++-------
 fs/overlayfs/overlayfs.h |   1 +
 fs/overlayfs/util.c      |  22 ++++++++++
 5 files changed, 124 insertions(+), 15 deletions(-)

fs/overlayfs/export.c +3 -0

index 5b7fee1a81ec..e1f6546b6c84 100644
@@ -311,6 +311,9 @@ static struct dentry *ovl_obtain_alias(struct super_block *sb,
                return ERR_CAST(inode);
        }
 
+       if (upper)
+               ovl_set_flag(OVL_UPPERDATA, inode);
+
        dentry = d_find_any_alias(inode);
        if (!dentry) {
                dentry = d_alloc_anon(inode->i_sb);

fs/overlayfs/inode.c +1 -1

index 59b1c84076ec..40509b7a50d2 100644
@@ -776,7 +776,7 @@ struct inode *ovl_get_inode(struct super_block *sb, struct dentry *upperdentry,
        struct dentry *lowerdentry = lowerpath ? lowerpath->dentry : NULL;
        bool bylower = ovl_hash_bylower(sb, upperdentry, lowerdentry, index);
        int fsid = bylower ? lowerpath->layer->fsid : 0;
-       bool is_dir;
+       bool is_dir, metacopy = false;
        unsigned long ino = 0;
        int err = -ENOMEM;
 

@@ -836,6 +836,15 @@ struct inode *ovl_get_inode(struct super_block *sb, struct dentry *upperdentry,
        if (index)
                ovl_set_flag(OVL_INDEX, inode);
 

fs/overlayfs/inode.c struct inode *ovl_get_inode(struct super_block *sb, struct dentry *upperdentry, · line 847

Quoted 9 lines

fs/overlayfs/inode.c +9 -0

+       if (upperdentry) {
+               err = ovl_check_metacopy_xattr(upperdentry);
+               if (err < 0)
+                       goto out_err;
+               metacopy = err;
+               if (!metacopy)
+                       ovl_set_flag(OVL_UPPERDATA, inode);
+       }
+

There is no reason to ovl_check_metacopy_xattr again here, right?

so it should get the same treatment as upperredirect.
This would make 3 new arguments to ovl_get_inode added by your patch set.

How about initializing this struct during lookup and passing it into ovl_get_inode()?:

struct ovl_inode_info {
        struct ovl_dir_cache *cache;
        const char *redirect;
        u64 version;
        unsigned long flags;
        struct dentry *__upperdentry;
        struct inode *lower;
};

struct ovl_inode {
        struct ovl_inode_info info;
        struct inode vfs_inode;

        /* synchronize copy up and more */
        struct mutex lock;
};

static inline struct ovl_inode_info *OVL_I_INFO(struct inode *inode)

{
        return &OVL_I(inode)->info;
}

Thanks,
Amir.

Quoted 60 lines · Amir Goldstein

Quoted 9 lines · Vivek Goyal

This patch modifies ovl_lookup() and friends to lookup metacopy dentries. It also allows for presence of metacopy dentries in lower layer.

During lookup, check for presence of OVL_XATTR_METACOPY and if not present, set OVL_UPPERDATA bit in flags.

We don't support metacopy feature with nfs_export. So in nfs_export code, we set OVL_UPPERDATA flag set unconditionally if upper inode exists.

Quoted 6 lines

Do not follow metacopy origin if we find a metacopy only inode and metacopy feature is not enabled for that mount. Like redirect, this can have security implications where an attacker could hand craft upper and try to gain access to file on lower which it should not have to begin with.

Signed-off-by Vivek Goyal <[email protected]>

Reviewed-by Amir Goldstein <[email protected]>

Except for one comment below...

Quoted 39 lines

---
 fs/overlayfs/export.c    |   3 ++
 fs/overlayfs/inode.c     |  11 ++++-
 fs/overlayfs/namei.c     | 102 ++++++++++++++++++++++++++++++++++++++++-------
 fs/overlayfs/overlayfs.h |   1 +
 fs/overlayfs/util.c      |  22 ++++++++++
 5 files changed, 124 insertions(+), 15 deletions(-)

fs/overlayfs/export.c +3 -0

index 5b7fee1a81ec..e1f6546b6c84 100644
@@ -311,6 +311,9 @@ static struct dentry *ovl_obtain_alias(struct super_block *sb,
                return ERR_CAST(inode);
        }
 
+       if (upper)
+               ovl_set_flag(OVL_UPPERDATA, inode);
+
        dentry = d_find_any_alias(inode);
        if (!dentry) {
                dentry = d_alloc_anon(inode->i_sb);

fs/overlayfs/inode.c +1 -1

index 59b1c84076ec..40509b7a50d2 100644
@@ -776,7 +776,7 @@ struct inode *ovl_get_inode(struct super_block *sb, struct dentry *upperdentry,
        struct dentry *lowerdentry = lowerpath ? lowerpath->dentry : NULL;
        bool bylower = ovl_hash_bylower(sb, upperdentry, lowerdentry, index);
        int fsid = bylower ? lowerpath->layer->fsid : 0;
-       bool is_dir;
+       bool is_dir, metacopy = false;
        unsigned long ino = 0;
        int err = -ENOMEM;
 

@@ -836,6 +836,15 @@ struct inode *ovl_get_inode(struct super_block *sb, struct dentry *upperdentry,
        if (index)
                ovl_set_flag(OVL_INDEX, inode);
 

Quoted 11 lines

fs/overlayfs/inode.c struct inode *ovl_get_inode(struct super_block *sb, struct dentry *upperdentry, · line 847

Quoted 9 lines

fs/overlayfs/inode.c +9 -0

+       if (upperdentry) {
+               err = ovl_check_metacopy_xattr(upperdentry);
+               if (err < 0)
+                       goto out_err;
+               metacopy = err;
+               if (!metacopy)
+                       ovl_set_flag(OVL_UPPERDATA, inode);
+       }
+

There is no reason to ovl_check_metacopy_xattr again here, right?

I think we need to check metacopy here otherwise it becomes racy. For example, what if there is a hard link (say, l1 and l2) with metacopy xattr. ovl_lookup(l1) will think metacopy is on while another thread on another cpu might have trigged copy up, remove metacopy xattr. And it is possible that inode got flushed out of cache. So by the time ovl_lookup(l1), calls iget5_locked(), it will get a new inode and it will initialize inode with wrong information.

I had done similar thing for REDIRECT, but once we removed logic to remove REDIRECT on copy up, I felt I did not have to check redirect again here.

In general, I feel that once we have the inode lock, we should check metacopy and redirect both and then initialize inode. And not rely on information which was checked outside the lock and might have been stale by now.

On Mon, Apr 30, 2018 at 5:02 PM, Vivek Goyal <[email protected]> wrote:

Quoted 9 lines

I think we need to check metacopy here otherwise it becomes racy. For example, what if there is a hard link (say, l1 and l2) with metacopy xattr. ovl_lookup(l1) will think metacopy is on while another thread on another cpu might have trigged copy up, remove metacopy xattr. And it is possible that inode got flushed out of cache. So by the time ovl_lookup(l1), calls iget5_locked(), it will get a new inode and it will initialize inode with wrong information.

Quoted 9 lines

I had done similar thing for REDIRECT, but once we removed logic to remove REDIRECT on copy up, I felt I did not have to check redirect again here.

In general, I feel that once we have the inode lock, we should check metacopy and redirect both and then initialize inode. And not rely on information which was checked outside the lock and might have been stale by now.

Hmmm... so as you once already said, we have a race with INDEX as well. In general, I feel it would be better to do a minimal sanity check inside inode lock for INDEX and METACOPY (maybe REDIRECT) and make sure they are consistent with the info in ovl_inode_info.
If they are not, return -ESTALE and that may result in re-calling lookup.

Quoted 1 line · Amir Goldstein

On Mon, Apr 30, 2018 at 5:02 PM, Vivek Goyal <[email protected]> wrote:

[...]

Quoted 19 lines

Quoted 9 lines

I think we need to check metacopy here otherwise it becomes racy. For example, what if there is a hard link (say, l1 and l2) with metacopy xattr. ovl_lookup(l1) will think metacopy is on while another thread on another cpu might have trigged copy up, remove metacopy xattr. And it is possible that inode got flushed out of cache. So by the time ovl_lookup(l1), calls iget5_locked(), it will get a new inode and it will initialize inode with wrong information.

Quoted 9 lines

I had done similar thing for REDIRECT, but once we removed logic to remove REDIRECT on copy up, I felt I did not have to check redirect again here.

In general, I feel that once we have the inode lock, we should check metacopy and redirect both and then initialize inode. And not rely on information which was checked outside the lock and might have been stale by now.

Quoted 7 lines

Hmmm... so as you once already said, we have a race with INDEX as well. In general, I feel it would be better to do a minimal sanity check inside inode lock for INDEX and METACOPY (maybe REDIRECT) and make sure they are consistent with the info in ovl_inode_info.
If they are not, return -ESTALE and that may result in re-calling lookup.

After thinking about it some more, I believe the way we should do it is implement ovl_iget_locked() and call it from ovl_lookup() *before* we lookup index.

Then, *after* looking up index,
If this turns out to be a broken hardlink, we throw away the inode that we got by lower and get a new inode hashed by upper.

Then we can initialize the inode with another helper or directly in ovl_lookup() and unlock_new_inode().

w.r.t METACOPY things will get complicated if we remove METACOPY xattr and the upper does not have ORIGIN.

In that case we may have an inode that is already hashed by lower, but for a new lookup that does not see METACOPY xattr it looks up the inode in cache by upper and this is not good.

There is one solution that you will surely not like - instead of removing METACOPY xattr set METACOPY=n.
That means we still need to lookup lower layers for origin inode, but we do set UPPERDATA flag.

There may be other solutions to the problem, but I cannot think of any right now.

Thanks,
Amir.

Quoted 38 lines · Amir Goldstein

Quoted 1 line · Amir Goldstein

On Mon, Apr 30, 2018 at 5:02 PM, Vivek Goyal <[email protected]> wrote:

[...]

Quoted 19 lines

Quoted 9 lines

I think we need to check metacopy here otherwise it becomes racy. For example, what if there is a hard link (say, l1 and l2) with metacopy xattr. ovl_lookup(l1) will think metacopy is on while another thread on another cpu might have trigged copy up, remove metacopy xattr. And it is possible that inode got flushed out of cache. So by the time ovl_lookup(l1), calls iget5_locked(), it will get a new inode and it will initialize inode with wrong information.

Quoted 9 lines

I had done similar thing for REDIRECT, but once we removed logic to remove REDIRECT on copy up, I felt I did not have to check redirect again here.

In general, I feel that once we have the inode lock, we should check metacopy and redirect both and then initialize inode. And not rely on information which was checked outside the lock and might have been stale by now.

Quoted 7 lines

Hmmm... so as you once already said, we have a race with INDEX as well. In general, I feel it would be better to do a minimal sanity check inside inode lock for INDEX and METACOPY (maybe REDIRECT) and make sure they are consistent with the info in ovl_inode_info.
If they are not, return -ESTALE and that may result in re-calling lookup.

After thinking about it some more, I believe the way we should do it is implement ovl_iget_locked() and call it from ovl_lookup() *before* we lookup index.

Then, *after* looking up index,
If this turns out to be a broken hardlink, we throw away the inode that we got by lower and get a new inode hashed by upper.

Quoted 5 lines

Then we can initialize the inode with another helper or directly in ovl_lookup() and unlock_new_inode().

w.r.t METACOPY things will get complicated if we remove METACOPY xattr and the upper does not have ORIGIN.

I am not able to understand the problem you are referring to.

Quoted 3 lines

In that case we may have an inode that is already hashed by lower, but for a new lookup that does not see METACOPY xattr it looks up the inode in cache by upper and this is not good.

Here is my understanding. Following happens without metacopy feature.

- As of now, only time we do not create ORIGIN on upper is for the case of broken hardlink. (for a non-dir file).

- In case of broken hard link (index=off), if file is on lower only, we do not put new inode in inode cache at all. Once file gets copied up, we hash it by upper inode. And reported inode number changes over copy up.

Now above behavior should remain same even with metacopy=on. For the case of broken hard link.

- If file is on lower only, we will not put inode in cache. And once file gets copied up metadata only, we will hash it with upper inode.

The only difference here is that without metacopy non-dir file will have lowerdentry=NULL while with metacopy on, lowerdentry will be non-null. But bylower should still be false because of following check in ovl_hash_bylower().

  /* No, if lower hardlink is or will be broken on copy up */
   if ((upper || !ovl_indexdir(sb)) &&
            !d_is_dir(lower) && d_inode(lower)->i_nlink > 1)
                return false;

And if bylower is false, we hash inode using upper inode as key.

struct inode *key = d_inode(bylower ? lowerdentry :
upperdentry);

IOW, despite the fact lowerdentry is there (without ORIGIN on upper), we don't use that lowerdentry for inode hashing. And hence behavior should not change.

What am I missing?

Quoted 1 line · Vivek Goyal

On Tue, May 01, 2018 at 05:37:09PM +0300, Amir Goldstein wrote:

Quoted 45 lines

Quoted 3 lines

w.r.t METACOPY things will get complicated if we remove METACOPY xattr and the upper does not have ORIGIN.

I am not able to understand the problem you are referring to.

Quoted 3 lines

In that case we may have an inode that is already hashed by lower, but for a new lookup that does not see METACOPY xattr it looks up the inode in cache by upper and this is not good.

Here is my understanding. Following happens without metacopy feature.

- As of now, only time we do not create ORIGIN on upper is for the case of broken hardlink. (for a non-dir file).

- In case of broken hard link (index=off), if file is on lower only, we do not put new inode in inode cache at all. Once file gets copied up, we hash it by upper inode. And reported inode number changes over copy up.

Now above behavior should remain same even with metacopy=on. For the case of broken hard link.

- If file is on lower only, we will not put inode in cache. And once file gets copied up metadata only, we will hash it with upper inode.

The only difference here is that without metacopy non-dir file will have lowerdentry=NULL while with metacopy on, lowerdentry will be non-null. But bylower should still be false because of following check in ovl_hash_bylower().

  /* No, if lower hardlink is or will be broken on copy up */
   if ((upper || !ovl_indexdir(sb)) &&
            !d_is_dir(lower) && d_inode(lower)->i_nlink > 1)
                return false;

And if bylower is false, we hash inode using upper inode as key.

struct inode *key = d_inode(bylower ? lowerdentry :
upperdentry);

IOW, despite the fact lowerdentry is there (without ORIGIN on upper), we don't use that lowerdentry for inode hashing. And hence behavior should not change.

Quoted 2 lines

What am I missing?

Just maybe missing the case of nlink=1 and ORIGIN cannot be followed because lower fs doesn't support file handles. I just wanted to make sure this case is handled correctly.

Quoted 2 lines · Amir Goldstein

Quoted 1 line · Vivek Goyal

On Tue, May 01, 2018 at 05:37:09PM +0300, Amir Goldstein wrote:

[...]

Quoted 45 lines

Quoted 45 lines

Quoted 3 lines

w.r.t METACOPY things will get complicated if we remove METACOPY xattr and the upper does not have ORIGIN.

I am not able to understand the problem you are referring to.

Quoted 3 lines

In that case we may have an inode that is already hashed by lower, but for a new lookup that does not see METACOPY xattr it looks up the inode in cache by upper and this is not good.

Here is my understanding. Following happens without metacopy feature.

- As of now, only time we do not create ORIGIN on upper is for the case of broken hardlink. (for a non-dir file).

- In case of broken hard link (index=off), if file is on lower only, we do not put new inode in inode cache at all. Once file gets copied up, we hash it by upper inode. And reported inode number changes over copy up.

Now above behavior should remain same even with metacopy=on. For the case of broken hard link.

- If file is on lower only, we will not put inode in cache. And once file gets copied up metadata only, we will hash it with upper inode.

The only difference here is that without metacopy non-dir file will have lowerdentry=NULL while with metacopy on, lowerdentry will be non-null. But bylower should still be false because of following check in ovl_hash_bylower().

  /* No, if lower hardlink is or will be broken on copy up */
   if ((upper || !ovl_indexdir(sb)) &&
            !d_is_dir(lower) && d_inode(lower)->i_nlink > 1)
                return false;

And if bylower is false, we hash inode using upper inode as key.

struct inode *key = d_inode(bylower ? lowerdentry :
upperdentry);

IOW, despite the fact lowerdentry is there (without ORIGIN on upper), we don't use that lowerdentry for inode hashing. And hence behavior should not change.

Quoted 7 lines

Quoted 2 lines

What am I missing?

Just maybe missing the case of nlink=1 and ORIGIN cannot be followed because lower fs doesn't support file handles. I just wanted to make sure this case is handled correctly.

OK. re-read the code and ready to explain the situation.

The meaning of new OVL_CONST_INO flag is:
"inode preserves st_ino across copy up".
If lower fs support file handles it also means:
"...and across eviction from inode cache".
but if lower fs does not support file handles it means:
"...while non-dir inode remains in cache"

How does METACOPY change the situation?
very slightly - with METACOPY, instead of st_ino change after copy_up+cache_evict, st_ino will not change after meta_copy_up+cache_evict, but will change after data_copy_up+cache_evict, which is not a problem as far as I can tell.

Specifically, removing METACOPY xattr will not change st_ino nor hash_by_lower until after cache eviction and as long as we have one-to-one mapping of upper inode to lower inode that's good enough.

Things will only break with filesystem inconsistencies like in xfstest overlay/049, but AFAICT, METACOPY does not introduce any real regressions, the only thing it does is add another way of creating an inconsistency with redirect on non-dir, which probably calls for a new xfstest.

I hope I got it all right.

Thanks,
Amir.

On Fri, May 04, 2018 at 10:04:22AM +0300, Amir Goldstein wrote:

Quoted 22 lines

Quoted 5 lines

Just maybe missing the case of nlink=1 and ORIGIN cannot be followed because lower fs doesn't support file handles. I just wanted to make sure this case is handled correctly.

OK. re-read the code and ready to explain the situation.

The meaning of new OVL_CONST_INO flag is:
"inode preserves st_ino across copy up".
If lower fs support file handles it also means:
"...and across eviction from inode cache".
but if lower fs does not support file handles it means:
"...while non-dir inode remains in cache"

How does METACOPY change the situation?
very slightly - with METACOPY, instead of st_ino change after copy_up+cache_evict, st_ino will not change after meta_copy_up+cache_evict, but will change after data_copy_up+cache_evict, which is not a problem as far as I can tell.

Quoted 4 lines

Specifically, removing METACOPY xattr will not change st_ino nor hash_by_lower until after cache eviction and as long as we have one-to-one mapping of upper inode to lower inode that's good enough.

Hi Amir,

Thanks for detailed explanation. I now understand your concern.

How about I pass in the origin information in ovl_get_inode() and to ovl_hash_bylower(). And we set bylower=1 for a metacopy dentry only if an upper origin could be found and verified?

That way behavior will be similar to a file with nlink=1 and ORIGIN not there. And we will not have to worry about thinking another special corner case and writing test cases for it.

Too many corner cases are bad for future code maintenance and easy to break.

WDYT?

Quoted 28 lines

Quoted 4 lines

Quoted 4 lines

fs/overlayfs/inode.c +0 -0

@@ -836,6 +836,15 @@ struct inode *ovl_get_inode(struct super_block *sb, struct dentry *upperdentry,
        if (index)
                ovl_set_flag(OVL_INDEX, inode);
 

Quoted 11 lines

fs/overlayfs/inode.c struct inode *ovl_get_inode(struct super_block *sb, struct dentry *upperdentry, · line 847

Quoted 9 lines

fs/overlayfs/inode.c +9 -0

+       if (upperdentry) {
+               err = ovl_check_metacopy_xattr(upperdentry);
+               if (err < 0)
+                       goto out_err;
+               metacopy = err;
+               if (!metacopy)
+                       ovl_set_flag(OVL_UPPERDATA, inode);
+       }
+

There is no reason to ovl_check_metacopy_xattr again here, right?

I think we need to check metacopy here otherwise it becomes racy. For example, what if there is a hard link (say, l1 and l2) with metacopy xattr. ovl_lookup(l1) will think metacopy is on while another thread on another cpu might have trigged copy up, remove metacopy xattr. And it is possible that inode got flushed out of cache. So by the time ovl_lookup(l1), calls iget5_locked(), it will get a new inode and it will initialize inode with wrong information.

I had done similar thing for REDIRECT, but once we removed logic to remove REDIRECT on copy up, I felt I did not have to check redirect again here.

Quoted 5 lines

In general, I feel that once we have the inode lock, we should check metacopy and redirect both and then initialize inode. And not rely on information which was checked outside the lock and might have been stale by now.

Hmmm... so as you once already said, we have a race with INDEX as well.

On Mon, Apr 30, 2018 at 09:08:01PM +0300, Amir Goldstein wrote:
[..]

Quoted 34 lines

Quoted 33 lines

Quoted 4 lines

Quoted 4 lines

fs/overlayfs/inode.c +0 -0

@@ -836,6 +836,15 @@ struct inode *ovl_get_inode(struct super_block *sb, struct dentry *upperdentry,
        if (index)
                ovl_set_flag(OVL_INDEX, inode);
 

Quoted 11 lines

fs/overlayfs/inode.c struct inode *ovl_get_inode(struct super_block *sb, struct dentry *upperdentry, · line 847

Quoted 9 lines

fs/overlayfs/inode.c +9 -0

+       if (upperdentry) {
+               err = ovl_check_metacopy_xattr(upperdentry);
+               if (err < 0)
+                       goto out_err;
+               metacopy = err;
+               if (!metacopy)
+                       ovl_set_flag(OVL_UPPERDATA, inode);
+       }
+

There is no reason to ovl_check_metacopy_xattr again here, right?

I think we need to check metacopy here otherwise it becomes racy. For example, what if there is a hard link (say, l1 and l2) with metacopy xattr. ovl_lookup(l1) will think metacopy is on while another thread on another cpu might have trigged copy up, remove metacopy xattr. And it is possible that inode got flushed out of cache. So by the time ovl_lookup(l1), calls iget5_locked(), it will get a new inode and it will initialize inode with wrong information.

I had done similar thing for REDIRECT, but once we removed logic to remove REDIRECT on copy up, I felt I did not have to check redirect again here.

In general, I feel that once we have the inode lock, we should check metacopy and redirect both and then initialize inode. And not rely on information which was checked outside the lock and might have been stale by now.

Quoted 1 line

Hmmm... so as you once already said, we have a race with INDEX as well.

I am not sure about INDEX race. I remember talking about INDEX flag race in ovl_inode but w.r.t ovl_get_inode(), what's the race you are seeing with index. Once ovl_lookup() has found index, can that index go away by the time ovl_get_inode() is called? If not, then there should not be any race.

Quoted 36 lines · Vivek Goyal

On Mon, Apr 30, 2018 at 09:08:01PM +0300, Amir Goldstein wrote:
[..]

Quoted 34 lines

Quoted 33 lines

Quoted 4 lines

Quoted 4 lines

fs/overlayfs/inode.c +0 -0

@@ -836,6 +836,15 @@ struct inode *ovl_get_inode(struct super_block *sb, struct dentry *upperdentry,
        if (index)
                ovl_set_flag(OVL_INDEX, inode);
 

Quoted 11 lines

fs/overlayfs/inode.c struct inode *ovl_get_inode(struct super_block *sb, struct dentry *upperdentry, · line 847

Quoted 9 lines

fs/overlayfs/inode.c +9 -0

+       if (upperdentry) {
+               err = ovl_check_metacopy_xattr(upperdentry);
+               if (err < 0)
+                       goto out_err;
+               metacopy = err;
+               if (!metacopy)
+                       ovl_set_flag(OVL_UPPERDATA, inode);
+       }
+

There is no reason to ovl_check_metacopy_xattr again here, right?

I think we need to check metacopy here otherwise it becomes racy. For example, what if there is a hard link (say, l1 and l2) with metacopy xattr. ovl_lookup(l1) will think metacopy is on while another thread on another cpu might have trigged copy up, remove metacopy xattr. And it is possible that inode got flushed out of cache. So by the time ovl_lookup(l1), calls iget5_locked(), it will get a new inode and it will initialize inode with wrong information.

I had done similar thing for REDIRECT, but once we removed logic to remove REDIRECT on copy up, I felt I did not have to check redirect again here.

In general, I feel that once we have the inode lock, we should check metacopy and redirect both and then initialize inode. And not rely on information which was checked outside the lock and might have been stale by now.

Quoted 7 lines

Quoted 1 line

Hmmm... so as you once already said, we have a race with INDEX as well.

I am not sure about INDEX race. I remember talking about INDEX flag race in ovl_inode but w.r.t ovl_get_inode(), what's the race you are seeing with index. Once ovl_lookup() has found index, can that index go away by the time ovl_get_inode() is called? If not, then there should not be any race.

May be you are referring to race when ovl_lookup() looks for index its not there (all lower), and then thread gets blocked, other cpu triggers a copy up and creates index, flushs ovl_inode and now first thread continues and does ovl_get_inode() and does not set OVL_INDEX flag (Despite the fact there is index)?

Quoted 45 lines · Vivek Goyal

Quoted 43 lines · Vivek Goyal

On Mon, Apr 30, 2018 at 09:08:01PM +0300, Amir Goldstein wrote:
[..]

Quoted 28 lines

Quoted 28 lines

Quoted 4 lines

Quoted 4 lines

(quoted diff) +0 -0

@@ -836,6 +836,15 @@ struct inode *ovl_get_inode(struct super_block *sb, struct dentry *upperdentry,
        if (index)
                ovl_set_flag(OVL_INDEX, inode);
 

Quoted 11 lines

Review struct inode *ovl_get_inode(struct super_block *sb, struct dentry *upperdentry, · line 847

Quoted 9 lines

(quoted diff) +9 -0

+       if (upperdentry) {
+               err = ovl_check_metacopy_xattr(upperdentry);
+               if (err < 0)
+                       goto out_err;
+               metacopy = err;
+               if (!metacopy)
+                       ovl_set_flag(OVL_UPPERDATA, inode);
+       }
+

There is no reason to ovl_check_metacopy_xattr again here, right?

I think we need to check metacopy here otherwise it becomes racy. For example, what if there is a hard link (say, l1 and l2) with metacopy xattr. ovl_lookup(l1) will think metacopy is on while another thread on another cpu might have trigged copy up, remove metacopy xattr. And it is possible that inode got flushed out of cache. So by the time ovl_lookup(l1), calls iget5_locked(), it will get a new inode and it will initialize inode with wrong information.

I had done similar thing for REDIRECT, but once we removed logic to remove REDIRECT on copy up, I felt I did not have to check redirect again here.

Quoted 7 lines

Quoted 5 lines

In general, I feel that once we have the inode lock, we should check metacopy and redirect both and then initialize inode. And not rely on information which was checked outside the lock and might have been stale by now.

Hmmm... so as you once already said, we have a race with INDEX as well.

I am not sure about INDEX race. I remember talking about INDEX flag race in ovl_inode but w.r.t ovl_get_inode(), what's the race you are seeing with index. Once ovl_lookup() has found index, can that index go away by the time ovl_get_inode() is called? If not, then there should not be any race.

Quoted 6 lines

May be you are referring to race when ovl_lookup() looks for index its not there (all lower), and then thread gets blocked, other cpu triggers a copy up and creates index, flushs ovl_inode and now first thread continues and does ovl_get_inode() and does not set OVL_INDEX flag (Despite the fact there is index)?

Yes, that's what I figured when you described the potential METACOPY race.

Besides that I am still worried about corners we missed in setup of metacopy=on,index=off.

I may be confused about whether lower with nlink=1 with index=off after metacopy up uses upper st_ino and hashed by upper inode? Need to understand whether the "hashed by" state is consistent with metacopy=on,index=off in all cases.

Quoted 8 lines

Besides that I am still worried about corners we missed in setup of metacopy=on,index=off.

I may be confused about whether lower with nlink=1 with index=off after metacopy up uses upper st_ino and hashed by upper inode? Need to understand whether the "hashed by" state is consistent with metacopy=on,index=off in all cases.

metacopy=on, index=off should not change inode number reporting. For the case of nlink=1 and index=off, st_ino of lower is reported and metacopy=on/off does not matter at all. Similarly, ovl_hash_bylower() remains untouched due to metacopy. So for the case of nlink=1, index=off, inode should still be hashed using lower inode number.

Only thing metacopy feature changes in stat() is st_blocks. It should not impact st_ino or st_dev reporting at all.

Review struct inode *ovl_get_inode(struct super_block *sb, struct dentry *upperdentry, · line 847

Quoted 13 lines

(quoted diff) +9 -0

@@ -836,6 +836,15 @@ struct inode *ovl_get_inode(struct super_block *sb, struct dentry *upperdentry,
        if (index)
                ovl_set_flag(OVL_INDEX, inode);
 
+       if (upperdentry) {
+               err = ovl_check_metacopy_xattr(upperdentry);
+               if (err < 0)
+                       goto out_err;
+               metacopy = err;
+               if (!metacopy)
+                       ovl_set_flag(OVL_UPPERDATA, inode);
+       }
+

There is no reason to ovl_check_metacopy_xattr again here, right? so it should get the same treatment as upperredirect.
This would make 3 new arguments to ovl_get_inode added by your patch set.

How about initializing this struct during lookup and passing it into ovl_get_inode()?:

On Sat, Apr 28, 2018 at 01:14:24AM -0700, Amir Goldstein wrote:

[..]

Quoted 18 lines

Review struct inode *ovl_get_inode(struct super_block *sb, struct dentry *upperdentry, · line 847

Quoted 13 lines

(quoted diff) +9 -0

@@ -836,6 +836,15 @@ struct inode *ovl_get_inode(struct super_block *sb, struct dentry *upperdentry,
        if (index)
                ovl_set_flag(OVL_INDEX, inode);
 
+       if (upperdentry) {
+               err = ovl_check_metacopy_xattr(upperdentry);
+               if (err < 0)
+                       goto out_err;
+               metacopy = err;
+               if (!metacopy)
+                       ovl_set_flag(OVL_UPPERDATA, inode);
+       }
+

There is no reason to ovl_check_metacopy_xattr again here, right? so it should get the same treatment as upperredirect.
This would make 3 new arguments to ovl_get_inode added by your patch set.

Quoted 2 lines

How about initializing this struct during lookup and passing it into ovl_get_inode()?:

How about just creating another structure to pass in parameters to ovl_get_inode() and then let it extract relevant info and initialize inode. Something like this.

ovl_dentry_real() returns the real dentry. (Either upper or lower). We also need an helper to ignore metacopy dentries and return "real data" dentry. This helper returns an upper/lower dentry which contains data.

Signed-off-by Vivek Goyal <[email protected]>

---
 fs/overlayfs/overlayfs.h |  1 +
 fs/overlayfs/util.c      | 12 ++++++++++++
 2 files changed, 13 insertions(+)

fs/overlayfs/overlayfs.h +1 -0

index 08df75f473fe..5077d2992d07 100644
@@ -230,6 +230,7 @@ struct dentry *ovl_dentry_lower(struct dentry *dentry);
 struct dentry *ovl_dentry_lowerdata(struct dentry *dentry);
 struct ovl_layer *ovl_layer_lower(struct dentry *dentry);
 struct dentry *ovl_dentry_real(struct dentry *dentry);
+struct dentry *ovl_dentry_real_data(struct dentry *dentry);
 struct dentry *ovl_i_dentry_upper(struct inode *inode);
 struct inode *ovl_inode_upper(struct inode *inode);
 struct inode *ovl_inode_lower(struct inode *inode);

fs/overlayfs/util.c +0 -0

index 7029036b9aa3..2bef013ca432 100644

fs/overlayfs/util.c +10 -1

+{
+       struct dentry *upperdentry;
+
+       upperdentry = ovl_dentry_upper(dentry);
+       if (upperdentry && ovl_has_upperdata(d_inode(dentry)))
+               return upperdentry;
+
+       return ovl_dentry_lowerdata(dentry);
+}
+
 struct dentry *ovl_i_dentry_upper(struct inode *inode)
 {
        return ovl_upperdentry_dereference(OVL_I(inode));
--

2.13.6

fs/overlayfs/util.c struct dentry *ovl_dentry_real(struct dentry *dentry) · line 232

Quoted 2 lines

fs/overlayfs/util.c +2 -0

+/* Return real dentry which contains data. Skip metacopy dentries */
+struct dentry *ovl_dentry_real_data(struct dentry *dentry)

The name real_data is a bit inconsistent with _upperdata and _lowerdata. Don't you think _realdata would be more coherent.

Right now ovl_inode stores inode pointer for lower inode. This helps with quickly getting lower inode given overlay inode (ovl_inode_lower()).

Now with metadata only copy-up, we can have metacopy inode in middle layer as well and inode containing data can be different from ->lower. I need to be able to open the real file in ovl_open_realfile() and for that I need to quickly find the lower data inode.

Hence store lower data inode also in ovl_inode. Also provide an helper ovl_inode_lowerdata() to access this field.

Signed-off-by Vivek Goyal <[email protected]>

---
 fs/overlayfs/export.c    |  3 ++-
 fs/overlayfs/inode.c     |  5 +++--
 fs/overlayfs/namei.c     |  5 ++++-
 fs/overlayfs/overlayfs.h |  6 ++++--
 fs/overlayfs/ovl_entry.h |  1 +
 fs/overlayfs/super.c     |  4 +++-
 fs/overlayfs/util.c      | 10 +++++++++-
 7 files changed, 26 insertions(+), 8 deletions(-)

fs/overlayfs/export.c +2 -1

index e1f6546b6c84..d43d926ae68e 100644
@@ -305,7 +305,8 @@ static struct dentry *ovl_obtain_alias(struct super_block *sb,
        if (d_is_dir(upper ?: lower))
                return ERR_PTR(-EIO);
 
-       inode = ovl_get_inode(sb, dget(upper), lowerpath, index, !!lower, NULL);
+       inode = ovl_get_inode(sb, dget(upper), lowerpath, index, !!lower, NULL,
+                             NULL);
        if (IS_ERR(inode)) {
                dput(upper);
                return ERR_CAST(inode);

fs/overlayfs/inode.c +3 -2

index 9d17f15480de..a6cd57741a1f 100644
@@ -802,7 +802,8 @@ static bool ovl_hash_bylower(struct super_block *sb, struct dentry *upper,
 
 struct inode *ovl_get_inode(struct super_block *sb, struct dentry *upperdentry,
                            struct ovl_path *lowerpath, struct dentry *index,
-                           unsigned int numlower, char *redirect)
+                           unsigned int numlower, char *redirect,
+                           struct dentry *lowerdata)
 {
        struct inode *realinode = upperdentry ? d_inode(upperdentry) : NULL;
        struct inode *inode;

@@ -861,7 +862,7 @@ struct inode *ovl_get_inode(struct super_block *sb, struct dentry *upperdentry,
                }
        }
        ovl_fill_inode(inode, realinode->i_mode, realinode->i_rdev, ino, fsid);
-       ovl_inode_init(inode, upperdentry, lowerdentry);
+       ovl_inode_init(inode, upperdentry, lowerdentry, lowerdata);
 
        if (upperdentry && ovl_is_impuredir(upperdentry))
                ovl_set_flag(OVL_IMPURE, inode);

fs/overlayfs/namei.c +4 -1

index 682f0d402986..b25e32cf5916 100644
@@ -1070,8 +1070,11 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
                upperdentry = dget(index);
 
        if (upperdentry || ctr) {
+               struct dentry *lowerdata = NULL;
+               if (ctr > 1 && !d.is_dir)
+                       lowerdata = stack[ctr - 1].dentry;
                inode = ovl_get_inode(dentry->d_sb, upperdentry, stack, index,
-                                     ctr, upperredirect);
+                                     ctr, upperredirect, lowerdata);
                err = PTR_ERR(inode);
                if (IS_ERR(inode))
                        goto out_free_oe;

fs/overlayfs/overlayfs.h +2 -1

index 29b28ee50b0f..29c3498a7e61 100644
@@ -234,6 +234,7 @@ struct dentry *ovl_dentry_real_data(struct dentry *dentry);
 struct dentry *ovl_i_dentry_upper(struct inode *inode);
 struct inode *ovl_inode_upper(struct inode *inode);
 struct inode *ovl_inode_lower(struct inode *inode);
+struct inode *ovl_inode_lowerdata(struct inode *inode);
 struct inode *ovl_inode_real(struct inode *inode);
 struct ovl_dir_cache *ovl_dir_cache(struct inode *inode);
 void ovl_set_dir_cache(struct inode *inode, struct ovl_dir_cache *cache);

@@ -253,7 +254,7 @@ bool ovl_redirect_dir(struct super_block *sb);
 const char *ovl_dentry_get_redirect(struct dentry *dentry);
 void ovl_dentry_set_redirect(struct dentry *dentry, const char *redirect);
 void ovl_inode_init(struct inode *inode, struct dentry *upperdentry,
-                   struct dentry *lowerdentry);
+                   struct dentry *lowerdentry, struct dentry *lowerdata);
 void ovl_inode_update(struct inode *inode, struct dentry *upperdentry);
 void ovl_dentry_version_inc(struct dentry *dentry, bool impurity);
 u64 ovl_dentry_version_get(struct dentry *dentry);

Add an helper to retrieve real data inode associated with overlay inode. This helper will ignore all metacopy inodes and will return only the real inode which has data.

Signed-off-by Vivek Goyal <[email protected]>

---
 fs/overlayfs/overlayfs.h |  1 +
 fs/overlayfs/util.c      | 12 ++++++++++++
 2 files changed, 13 insertions(+)

fs/overlayfs/overlayfs.h +1 -0

index 29c3498a7e61..e24af484967f 100644
@@ -236,6 +236,7 @@ struct inode *ovl_inode_upper(struct inode *inode);
 struct inode *ovl_inode_lower(struct inode *inode);
 struct inode *ovl_inode_lowerdata(struct inode *inode);
 struct inode *ovl_inode_real(struct inode *inode);
+struct inode *ovl_inode_real_data(struct inode *inode);
 struct ovl_dir_cache *ovl_dir_cache(struct inode *inode);
 void ovl_set_dir_cache(struct inode *inode, struct ovl_dir_cache *cache);
 void ovl_dentry_set_flag(unsigned long flag, struct dentry *dentry);

fs/overlayfs/util.c +12 -1

index 58aa14a02bb0..c7a1bd40dc6a 100644
@@ -268,6 +268,18 @@ struct inode *ovl_inode_lowerdata(struct inode *inode)
        return OVL_I(inode)->lowerdata ?: ovl_inode_lower(inode);
 }
 
+/* Return real inode which contains data. Does not return metacopy inode */
+struct inode *ovl_inode_real_data(struct inode *inode)
+{
+       struct inode *upperinode;
+
+       upperinode = ovl_inode_upper(inode);
+       if (upperinode && ovl_has_upperdata(inode))
+               return upperinode;
+
+       return ovl_inode_lowerdata(inode);
+}
+
 
 struct ovl_dir_cache *ovl_dir_cache(struct inode *inode)
 {
--

2.13.6

--
To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

ovl_open() should open file which contains data and not open metacopy inode.

Signed-off-by Vivek Goyal <[email protected]>

---
 fs/overlayfs/file.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

fs/overlayfs/file.c +12 -5

index 23638d8ebab5..558a859b2658 100644
@@ -18,18 +18,26 @@ static struct file *ovl_open_realfile(const struct file *file)
 {
        struct inode *inode = file_inode(file);
        struct inode *upperinode = ovl_inode_upper(inode);
-       struct inode *realinode = upperinode ?: ovl_inode_lower(inode);
+       struct inode *realinode;
        struct file *realfile;
+       bool upperreal = false;
        const struct cred *old_cred;
 
+       if (upperinode && ovl_has_upperdata(inode))
+               upperreal = true;
+
+       /* Always open file which contains data. Do not open metacopy. */
+       realinode = upperreal ? upperinode : ovl_inode_lowerdata(inode);
+
        old_cred = ovl_override_creds(inode->i_sb);
        realfile = path_open(&file->f_path, file->f_flags | O_NOATIME,
                             realinode, current_cred(), false);
        revert_creds(old_cred);
 
        pr_debug("open(%p[%pD2/%c], 0%o) -> (%p, 0%o)\n",
-                file, file, upperinode ? 'u' : 'l', file->f_flags,
-                realfile, IS_ERR(realfile) ? 0 : realfile->f_flags);
+                file, file, upperreal ? 'u' : 'l',
+                file->f_flags, realfile,
+                IS_ERR(realfile) ? 0 : realfile->f_flags);
 
        return realfile;
 }

@@ -80,7 +88,7 @@ static int ovl_real_file(const struct file *file, struct fd *real)
        real->file = file->private_data;
 
        /* Has it been copied up since we'd opened it? */
-       if (unlikely(file_inode(real->file) != ovl_inode_real(inode))) {
+       if (unlikely(file_inode(real->file) != ovl_inode_real_data(inode))) {
                real->flags = FDPUT_FPUT;
                real->file = ovl_open_realfile(file);
 
--

2.13.6

--
To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 4 lines · Vivek Goyal

ovl_open() should open file which contains data and not open metacopy inode.

Signed-off-by Vivek Goyal <[email protected]>

This looks fine, but I don't see a reason to separate it from 26/31 first allow only data open, then allow also meta open.. Anyway, I am not sure about the cleanest way to sort all the cases.

Quoted 51 lines

---
 fs/overlayfs/file.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

fs/overlayfs/file.c +12 -5

index 23638d8ebab5..558a859b2658 100644
@@ -18,18 +18,26 @@ static struct file *ovl_open_realfile(const struct file *file)
 {
        struct inode *inode = file_inode(file);
        struct inode *upperinode = ovl_inode_upper(inode);
-       struct inode *realinode = upperinode ?: ovl_inode_lower(inode);
+       struct inode *realinode;
        struct file *realfile;
+       bool upperreal = false;
        const struct cred *old_cred;
 
+       if (upperinode && ovl_has_upperdata(inode))
+               upperreal = true;
+
+       /* Always open file which contains data. Do not open metacopy. */
+       realinode = upperreal ? upperinode : ovl_inode_lowerdata(inode);
+
        old_cred = ovl_override_creds(inode->i_sb);
        realfile = path_open(&file->f_path, file->f_flags | O_NOATIME,
                             realinode, current_cred(), false);
        revert_creds(old_cred);
 
        pr_debug("open(%p[%pD2/%c], 0%o) -> (%p, 0%o)\n",
-                file, file, upperinode ? 'u' : 'l', file->f_flags,
-                realfile, IS_ERR(realfile) ? 0 : realfile->f_flags);
+                file, file, upperreal ? 'u' : 'l',
+                file->f_flags, realfile,
+                IS_ERR(realfile) ? 0 : realfile->f_flags);
 
        return realfile;
 }

@@ -80,7 +88,7 @@ static int ovl_real_file(const struct file *file, struct fd *real)
        real->file = file->private_data;
 
        /* Has it been copied up since we'd opened it? */
-       if (unlikely(file_inode(real->file) != ovl_inode_real(inode))) {
+       if (unlikely(file_inode(real->file) != ovl_inode_real_data(inode))) {
                real->flags = FDPUT_FPUT;
                real->file = ovl_open_realfile(file);
 
--

2.13.6

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Metacopy dentry/inode is internal to overlay and is never exposed outside of it. Modify d_real() to look for only dentries/inode which have data and which are not metacopy.

Signed-off-by Vivek Goyal <[email protected]>

---
 fs/overlayfs/super.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/overlayfs/super.c +1 -2

index b81ec9d22ac5..4efb7cd76cc1 100644
@@ -100,7 +100,7 @@ static struct dentry *ovl_d_real(struct dentry *dentry,
        if (!d_is_reg(dentry))
                goto bug;
 
-       real = ovl_dentry_real(dentry);
+       real = ovl_dentry_real_data(dentry);
        if (inode == d_inode(real))
                return real;
 
--

2.13.6

Move some directory related code in else block. This is pure code reorganization and no functionality change.

Next patch enables redirect processing on metacopy files and needs this change. By keeping non-functional changes in a separate patch, next patch looks much smaller and cleaner.

Signed-off-by Vivek Goyal <[email protected]>

Right now we rely on path based lookup for data origin of metacopy upper. This will work only if upper has not been renamed. We solved this problem already for merged directories using redirect. Use same logic for metacopy files.

This patch just goes on to check redirects for metacopy files.

Signed-off-by Vivek Goyal <[email protected]>

---
 fs/overlayfs/namei.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

fs/overlayfs/namei.c +2 -3

index 880c7faea41a..41cbde97a212 100644
@@ -268,7 +268,8 @@ static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d,
                        goto out_err;
                d->stop = !err;
                d->metacopy = !!err;
-               goto out;
+               if (!d->metacopy || d->last)
+                       goto out;
        } else {
                if (last_element) {
                        if (d->metacopy) {

@@ -874,7 +875,6 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
                }
                if (upperdentry && !d.is_dir) {
                        unsigned int origin_ctr = 0;
-                       BUG_ON(d.redirect);
                        /*
                         * Lookup copy up origin by decoding origin file handle.
                         * We may get a disconnected dentry, which is fine,
--

2.13.6

--
To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Add an ovl_inode flag OVL_CONST_INO. This flag signifies if inode number will remain constant over copy up or not. This flag does not get updated over copy up and remains unmodifed after setting once.

Next patch in the series will make use of this flag. It will basically figure out if dentry is of type ORIGIN or not. And this can be derived by this flag.

ORIGIN = (upperdentry && ovl_test_flag(OVL_CONST_INO, inode)).

Suggested-by Amir Goldstein <[email protected]>

Signed-off-by Vivek Goyal <[email protected]>

---
 fs/overlayfs/inode.c     | 3 +++
 fs/overlayfs/overlayfs.h | 2 ++
 2 files changed, 5 insertions(+)

fs/overlayfs/inode.c +3 -0

index a6cd57741a1f..19e96d4717bd 100644
@@ -881,6 +881,9 @@ struct inode *ovl_get_inode(struct super_block *sb, struct dentry *upperdentry,
 
        OVL_I(inode)->redirect = redirect;
 
+       if (bylower)
+               ovl_set_flag(OVL_CONST_INO, inode);
+
        /* Check for non-merge dir that may have whiteouts */
        if (is_dir) {
                if (((upperdentry && lowerdentry) || numlower > 1) ||

fs/overlayfs/overlayfs.h +2 -1

index e24af484967f..e2c6a2f5addf 100644
@@ -38,6 +38,8 @@ enum ovl_inode_flag {
        OVL_WHITEOUTS,
        OVL_INDEX,
        OVL_UPPERDATA,
+       /* Inode number will remain constant over copy up. */
+       OVL_CONST_INO,
 };
 
 enum ovl_entry_flag {
--

2.13.6

If a dentry has copy up origin, we set flag OVL_PATH_ORIGIN. So far this decision was easy that we had to check only for oe->numlower and if it is non-zero, we knew there is copy up origin. (For non-dir we installed origin dentry in lowerstack[0]).

But we don't create ORGIN xattr for broken hardlinks (index=off). And with metacopy feature it is possible that we will install lowerstack[0] but ORIGIN xattr is not there. It is data dentry of upper metacopy dentry which has been found using regular name based lookup or using REDIRECT. So with addition of this new case, just presence of oe->numlower is not sufficient to guarantee that ORIGIN xattr is present.

So two differentiate between two cases, look at OVL_CONST_INO flag. If

this flag is set and upperdentry is there, that means ORIGIN xattr must be present.

OVL_CONST_INO is not set if lower hardlink is broken or will be broken over copy up.

Signed-off-by Vivek Goyal <[email protected]>

---
 fs/overlayfs/util.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

fs/overlayfs/util.c +2 -2

index f85ff5714852..69acce1c9026 100644
@@ -133,7 +133,8 @@ enum ovl_path_type ovl_path_type(struct dentry *dentry)
                 * Non-dir dentry can hold lower dentry of its copy up origin.
                 */
                if (oe->numlower) {
-                       type |= __OVL_PATH_ORIGIN;
+                       if (ovl_test_flag(OVL_CONST_INO, d_inode(dentry)))
+                               type |= __OVL_PATH_ORIGIN;
                        if (d_is_dir(dentry) ||
                            !ovl_has_upperdata(d_inode(dentry)))
                                type |= __OVL_PATH_MERGE;
--

2.13.6

--
To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Set redirect on metacopy files upon rename. This will help find data dentry in lower dirs.

Signed-off-by Vivek Goyal <[email protected]>

fs/overlayfs/dir.c +11 -0

+{
+       struct dentry *lowerdentry;
+
+       if (d_is_dir(dentry) || !samedir)
+               return samedir;
+
+       /*
+        * For non-dir hardlinked files, we need absolute redirects
+        * in general as two upper hardlinks could be in different
+        * dirs. We could put a relative redirect now and convert
+        * it to absolute redirect later. But when nlink > 1 and