Re: [PATCH 01/18] VFS: move mnt_want_write() and locking into lookup_open()

> Op 01-06-2026 08:37 CEST schreef NeilBrown <[email protected]>:
> 
>  
> From: NeilBrown <neil-+NVA1uvv1dVBDLzU/[email protected]>
> 
> The mnt_want_write() call and the parent inode locking in
> open_last_lookups() are only needed for lookup_open().  So we can move
> them and all the got_write handling into lookup_open().
> 
> Note that we need to also check create_error when determining whether to
> unlock shared or not, as O_CREAT can be cleared.
> 
> The fsnotify calls come too as they must be in the locked region.
> 
> Also use the existing dir_inode uniformly for dir->d_inode.
> 
> This is a step towards exporting an better "open/create" interface to nfsd.
> 
> Signed-off-by: NeilBrown <neil-+NVA1uvv1dVBDLzU/[email protected]>
> ---
> +	if (open_flag & O_CREAT)
> +		inode_lock(dir_inode);
> +	else
> +		inode_lock_shared(dir_inode);
> +

This is not about a change in your patch, but I do wonder whether we should
also set the lockdep subclass here. We aren't taking any other locks, but
that is also true in the create path of mknod, which ultimately calls start_dirop
and there we do

		inode_lock_nested(dir, I_MUTEX_PARENT);


> +	if (unlikely(IS_DEADDIR(dir_inode))) {
> +		dentry = ERR_PTR(-ENOENT);
> +		goto out;
> +	}
>  
>  	file->f_mode &= ~FMODE_CREATED;
>  	dentry = d_lookup(dir, &nd->last);
> @@ -4423,7 +4439,7 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
>  		if (!dentry) {
>  			dentry = d_alloc_parallel(dir, &nd->last);
>  			if (IS_ERR(dentry))
> -				return dentry;
> +				goto out;
>  		}
>  		if (d_in_lookup(dentry))
>  			break;
> @@ -4439,7 +4455,7 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
>  	}
>  	if (dentry->d_inode) {
>  		/* Cached positive dentry: will open in f_op->open */
> -		return dentry;
> +		goto out;
>  	}
>  
>  	if (open_flag & O_CREAT)
> @@ -4460,7 +4476,7 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
>  	if (open_flag & O_CREAT) {
>  		if (open_flag & O_EXCL)
>  			open_flag &= ~O_TRUNC;
> -		mode = vfs_prepare_mode(idmap, dir->d_inode, mode, mode, mode);
> +		mode = vfs_prepare_mode(idmap, dir_inode, mode, mode, mode);
>  		if (likely(got_write))
>  			create_error = may_o_create(idmap, &nd->path,
>  						    dentry, mode);
> @@ -4475,7 +4491,7 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
>  		dentry = atomic_open(&nd->path, dentry, file, open_flag, mode);
>  		if (unlikely(create_error) && dentry == ERR_PTR(-ENOENT))
>  			dentry = ERR_PTR(create_error);
> -		return dentry;
> +		goto out;
>  	}
>  
>  	if (d_in_lookup(dentry)) {
> @@ -4515,11 +4531,27 @@ static struct dentry *lookup_open(struct nameidata *nd, struct file *file,
>  		error = create_error;
>  		goto out_dput;
>  	}
> +out:
> +	if (!IS_ERR(dentry)) {
> +		if (file->f_mode & FMODE_CREATED)
> +			fsnotify_create(dir_inode, dentry);
> +		if (file->f_mode & FMODE_OPENED)
> +			fsnotify_open(file);
> +	}

We can move this later if my changes land to the vfs_* and atomic_open() functions.

> +	if ((open_flag & O_CREAT) || create_error)
> +		inode_unlock(dir_inode);
> +	else
> +		inode_unlock_shared(dir_inode);
> +
> +	if (got_write)
> +		mnt_drop_write(nd->path.mnt);
> +
>  	return dentry;
>  
>  out_dput:
>  	dput(dentry);
> -	return ERR_PTR(error);
> +	dentry = ERR_PTR(error);
> +	goto out;
>  }
>  
>  static inline bool trailing_slashes(struct nameidata *nd)
> @@ -4562,9 +4594,7 @@ static const char *open_last_lookups(struct nameidata *nd,
>  		   struct file *file, const struct open_flags *op)
>  {
>  	struct delegated_inode delegated_inode = { };
> -	struct dentry *dir = nd->path.dentry;
>  	int open_flag = op->open_flag;
> -	bool got_write = false;
>  	struct dentry *dentry;
>  	const char *res;
>  
> @@ -4594,32 +4624,7 @@ static const char *open_last_lookups(struct nameidata *nd,
>  		}
>  	}
>  retry:
> -	if (open_flag & (O_CREAT | O_TRUNC | O_WRONLY | O_RDWR)) {
> -		got_write = !mnt_want_write(nd->path.mnt);
> -		/*
> -		 * do _not_ fail yet - we might not need that or fail with
> -		 * a different error; let lookup_open() decide; we'll be
> -		 * dropping this one anyway.
> -		 */
> -	}
> -	if (open_flag & O_CREAT)
> -		inode_lock(dir->d_inode);
> -	else
> -		inode_lock_shared(dir->d_inode);
> -	dentry = lookup_open(nd, file, op, got_write, &delegated_inode);
> -	if (!IS_ERR(dentry)) {
> -		if (file->f_mode & FMODE_CREATED)
> -			fsnotify_create(dir->d_inode, dentry);
> -		if (file->f_mode & FMODE_OPENED)
> -			fsnotify_open(file);
> -	}
> -	if (open_flag & O_CREAT)
> -		inode_unlock(dir->d_inode);
> -	else
> -		inode_unlock_shared(dir->d_inode);
> -
> -	if (got_write)
> -		mnt_drop_write(nd->path.mnt);
> +	dentry = lookup_open(nd, file, op, &delegated_inode);
>  
>  	if (IS_ERR(dentry)) {
>  		if (is_delegated(&delegated_inode)) {
> -- 
> 2.50.0.107.gf914562f5916.dirty

I will say we are doing a lot in lookup_open(), but since open_last_lookups() is
its only caller, and there is no particular reason that I see for doing this work
there, this is fine by me if it helps your nfsd refactor.

Reviewed-by: Jori Koolstra <jkoolstra-qWit8jRvyhVmR6Xm/[email protected]>