Re: [PATCH v2 1/6] fuse: create fuse_dev on /dev/fuse open instead of mount

["Darrick J. Wong" <djwong@kernel.org>]

On Thu, Mar 12, 2026 at 08:39:59PM +0100, Miklos Szeredi wrote:
> Allocate struct fuse_dev when opening the device.  This means that unlike
> before, ->private_data is always set to a valid pointer.
> 
> The use of USE_DEV_SYNC_INIT magic pointer for the private_data is now
> replaced with a simple bool sync_init member.

Oh thank goodness, this became less clean in the iomap patchset when it
came to add pre-approval for iomap. :)

> If sync INIT is not set, I/O on the device returns error before mount.
> Keep this behavior by checking for the ->fc member.  If fud->fc is set, the
> mount has succeeded.  Testing this used READ_ONCE(file->private_data) and
> smp_mb() to try and provide the necessary semantics.  Switch this to
> smp_store_release() and smp_load_acquire().
> 
> Setting fud->fc is protected by fuse_mutex, this is unchanged.
> 
> Will need this later so the /dev/fuse open file reference is not held
> during FSCONFIG_CMD_CREATE.
> 
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> ---
>  fs/fuse/dev.c        | 47 +++++++++++++++++---------------------------
>  fs/fuse/fuse_dev_i.h | 33 +++++++++++++++++++++++--------
>  fs/fuse/fuse_i.h     |  5 ++---
>  fs/fuse/inode.c      | 35 +++++++++++----------------------
>  fs/fuse/virtio_fs.c  |  2 --
>  5 files changed, 56 insertions(+), 66 deletions(-)
> 
> diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
> index 2c16b94357d5..4795e3d01b75 100644
> --- a/fs/fuse/dev.c
> +++ b/fs/fuse/dev.c
> @@ -1542,32 +1542,24 @@ static ssize_t fuse_dev_do_read(struct fuse_dev *fud, struct file *file,
>  
>  static int fuse_dev_open(struct inode *inode, struct file *file)
>  {
> -	/*
> -	 * The fuse device's file's private_data is used to hold
> -	 * the fuse_conn(ection) when it is mounted, and is used to
> -	 * keep track of whether the file has been mounted already.
> -	 */
> -	file->private_data = NULL;
> +	struct fuse_dev *fud = fuse_dev_alloc();
> +
> +	if (!fud)
> +		return -ENOMEM;
> +
> +	file->private_data = fud;
>  	return 0;
>  }
>  
>  struct fuse_dev *fuse_get_dev(struct file *file)
>  {
> -	struct fuse_dev *fud = __fuse_get_dev(file);
> +	struct fuse_dev *fud = fuse_file_to_fud(file);
>  	int err;
>  
> -	if (likely(fud))
> -		return fud;
> -
> -	err = wait_event_interruptible(fuse_dev_waitq,
> -				       READ_ONCE(file->private_data) != FUSE_DEV_SYNC_INIT);
> +	err = wait_event_interruptible(fuse_dev_waitq, fuse_dev_fc_get(fud));

Nit: Would it be clearer that we're waiting on a condition if this were
phrased as:

	err = wait_event_interruptible(fuse_dev_waitq,
			fuse_dev_fc_get(fud) != NULL);

?

>  	if (err)
>  		return ERR_PTR(err);
>  
> -	fud = __fuse_get_dev(file);
> -	if (!fud)
> -		return ERR_PTR(-EPERM);
> -
>  	return fud;
>  }

<snip>

> diff --git a/fs/fuse/fuse_dev_i.h b/fs/fuse/fuse_dev_i.h
> index 134bf44aff0d..522b2012cd1f 100644
> --- a/fs/fuse/fuse_dev_i.h
> +++ b/fs/fuse/fuse_dev_i.h
> @@ -39,18 +39,35 @@ struct fuse_copy_state {
>  	} ring;
>  };
>  
> -#define FUSE_DEV_SYNC_INIT ((struct fuse_dev *) 1)
> -#define FUSE_DEV_PTR_MASK (~1UL)
> +/*
> + * Lockless access is OK, because fud->fc is set once during mount and is valid
> + * until the file is released.
> + */
> +static inline struct fuse_conn *fuse_dev_fc_get(struct fuse_dev *fud)
> +{
> +	/* Pairs with smp_store_release() in fuse_dev_fc_set() */
> +	return smp_load_acquire(&fud->fc);
> +}
> +
> +static inline void fuse_dev_fc_set(struct fuse_dev *fud, struct fuse_conn *fc)
> +{
> +	/* Pairs with smp_load_acquire() in fuse_dev_fc_get() */
> +	smp_store_release(&fud->fc, fc);
> +}
> +
> +static inline struct fuse_dev *fuse_file_to_fud(struct file *file)
> +{
> +	return file->private_data;
> +}

/me is no expert on load-acquire/store-release, but I think this works.
We can't reorder code below the store-release, which means that if
anyone calling _fc_get actually sees a non-null fuse_conn, then the fuse
connection is fully initialized and ready to go, right?

>  static inline struct fuse_dev *__fuse_get_dev(struct file *file)
>  {
> -	/*
> -	 * Lockless access is OK, because file->private data is set
> -	 * once during mount and is valid until the file is released.
> -	 */
> -	struct fuse_dev *fud = READ_ONCE(file->private_data);
> +	struct fuse_dev *fud = fuse_file_to_fud(file);
> +
> +	if (!fuse_dev_fc_get(fud))
> +		return NULL;
>  
> -	return (typeof(fud)) ((unsigned long) fud & FUSE_DEV_PTR_MASK);
> +	return fud;
>  }
>  
>  struct fuse_dev *fuse_get_dev(struct file *file);
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index 7f16049387d1..739220d96b6f 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -576,6 +576,8 @@ struct fuse_pqueue {
>   * Fuse device instance
>   */
>  struct fuse_dev {
> +	bool sync_init;

Does this need a kerneldoc comment, since the other fields have one?
Granted the only thing I can think of is

	/** Issue FUSE_INIT synchronously */


> +
>  	/** Fuse connection for this device */
>  	struct fuse_conn *fc;
>  
> @@ -622,9 +624,6 @@ struct fuse_fs_context {
>  
>  	/* DAX device, may be NULL */
>  	struct dax_device *dax_dev;
> -
> -	/* fuse_dev pointer to fill in, should contain NULL on entry */
> -	void **fudptr;

I'm glad the fudptr goes away!

If I got the smp_store_release part right then
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

>  };
>  
>  struct fuse_sync_bucket {
> diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
> index e57b8af06be9..53663846db36 100644
> --- a/fs/fuse/inode.c
> +++ b/fs/fuse/inode.c
> @@ -1637,7 +1637,7 @@ EXPORT_SYMBOL_GPL(fuse_dev_alloc);
>  
>  void fuse_dev_install(struct fuse_dev *fud, struct fuse_conn *fc)
>  {
> -	fud->fc = fuse_conn_get(fc);
> +	fuse_dev_fc_set(fud, fuse_conn_get(fc));
>  	spin_lock(&fc->lock);
>  	list_add_tail(&fud->entry, &fc->devices);
>  	spin_unlock(&fc->lock);
> @@ -1659,7 +1659,7 @@ EXPORT_SYMBOL_GPL(fuse_dev_alloc_install);
>  
>  void fuse_dev_free(struct fuse_dev *fud)
>  {
> -	struct fuse_conn *fc = fud->fc;
> +	struct fuse_conn *fc = fuse_dev_fc_get(fud);
>  
>  	if (fc) {
>  		spin_lock(&fc->lock);
> @@ -1822,7 +1822,7 @@ EXPORT_SYMBOL_GPL(fuse_init_fs_context_submount);
>  
>  int fuse_fill_super_common(struct super_block *sb, struct fuse_fs_context *ctx)
>  {
> -	struct fuse_dev *fud = NULL;
> +	struct fuse_dev *fud = ctx->file ? fuse_file_to_fud(ctx->file) : NULL;
>  	struct fuse_mount *fm = get_fuse_mount_super(sb);
>  	struct fuse_conn *fc = fm->fc;
>  	struct inode *root;
> @@ -1856,18 +1856,11 @@ int fuse_fill_super_common(struct super_block *sb, struct fuse_fs_context *ctx)
>  			goto err;
>  	}
>  
> -	if (ctx->fudptr) {
> -		err = -ENOMEM;
> -		fud = fuse_dev_alloc_install(fc);
> -		if (!fud)
> -			goto err_free_dax;
> -	}
> -
>  	fc->dev = sb->s_dev;
>  	fm->sb = sb;
>  	err = fuse_bdi_init(fc, sb);
>  	if (err)
> -		goto err_dev_free;
> +		goto err_free_dax;
>  
>  	/* Handle umasking inside the fuse code */
>  	if (sb->s_flags & SB_POSIXACL)
> @@ -1889,15 +1882,15 @@ int fuse_fill_super_common(struct super_block *sb, struct fuse_fs_context *ctx)
>  	set_default_d_op(sb, &fuse_dentry_operations);
>  	root_dentry = d_make_root(root);
>  	if (!root_dentry)
> -		goto err_dev_free;
> +		goto err_free_dax;
>  
>  	mutex_lock(&fuse_mutex);
>  	err = -EINVAL;
> -	if (ctx->fudptr && *ctx->fudptr) {
> -		if (*ctx->fudptr == FUSE_DEV_SYNC_INIT)
> -			fc->sync_init = 1;
> -		else
> +	if (fud) {
> +		if (fuse_dev_fc_get(fud))
>  			goto err_unlock;
> +		if (fud->sync_init)
> +			fc->sync_init = 1;
>  	}
>  
>  	err = fuse_ctl_add_conn(fc);
> @@ -1906,8 +1899,8 @@ int fuse_fill_super_common(struct super_block *sb, struct fuse_fs_context *ctx)
>  
>  	list_add_tail(&fc->entry, &fuse_conn_list);
>  	sb->s_root = root_dentry;
> -	if (ctx->fudptr) {
> -		*ctx->fudptr = fud;
> +	if (fud) {
> +		fuse_dev_install(fud, fc);
>  		wake_up_all(&fuse_dev_waitq);
>  	}
>  	mutex_unlock(&fuse_mutex);
> @@ -1916,9 +1909,6 @@ int fuse_fill_super_common(struct super_block *sb, struct fuse_fs_context *ctx)
>   err_unlock:
>  	mutex_unlock(&fuse_mutex);
>  	dput(root_dentry);
> - err_dev_free:
> -	if (fud)
> -		fuse_dev_free(fud);
>   err_free_dax:
>  	if (IS_ENABLED(CONFIG_FUSE_DAX))
>  		fuse_dax_conn_free(fc);
> @@ -1944,13 +1934,10 @@ static int fuse_fill_super(struct super_block *sb, struct fs_context *fsc)
>  	if ((ctx->file->f_op != &fuse_dev_operations) ||
>  	    (ctx->file->f_cred->user_ns != sb->s_user_ns))
>  		return -EINVAL;
> -	ctx->fudptr = &ctx->file->private_data;
>  
>  	err = fuse_fill_super_common(sb, ctx);
>  	if (err)
>  		return err;
> -	/* file->private_data shall be visible on all CPUs after this */
> -	smp_mb();
>  
>  	fm = get_fuse_mount_super(sb);
>  
> diff --git a/fs/fuse/virtio_fs.c b/fs/fuse/virtio_fs.c
> index 2f7485ffac52..f685916754ad 100644
> --- a/fs/fuse/virtio_fs.c
> +++ b/fs/fuse/virtio_fs.c
> @@ -1590,8 +1590,6 @@ static int virtio_fs_fill_super(struct super_block *sb, struct fs_context *fsc)
>  			goto err_free_fuse_devs;
>  	}
>  
> -	/* virtiofs allocates and installs its own fuse devices */
> -	ctx->fudptr = NULL;
>  	if (ctx->dax_mode != FUSE_DAX_NEVER) {
>  		if (ctx->dax_mode == FUSE_DAX_ALWAYS && !fs->dax_dev) {
>  			err = -EINVAL;
> -- 
> 2.53.0
> 
>