Re: EIO for removed redirected files?

[Amir Goldstein <amir73il@gmail.com>]

On Wed, Aug 12, 2020 at 5:06 PM Kevin Locke <kevin@kevinlocke.name> wrote:
>
> Hi All,
>
> I recently encountered files on an overlayfs which returned EIO
> (Input/output error) for open, stat, and unlink (and presumably other)
> syscalls.  I eventually determined that the files had been redirected

It's *empty* redirected files that cause the alleged problem.
If files were not empty, the EIO would have been expected, because...

> and the target removed from the lower level.  The behavior can be
> reproduced as follows:
>
> # Create overlay with foo.txt on lower level
> mkdir -p lower upper work merged
> touch lower/foo.txt

Suppose this was:
echo 123 >  lower/foo.txt

> mount -t overlay -o lowerdir=lower,upperdir=upper,workdir=work,metacopy=on none merged
>
> # Redirect bar.txt on upper to foo.txt on lower
> mv merged/foo.txt merged/bar.txt
>

...this mv does not copy up "123" to foo.txt before renaming it to bar.txt...

> # Remove foo.txt on lower while unmounted
> umount merged
> rm lower/foo.txt
>
> # open, stat, and unlink on bar.txt now fail with EIO:
> mount -t overlay -o lowerdir=lower,upperdir=upper,workdir=work,metacopy=on none merged
> cat merged/bar.txt
> stat merged/bar.txt
> rm merged/bar.txt
>
> At this point, the only way to recover appears to be unmounting the
> overlay and removing the file from upper (or updating the
> overlay.redirect xattr to a valid location).  Is that correct?
>
> Is this the intended behavior?

Yes.
What would you expect to happen when data of metacopy file has been removed?

> I didn't see any tests covering it in

We have some tests in xfstests to prove that modifying underlying
layers does not
result in a crash, but otherwise the behavior is undefined, so it is
hard to write tests.
There is some code and tests in place for better behavior on
underlying file changes
like not exposing whiteouts in upper dir when lower dir was removed.

> unionmount-testsuite.  If so, perhaps the behavior could be noted in
> "Changes to underlying filesystems" in
> Documentation/filesystems/overlayfs.rst?  I'd be willing to write a
> first draft.  (I doubt I understand it well enough to get everything
> right on the first try.)
>

I guess the only thing we could document is that changes to underlying
layers with metacopy and redirects have undefined results.
Vivek was a proponent of making the statements about outcome of
changes to underlying layers sound more harsh.

> Also, if there is any way this could be made easier to debug, it might
> be helpful for users, particularly newbies like myself.  Perhaps logging
> bad redirects at KERN_ERR?  If that would be too noisy, perhaps only
> behind a debug module option?  Again, if this is acceptable I'd be
> willing to draft a patch.  (Same caveat as above.)
>

There are a handful of places in overlayfs where returning EIO is
combined with informative pr_warn_ratelimited().
You can see some examples in ovl_lookup(), which is where the reported
EIO is coming from:
        if (d.metacopy || (uppermetacopy && !ctr)) {
                err = -EIO;


Having said all that, metacopy and redirects on lower empty files seems
like an unintentional outcome.

If you care about this use case particularly, you may try this untested patch:

Thanks,
Amir.

---
diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index d07fb92b7253..178067cb6583 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -764,7 +764,7 @@ static int ovl_do_copy_up(struct ovl_copy_up_ctx *c)
        return err;
 }

-static bool ovl_need_meta_copy_up(struct dentry *dentry, umode_t mode,
+static bool ovl_need_meta_copy_up(struct dentry *dentry, struct kstat *stat,
                                  int flags)
 {
        struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
@@ -772,7 +772,7 @@ static bool ovl_need_meta_copy_up(struct dentry
*dentry, umode_t mode,
        if (!ofs->config.metacopy)
                return false;

-       if (!S_ISREG(mode))
+       if (!S_ISREG(stat->mode) || !stat->size)
                return false;

        if (flags && ((OPEN_FMODE(flags) & FMODE_WRITE) || (flags & O_TRUNC)))
@@ -852,8 +852,6 @@ static int ovl_copy_up_one(struct dentry *parent,
struct dentry *dentry,
        if (err)
                return err;

-       ctx.metacopy = ovl_need_meta_copy_up(dentry, ctx.stat.mode, flags);
-
        if (parent) {
                ovl_path_upper(parent, &parentpath);
                ctx.destdir = parentpath.dentry;
@@ -870,6 +868,8 @@ static int ovl_copy_up_one(struct dentry *parent,
struct dentry *dentry,
        if (flags & O_TRUNC)
                ctx.stat.size = 0;

+       ctx.metacopy = ovl_need_meta_copy_up(dentry, &ctx.stat, flags);
+
        if (S_ISLNK(ctx.stat.mode)) {
                ctx.link = vfs_get_link(ctx.lowerpath.dentry, &done);
                if (IS_ERR(ctx.link))