On Tue, Jul 13, 2021 at 3:28 AM Al Viro <viro@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxx> 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...
