[PATCH] ovl: warn about orphan metacopy

[PATCH] ovl: warn about orphan metacopy

[Kevin Locke]

When the lower file of a metacopy is inaccessible, -EIO is returned.
For users not familiar with overlayfs internals, such as myself, the
meaning of this error may not be apparent or easy to determine, since
the (metacopy) file is present and open/stat succeed when accessed
outside of the overlay.

Add a rate-limited warning for invalid metacopy to give users a hint
when investigating such errors, as discussed on linux-unionfs[0].  Use
"orphan metacopy" terminology to match "orphan index entry" in
ovl_verify_index.

[0]: https://lore.kernel.org/linux-unionfs/CAOQ4uxi23Zsmfb4rCed1n=On0NNA5KZD74jjjeyz+et32sk-gg@mail.gmail.com/

Signed-off-by: Kevin Locke <kevin@kevinlocke.name>
---
 fs/overlayfs/namei.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index f7d4358db637..30e1c10800ab 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -1000,6 +1000,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 	 * Just make sure a corresponding data dentry has been found.
 	 */
 	if (d.metacopy || (uppermetacopy && !ctr)) {
+		pr_warn_ratelimited("orphan metacopy (%pd2)\n", dentry);
 		err = -EIO;
 		goto out_put;
 	} else if (!d.is_dir && upperdentry && !ctr && origin_path) {
-- 
2.28.0

Re: [PATCH] ovl: warn about orphan metacopy

[Amir Goldstein]

On Sun, Aug 23, 2020 at 5:14 AM Kevin Locke <kevin@kevinlocke.name> wrote:
>
> When the lower file of a metacopy is inaccessible, -EIO is returned.
> For users not familiar with overlayfs internals, such as myself, the
> meaning of this error may not be apparent or easy to determine, since
> the (metacopy) file is present and open/stat succeed when accessed
> outside of the overlay.
>
> Add a rate-limited warning for invalid metacopy to give users a hint
> when investigating such errors, as discussed on linux-unionfs[0].  Use
> "orphan metacopy" terminology to match "orphan index entry" in
> ovl_verify_index.
>
> [0]: https://lore.kernel.org/linux-unionfs/CAOQ4uxi23Zsmfb4rCed1n=On0NNA5KZD74jjjeyz+et32sk-gg@mail.gmail.com/
>
> Signed-off-by: Kevin Locke <kevin@kevinlocke.name>
> ---
>  fs/overlayfs/namei.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
> index f7d4358db637..30e1c10800ab 100644
> --- a/fs/overlayfs/namei.c
> +++ b/fs/overlayfs/namei.c
> @@ -1000,6 +1000,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>          * Just make sure a corresponding data dentry has been found.
>          */
>         if (d.metacopy || (uppermetacopy && !ctr)) {
> +               pr_warn_ratelimited("orphan metacopy (%pd2)\n", dentry);

Funny. You started this thread because of a pain point - you did not know
what caused EIO in your setup.

Try to go back to where you stood when you got EIO.
Would that message in the log would have helped you understand?
Would it have helped someone who is less skilled than you are in reading
kernel code? I doubt it.

You better be more explicit about what has gone wrong, e.g.:
"metacopy upper with no lower data found - abort lookup..."

It is nice that you followed a precedent of "orphan index", but if you
look closely you will see that those cases do not end up with a user
error - they end up with auto cleaning those "orphan index", so the
kernel messages are just FYI - it doesn't matter if users understand them
because they do not require users to take any action.

Thanks,
Amir.

Re: [PATCH] ovl: warn about orphan metacopy

[Kevin Locke]

On Sun, 2020-08-23 at 17:12 +0300, Amir Goldstein wrote:
> On Sun, Aug 23, 2020 at 5:14 AM Kevin Locke <kevin@kevinlocke.name> wrote:
>> diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
>> index f7d4358db637..30e1c10800ab 100644
>> --- a/fs/overlayfs/namei.c
>> +++ b/fs/overlayfs/namei.c
>> @@ -1000,6 +1000,7 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
>>          * Just make sure a corresponding data dentry has been found.
>>          */
>>         if (d.metacopy || (uppermetacopy && !ctr)) {
>> +               pr_warn_ratelimited("orphan metacopy (%pd2)\n", dentry);
> 
> Funny. You started this thread because of a pain point - you did not know
> what caused EIO in your setup.
> 
> Try to go back to where you stood when you got EIO.
> Would that message in the log would have helped you understand?
> Would it have helped someone who is less skilled than you are in reading
> kernel code? I doubt it.

After I was unable to reproduce EIO by accessing the file on upper
directly and had no error message, I started grepping for EIO in
overlayfs, which led to multiple results and no clear next step (kernel
tracing helped, but was still difficult to isolate the source of EIO).
Having any error message would get me to the point in the code where the
error was encountered.  Mentioning metacopy gets me to a causal feature,
which would have been helpful for understanding.

> You better be more explicit about what has gone wrong, e.g.:
> "metacopy upper with no lower data found - abort lookup..."
> 
> It is nice that you followed a precedent of "orphan index", but if you
> look closely you will see that those cases do not end up with a user
> error - they end up with auto cleaning those "orphan index", so the
> kernel messages are just FYI - it doesn't matter if users understand them
> because they do not require users to take any action.

Sure.  That wording sounds better to me.  I'll send an updated patch
shortly.

Thanks,
Kevin

[PATCH v2] ovl: warn about orphan metacopy

[Kevin Locke]

When the lower file of a metacopy is inaccessible, -EIO is returned.
For users not familiar with overlayfs internals, such as myself, the
meaning of this error may not be apparent or easy to determine, since
the (metacopy) file is present and open/stat succeed when accessed
outside of the overlay.

Add a rate-limited warning for orphan metacopy to give users a hint
when investigating such errors, as discussed on linux-unionfs[0].

[0]: https://lore.kernel.org/linux-unionfs/CAOQ4uxi23Zsmfb4rCed1n=On0NNA5KZD74jjjeyz+et32sk-gg@mail.gmail.com/

Signed-off-by: Kevin Locke <kevin@kevinlocke.name>
---

Changes since v1:
- Use message text similar to suggestion by Amir Goldstein, except that
  "upper" is removed to avoid confusion if the metacopy is on a middle
  (i.e. lower-but-not-lowest) layer.

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

diff --git a/fs/overlayfs/namei.c b/fs/overlayfs/namei.c
index f7d4358db637..57cd048ce0af 100644
--- a/fs/overlayfs/namei.c
+++ b/fs/overlayfs/namei.c
@@ -1000,6 +1000,8 @@ struct dentry *ovl_lookup(struct inode *dir, struct dentry *dentry,
 	 * Just make sure a corresponding data dentry has been found.
 	 */
 	if (d.metacopy || (uppermetacopy && !ctr)) {
+		pr_warn_ratelimited("metacopy with no lower data found - abort lookup (%pd2)\n",
+				    dentry);
 		err = -EIO;
 		goto out_put;
 	} else if (!d.is_dir && upperdentry && !ctr && origin_path) {
-- 
2.28.0

Re: [PATCH v2] ovl: warn about orphan metacopy

[Miklos Szeredi]

On Sun, Aug 23, 2020 at 4:38 PM Kevin Locke <kevin@kevinlocke.name> wrote:
>
> When the lower file of a metacopy is inaccessible, -EIO is returned.
> For users not familiar with overlayfs internals, such as myself, the
> meaning of this error may not be apparent or easy to determine, since
> the (metacopy) file is present and open/stat succeed when accessed
> outside of the overlay.
>
> Add a rate-limited warning for orphan metacopy to give users a hint
> when investigating such errors, as discussed on linux-unionfs[0].
>
> [0]: https://lore.kernel.org/linux-unionfs/CAOQ4uxi23Zsmfb4rCed1n=On0NNA5KZD74jjjeyz+et32sk-gg@mail.gmail.com/
>
> Signed-off-by: Kevin Locke <kevin@kevinlocke.name>

Thanks, applied.

Miklos