Re: EIO for removed redirected files?

[Kevin Locke <kevin@kevinlocke.name>]

Thanks for the quick response Amir!

On Wed, 2020-08-12 at 18:21 +0300, Amir Goldstein wrote:
> On Wed, Aug 12, 2020 at 5:06 PM Kevin Locke <kevin@kevinlocke.name> wrote:
>> 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.

When I replace `touch foo.txt` with `echo 123 > foo.txt` I observe the
same behavior.  If I understand you correctly, you are saying that EIO
is correct for non-empty files, but potentially incorrect for empty
files (which could be copied rather than redirected, since there is no
space saving)?

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

After reflection, EIO probably makes the most sense for open/stat.  It
might be nice to be able to unlink the file to allow recovery (in the
sense of being able to reuse the name) without unmounting the overlay,
but the documentation updates may be sufficient to keep users from
getting into this state.

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

That sounds good to me.  My current use case involves offline changes to
the lower layer on a routine basis, and I interpreted the current
wording "Offline changes, when the overlay is not mounted, are allowed
to either the upper or the lower trees." to mean that such offline
modifications would not break things in unexpected ways.

In retrospect, I should have expected this behavior, but as someone
previously unfamiliar with overlayfs, I hadn't considered that metacopy
results in file redirects and that if the underlying file were removed
without removing any redirects pointing to it that it would manifest in
this way and be so difficult to clean up.

If metacopy and dir_redirect are disabled, are offline modifications to
the lower layer permitted, or could any such modification result in
undefined behavior?

>> 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().

Ah, indeed.  Would doing so for missing/invalid metacopy/redirect make
sense?

> 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 for the patch!  (Especially so quickly.)  I'll try it out soon.

Thanks again,
Kevin