Re: [PATCH v2 01/14] VFS: revise and expand documentation for atomic_open.

[Jeff Layton <jlayton@kernel.org>]

On Sat, 2026-09-19 at 12:06 +1000, NeilBrown wrote:
> From: NeilBrown <neil@brown.name>
> 
> atomic_open is a complex operation which different filesystems implement
> quite differently.  The available documentation doesn't give clear
> guidance on how it should be implemented.
> 
> nfsd has a particular need to open only regular files, but to get
> precise information about what was found if it wasn't a regular file.
> This is slightly different to the syscall calling needs.  In particular
> it suggests that __O_REGULAR shouldn't always result in -EFTYPE.
> 
> In any case that does involve creating open state, using
> finish_no_open() is simplest as it reduces the need to check
> __O_REGULAR, O_DIRECTORY, O_NOFOLLOW.
> 
> So refresh the documentation to give guidance on the choice between
> finish_no_open, finish_open, and an error.  Efficiency always wins, but
> when that isn't an issue, prefer finish_no_open().
> 
> Also clarify the required behaviour when __O_REGULAR is given.  This
> should return -EISDIR if a directory is found as nfsd needs this.  If a
> symlink is found then __O_REGULAR does NOT apply: O_NOFOLLOW must be
> used to decided if it is safe to not return the looked-up dentry.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> ---
>  Documentation/filesystems/vfs.rst | 67 ++++++++++++++++++++++++++-----
>  fs/namei.c                        |  3 ++
>  2 files changed, 59 insertions(+), 11 deletions(-)
> 
> diff --git a/Documentation/filesystems/vfs.rst b/Documentation/filesystems/vfs.rst
> index d3a93eec3945..00ada8cc85ae 100644
> --- a/Documentation/filesystems/vfs.rst
> +++ b/Documentation/filesystems/vfs.rst
> @@ -599,17 +599,62 @@ otherwise noted.
>  
>  ``atomic_open``
>  	called on the last component of an open.  Using this optional
> -	method the filesystem can look up, possibly create and open the
> -	file in one atomic operation.  If it wants to leave actual
> -	opening to the caller (e.g. if the file turned out to be a
> -	symlink, device, or just something filesystem won't do atomic
> -	open for), it may signal this by returning finish_no_open(file,
> -	dentry).  This method is only called if the last component is
> -	negative or needs lookup.  Cached positive dentries are still
> -	handled by f_op->open().  If the file was created, FMODE_CREATED
> -	flag should be set in file->f_mode.  In case of O_EXCL the
> -	method must only succeed if the file didn't exist and hence
> -	FMODE_CREATED shall always be set on success.
> +	method the filesystem can look up, create, truncate, and open
> +	the file in one atomic operation.  This is needed if the
> +	filesystem content can be changed asynchronously and
> +	specifically if a negative dentry is not a guarantee that the
> +	object doesn't exist.  It is also useful if it is possible to
> +	perform combinations of revalidate, lookup, create, open, and
> +	truncate more efficiently what with a sequence of individual
> +	operations.
> +
> +	If the object found is not a file or directory, or if
> +	lookup/create succeeded without establishing any "open" state,
> +	then finish_no_open() should be called to confirm that the
> +	dentry is ready to be handled by normal VFS processing.
> +	FMODE_CREATED should be set in the "file" if the object was
> +	created, and this will prevent further access permission checks,
> +	or handling of O_TRUNC and O_EXCL.
> +
> +	If the lookup/create operation established some open state for a
> +	file or directory, the open should be completed by calling
> +	finish_open().  Passing NULL as the "open" function to
> +	finish_open() is unlikely to be useful as that assumes that no
> +	open state has been established.
> +
> +	atomic_open() may generate errors related to O_DIRECTORY,
> +	__O_REGULAR, O_EXCL, O_NOFOLLOW but is not required to as the
> +	caller will check those against the resulting dentry and
> +	generate any error needed, possibly closing the file if it was
> +	opened by finish_open().  atomic_open() is encouraged to handle
> +	these flags only when doing so is more efficient than not.
> +
> +	If __O_REGULAR is handled, it should generate -EISDIR if the
> +	name is known to be a directory or -EFTYPE if it is some other
> +	non-regular file other than a symbolic link.  Handling of a
> +	symbolic link should be guided by O_NOFOLLOW, not __O_REGULAR:
> +	-ELOOP can be return if O_NOFOLLOW is set, otherwise the symlink
> +	should be returned through finish_no_open().
> +
> +	The focus for atomic_open() is to provide the correct dentry and
> +	to set FMODE_CREATED as accurately as possible.  If O_EXCL was
> +	set, FMODE_CREATED should only be set if this operation
> +	certainly created the object.  If O_EXCL was not set,
> +	FMODE_CREATE should be set if it is possible that this operation
> +	created the object.
> +
> +	This method is only called if the last component is negative or
> +	needs lookup.  Cached positive dentries are still handled by
> +	f_op->open().
> +
> +	If the dentry provided is negative (not in-lookup) and O_CREAT
> +	isn't set, then there is no guarantee of exclusive access to the
> +	dentry - another thread might call ->atomic_open() on the same
> +	dentry at the same time.  If needed a filesystem can ensure this
> +	doesn't happen by returning 0 from ->d_revalidate when that is
> +	called with LOOKUP_OPEN on a negative dentry.  This will ensure
> +	that ->atomic_open() only receives an in-lookup dentry, which
> +	always ensures exclusive access.
>  
>  ``tmpfile``
>  	called in the end of O_TMPFILE open().  Optional, equivalent to
> diff --git a/fs/namei.c b/fs/namei.c
> index d95249dd527c..0f69abb3743b 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -5007,6 +5007,9 @@ static struct file *path_openat(struct nameidata *nd,
>  		error = -EINVAL;
>  	}
>  	fput_close(file);
> +	if (error == -EISDIR &&
> +	    (op->open_flag & __O_REGULAR))
> +		error = -EFTYPE;
>  	if (error == -EOPENSTALE) {
>  		if (flags & LOOKUP_RCU)
>  			error = -ECHILD;
> 
> base-commit: 9189e6a6f89e32d3a604b221ea64e67e1a35957c

It just keeps growing! But on a more serious note, it's better to have
more clear verbiage here.

Reviewed-by: Jeff Layton <jlayton@kernel.org>