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

[Amir Goldstein <amir73il@gmail.com>]

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

Will wait for Miklos' feedback before posting.

Thanks,
Amir.