Re: [PATCH v2 05/14] fs/namei.c: don't dput() when atomic_open() errors

[NeilBrown]

On Tue, 30 Jun 2026, Jori Koolstra wrote:
> Both users of atomic_open() now need the dentry alive after return from
> atomic_open() on error, so we can remove the dput() from atomic_open()
> to prevent useless dget()/dput() calls on the normal path.

I don't really like this.
There is no need to improve dentry_create() as I'll be getting rid of it
as soon as I can.  And I think atomic_open() works better if all the
logic is moved into it.

NeilBrown


> 
> Signed-off-by: Jori Koolstra <jkoolstra@xxxxxxxxx>
> ---
>  fs/namei.c | 30 +++++++++++++-----------------
>  1 file changed, 13 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/namei.c b/fs/namei.c
> index 4f747546ff3d..85ce44b068c1 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -4365,7 +4365,7 @@ static int may_o_create(struct mnt_idmap *idmap,
>   * be set.  The caller will need to perform the open themselves.  @path will
>   * have been updated to point to the new dentry.  This may be negative.
>   *
> - * Returns an error code otherwise.
> + * Returns an error code otherwise. On error the ref to dentry is kept.
>   */
>  static struct dentry *atomic_open(const struct path *path, struct dentry *dentry,
>  				  struct file *file,
> @@ -4389,17 +4389,24 @@ static struct dentry *atomic_open(const struct path *path, struct dentry *dentry
>  		} else if (WARN_ON(file->f_path.dentry == DENTRY_NOT_SET)) {
>  			error = -EIO;
>  		} else {
> +			struct dentry *old = dentry;
> +			bool replaced = false;
>  			if (file->f_path.dentry) {
> -				dput(dentry);
>  				dentry = file->f_path.dentry;
> +				replaced = dentry != old;
>  			}
> -			if (unlikely(d_is_negative(dentry)))
> +			if (unlikely(d_is_negative(dentry))) {
> +				/* Do not drop original dentry ref on error */
> +				if (replaced)
> +					dput(dentry);
>  				error = -ENOENT;
> +			} else if (replaced) {
> +				dput(old);
> +			}
>  		}
>  	}
>  
>  	if (error) {
> -		dput(dentry);
>  		dentry = ERR_PTR(error);
>  	} else {
>  		if (file->f_mode & FMODE_CREATED)
> @@ -4497,13 +4504,10 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
>  		struct dentry *orig_dentry = dentry;
>  		if (nd->flags & LOOKUP_DIRECTORY)
>  			open_flag |= O_DIRECTORY;
> -		dget(orig_dentry);
>  		dentry = atomic_open(&nd->path, dentry, file, open_flag, mode);
>  		error = PTR_ERR_OR_ZERO(dentry);
>  		if (unlikely(error))
> -			dentry = orig_dentry;
> -		else
> -			dput(orig_dentry);
> +			dentry = orig_dentry; /* atomic_open() keeps ref on error */
>  		if (unlikely(create_error) && error == -ENOENT) {
>  			/* Should have done a create, but we already errored. */
>  			audit_inode_child(dir->d_inode, dentry, AUDIT_TYPE_CHILD_CREATE);
> @@ -5081,18 +5085,10 @@ struct file *dentry_create(struct path *path, int flags, umode_t mode,
>  		if (create_error)
>  			flags &= ~O_CREAT;
>  
> -		/* atomic_open will dput(dentry) on error */
> -		dget(orig_dentry);
>  		dentry = atomic_open(path, dentry, file, flags, mode);
>  		error = PTR_ERR_OR_ZERO(dentry);
> -
> -		if (IS_ERR(dentry))
> -			/* keep the original */
> +		if (unlikely(error))
>  			dentry = orig_dentry;
> -		else
> -			/* Drop the extra reference */
> -			dput(orig_dentry);
> -
>  		if (unlikely(create_error) && error == -ENOENT)
>  			error = create_error;
>  
> -- 
> 2.54.0
> 
> 
>