Message-id: <176367758664.634289.10094974539440300671@noble.neil.brown.name>
Date: Fri, 21 Nov 2025 09:26:26 +1100
From: NeilBrown <neilb@...mail.net>
To: "Benjamin Coddington" <bcodding@...merspace.com>
Cc: "Alexander Viro" <viro@...iv.linux.org.uk>,
 "Christian Brauner" <brauner@...nel.org>, "Jan Kara" <jack@...e.cz>,
 "Chuck Lever" <chuck.lever@...cle.com>, "Jeff Layton" <jlayton@...nel.org>,
 "Olga Kornievskaia" <okorniev@...hat.com>, "Dai Ngo" <Dai.Ngo@...cle.com>,
 "Tom Talpey" <tom@...pey.com>, linux-fsdevel@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-nfs@...r.kernel.org,
 "Trond Myklebust" <trondmy@...nel.org>, "Mike Snitzer" <snitzer@...nel.org>
Subject: Re: [PATCH v1 0/3] Allow knfsd to use atomic_open()

On Wed, 19 Nov 2025, Benjamin Coddington wrote:
> On 18 Nov 2025, at 20:23, NeilBrown wrote:
> 
> > On Wed, 19 Nov 2025, Benjamin Coddington wrote:
> >> We have workloads that will benefit from allowing knfsd to use atomic_open()
> >> in the open/create path.  There are two benefits; the first is the original
> >> matter of correctness: when knfsd must perform both vfs_create() and
> >> vfs_open() in series there can be races or error results that cause the
> >> caller to receive unexpected results.  The second benefit is that for some
> >> network filesystems, we can reduce the number of remote round-trip
> >> operations by using a single atomic_open() path which provides a performance
> >> benefit.
> >>
> >> I've implemented this with the simplest possible change - by modifying
> >> dentry_create() which has a single user: knfsd.  The changes cause us to
> >> insert ourselves part-way into the previously closed/static atomic_open()
> >> path, so I expect VFS folks to have some good ideas about potentially
> >> superior approaches.
> >
> > I think using atomic_open is important - thanks for doing this.
> >
> > I think there is another race this fixes.
> > If the client ends and unchecked v4 OPEN request, nfsd does a lookup and
> > finds the name doesn't exist, it will then (currently) use vfs_create()
> > requesting an exclusive create.  If this races with a create happening
> > from another client, this could result in -EEXIST which is not what the
> > client would expect.  Using atomic_open would fix this.
> >
> > However I cannot see that you ever pass O_EXCL to atomic_open (or did I
> > miss something?).  So I don't think the code is quite right yet.  O_EXCL
> > should be passed is an exclusive or checked create was requested.
> 
> Ah, it's true.  I did not validate knfsd's behaviors, only its interface with
> VFS.  IIUC knfsd gets around needing to pass O_EXCL by holding the directory
> inode lock over the create, and since it doesn't need to do lookup because
> it already has a filehandle, I think O_EXCL is moot.

Holding the directory lock is sufficient for providing O_EXCL for local
filesystems which will be blocked from creating while that lock is held.
It is *not* sufficient for remote filesystems which are precisely those
which provide ->atomic_open.

The fact that you are adding support for atomic_open means that O_EXCL
isn't moot.

I don't know what you mean by "since it doesn't need to do lookup because
it already has a filehandle".  What filehandle does it already have?

Thanks,
NeilBrown


> 
> > With a VFS hat on, I would rather there were more shared code between
> > dentry_create() and lookup_open().  I don't know exactly what this would
> > look like, and I wouldn't want that desire to hold up this patch, but it
> > might be worth thinking about to see if there are any easy similarities
> > to exploit.
> 
> I agree, that would be nice.  It would definitely be a bigger touch, and I
> was going for the minimal change here.
> 
> Thanks for looking at this Neil.
> 
> Ben
> 

Powered by blists - more mailing lists