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>
