Re: [PATCH v2] fs: try an opportunistic lookup for O_CREAT opens too

[Al Viro <viro@zeniv.linux.org.uk>]

On Wed, Aug 14, 2024 at 07:48:17AM -0400, Jeff Layton wrote:
> On Wed, 2024-08-14 at 03:40 +0100, Al Viro wrote:
> > On Wed, Aug 14, 2024 at 03:18:17AM +0100, Al Viro wrote:
> > 
> > > That's not the only problem; your "is it negative" test is inherently
> > > racy in RCU mode.  IOW, what is positive at the time you get here can
> > > bloody well go negative immediately afterwards.  Hit that with
> > > O_CREAT and you've got a bogus ENOENT...
> > 
> > Hmm...  OTOH, in that case you end up in step_into(), which will do the
> > right thing...
> > 
> > 	How well does that series survive NFS client regression tests?
> > That's where I'd expect potentially subtle shite, what with short-circuited
> > ->d_revalidate() on the final pathwalk step in open()...
> 
> Christian took in my v3 patch which is a bit different from this one.
> It seems to be doing fine in testing with NFS and otherwise.
> 
> I don't think we short-circuit the d_revalidate though, do we? That
> version calls lookup_fast on the last component which should
> d_revalidate the last dentry before returning it.

It's not about a skipped call of ->d_revalidate(); it's about the NFS
(especially NFS4) dances inside ->d_revalidate(), where it tries to
cut down on roundtrips where possible.  The interplay with ->atomic_open()
and ->open() is subtle and I'm not sure that we do not depend upon the
details of ->i_rwsem locking by fs/namei.c in there - proof of correctness
used to be rather convoluted there, especially wrt the unhashing and
rehashing aliases.

I'm not saying that your changes break things in there, but that's one
area where I would look for trouble.  NFS has fairly extensive regression
tests, and it would be a good idea to beat that patchset with those.