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

[Benjamin Coddington]

On 20 Nov 2025, at 15:09, Chuck Lever wrote:

> On 11/20/25 10:57 AM, Benjamin Coddington wrote:
>
>> +
>> +		dentry = atomic_open(path, dentry, file, flags, mode);
>> +		error = PTR_ERR_OR_ZERO(dentry);
>> +
>> +		if (unlikely(create_error) && error == -ENOENT)
>> +			error = create_error;
>> +
>> +		if (!error) {
>> +			if (file->f_mode & FMODE_CREATED)
>> +				fsnotify_create(dir->d_inode, dentry);
>> +			if (file->f_mode & FMODE_OPENED)
>> +				fsnotify_open(file);
>> +		}
>> +
>> +		path->dentry = dentry;
>
> When atomic_open() fails, it returns ERR_PTR. Then path->dentry gets set
> to ERR_PTR unconditionally here.
>
> Should path->dentry restoration be conditional, only updating on
> success? Or perhaps should the original dentry be preserved in the local
> variable and restored on error?

No, we want to assign it and pass it along because there's a conditional
dput() at the bottom of nfsd4_create_file() that wants to know what happened
to the dentry.  And that conditional dput() needs to be there in case other
paths error out before we get to atomic_open().

>> +
>> +	} else {
>> +		error = vfs_create(idmap, dir_inode, dentry, mode, true);
>> +		if (!error)
>> +			error = vfs_open(path, file);
>
> Revisiting this, I wonder if the non-atomic error flow needs specific
> code to clean up after creation/open failures.

I think the fput() is all you need here.  I have throughly exercised the
non-atomic vfs_create() failure as well as the vfs_create() success then
vfs_open() failure paths in my testing and development.

Thanks for the review!

Ben