[RFC PATCH 0/3] Misc ovl_lookup() related fixes

[RFC PATCH 0/3] Misc ovl_lookup() related fixes

[Vivek Goyal]

Hi Amir,

Here is another version of ovl_lookup() related fixes for the issues we
just talked.

I have tested first two patches and they don't seem to introduce
regressions. But third patch seemed to break tests/overlay/054. Not sure
yet why. Will look at it tomorrow.

This is still RFC and should fix d->last issue as well as issue of
setting d->is_dir and d->opaque for intermediate patch components.

What do you think?


Thanks
Vivek

Vivek Goyal (3):
  ovl: Set d->last properly during lookup
  ovl: Do not check for redirect if this is last layer
  ovl: set d->is_dir and d->opaque for last path element

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

-- 
2.13.6

[PATCH 1/3] ovl: Set d->last properly during lookup

[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.

- 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 <amir73il@gmail.com>
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/namei.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index de3e6da1d5a5..2e173cfbda0e 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -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];
 
-		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;
+
 		err = ovl_lookup_layer(lower.dentry, &d, &this);
 		if (err)
 			goto out_put;
-- 
2.13.6

Re: [PATCH 1/3] ovl: Set d->last properly during lookup

[Amir Goldstein]

On Fri, Mar 9, 2018 at 12:18 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
> 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 <amir73il@gmail.com>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

Reviewed-by: Amir Goldstein <amir73il@gmail.com>

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

> ---
>  fs/overlayfs/namei.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index de3e6da1d5a5..2e173cfbda0e 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -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];
>
> -               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.

Re: [PATCH 1/3] ovl: Set d->last properly during lookup

[Vivek Goyal]

On Fri, Mar 09, 2018 at 12:25:01AM +0200, Amir Goldstein wrote:
> On Fri, Mar 9, 2018 at 12:18 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > 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 <amir73il@gmail.com>
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> 
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> 
> Much better description than my RFC patch :-)
> One minor error
> 
> > ---
> >  fs/overlayfs/namei.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> > index de3e6da1d5a5..2e173cfbda0e 100644
> > --- a/fs/overlayfs/namei.c
> > +++ b/fs/overlayfs/namei.c
> > @@ -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];
> >
> > -               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)

Ok, got it. I see following in super.c

ofs->lower_layers[ofs->numlower].idx = i + 1;

Will fix it.

Thanks
Vivek
> 
> 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.

[PATCH 2/3] ovl: Do not check for redirect if this is last layer

[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 <vgoyal@redhat.com>
---
 fs/overlayfs/namei.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 2e173cfbda0e..32d1d96f80b6 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -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;
+	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

Re: [PATCH 2/3] ovl: Do not check for redirect if this is last layer

[Amir Goldstein]

On Fri, Mar 9, 2018 at 12:18 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
> 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 <vgoyal@redhat.com>

Reviewed-by: Amir Goldstein <amir73il@gmail.com>

> ---
>  fs/overlayfs/namei.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index 2e173cfbda0e..32d1d96f80b6 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -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.

> +       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
>

Re: [PATCH 2/3] ovl: Do not check for redirect if this is last layer

[Vivek Goyal]

On Fri, Mar 09, 2018 at 12:27:21AM +0200, Amir Goldstein wrote:
> On Fri, Mar 9, 2018 at 12:18 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > 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 <vgoyal@redhat.com>
> 
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> 
> > ---
> >  fs/overlayfs/namei.c | 15 +++++++++------
> >  1 file changed, 9 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> > index 2e173cfbda0e..32d1d96f80b6 100644
> > --- a/fs/overlayfs/namei.c
> > +++ b/fs/overlayfs/namei.c
> > @@ -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.

I liked your version better as well. :-). Will do if (d->last) goto out;

Vivek

> 
> > +       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
> >

[PATCH 3/3] ovl: set d->is_dir and d->opaque for last path element

[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.

Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/namei.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 32d1d96f80b6..dfefe6277659 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -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

Re: [PATCH 3/3] ovl: set d->is_dir and d->opaque for last path element

[Amir Goldstein]

On Fri, Mar 9, 2018 at 12:18 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
> 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.

>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/overlayfs/namei.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index 32d1d96f80b6..dfefe6277659 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -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
>

Re: [PATCH 3/3] ovl: set d->is_dir and d->opaque for last path element

[Vivek Goyal]

On Fri, Mar 09, 2018 at 12:34:57AM +0200, Amir Goldstein wrote:
> On Fri, Mar 9, 2018 at 12:18 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > 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.

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.

Vivek

> 
> Hence my fix patch.
> 
> >
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  fs/overlayfs/namei.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> > index 32d1d96f80b6..dfefe6277659 100644
> > --- a/fs/overlayfs/namei.c
> > +++ b/fs/overlayfs/namei.c
> > @@ -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
> >

Re: [PATCH 3/3] ovl: set d->is_dir and d->opaque for last path element

[Amir Goldstein]

On Fri, Mar 9, 2018 at 4:44 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Fri, Mar 09, 2018 at 12:34:57AM +0200, Amir Goldstein wrote:
>> On Fri, Mar 9, 2018 at 12:18 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> > 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.
>
> 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...

Thanks,
Amir.

Re: [PATCH 3/3] ovl: set d->is_dir and d->opaque for last path element

[Vivek Goyal]

On Fri, Mar 09, 2018 at 05:20:30PM +0200, Amir Goldstein wrote:
> On Fri, Mar 9, 2018 at 4:44 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Fri, Mar 09, 2018 at 12:34:57AM +0200, Amir Goldstein wrote:
> >> On Fri, Mar 9, 2018 at 12:18 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
> >> > 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.
> >
> > 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...

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?

Vivek

Re: [PATCH 3/3] ovl: set d->is_dir and d->opaque for last path element

[Amir Goldstein]

On Fri, Mar 9, 2018 at 6:30 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Fri, Mar 09, 2018 at 05:20:30PM +0200, Amir Goldstein wrote:
>> On Fri, Mar 9, 2018 at 4:44 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> > On Fri, Mar 09, 2018 at 12:34:57AM +0200, Amir Goldstein wrote:
>> >> On Fri, Mar 9, 2018 at 12:18 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
>> >> > 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.
>> >
>> > 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...
>
> 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?
>

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

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

Thanks,
Amir.

Re: [PATCH 3/3] ovl: set d->is_dir and d->opaque for last path element

[Vivek Goyal]

On Fri, Mar 09, 2018 at 06:38:34PM +0200, Amir Goldstein wrote:
> On Fri, Mar 9, 2018 at 6:30 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Fri, Mar 09, 2018 at 05:20:30PM +0200, Amir Goldstein wrote:
> >> On Fri, Mar 9, 2018 at 4:44 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> >> > On Fri, Mar 09, 2018 at 12:34:57AM +0200, Amir Goldstein wrote:
> >> >> On Fri, Mar 9, 2018 at 12:18 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
> >> >> > 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.
> >> >
> >> > 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...
> >
> > 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?
> >
> 
> Yeh, ok. I guess d->stop is sufficient.
> 
> You may take my patch and modify it if you like.

Ok, I will take your patch and modify it to drop d->opaque = false and put
it in the series.

Thanks
Vivek