Misc ovl_lookup() related fixes

d->last signifies that this is the last layer we are looking into and there is no more. And that means this allows for some optimzation opportunities during lookup. For example, in ovl_lookup_single() we don't have to check for opaque xattr of a directory is this is the last layer we are looking into (d->last = true).

But knowing for sure whether we are looking into last layer can be very tricky. If redirects are not enabled, then we can look at poe->numlower and figure out if the lookup we are about to is last layer or not. But if redircts are enabled then it is possible poe->numlower suggests that we are looking in last layer, but there is an absolute redirect present in found element and that redirects us to a layer in root and that means lookup will continue in lower layers further.

For example, consider following.

/upperdir/pure (opaque=y)
/upperdir/pure/foo (opaque=y,redirect=/bar)
/lowerdir/bar

In this case pure is "pure upper". When we look for "foo", that time poe->numlower=0. But that alone does not mean that we will not search for a merge candidate in /lowerdir. Absolute redirect changes that.

IOW, d->last should not be set just based on poe->numlower if redirects are enabled. That can lead to setting d->last while it should not have and that means we will not check for opaque xattr while we should have.

So do this.

- If redirects are not enabled, then continue to rely on poe->numlower information to determine if it is last layer or not.

- If redirects are enabled, then set d->last = true only if this is the last layer in root ovl_entry (roe).

Suggested-by Amir Goldstein <[email protected]>

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

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

fs/overlayfs/namei.c +1 -1

index de3e6da1d5a5..2e173cfbda0e 100644
@@ -815,7 +815,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
                .is_dir = false,
                .opaque = false,
                .stop = false,
-               .last = !poe->numlower,
+               .last = ofs->config.redirect_follow ? false : !poe->numlower,
                .redirect = NULL,
        };
 

Quoted 30 lines · Vivek Goyal

d->last signifies that this is the last layer we are looking into and there is no more. And that means this allows for some optimzation opportunities during lookup. For example, in ovl_lookup_single() we don't have to check for opaque xattr of a directory is this is the last layer we are looking into (d->last = true).

But knowing for sure whether we are looking into last layer can be very tricky. If redirects are not enabled, then we can look at poe->numlower and figure out if the lookup we are about to is last layer or not. But if redircts are enabled then it is possible poe->numlower suggests that we are looking in last layer, but there is an absolute redirect present in found element and that redirects us to a layer in root and that means lookup will continue in lower layers further.

For example, consider following.

/upperdir/pure (opaque=y)
/upperdir/pure/foo (opaque=y,redirect=/bar)
/lowerdir/bar

In this case pure is "pure upper". When we look for "foo", that time poe->numlower=0. But that alone does not mean that we will not search for a merge candidate in /lowerdir. Absolute redirect changes that.

IOW, d->last should not be set just based on poe->numlower if redirects are enabled. That can lead to setting d->last while it should not have and that means we will not check for opaque xattr while we should have.

So do this.

Quoted 8 lines

- If redirects are not enabled, then continue to rely on poe->numlower information to determine if it is last layer or not.

- If redirects are enabled, then set d->last = true only if this is the last layer in root ovl_entry (roe).

Suggested-by Amir Goldstein <[email protected]>

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

Reviewed-by Amir Goldstein <[email protected]>

Much better description than my RFC patch :-)
One minor error

Quoted 21 lines

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

fs/overlayfs/namei.c +1 -1

index de3e6da1d5a5..2e173cfbda0e 100644
@@ -815,7 +815,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
                .is_dir = false,
                .opaque = false,
                .stop = false,
-               .last = !poe->numlower,
+               .last = ofs->config.redirect_follow ? false : !poe->numlower,
                .redirect = NULL,
        };
 

@@ -873,7 +873,11 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
        for (i = 0; !d.stop && i < poe->numlower; i++) {
                struct ovl_path lower = poe->lowerstack[i];
 

fs/overlayfs/namei.c struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry, · line 880

Quoted 6 lines

fs/overlayfs/namei.c +5 -1

-               d.last = i == poe->numlower - 1;
+               if (!ofs->config.redirect_follow)
+                       d.last = i == poe->numlower - 1;
+               else
+                       d.last = lower.layer->idx == roe->numlower - 1;
+

Should be lower.layer->idx == roe->numlower. (idx 0 is upper)

But to be honest I did not verify that xattr checks are optimized away with my RFC patch, just that the test case above behaves as expected.

Thanks,
Amir.

If we are looking in last layer, then there should not be any need to process redirect. redirect information is used only for lookup in next lower layer and there is no more lower layer to look into. So no need to process redirects.

IOW, ignore redirects on lowest layer.

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

---
 fs/overlayfs/namei.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

fs/overlayfs/namei.c +0 -0

index 2e173cfbda0e..32d1d96f80b6 100644

fs/overlayfs/namei.c +9 -4

+       if (!d->last) {
+               if (ovl_is_opaquedir(this)) {
+                       d->stop = d->opaque = true;
+                       goto out;
+               }
+
+               err = ovl_check_redirect(this, d, prelen, post);
+               if (err)
+                       goto out_err;
        }
-       err = ovl_check_redirect(this, d, prelen, post);
-       if (err)
-               goto out_err;
 out:
        *ret = this;
        return 0;
--

2.13.6

Quoted 8 lines · Vivek Goyal

If we are looking in last layer, then there should not be any need to process redirect. redirect information is used only for lookup in next lower layer and there is no more lower layer to look into. So no need to process redirects.

IOW, ignore redirects on lowest layer.

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

Reviewed-by Amir Goldstein <[email protected]>

Quoted 4 lines

---
 fs/overlayfs/namei.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

fs/overlayfs/namei.c static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d, · line 253

Quoted 11 lines

fs/overlayfs/namei.c +0 -3

index 2e173cfbda0e..32d1d96f80b6 100644
@@ -248,13 +248,16 @@ static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d,
                goto out;
        }
        d->is_dir = true;
-       if (!d->last && ovl_is_opaquedir(this)) {
-               d->stop = d->opaque = true;
-               goto out;

I preferred to go with if (d->last) goto out; I like to avoid nesting when possible.
but choose whichever you like.

Quoted 20 lines

fs/overlayfs/namei.c +9 -4

+       if (!d->last) {
+               if (ovl_is_opaquedir(this)) {
+                       d->stop = d->opaque = true;
+                       goto out;
+               }
+
+               err = ovl_check_redirect(this, d, prelen, post);
+               if (err)
+                       goto out_err;
        }
-       err = ovl_check_redirect(this, d, prelen, post);
-       if (err)
-               goto out_err;
 out:
        *ret = this;
        return 0;
--

2.13.6

Certain properties in ovl_lookup_data should be set only for the last element of the path. IOW, of we are calling ovl_lookup_single() for an absolute redirect, then d->is_dir and d->opaque do not make much sense for intermediate path elements. Instead set them only if dentry being lookup is last path element.

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

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

fs/overlayfs/namei.c +4 -3

index 32d1d96f80b6..dfefe6277659 100644
@@ -220,6 +220,7 @@ static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d,
 {
        struct dentry *this;
        int err;
+       bool last_element = !post[0];
 
        this = lookup_one_len_unlocked(name, base, namelen);
        if (IS_ERR(this)) {

@@ -247,9 +248,10 @@ static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d,
                        goto put_and_out;
                goto out;
        }
-       d->is_dir = true;
+       if (last_element)
+               d->is_dir = true;
        if (!d->last) {
-               if (ovl_is_opaquedir(this)) {
+               if (last_element && ovl_is_opaquedir(this)) {
                        d->stop = d->opaque = true;
                        goto out;
                }
--

2.13.6

Quoted 5 lines · Vivek Goyal

Certain properties in ovl_lookup_data should be set only for the last element of the path. IOW, of we are calling ovl_lookup_single() for an absolute redirect, then d->is_dir and d->opaque do not make much sense for intermediate path elements. Instead set them only if dentry being lookup is last path element.

Yeh, that's what I said, but I realized later that this is not accurate. it's true for d->is_dir, but not true for d->opaque. opaqueness of path elements *can* determine that the redirect result is opaque, for example when redirecting to /a/b/c and /a is opaque, then the resolved redirection is opaque *unless* either /a/b or /a/b/c has an absolute redirect to escape the opaqueness of /a.

Hence my fix patch.

Quoted 34 lines

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

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

fs/overlayfs/namei.c +4 -3

index 32d1d96f80b6..dfefe6277659 100644
@@ -220,6 +220,7 @@ static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d,
 {
        struct dentry *this;
        int err;
+       bool last_element = !post[0];
 
        this = lookup_one_len_unlocked(name, base, namelen);
        if (IS_ERR(this)) {

@@ -247,9 +248,10 @@ static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d,
                        goto put_and_out;
                goto out;
        }
-       d->is_dir = true;
+       if (last_element)
+               d->is_dir = true;
        if (!d->last) {
-               if (ovl_is_opaquedir(this)) {
+               if (last_element && ovl_is_opaquedir(this)) {
                        d->stop = d->opaque = true;
                        goto out;
                }
--

2.13.6

Quoted 7 lines · Amir Goldstein

Quoted 5 lines · Vivek Goyal

Certain properties in ovl_lookup_data should be set only for the last element of the path. IOW, of we are calling ovl_lookup_single() for an absolute redirect, then d->is_dir and d->opaque do not make much sense for intermediate path elements. Instead set them only if dentry being lookup is last path element.

Quoted 6 lines

Yeh, that's what I said, but I realized later that this is not accurate. it's true for d->is_dir, but not true for d->opaque. opaqueness of path elements *can* determine that the redirect result is opaque, for example when redirecting to /a/b/c and /a is opaque, then the resolved redirection is opaque *unless* either /a/b or /a/b/c has an absolute redirect to escape the opaqueness of /a.

Hi Amir,

I am not sure I understand this argument about "opaque". Why opaqueness of parent matters to child. Can you please give an example.

Quoted 15 lines · Vivek Goyal

Quoted 7 lines · Amir Goldstein

Quoted 5 lines · Vivek Goyal

Certain properties in ovl_lookup_data should be set only for the last element of the path. IOW, of we are calling ovl_lookup_single() for an absolute redirect, then d->is_dir and d->opaque do not make much sense for intermediate path elements. Instead set them only if dentry being lookup is last path element.

Quoted 6 lines

Yeh, that's what I said, but I realized later that this is not accurate. it's true for d->is_dir, but not true for d->opaque. opaqueness of path elements *can* determine that the redirect result is opaque, for example when redirecting to /a/b/c and /a is opaque, then the resolved redirection is opaque *unless* either /a/b or /a/b/c has an absolute redirect to escape the opaqueness of /a.

Quoted 5 lines

Hi Amir,

I am not sure I understand this argument about "opaque". Why opaqueness of parent matters to child. Can you please give an example.

upper:  /redirect (redirect=/a/b/c)
lower1: /a/[b]/c       ([b] is opaque)
lower0: /a/b/c/foo

upper /redirect was created by 'mv /mnt/a/b/c/ /mnt/redirect' before rename /mnt/a/b/c did not contain 'foo' because /mnt/a/b is not a merge dir and therefore neither is /mnt/a/b/c. after rename /redirect should not contain 'foo' as well. This is handles by ovl_lookup_layer() when iterating absolute redirect element [b] d->opaque is set in the lookup state.

The fix I sent for the case where /a/[b]/c is again an absolute redirect (say to /a/b/c in lower0) and that *should* results in the merge dir containing 'foo'.

Not easy...

Quoted 33 lines · Amir Goldstein

Quoted 15 lines · Vivek Goyal

Quoted 7 lines · Amir Goldstein

Quoted 5 lines · Vivek Goyal

Certain properties in ovl_lookup_data should be set only for the last element of the path. IOW, of we are calling ovl_lookup_single() for an absolute redirect, then d->is_dir and d->opaque do not make much sense for intermediate path elements. Instead set them only if dentry being lookup is last path element.

Quoted 6 lines

Yeh, that's what I said, but I realized later that this is not accurate. it's true for d->is_dir, but not true for d->opaque. opaqueness of path elements *can* determine that the redirect result is opaque, for example when redirecting to /a/b/c and /a is opaque, then the resolved redirection is opaque *unless* either /a/b or /a/b/c has an absolute redirect to escape the opaqueness of /a.

Quoted 5 lines

Hi Amir,

I am not sure I understand this argument about "opaque". Why opaqueness of parent matters to child. Can you please give an example.

upper:  /redirect (redirect=/a/b/c)
lower1: /a/[b]/c       ([b] is opaque)
lower0: /a/b/c/foo

upper /redirect was created by 'mv /mnt/a/b/c/ /mnt/redirect' before rename /mnt/a/b/c did not contain 'foo' because /mnt/a/b is not a merge dir and therefore neither is /mnt/a/b/c. after rename /redirect should not contain 'foo' as well. This is handles by ovl_lookup_layer() when iterating absolute redirect element [b] d->opaque is set in the lookup state.

Quoted 5 lines

The fix I sent for the case where /a/[b]/c is again an absolute redirect (say to /a/b/c in lower0) and that *should* results in the merge dir containing 'foo'.

Not easy...

Aha.., I get it now. So I have couple of observations.

- d->opaque is still seems to be the property of last element we are searching in the path. It is d->stop which should get set for intermediate elements if we find an opaque dir in the path.

In fact, ovl_lookup() does not even look at d->opaque until and unless it is set on upperdentry. Right?

So if we don't set d->opaque on a lower dentry, looks like nobody will care as of now. But just to define semantics right, we can say d->opaque represents the property of last element of the path.

- And apply your patch on top which will just reset d->stop = false if an absolute redirect was found in the path and leave d->opaque untouched.

Does it make sense?

Quoted 57 lines · Vivek Goyal

Quoted 33 lines · Amir Goldstein

Quoted 15 lines · Vivek Goyal

Quoted 7 lines · Amir Goldstein

Quoted 5 lines · Vivek Goyal

Certain properties in ovl_lookup_data should be set only for the last element of the path. IOW, of we are calling ovl_lookup_single() for an absolute redirect, then d->is_dir and d->opaque do not make much sense for intermediate path elements. Instead set them only if dentry being lookup is last path element.

Quoted 6 lines

Yeh, that's what I said, but I realized later that this is not accurate. it's true for d->is_dir, but not true for d->opaque. opaqueness of path elements *can* determine that the redirect result is opaque, for example when redirecting to /a/b/c and /a is opaque, then the resolved redirection is opaque *unless* either /a/b or /a/b/c has an absolute redirect to escape the opaqueness of /a.

Quoted 5 lines

Hi Amir,

I am not sure I understand this argument about "opaque". Why opaqueness of parent matters to child. Can you please give an example.

upper:  /redirect (redirect=/a/b/c)
lower1: /a/[b]/c       ([b] is opaque)
lower0: /a/b/c/foo

upper /redirect was created by 'mv /mnt/a/b/c/ /mnt/redirect' before rename /mnt/a/b/c did not contain 'foo' because /mnt/a/b is not a merge dir and therefore neither is /mnt/a/b/c. after rename /redirect should not contain 'foo' as well. This is handles by ovl_lookup_layer() when iterating absolute redirect element [b] d->opaque is set in the lookup state.

Quoted 5 lines

The fix I sent for the case where /a/[b]/c is again an absolute redirect (say to /a/b/c in lower0) and that *should* results in the merge dir containing 'foo'.

Not easy...

Aha.., I get it now. So I have couple of observations.

- d->opaque is still seems to be the property of last element we are searching in the path. It is d->stop which should get set for intermediate elements if we find an opaque dir in the path.

In fact, ovl_lookup() does not even look at d->opaque until and unless it is set on upperdentry. Right?

So if we don't set d->opaque on a lower dentry, looks like nobody will care as of now. But just to define semantics right, we can say d->opaque represents the property of last element of the path.

- And apply your patch on top which will just reset d->stop = false if an absolute redirect was found in the path and leave d->opaque untouched.

Quoted 2 lines

Does it make sense?

Yeh, ok. I guess d->stop is sufficient.

You may take my patch and modify it if you like.