On Tue, Jul 13, 2021 at 3:28 AM Al Viro [off-list ref] wrote:

On Mon, Jul 12, 2021 at 12:01:52PM -0700, Linus Torvalds wrote:

quoted

On Mon, Jul 12, 2021 at 5:41 AM Dmitry Kadashev [off-list ref] wrote:

quoted

Since this is on top of the stuff that is going to be in the Jens' tree
only until the 5.15 merge window, I'm assuming this series should go
there as well.

Yeah. Unless Al wants to pick this whole series up.

See my comments about the individual patches - some of them change
code flow, others do. And I think changing code flow as part of
cleanup is ok, but it at the very least needs to be mentioned (and it
might be good to do the "move code that is idempotent inside the
retry" as a separate patch from documentation purposes)

TBH, my main problem with this is that ESTALE retry logics had never
felt right.  We ask e.g. filename_create() to get us the parent.  We
tell it whether we want it to be maximally suspicious or not.  It
still does the same RCU-normal-LOOKUP_REVAL sequence, only for "trust
no one" variant it's RCU-LOOKUP_REVAL-LOOKUP_REVAL instead.

Regardless of the bigger changes discussed below, should we change
direct comparison to ESTALE to retry_estale(retval, lookup_flags) in
filename_lookup() and filename_parentat() (and probably also
do_filp_open() and do_file_open_root())? At least it won't do two
consecutive LOOKUP_REVAL lookups and the change is trivial.

We are
*not* told how far in that sequence did it have to get.  What's more,
even if we had to get all way up to LOOKUP_REVAL, we ignore that
when we do dcache lookup for the last component - only the argument
of filename_create() is looked at.

It really smells like the calling conventions are wrong.  I agree that
all of that is, by definition, a very slow path - it's just that the
logics makes me go "WTF?" every time I see it... ;-/

The current series does not make it worse though. I'm happy to work on
further improvements with some guidance, but hopefully in a separate
patchset?

Hell knows - perhaps the lookup_flags thing wants to be passed by
reference (all the way into path_parentat()) and have the "we had
to go for LOOKUP_REVAL" returned that way.  Not sure...

Will that allow to get rid of the retries completely? I'm not sure I
understand all the code paths that can return ESTALE, I'd assume we'd
still have to keep the whole retry logic.

-- 
Dmitry Kadashev

On Tue, Jul 13, 2021 at 3:28 AM Al Viro [off-list ref] wrote:

On Mon, Jul 12, 2021 at 12:01:52PM -0700, Linus Torvalds wrote:

quoted

On Mon, Jul 12, 2021 at 5:41 AM Dmitry Kadashev [off-list ref] wrote:

quoted

Since this is on top of the stuff that is going to be in the Jens' tree
only until the 5.15 merge window, I'm assuming this series should go
there as well.

Yeah. Unless Al wants to pick this whole series up.

See my comments about the individual patches - some of them change
code flow, others do. And I think changing code flow as part of
cleanup is ok, but it at the very least needs to be mentioned (and it
might be good to do the "move code that is idempotent inside the
retry" as a separate patch from documentation purposes)

TBH, my main problem with this is that ESTALE retry logics had never
felt right.  We ask e.g. filename_create() to get us the parent.  We
tell it whether we want it to be maximally suspicious or not.  It
still does the same RCU-normal-LOOKUP_REVAL sequence, only for "trust
no one" variant it's RCU-LOOKUP_REVAL-LOOKUP_REVAL instead.  We are
*not* told how far in that sequence did it have to get.  What's more,
even if we had to get all way up to LOOKUP_REVAL, we ignore that
when we do dcache lookup for the last component - only the argument
of filename_create() is looked at.

It really smells like the calling conventions are wrong.  I agree that
all of that is, by definition, a very slow path - it's just that the
logics makes me go "WTF?" every time I see it... ;-/

Hell knows - perhaps the lookup_flags thing wants to be passed by
reference (all the way into path_parentat()) and have the "we had
to go for LOOKUP_REVAL" returned that way.  Not sure...

Al, still crawling out of the bloody ptrace/asm glue horrors at the moment...