Re: [PATCH v1 3/3] VFS/knfsd: Teach dentry_create() to use atomic_open()

[Benjamin Coddington <bcodding@hammerspace.com>]

On 18 Nov 2025, at 20:41, NeilBrown wrote:

> On Wed, 19 Nov 2025, Benjamin Coddington wrote:
>> While knfsd offers combined exclusive create and open results to clients,
>> on some filesystems those results may not be atomic.  This behavior can be
>> observed.  For example, an open O_CREAT with mode 0 will succeed in creating
>> the file but unexpectedly return -EACCES from vfs_open().
>>
>> Additionally reducing the number of remote RPC calls required for O_CREAT
>> on network filesystem provides a performance benefit in the open path.
>>
>> Teach knfsd's helper create_dentry() to use atomic_open() for filesystems
>> that support it.
>>
>> Signed-off-by: Benjamin Coddington <bcodding@hammerspace.com>
>> ---
>>  fs/namei.c         | 43 ++++++++++++++++++++++++++++++++++++-------
>>  fs/nfsd/nfs4proc.c |  8 +++++---
>>  include/linux/fs.h |  2 +-
>>  3 files changed, 42 insertions(+), 11 deletions(-)
>>
>> diff --git a/fs/namei.c b/fs/namei.c
>> index 9c0aad5bbff7..70ab74fb5e95 100644
>> --- a/fs/namei.c
>> +++ b/fs/namei.c
>> @@ -4208,21 +4208,50 @@ EXPORT_SYMBOL(user_path_create);
>>   * On success, returns a "struct file *". Otherwise a ERR_PTR
>>   * is returned.
>>   */
>> -struct file *dentry_create(const struct path *path, int flags, umode_t mode,
>> +struct file *dentry_create(struct path *path, int flags, umode_t mode,
>
> I don't like that you dropped "const" without telling us why.
> It is because we not assign to path->dentry, which is because
> atomic_open() returns a dentry....  which will only be different for
> directories (I think).
>
> But do we need to update path?  The returned file will point to the
> correct dentry - isn't that all that matters?
>
> I guess that I'd like an explanation for why the const is being dropped,
> and why 'path' is being changed.

Well, the first reason was that I was embarrassed at all the new local
variables being added and atomic_open() wanted path->parent and
dentry_create() already had path->child, it was convenient to just re-use
it.  Then it became clear that nfsd4_create_file() really wants to clean up
(or not) the reference to its "child" dentry based on whether the dentry had
been consumed or was an error - so passing back that dentry rather than
re-arrange the tail of nfsd4_create_file() seemed nicer.

Its true that we can acquire the dentry from file->f_path.dentry, but only
in the successful case for both the atomic_open() and the
vfs_create()/vfs_open() path.  Atomic_open() does the work of swapping and
fiddling with the dentry refcounts for us so we don't need to check, so that
is the 2nd reason I passed the dentry back on struct path.

I don't understand the cases atomic_open() is handling here:

3555 static struct dentry *atomic_open(const struct path *path, struct dentry *dentry,
...
3568     if (!error) {
3569         if (file->f_mode & FMODE_OPENED) {
3570             if (unlikely(dentry != file->f_path.dentry)) {
3571                 dput(dentry);
3572                 dentry = dget(file->f_path.dentry);
3573             }

You think this can only happen for a directory?  I figured VFS trying to
work around whatever might have happened inside the filesystem.

One thing that's not happening is that if knfsd /does/ get a different
dentry back, its not updating struct svc_fh->fh_dentry.

Ben