[PATCH 0/3] overlayfs: Do not check metacopy in ovl_get_inode()

[PATCH 0/3] overlayfs: Do not check metacopy in ovl_get_inode()

[Vivek Goyal]

This series tries to implement Amir's suggestion of initializing
OVL_UPPERDATA in callers of ovl_get_inode() and move checking of 
metacopy xattr out of ovl_get_inode().

It also has to patches to cleanup metacopy logic a bit and make it
little more readable and understandable in ovl_lookup().

yangerkun, can you please make sure if this patch series fixes the
xfstest issue you were facing once in a while.

Vivek Goyal (3):
  overlayfs: Simplify setting of origin for index lookup
  overlayfs: ovl_lookup(): Use only uppermetacopy state
  overlayfs: Initialize OVL_UPPERDATA in ovl_lookup()

 fs/overlayfs/dir.c   |  2 +
 fs/overlayfs/inode.c | 11 +-----
 fs/overlayfs/namei.c | 88 +++++++++++++++++++++++---------------------
 3 files changed, 50 insertions(+), 51 deletions(-)

-- 
2.25.4

[PATCH 1/3] overlayfs: Simplify setting of origin for index lookup

[Vivek Goyal]

overlayfs can keep index of copied up files and directories and it
seems to serve two primary puroposes. For regular files, it avoids
breaking lower hardlinks over copy up. For directories it seems to
be used for various error checks.

During ovl_lookup(), we lookup for index using lower dentry in many
a cases. That lower dentry is called "origin" and following is a summary
of current logic.

If there is no upperdentry, always lookup for index using lower dentry.
For regular files it helps avoiding breaking hard links over copyup
and for directories it seems to be just error checks.

If there is an upperdentry, then there are 3 possible cases.

- For directories, lower dentry is found using two ways. One is regular
  path based lookup in lower layers and second is using ORIGIN xattr
  on upper dentry. First verify that path based lookup lower dentry
  matches the one pointed by upper ORIGIN xattr. If yes, use this
  verified origin for index lookup.

- For regular files (non-metacopy), there is no path based lookup in
  lower layers as lookup stops once we find upper dentry. So there
  is no origin verification. If there is ORIGIN xattr present on upper,
  use that to lookup index otherwise don't.

- For regular metacopy files, again lower dentry is found using
  path based lookup as well as ORIGIN xattr on upper. Path based lookup
  is continued in this case to find lower data dentry for metacopy
  upper. So like directories we only use verified origin. If ORIGIN
  xattr is not present (Either because lower did not support file
  handles or because this is hardlink copied up with index=off), then
  don't use path lookup based lower dentry as origin. This is same
  as regular non-metacopy file case.

Suggested-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/namei.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 0db23baf98e7..5d80d8cc0063 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -1005,25 +1005,30 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		}
 		stack = origin_path;
 		ctr = 1;
+		origin = origin_path->dentry;
 		origin_path = NULL;
 	}
 
 	/*
-	 * Lookup index by lower inode and verify it matches upper inode.
-	 * We only trust dir index if we verified that lower dir matches
-	 * origin, otherwise dir index entries may be inconsistent and we
-	 * ignore them.
+	 * Always lookup index if there is no-upperdentry.
 	 *
-	 * For non-dir upper metacopy dentry, we already set "origin" if we
-	 * verified that lower matched upper origin. If upper origin was
-	 * not present (because lower layer did not support fh encode/decode),
-	 * or indexing is not enabled, do not set "origin" and skip looking up
-	 * index. This case should be handled in same way as a non-dir upper
-	 * without ORIGIN is handled.
+	 * For the case of upperdentry, we have set origin by now if it
+	 * needed to be set. There are basically three cases.
+	 *
+	 * For directories, lookup index by lower inode and verify it matches
+	 * upper inode. We only trust dir index if we verified that lower dir
+	 * matches origin, otherwise dir index entries may be inconsistent
+	 * and we ignore them.
+	 *
+	 * For regular upper, we already set origin if upper had ORIGIN
+	 * xattr. There is no verification though as there is no path
+	 * based dentry lookup in lower in this case.
+	 *
+	 * For metacopy upper, we set a verified origin already if index
+	 * is enabled and if upper had an ORIGIN xattr.
 	 *
-	 * Always lookup index of non-dir non-metacopy and non-upper.
 	 */
-	if (ctr && (!upperdentry || (!d.is_dir && !metacopy)))
+	if (!origin && ctr && !upperdentry)
 		origin = stack[0].dentry;
 
 	if (origin && ovl_indexdir(dentry->d_sb) &&
-- 
2.25.4

[PATCH 2/3] overlayfs: ovl_lookup(): Use only uppermetacopy state

[Vivek Goyal]

Currently we use a variable "metacopy" which signifies that dentry
could be either uppermetacopy or lowermetacopy. Amir suggested that
we can move code around and use d.metacopy in such a way that we
don't need lowermetacopy and just can do away with uppermetacopy.

So this patch replaces "metacopy" with "uppermetacopy".

It also moves some code little higher to keep reading little simpler.

Suggested-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/namei.c | 57 ++++++++++++++++++++++----------------------
 1 file changed, 28 insertions(+), 29 deletions(-)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index 5d80d8cc0063..a1889a160708 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -823,7 +823,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 	struct dentry *this;
 	unsigned int i;
 	int err;
-	bool metacopy = false;
+	bool uppermetacopy = false;
 	struct ovl_lookup_data d = {
 		.sb = dentry->d_sb,
 		.name = dentry->d_name,
@@ -869,7 +869,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 				goto out_put_upper;
 
 			if (d.metacopy)
-				metacopy = true;
+				uppermetacopy = true;
 		}
 
 		if (d.redirect) {
@@ -906,6 +906,22 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		if (!this)
 			continue;
 
+		if ((uppermetacopy || d.metacopy) && !ofs->config.metacopy) {
+			err = -EPERM;
+			pr_warn_ratelimited("refusing to follow metacopy origin"
+					    " for (%pd2)\n", dentry);
+			goto out_put;
+		}
+
+		/*
+		 * Do not store intermediate metacopy dentries in chain,
+		 * except top most lower metacopy dentry
+		 */
+		if (d.metacopy && ctr) {
+			dput(this);
+			continue;
+		}
+
 		/*
 		 * If no origin fh is stored in upper of a merge dir, store fh
 		 * of lower dir and set upper parent "impure".
@@ -940,17 +956,6 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 			origin = this;
 		}
 
-		if (d.metacopy)
-			metacopy = true;
-		/*
-		 * Do not store intermediate metacopy dentries in chain,
-		 * except top most lower metacopy dentry
-		 */
-		if (d.metacopy && ctr) {
-			dput(this);
-			continue;
-		}
-
 		stack[ctr].dentry = this;
 		stack[ctr].layer = lower.layer;
 		ctr++;
@@ -982,23 +987,17 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		}
 	}
 
-	if (metacopy) {
-		/*
-		 * Found a metacopy dentry but did not find corresponding
-		 * data dentry
-		 */
-		if (d.metacopy) {
-			err = -EIO;
-			goto out_put;
-		}
+	/* Found a metacopy dentry but did not find corresponding data dentry */
+	if (d.metacopy) {
+		err = -EIO;
+		goto out_put;
+	}
 
-		err = -EPERM;
-		if (!ofs->config.metacopy) {
-			pr_warn_ratelimited("refusing to follow metacopy origin for (%pd2)\n",
-					    dentry);
-			goto out_put;
-		}
-	} else if (!d.is_dir && upperdentry && !ctr && origin_path) {
+	/* For regular non-metacopy upper dentries, there is no lower
+	 * path based lookup, hence ctr will be zero. dentry found using
+	 * ORIGIN xattr on upper, install it in stack.
+	 */
+	if (!d.is_dir && upperdentry && !ctr && origin_path) {
 		if (WARN_ON(stack != NULL)) {
 			err = -EIO;
 			goto out_put;
-- 
2.25.4

[PATCH 3/3] overlayfs: Initialize OVL_UPPERDATA in ovl_lookup()

[Vivek Goyal]

Currently ovl_get_inode() initializes OVL_UPPERDATA flag and for that it
has to call ovl_check_metacopy_xattr() and check if metacopy xattr is
present or not.

yangerkun reported sometimes underlying filesystem might return -EIO
and in that case error handling path does not cleanup properly leading
to various warnings.

Run generic/461 with ext4 upper/lower layer sometimes may trigger the
bug as below(linux 4.19):

[  551.001349] overlayfs: failed to get metacopy (-5)
[  551.003464] overlayfs: failed to get inode (-5)
[  551.004243] overlayfs: cleanup of 'd44/fd51' failed (-5)
[  551.004941] overlayfs: failed to get origin (-5)
[  551.005199] ------------[ cut here ]------------
[  551.006697] WARNING: CPU: 3 PID: 24674 at fs/inode.c:1528 iput+0x33b/0x400
...
[  551.027219] Call Trace:
[  551.027623]  ovl_create_object+0x13f/0x170
[  551.028268]  ovl_create+0x27/0x30
[  551.028799]  path_openat+0x1a35/0x1ea0
[  551.029377]  do_filp_open+0xad/0x160
[  551.029944]  ? vfs_writev+0xe9/0x170
[  551.030499]  ? page_counter_try_charge+0x77/0x120
[  551.031245]  ? __alloc_fd+0x160/0x2a0
[  551.031832]  ? do_sys_open+0x189/0x340
[  551.032417]  ? get_unused_fd_flags+0x34/0x40
[  551.033081]  do_sys_open+0x189/0x340
[  551.033632]  __x64_sys_creat+0x24/0x30
[  551.034219]  do_syscall_64+0xd5/0x430
[  551.034800]  entry_SYSCALL_64_after_hwframe+0x44/0xa9

One solution is to improve error handling and call iget_failed() if error
is encountered. Amir thinks that this path is little intricate and there
is not real need to check and initialize OVL_UPPERDATA in ovl_get_inode().
Instead caller of ovl_get_inode() can initialize this state. And this
will avoid double checking of metacopy xattr lookup in ovl_lookup()
and ovl_get_inode().

OVL_UPPERDATA is inode flag. So I was little concerned that initializing
it outside ovl_get_inode() might have some races. But this is one way
transition. That is once a file has been fully copied up, it can't go
back to metacopy file again. And that seems to help avoid races. So
as of now I can't see any races w.r.t OVL_UPPERDATA being set wrongly. So
move settingof OVL_UPPERDATA inside the callers of ovl_get_inode().
ovl_obtain_alias() already does it. So only two callers now left
are ovl_lookup() and ovl_instantiate().

Reported-by: yangerkun <yangerkun@huawei.com>
Suggested-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 fs/overlayfs/dir.c   |  2 ++
 fs/overlayfs/inode.c | 11 +----------
 fs/overlayfs/namei.c |  2 ++
 3 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
index 279009dee366..a7cac2ce0fad 100644
--- a/fs/overlayfs/dir.c
+++ b/fs/overlayfs/dir.c
@@ -262,6 +262,8 @@ static int ovl_instantiate(struct dentry *dentry, struct inode *inode,
 		inode = ovl_get_inode(dentry->d_sb, &oip);
 		if (IS_ERR(inode))
 			return PTR_ERR(inode);
+		if (inode == oip.newinode)
+			ovl_set_flag(OVL_UPPERDATA, inode);
 	} else {
 		WARN_ON(ovl_inode_real(inode) != d_inode(newdentry));
 		dput(newdentry);
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index 981f11ec51bc..f2aaf00821c0 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -957,7 +957,7 @@ struct inode *ovl_get_inode(struct super_block *sb,
 	bool bylower = ovl_hash_bylower(sb, upperdentry, lowerdentry,
 					oip->index);
 	int fsid = bylower ? lowerpath->layer->fsid : 0;
-	bool is_dir, metacopy = false;
+	bool is_dir;
 	unsigned long ino = 0;
 	int err = oip->newinode ? -EEXIST : -ENOMEM;
 
@@ -1018,15 +1018,6 @@ struct inode *ovl_get_inode(struct super_block *sb,
 	if (oip->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);
-	}
-
 	OVL_I(inode)->redirect = oip->redirect;
 
 	if (bylower)
diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index a1889a160708..36e2b88a2fd1 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -1078,6 +1078,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 		err = PTR_ERR(inode);
 		if (IS_ERR(inode))
 			goto out_free_oe;
+		if (upperdentry && !uppermetacopy)
+			ovl_set_flag(OVL_UPPERDATA, inode);
 	}
 
 	ovl_dentry_update_reval(dentry, upperdentry,
-- 
2.25.4

Re: [PATCH 0/3] overlayfs: Do not check metacopy in ovl_get_inode()

[yangerkun]

Ok, I will try it.

在 2020/5/30 5:29, Vivek Goyal 写道:
> This series tries to implement Amir's suggestion of initializing
> OVL_UPPERDATA in callers of ovl_get_inode() and move checking of
> metacopy xattr out of ovl_get_inode().
> 
> It also has to patches to cleanup metacopy logic a bit and make it
> little more readable and understandable in ovl_lookup().
> 
> yangerkun, can you please make sure if this patch series fixes the
> xfstest issue you were facing once in a while.
> 
> Vivek Goyal (3):
>    overlayfs: Simplify setting of origin for index lookup
>    overlayfs: ovl_lookup(): Use only uppermetacopy state
>    overlayfs: Initialize OVL_UPPERDATA in ovl_lookup()
> 
>   fs/overlayfs/dir.c   |  2 +
>   fs/overlayfs/inode.c | 11 +-----
>   fs/overlayfs/namei.c | 88 +++++++++++++++++++++++---------------------
>   3 files changed, 50 insertions(+), 51 deletions(-)
> 

Re: [PATCH 0/3] overlayfs: Do not check metacopy in ovl_get_inode()

[yangerkun]

I have try repeat this testcase for about 4 hours. Wont happen again.

Thanks,

在 2020/5/30 8:59, yangerkun 写道:
> Ok, I will try it.
> 
> 在 2020/5/30 5:29, Vivek Goyal 写道:
>> This series tries to implement Amir's suggestion of initializing
>> OVL_UPPERDATA in callers of ovl_get_inode() and move checking of
>> metacopy xattr out of ovl_get_inode().
>>
>> It also has to patches to cleanup metacopy logic a bit and make it
>> little more readable and understandable in ovl_lookup().
>>
>> yangerkun, can you please make sure if this patch series fixes the
>> xfstest issue you were facing once in a while.
>>
>> Vivek Goyal (3):
>>    overlayfs: Simplify setting of origin for index lookup
>>    overlayfs: ovl_lookup(): Use only uppermetacopy state
>>    overlayfs: Initialize OVL_UPPERDATA in ovl_lookup()
>>
>>   fs/overlayfs/dir.c   |  2 +
>>   fs/overlayfs/inode.c | 11 +-----
>>   fs/overlayfs/namei.c | 88 +++++++++++++++++++++++---------------------
>>   3 files changed, 50 insertions(+), 51 deletions(-)
>>
> 
> 
> .

Re: [PATCH 1/3] overlayfs: Simplify setting of origin for index lookup

[Amir Goldstein]

On Sat, May 30, 2020 at 12:30 AM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> overlayfs can keep index of copied up files and directories and it
> seems to serve two primary puroposes. For regular files, it avoids
> breaking lower hardlinks over copy up. For directories it seems to
> be used for various error checks.
>
> During ovl_lookup(), we lookup for index using lower dentry in many
> a cases. That lower dentry is called "origin" and following is a summary
> of current logic.
>
> If there is no upperdentry, always lookup for index using lower dentry.
> For regular files it helps avoiding breaking hard links over copyup
> and for directories it seems to be just error checks.
>
> If there is an upperdentry, then there are 3 possible cases.
>
> - For directories, lower dentry is found using two ways. One is regular
>   path based lookup in lower layers and second is using ORIGIN xattr
>   on upper dentry. First verify that path based lookup lower dentry
>   matches the one pointed by upper ORIGIN xattr. If yes, use this
>   verified origin for index lookup.
>
> - For regular files (non-metacopy), there is no path based lookup in
>   lower layers as lookup stops once we find upper dentry. So there
>   is no origin verification. If there is ORIGIN xattr present on upper,
>   use that to lookup index otherwise don't.
>
> - For regular metacopy files, again lower dentry is found using
>   path based lookup as well as ORIGIN xattr on upper. Path based lookup
>   is continued in this case to find lower data dentry for metacopy
>   upper. So like directories we only use verified origin. If ORIGIN
>   xattr is not present (Either because lower did not support file
>   handles or because this is hardlink copied up with index=off), then
>   don't use path lookup based lower dentry as origin. This is same
>   as regular non-metacopy file case.
>

Very good summary.
You may add:
Reviewed-by: Amir Goldstein <amir73il@gmail.com>

But see one improvement below.
Also, please make sure to run unionmount setups:

./run --ov=10 --verify
./run --ov=10 --meta --verify

--verify will enable index and check st_dev;st_ino are not broken
on copy up. --ov=10 will cause lower hardlink copy up, because
after hardlink is creates by some test, upper is rotated to mid layer
and next modifying operation will trigger the hardlink copy up.

> Suggested-by: Amir Goldstein <amir73il@gmail.com>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/overlayfs/namei.c | 29 +++++++++++++++++------------
>  1 file changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index 0db23baf98e7..5d80d8cc0063 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -1005,25 +1005,30 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                 }
>                 stack = origin_path;
>                 ctr = 1;
> +               origin = origin_path->dentry;
>                 origin_path = NULL;
>         }
>
[...]
> -       if (ctr && (!upperdentry || (!d.is_dir && !metacopy)))
> +       if (!origin && ctr && !upperdentry)
>                 origin = stack[0].dentry;
>

No need to understand the long story to verify this change is correct.
This is true simply because the conditions to set stack = origin_path are:

       if (!metacopy && !d.is_dir && upperdentry && !ctr && origin_path)

And after getting there and setting ctr = 1, the complex conditions to
setting origin are met for certain:

        if (ctr && (!upperdentry || (!d.is_dir && !metacopy)))

Therefore, it is logically equivalent (and makes much more sense)
to assign origin near stack = origin_path.

Further, thanks to Vivek's explanation, it is now clear to me that after
setting origin above, all that is left to do here is:

         /* Always lookup index of non-upper */
        if (!upperdentry)
                origin = stack[0].dentry;

All the upperdentry cases described in the commit message have already
been dealt with by the time we get here.

Thanks,
Amir.

Re: [PATCH 2/3] overlayfs: ovl_lookup(): Use only uppermetacopy state

[Amir Goldstein]

On Sat, May 30, 2020 at 12:30 AM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> Currently we use a variable "metacopy" which signifies that dentry
> could be either uppermetacopy or lowermetacopy. Amir suggested that
> we can move code around and use d.metacopy in such a way that we
> don't need lowermetacopy and just can do away with uppermetacopy.
>
> So this patch replaces "metacopy" with "uppermetacopy".
>
> It also moves some code little higher to keep reading little simpler.
>
> Suggested-by: Amir Goldstein <amir73il@gmail.com>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  fs/overlayfs/namei.c | 57 ++++++++++++++++++++++----------------------
>  1 file changed, 28 insertions(+), 29 deletions(-)
>
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index 5d80d8cc0063..a1889a160708 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -823,7 +823,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>         struct dentry *this;
>         unsigned int i;
>         int err;
> -       bool metacopy = false;
> +       bool uppermetacopy = false;
>         struct ovl_lookup_data d = {
>                 .sb = dentry->d_sb,
>                 .name = dentry->d_name,
> @@ -869,7 +869,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                                 goto out_put_upper;
>
>                         if (d.metacopy)
> -                               metacopy = true;
> +                               uppermetacopy = true;
>                 }
>
>                 if (d.redirect) {
> @@ -906,6 +906,22 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                 if (!this)
>                         continue;
>
> +               if ((uppermetacopy || d.metacopy) && !ofs->config.metacopy) {
> +                       err = -EPERM;
> +                       pr_warn_ratelimited("refusing to follow metacopy origin"
> +                                           " for (%pd2)\n", dentry);
> +                       goto out_put;
> +               }
> +
> +               /*
> +                * Do not store intermediate metacopy dentries in chain,
> +                * except top most lower metacopy dentry
> +                */
> +               if (d.metacopy && ctr) {
> +                       dput(this);
> +                       continue;
> +               }
> +
>                 /*
>                  * If no origin fh is stored in upper of a merge dir, store fh
>                  * of lower dir and set upper parent "impure".
> @@ -940,17 +956,6 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                         origin = this;
>                 }
>
> -               if (d.metacopy)
> -                       metacopy = true;
> -               /*
> -                * Do not store intermediate metacopy dentries in chain,
> -                * except top most lower metacopy dentry
> -                */
> -               if (d.metacopy && ctr) {
> -                       dput(this);
> -                       continue;
> -               }
> -
>                 stack[ctr].dentry = this;
>                 stack[ctr].layer = lower.layer;
>                 ctr++;
> @@ -982,23 +987,17 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                 }
>         }
>
> -       if (metacopy) {
> -               /*
> -                * Found a metacopy dentry but did not find corresponding
> -                * data dentry
> -                */
> -               if (d.metacopy) {
> -                       err = -EIO;
> -                       goto out_put;
> -               }
> +       /* Found a metacopy dentry but did not find corresponding data dentry */
> +       if (d.metacopy) {

I suggested this change and I think it is correct, but it is correct for a bit
of a subtle reason.
It is correct because ovl_lookup_layer() (currently) cannot return NULL
and set d.metacopy to false.
So I suggest to be a bit more defensive and write this condition as:

       if (d.metacopy || (uppermetacopy && !ctr)) {

> +               err = -EIO;
> +               goto out_put;
> +       }
>
> -               err = -EPERM;
> -               if (!ofs->config.metacopy) {
> -                       pr_warn_ratelimited("refusing to follow metacopy origin for (%pd2)\n",
> -                                           dentry);
> -                       goto out_put;
> -               }
> -       } else if (!d.is_dir && upperdentry && !ctr && origin_path) {
> +       /* For regular non-metacopy upper dentries, there is no lower
> +        * path based lookup, hence ctr will be zero. dentry found using
> +        * ORIGIN xattr on upper, install it in stack.
> +        */
> +       if (!d.is_dir && upperdentry && !ctr && origin_path) {

I don't like this comment style for multi line comment and I don't
like that you detached this if statement from else if.
I think it made more sense with the else because this is (as you write)
the non-metacopy case.

Thanks,
Amir.

Re: [PATCH 3/3] overlayfs: Initialize OVL_UPPERDATA in ovl_lookup()

[Amir Goldstein]

On Sat, May 30, 2020 at 12:30 AM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> Currently ovl_get_inode() initializes OVL_UPPERDATA flag and for that it
> has to call ovl_check_metacopy_xattr() and check if metacopy xattr is
> present or not.
>
> yangerkun reported sometimes underlying filesystem might return -EIO
> and in that case error handling path does not cleanup properly leading
> to various warnings.
>
> Run generic/461 with ext4 upper/lower layer sometimes may trigger the
> bug as below(linux 4.19):
>
> [  551.001349] overlayfs: failed to get metacopy (-5)
> [  551.003464] overlayfs: failed to get inode (-5)
> [  551.004243] overlayfs: cleanup of 'd44/fd51' failed (-5)
> [  551.004941] overlayfs: failed to get origin (-5)
> [  551.005199] ------------[ cut here ]------------
> [  551.006697] WARNING: CPU: 3 PID: 24674 at fs/inode.c:1528 iput+0x33b/0x400
> ...
> [  551.027219] Call Trace:
> [  551.027623]  ovl_create_object+0x13f/0x170
> [  551.028268]  ovl_create+0x27/0x30
> [  551.028799]  path_openat+0x1a35/0x1ea0
> [  551.029377]  do_filp_open+0xad/0x160
> [  551.029944]  ? vfs_writev+0xe9/0x170
> [  551.030499]  ? page_counter_try_charge+0x77/0x120
> [  551.031245]  ? __alloc_fd+0x160/0x2a0
> [  551.031832]  ? do_sys_open+0x189/0x340
> [  551.032417]  ? get_unused_fd_flags+0x34/0x40
> [  551.033081]  do_sys_open+0x189/0x340
> [  551.033632]  __x64_sys_creat+0x24/0x30
> [  551.034219]  do_syscall_64+0xd5/0x430
> [  551.034800]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> One solution is to improve error handling and call iget_failed() if error
> is encountered. Amir thinks that this path is little intricate and there
> is not real need to check and initialize OVL_UPPERDATA in ovl_get_inode().
> Instead caller of ovl_get_inode() can initialize this state. And this
> will avoid double checking of metacopy xattr lookup in ovl_lookup()
> and ovl_get_inode().
>
> OVL_UPPERDATA is inode flag. So I was little concerned that initializing
> it outside ovl_get_inode() might have some races. But this is one way
> transition. That is once a file has been fully copied up, it can't go
> back to metacopy file again. And that seems to help avoid races. So
> as of now I can't see any races w.r.t OVL_UPPERDATA being set wrongly. So
> move settingof OVL_UPPERDATA inside the callers of ovl_get_inode().
> ovl_obtain_alias() already does it. So only two callers now left
> are ovl_lookup() and ovl_instantiate().
>
> Reported-by: yangerkun <yangerkun@huawei.com>
> Suggested-by: Amir Goldstein <amir73il@gmail.com>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>

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

> ---
>  fs/overlayfs/dir.c   |  2 ++
>  fs/overlayfs/inode.c | 11 +----------
>  fs/overlayfs/namei.c |  2 ++
>  3 files changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/fs/overlayfs/dir.c b/fs/overlayfs/dir.c
> index 279009dee366..a7cac2ce0fad 100644
> --- a/fs/overlayfs/dir.c
> +++ b/fs/overlayfs/dir.c
> @@ -262,6 +262,8 @@ static int ovl_instantiate(struct dentry *dentry, struct inode *inode,
>                 inode = ovl_get_inode(dentry->d_sb, &oip);
>                 if (IS_ERR(inode))
>                         return PTR_ERR(inode);
> +               if (inode == oip.newinode)
> +                       ovl_set_flag(OVL_UPPERDATA, inode);
>         } else {
>                 WARN_ON(ovl_inode_real(inode) != d_inode(newdentry));
>                 dput(newdentry);
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index 981f11ec51bc..f2aaf00821c0 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -957,7 +957,7 @@ struct inode *ovl_get_inode(struct super_block *sb,
>         bool bylower = ovl_hash_bylower(sb, upperdentry, lowerdentry,
>                                         oip->index);
>         int fsid = bylower ? lowerpath->layer->fsid : 0;
> -       bool is_dir, metacopy = false;
> +       bool is_dir;
>         unsigned long ino = 0;
>         int err = oip->newinode ? -EEXIST : -ENOMEM;
>
> @@ -1018,15 +1018,6 @@ struct inode *ovl_get_inode(struct super_block *sb,
>         if (oip->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);
> -       }
> -
>         OVL_I(inode)->redirect = oip->redirect;
>
>         if (bylower)
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index a1889a160708..36e2b88a2fd1 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -1078,6 +1078,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>                 err = PTR_ERR(inode);
>                 if (IS_ERR(inode))
>                         goto out_free_oe;
> +               if (upperdentry && !uppermetacopy)
> +                       ovl_set_flag(OVL_UPPERDATA, inode);
>         }
>
>         ovl_dentry_update_reval(dentry, upperdentry,
> --
> 2.25.4
>

Re: [PATCH 1/3] overlayfs: Simplify setting of origin for index lookup

[Vivek Goyal]

On Sat, May 30, 2020 at 01:37:37PM +0300, Amir Goldstein wrote:
> On Sat, May 30, 2020 at 12:30 AM Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > overlayfs can keep index of copied up files and directories and it
> > seems to serve two primary puroposes. For regular files, it avoids
> > breaking lower hardlinks over copy up. For directories it seems to
> > be used for various error checks.
> >
> > During ovl_lookup(), we lookup for index using lower dentry in many
> > a cases. That lower dentry is called "origin" and following is a summary
> > of current logic.
> >
> > If there is no upperdentry, always lookup for index using lower dentry.
> > For regular files it helps avoiding breaking hard links over copyup
> > and for directories it seems to be just error checks.
> >
> > If there is an upperdentry, then there are 3 possible cases.
> >
> > - For directories, lower dentry is found using two ways. One is regular
> >   path based lookup in lower layers and second is using ORIGIN xattr
> >   on upper dentry. First verify that path based lookup lower dentry
> >   matches the one pointed by upper ORIGIN xattr. If yes, use this
> >   verified origin for index lookup.
> >
> > - For regular files (non-metacopy), there is no path based lookup in
> >   lower layers as lookup stops once we find upper dentry. So there
> >   is no origin verification. If there is ORIGIN xattr present on upper,
> >   use that to lookup index otherwise don't.
> >
> > - For regular metacopy files, again lower dentry is found using
> >   path based lookup as well as ORIGIN xattr on upper. Path based lookup
> >   is continued in this case to find lower data dentry for metacopy
> >   upper. So like directories we only use verified origin. If ORIGIN
> >   xattr is not present (Either because lower did not support file
> >   handles or because this is hardlink copied up with index=off), then
> >   don't use path lookup based lower dentry as origin. This is same
> >   as regular non-metacopy file case.
> >
> 
> Very good summary.
> You may add:
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> 
> But see one improvement below.
> Also, please make sure to run unionmount setups:
> 
> ./run --ov=10 --verify
> ./run --ov=10 --meta --verify
> 
> --verify will enable index and check st_dev;st_ino are not broken
> on copy up. --ov=10 will cause lower hardlink copy up, because
> after hardlink is creates by some test, upper is rotated to mid layer
> and next modifying operation will trigger the hardlink copy up.

Hi Amir,

I ran above configurations and it passes with the patches.

Thanks for these suggestions. I used to run only "./run --ov" so far.
It will be nice to have some documentation about --meta, --verify in README.

> 
> > Suggested-by: Amir Goldstein <amir73il@gmail.com>
> > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > ---
> >  fs/overlayfs/namei.c | 29 +++++++++++++++++------------
> >  1 file changed, 17 insertions(+), 12 deletions(-)
> >
> > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> > index 0db23baf98e7..5d80d8cc0063 100644
> > --- a/fs/overlayfs/namei.c
> > +++ b/fs/overlayfs/namei.c
> > @@ -1005,25 +1005,30 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> >                 }
> >                 stack = origin_path;
> >                 ctr = 1;
> > +               origin = origin_path->dentry;
> >                 origin_path = NULL;
> >         }
> >
> [...]
> > -       if (ctr && (!upperdentry || (!d.is_dir && !metacopy)))
> > +       if (!origin && ctr && !upperdentry)
> >                 origin = stack[0].dentry;
> >
> 
> No need to understand the long story to verify this change is correct.
> This is true simply because the conditions to set stack = origin_path are:
> 
>        if (!metacopy && !d.is_dir && upperdentry && !ctr && origin_path)
> 
> And after getting there and setting ctr = 1, the complex conditions to
> setting origin are met for certain:
> 
>         if (ctr && (!upperdentry || (!d.is_dir && !metacopy)))
> 
> Therefore, it is logically equivalent (and makes much more sense)
> to assign origin near stack = origin_path.
> 
> Further, thanks to Vivek's explanation, it is now clear to me that after
> setting origin above, all that is left to do here is:
> 
>          /* Always lookup index of non-upper */
>         if (!upperdentry)
>                 origin = stack[0].dentry;

This looks better. What about the case of non existing dentry with ctr=0.
In that case we will set origin to NULL. It still works but it probably
will be nice if we do.

	/* Always lookup index of non-upper */
	if (!upperdentry && ctr)
                 origin = stack[0].dentry;

Just making it explicit that we try to use lower as origin only if
some lower dentry was found.

Thanks
Vivek

Re: [PATCH 1/3] overlayfs: Simplify setting of origin for index lookup

[Amir Goldstein]

On Mon, Jun 1, 2020 at 5:04 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Sat, May 30, 2020 at 01:37:37PM +0300, Amir Goldstein wrote:
> > On Sat, May 30, 2020 at 12:30 AM Vivek Goyal <vgoyal@redhat.com> wrote:
> > >
> > > overlayfs can keep index of copied up files and directories and it
> > > seems to serve two primary puroposes. For regular files, it avoids
> > > breaking lower hardlinks over copy up. For directories it seems to
> > > be used for various error checks.
> > >
> > > During ovl_lookup(), we lookup for index using lower dentry in many
> > > a cases. That lower dentry is called "origin" and following is a summary
> > > of current logic.
> > >
> > > If there is no upperdentry, always lookup for index using lower dentry.
> > > For regular files it helps avoiding breaking hard links over copyup
> > > and for directories it seems to be just error checks.
> > >
> > > If there is an upperdentry, then there are 3 possible cases.
> > >
> > > - For directories, lower dentry is found using two ways. One is regular
> > >   path based lookup in lower layers and second is using ORIGIN xattr
> > >   on upper dentry. First verify that path based lookup lower dentry
> > >   matches the one pointed by upper ORIGIN xattr. If yes, use this
> > >   verified origin for index lookup.
> > >
> > > - For regular files (non-metacopy), there is no path based lookup in
> > >   lower layers as lookup stops once we find upper dentry. So there
> > >   is no origin verification. If there is ORIGIN xattr present on upper,
> > >   use that to lookup index otherwise don't.
> > >
> > > - For regular metacopy files, again lower dentry is found using
> > >   path based lookup as well as ORIGIN xattr on upper. Path based lookup
> > >   is continued in this case to find lower data dentry for metacopy
> > >   upper. So like directories we only use verified origin. If ORIGIN
> > >   xattr is not present (Either because lower did not support file
> > >   handles or because this is hardlink copied up with index=off), then
> > >   don't use path lookup based lower dentry as origin. This is same
> > >   as regular non-metacopy file case.
> > >
> >
> > Very good summary.
> > You may add:
> > Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> >
> > But see one improvement below.
> > Also, please make sure to run unionmount setups:
> >
> > ./run --ov=10 --verify
> > ./run --ov=10 --meta --verify
> >
> > --verify will enable index and check st_dev;st_ino are not broken
> > on copy up. --ov=10 will cause lower hardlink copy up, because
> > after hardlink is creates by some test, upper is rotated to mid layer
> > and next modifying operation will trigger the hardlink copy up.
>
> Hi Amir,
>
> I ran above configurations and it passes with the patches.
>
> Thanks for these suggestions. I used to run only "./run --ov" so far.
> It will be nice to have some documentation about --meta, --verify in README.
>

Yeh, I never get to it.
But now I finally posted xfstest integration, so we can just add more
xfstests to run metacopy configurations on a full test cycle.

> >
> > > Suggested-by: Amir Goldstein <amir73il@gmail.com>
> > > Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> > > ---
> > >  fs/overlayfs/namei.c | 29 +++++++++++++++++------------
> > >  1 file changed, 17 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> > > index 0db23baf98e7..5d80d8cc0063 100644
> > > --- a/fs/overlayfs/namei.c
> > > +++ b/fs/overlayfs/namei.c
> > > @@ -1005,25 +1005,30 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> > >                 }
> > >                 stack = origin_path;
> > >                 ctr = 1;
> > > +               origin = origin_path->dentry;
> > >                 origin_path = NULL;
> > >         }
> > >
> > [...]
> > > -       if (ctr && (!upperdentry || (!d.is_dir && !metacopy)))
> > > +       if (!origin && ctr && !upperdentry)
> > >                 origin = stack[0].dentry;
> > >
> >
> > No need to understand the long story to verify this change is correct.
> > This is true simply because the conditions to set stack = origin_path are:
> >
> >        if (!metacopy && !d.is_dir && upperdentry && !ctr && origin_path)
> >
> > And after getting there and setting ctr = 1, the complex conditions to
> > setting origin are met for certain:
> >
> >         if (ctr && (!upperdentry || (!d.is_dir && !metacopy)))
> >
> > Therefore, it is logically equivalent (and makes much more sense)
> > to assign origin near stack = origin_path.
> >
> > Further, thanks to Vivek's explanation, it is now clear to me that after
> > setting origin above, all that is left to do here is:
> >
> >          /* Always lookup index of non-upper */
> >         if (!upperdentry)
> >                 origin = stack[0].dentry;
>
> This looks better. What about the case of non existing dentry with ctr=0.
> In that case we will set origin to NULL. It still works but it probably
> will be nice if we do.

Forgot about that.

>
>         /* Always lookup index of non-upper */
>         if (!upperdentry && ctr)
>                  origin = stack[0].dentry;
>
> Just making it explicit that we try to use lower as origin only if
> some lower dentry was found.
>

Looks good.
Sustaining my RVB

Thanks,
Amir.

Re: [PATCH 2/3] overlayfs: ovl_lookup(): Use only uppermetacopy state

[Vivek Goyal]

On Sat, May 30, 2020 at 02:01:28PM +0300, Amir Goldstein wrote:
[..]
> > @@ -982,23 +987,17 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
> >                 }
> >         }
> >
> > -       if (metacopy) {
> > -               /*
> > -                * Found a metacopy dentry but did not find corresponding
> > -                * data dentry
> > -                */
> > -               if (d.metacopy) {
> > -                       err = -EIO;
> > -                       goto out_put;
> > -               }
> > +       /* Found a metacopy dentry but did not find corresponding data dentry */
> > +       if (d.metacopy) {
> 
> I suggested this change and I think it is correct, but it is correct for a bit
> of a subtle reason.
> It is correct because ovl_lookup_layer() (currently) cannot return NULL
> and set d.metacopy to false.
> So I suggest to be a bit more defensive and write this condition as:
> 
>        if (d.metacopy || (uppermetacopy && !ctr)) {

Ok, will do.

> 
> > +               err = -EIO;
> > +               goto out_put;
> > +       }
> >
> > -               err = -EPERM;
> > -               if (!ofs->config.metacopy) {
> > -                       pr_warn_ratelimited("refusing to follow metacopy origin for (%pd2)\n",
> > -                                           dentry);
> > -                       goto out_put;
> > -               }
> > -       } else if (!d.is_dir && upperdentry && !ctr && origin_path) {
> > +       /* For regular non-metacopy upper dentries, there is no lower
> > +        * path based lookup, hence ctr will be zero. dentry found using
> > +        * ORIGIN xattr on upper, install it in stack.
> > +        */
> > +       if (!d.is_dir && upperdentry && !ctr && origin_path) {
> 
> I don't like this comment style for multi line comment and I don't
> like that you detached this if statement from else if.
> I think it made more sense with the else because this is (as you write)
> the non-metacopy case.

Will do in V2.

Thanks
Vivek

Re: [PATCH 1/3] overlayfs: Simplify setting of origin for index lookup

[Vivek Goyal]

On Mon, Jun 01, 2020 at 06:15:44PM +0300, Amir Goldstein wrote:

[..]
> > > But see one improvement below.
> > > Also, please make sure to run unionmount setups:
> > >
> > > ./run --ov=10 --verify
> > > ./run --ov=10 --meta --verify
> > >
> > > --verify will enable index and check st_dev;st_ino are not broken
> > > on copy up. --ov=10 will cause lower hardlink copy up, because
> > > after hardlink is creates by some test, upper is rotated to mid layer
> > > and next modifying operation will trigger the hardlink copy up.
> >
> > Hi Amir,
> >
> > I ran above configurations and it passes with the patches.
> >
> > Thanks for these suggestions. I used to run only "./run --ov" so far.
> > It will be nice to have some documentation about --meta, --verify in README.
> >
> 
> Yeh, I never get to it.
> But now I finally posted xfstest integration, so we can just add more
> xfstests to run metacopy configurations on a full test cycle.

Yes, I saw those patches. Very nice. Will be good to run all overlay
tests from single place. Once your patches get merged, I definitely
want to add some more tests to run "--meta" tests.

Vivek