Re: [PATCH 0/7] namei: clean up retry logic in various do_* functions

[Dmitry Kadashev <dkadashev@gmail.com>]

On Tue, Jul 13, 2021 at 3:28 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Mon, Jul 12, 2021 at 12:01:52PM -0700, Linus Torvalds wrote:
> > On Mon, Jul 12, 2021 at 5:41 AM Dmitry Kadashev <dkadashev@gmail.com> wrote:
> > >
> > > 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 <viro@zeniv.linux.org.uk> wrote:
>
> On Mon, Jul 12, 2021 at 12:01:52PM -0700, Linus Torvalds wrote:
> > On Mon, Jul 12, 2021 at 5:41 AM Dmitry Kadashev <dkadashev@gmail.com> wrote:
> > >
> > > 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...