Re: EIO for removed redirected files?

[Vivek Goyal <vgoyal@redhat.com>]

On Wed, Aug 12, 2020 at 08:06:34PM +0300, Amir Goldstein wrote:
> On Wed, Aug 12, 2020 at 7:05 PM Kevin Locke <kevin@kevinlocke.name> wrote:
> >
> > 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)?
> >
> 
> I wouldn't call it "incorrect" more like "unnecessary".
> 
> > >> 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,
> 
> It would be nice, but somebody needs to care enough to implement it
> and it is not going to be trivial, because error on lookup is much easier
> then selective error on a "broken" dentry depending on the operation...
> 
> > 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
> 
> You are not the only one, I hear of many users that do that, but nobody ever
> bothered to sit down and document the requirements - what exactly is the
> use case and what is the expected outcome.
> 
> > 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.
> >
> 
> The truth is that this documentation is old, before all the new features
> were added. See here [1], Vivek suggested:
> "Modifying/recreating lower layer only works when
>  metacopy/index/nfs_export are not enabled at any point of time. This
>  also will change inode number reporting behavior."
> 
> > 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?
> >
> 
> With metacopy/index/nfs_export/redirect_dir disabled code behaves mostly
> like it did at the time that this documentation was written, so I guess you may
> say that changes are permitted and result in "defined" behavior.

I still think that we should make it clear in documentation that
modifying and recreating lower layers is not allowed when advanced
features like metacopy/index/nfs_export/redirect_dir are enabled. If
one does so, expect the unexpected.

https://lore.kernel.org/linux-unionfs/20200709153616.GE150543@redhat.com/T/#t

To me, if we allow any random modification/recreation of lower filesystem,
then definiting the behavior in all the corner cases becomes hard and
it also makes design of overlayfs more complicated because anytime you
are implementing something, now you have to worry about modifications to
lower layer as well.

And I am also not aware of any good use cases which justify supporting
lower layer modification with new features. So remain of the opinion
that don't support it.

Thanks
Vivek