Re: [PATCH RFC 0/2] Invalidate overlayfs dentries on underlying changes

[Vivek Goyal <vgoyal@redhat.com>]

On Tue, Jul 14, 2020 at 12:29:08PM +0300, Amir Goldstein wrote:
> On Mon, Jul 13, 2020 at 1:57 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > Miklos,Vivek,
> >
> > These patches are part of the new overlay "fsnotify snapshot" series
> > I have been working on.
> >
> > Conterary to the trend to disallow underlying offline changes with more
> > configurations, I have seen that some people do want to be able to make
> > some "careful" underlying online changes and survive [1].
> >
> > In the following patches, I argue for improving the robustness of
> > overlayfs in the face of online underlying changes, but I have not
> > really proved my claims, so feel free to challenge them.
> >
> 
> This wasn't actually working unless underlying fs was remote, because
> overlayfs clears the DCACHE_OP_REVALIDATE flags in that case.
> 
> I added this hunk for revalidate of local lower fs with nfs_export=on:
> 
> @@ -111,6 +111,10 @@ void ovl_dentry_update_reval(struct dentry
> *dentry, struct dentry *upperdentry,
>         for (i = 0; i < oe->numlower; i++)
>                 flags |= oe->lowerstack[i].dentry->d_flags;
> 
> +       /* Revalidate on local fs lower changes */
> +       if (oe->numlower && ovl_verify_lower(dentry->d_sb))
> +               flags |= mask;
> +
> 
> 
> > I also remember we discussed several times about the conversion of
> > zero return value to -ESTALE, including in the linked thread.
> > I did not change this behavior, but I left a boolean 'strict', which
> > controls this behavior. I am using this boolean to relax strict behavior
> > for snapshot mount later in my snapshot series. Relaxing the strict
> > behavior for other use cases can be considered if someone comes up with
> > a valid use case.
> >
> 
> After giving this some more though, I came to a conclusion that it is actually
> wrong to convert 0 to error because 0 could mean cache timeout expiry
> or other things that do not imply anyone has made underlying changes.
> I see that fuse_dentry_revalidate() handles timeout expiry internally and
> other network filesystems may also do that, but there is nothing in the
> "contract" about not returning 0 if entry MAY be valid.
> Am I wrong?
> 
> I can even think of a network filesystem that marks its own dentry for lazy
> revalidate after some local changes, so this behavior is even more dodgy
> when dealing with remote upper fs.
> 
> So I added another patch to remove the conversion 0 => -ESTALE.
> 
> Pushed these patches to
> https://github.com/amir73il/linux/commits/ovl-revalidate:
>  ovl: invalidate dentry if lower was renamed
>  ovl: invalidate dentry with deleted real dir
>  ovl: do not return error on remote dentry cache expiry

So what's the end goal. We don't want to return error during lookup,
if underlying layer changed and instead force re-lookup. And re-lookup
might work in a slightly different way and that's allowed?

IOW, we don't want to return error if we detected lower layer change
and just continue to run. It might fail later or it might subtly
change behavior in some way (inode number reporting etc). But that's
fine?

What will documentation says. Lower layer changes are allowed? Or
we say lower layer changes are not allowed but overlay will not
flag it at runtime even if we detect it.

Thanks
Vivek