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

[Jeff Layton]

On Wed, 2024-08-14 at 16:42 +0100, Al Viro wrote:
> 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.

I've already run a bunch of NFS tests on it and it seems to be OK so
far, but I'll keep testing it. My take:

Opening an extant file with O_CREAT set should behave the same as with
O_CREAT not set.

I did crawl through NFS's d_revalidate functions. There are a couple of
places that check for O_CREAT, but they didn't seem to depend on the
i_rwsem or any particular locking.

Please do let me know if you see anything I missed though.

Thanks,
-- 
Jeff Layton <jlayton@xxxxxxxxxx>