[RFC] EOPENSTALE handling in path_openat()

[RFC] EOPENSTALE handling in path_openat()

[Al Viro]

	We have something very odd in the end of path_openat():

        if (error == -EOPENSTALE) {
		if (flags & LOOKUP_RCU)
			error = -ECHILD;
		else   
			error = -ESTALE;
	}

Note that *nothing* that may return EOPENSTALE is ever called in
RCU mode, pretty much by definition - we must have done something
blocking to have gotten that and in RCU mode that's not going to
happen.

This check does not look for RCU mode, though - it checks whether
we'd *started* in RCU mode, ignoring any successful unlazy that
might have landed us in non-RCU mode.  So this ECHILD is not
a dead code.

It really looks like it ought to have been - there's nothing
a retry in non-RCU mode would have solved; the checks on ->d_seq
should've guaranteed that there had been possible timings for
just that tree traversal in non-RCU mode, hitting exact same
dentries.  If they hadn't, we have a much worse problem there...

Miklos, could you recall what was the original intent of that?
Do we want to keep that logics there, or should it just turn into
"map -ENOPENSTALE to -ESTALE??

Re: [RFC] EOPENSTALE handling in path_openat()

[Miklos Szeredi]

On Sun, 19 Jan 2025 at 06:40, Al Viro <viro@zeniv.linux.org.uk> wrote:

> Miklos, could you recall what was the original intent of that?
> Do we want to keep that logics there, or should it just turn into
> "map -ENOPENSTALE to -ESTALE??

I think the intent was to prevent a full LOOKUP_REVAL if this happened
on the first try in do_filp_open().  I still think that makes sense,
but needs a comment since it's not obvious.

Thanks,
Miklos

Re: [RFC] EOPENSTALE handling in path_openat()

[Al Viro]

On Tue, Jan 21, 2025 at 01:07:38PM +0100, Miklos Szeredi wrote:
> On Sun, 19 Jan 2025 at 06:40, Al Viro <viro@zeniv.linux.org.uk> wrote:
> 
> > Miklos, could you recall what was the original intent of that?
> > Do we want to keep that logics there, or should it just turn into
> > "map -ENOPENSTALE to -ESTALE??
> 
> I think the intent was to prevent a full LOOKUP_REVAL if this happened
> on the first try in do_filp_open().  I still think that makes sense,
> but needs a comment since it's not obvious.

For that to have happened we need the following sequence:
	* we started in RCU mode
	* we'd run into something that needs fallback to non-RCU (e.g.
open() callback itself)
	* we had successfully switched to non-lazy mode - grabbed
references, etc.
	* we got to call of open() callback and that returned us -EOPENSTALE.

What's the point of re-walking the same trajectory in dcache again
and why would it yield something different this time around?

IDGI.  We *can't* get to open callback without having already dealt
with leaving RCU mode - any chance of having walked into the wrong
place due to lack of locking has already been excluded when we'd
successfully left RCU mode; otherwise we would've gotten to that
check with error already equal to -ECHILD.

What sequence of events do you have in mind?

Re: [RFC] EOPENSTALE handling in path_openat()

[Miklos Szeredi]

On Thu, 23 Jan 2025 at 03:03, Al Viro <viro@zeniv.linux.org.uk> wrote:

> What's the point of re-walking the same trajectory in dcache again
> and why would it yield something different this time around?

Nfs4 skips revalidate on last component and returns EOPENSTALE from
its ->open() if it turned out to be wrong.  All components leading up
to that were revalidated and the invalid dentry dropped, so what's
really needed is just redoing the last component.

There was an optimization which tried to do this bug eventually got
removed because it was buggy (fac7d1917dfd ("fix EOPENSTALE bug in
do_last()")).   That leaves the choice of redoing the cached lookup
(which should succeed again) or starting from scratch.  I see no point
in doing the latter, although the rarity of this event probably means
that no one would ever notice the difference.

> IDGI.  We *can't* get to open callback without having already dealt
> with leaving RCU mode - any chance of having walked into the wrong
> place due to lack of locking has already been excluded when we'd
> successfully left RCU mode; otherwise we would've gotten to that
> check with error already equal to -ECHILD.

Right.  The point is to not do LOOKUP_REVALIDATE unless we really need
to.   Otherwise EOPENSTALE would just be equivalent to ESTALE.

Thanks,
Miklos



>
> What sequence of events do you have in mind?