[Devel] [PATCH VZ10 v3] fs/fuse: align fuse_create_open()'s open path with fuse_open()

[Pavel Tikhomirov]

Reviewed-by: Pavel Tikhomirov <ptikhomirov at virtuozzo.com>

Generally looks good, though, please, see comments inline.

On 6/26/26 16:32, Liu Kui wrote:
> fuse_create_open()'s close_wait branch was not correct; in particular the
> fput() in its error path was wrong. fuse_open()'s close_wait branch should
> apply instead.
> 
> The two sites differ in one important way, though: fuse_open() runs the
> block before FMODE_OPENED is set, where fuse_sync_release() is the correct
> cleanup on error, whereas fuse_create_open() runs it after FMODE_OPENED is
> set, where fuse_sync_release() must not be called. To unify them, refactor
> the sequence into fuse_open_close_wait() and call it from
> fuse_finish_open(), so the block runs before FMODE_OPENED on both paths.
> The error then propagates to the caller and the block itself no longer
> needs to call fuse_sync_release().
> 
> Add a per-open FUSE_S_INODE_LOCKED flag telling fuse_open_close_wait() that
> the caller already holds the inode lock, so it is not re-acquired and
> deadlocks on the atomic O_TRUNC path.
> 
> https://virtuozzo.atlassian.net/browse/VSTOR-136027
> Signed-off-by: Liu Kui <kui.liu at virtuozzo.com>
> ---
> Changes in v3:
>   - Call fuse_open_close_wait() from fuse_finish_open() instead of from
>     the fuse_open() and fuse_create_open() call sites, so the sequence
>     runs before FMODE_OPENED is set on both paths.
>   - Drop the fuse_sync_release() call inside fuse_open_close_wait(); the
>     error now propagates to the caller, which releases the file before
>     FMODE_OPENED is set.
>   - Make fuse_open_close_wait() static and drop its fuse_i.h prototype.
>   - Acquire the inode lock in fuse_open_close_wait() only when the caller
>     does not already hold it, tracked by a per-open FUSE_S_INODE_LOCKED
>     flag, so the atomic O_TRUNC path (which already holds it) does not
>     self-deadlock.
> 
> Changes in v2:
>   - Factor the close_wait open branch into fuse_open_close_wait();
>     call it from both fuse_open() and fuse_create_open() instead of
>     duplicating the block in each.
>   - Use fuse_sync_release() on the close_wait error path for both
>     callers. It is functionally equivalent to fuse_release_common()
>     used in fuse_open().
> 
>  fs/fuse/dir.c    | 17 -----------
>  fs/fuse/file.c   | 73 ++++++++++++++++++++++++++++--------------------
>  fs/fuse/fuse_i.h |  4 ++-
>  3 files changed, 46 insertions(+), 48 deletions(-)
> 
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index 45ee4f1e4b68..9799c2b6eb4d 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -722,23 +722,6 @@ static int fuse_create_open(struct mnt_idmap *idmap, struct inode *dir,
>  			invalidate_inode_pages2(inode->i_mapping);
>  	}
>  
> -	if (fm->fc->close_wait) {
> -		struct fuse_inode *fi = get_fuse_inode(inode);
> -		bool need_open;
> -
> -		inode_lock(inode);
> -		spin_lock(&fi->lock);
> -		need_open = (++fi->num_openers == 1);
> -		spin_unlock(&fi->lock);
> -
> -		if (need_open && fm->fc->kio.op && fm->fc->kio.op->file_open) {
> -			err = fm->fc->kio.op->file_open(file, inode);
> -			if (err)
> -				fput(file);
> -		}
> -		inode_unlock(inode);
> -	}
> -
>  	return err;
>  
>  out_free_ff:
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index 58202a96517e..8750cbdf0596 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -286,6 +286,43 @@ static void fuse_link_rw_file(struct file *file)
>  	spin_unlock(&fi->lock);
>  }
>  
> +static int fuse_open_close_wait(struct inode *inode, struct file *file)
> +{
> +	struct fuse_conn *fc = get_fuse_conn(inode);
> +	struct fuse_inode *fi = get_fuse_inode(inode);
> +	struct fuse_file *ff = file->private_data;
> +	/* the caller already holds the inode lock (e.g. truncate open) */
> +	bool locked = test_and_clear_bit(FUSE_S_INODE_LOCKED, &ff->ff_state);
> +	int err = 0;
> +
> +	if (!locked)
> +		inode_lock(inode);
> +	spin_lock(&fi->lock);
> +
> +	if (++fi->num_openers == 1 || fi->i_size_unstable) {
> +		fi->i_size_unstable = 1;
> +		fi->inval_mask = ~0;
> +		spin_unlock(&fi->lock);
> +		err = fuse_update_attributes(inode, file, ~0);
> +
> +		if (!err && fc->kio.op && fc->kio.op->file_open)
> +			err = fc->kio.op->file_open(file, inode);
> +
> +		spin_lock(&fi->lock);
> +		fi->i_size_unstable = 0;
> +		if (err)
> +			fi->num_openers--;
> +	}
> +
> +	file->f_mode |= FMODE_NOWAIT;
> +
> +	spin_unlock(&fi->lock);
> +	if (!locked)
> +		inode_unlock(inode);
> +
> +	return err;
> +}
> +
>  int fuse_finish_open(struct inode *inode, struct file *file)
>  {
>  	struct fuse_file *ff = file->private_data;
> @@ -319,6 +356,9 @@ int fuse_finish_open(struct inode *inode, struct file *file)
>  		*/
>  		if (!test_bit(FUSE_S_FAIL_IMMEDIATELY, &ff->ff_state))
>  			err = fuse_wait_on_inval_files(inode);
> +
> +		if (!err)
> +			err = fuse_open_close_wait(inode, file);
>  	}
>  
>  	return err;
> @@ -378,6 +418,9 @@ static int fuse_open(struct inode *inode, struct file *file)
>  	err = fuse_do_open(fm, get_node_id(inode), file, false);
>  	if (!err) {
>  		ff = file->private_data;
> +		/* tell fuse_open_close_wait() we already hold the inode lock */
> +		if ((is_wb_truncate || dax_truncate) && fc->close_wait)
> +			set_bit(FUSE_S_INODE_LOCKED, &ff->ff_state);

Technically correct. Some concerns:

- Flag name suggests it should always indicate that inode is locked,
but for !close_wait path it does not, this can be a problem if we reuse
this flag without checking that it is not always set.

- Let's maybe indicate the flag scope, we currently use it only on fuse_open,
maybe we should rename it to FUSE_S_OPEN_INODE_LOCKED?

- What if after rebase the lock condition will change e.g. not (is_wb_truncate
|| dax_truncate) but something else, maybe we should use a helper variable
"locked" in fuse_open which is set only when lock is taken?

>  		err = fuse_finish_open(inode, file);
>  		if (err)
>  			fuse_sync_release(fi, ff, file->f_flags);
> @@ -399,36 +442,6 @@ static int fuse_open(struct inode *inode, struct file *file)
>  	if (is_wb_truncate || dax_truncate)
>  		inode_unlock(inode);
>  
> -	if (!err && fc->close_wait) {
> -		inode_lock(inode);
> -		spin_lock(&fi->lock);
> -
> -		if (++fi->num_openers == 1 || fi->i_size_unstable) {
> -			fi->i_size_unstable = 1;
> -			fi->inval_mask = ~0;
> -			spin_unlock(&fi->lock);
> -			err = fuse_update_attributes(inode, file, ~0);
> -
> -			if (!err && fc->kio.op && fc->kio.op->file_open)
> -				err = fc->kio.op->file_open(file, inode);
> -
> -			spin_lock(&fi->lock);
> -			fi->i_size_unstable = 0;
> -			if (err)
> -				fi->num_openers--;
> -		}
> -
> -		file->f_mode |= FMODE_NOWAIT;
> -
> -		spin_unlock(&fi->lock);
> -		inode_unlock(inode);
> -
> -		if (err) {
> -			fuse_release_common(file, false);
> -			return err;
> -		}
> -	}
> -
>  	return err;
>  }
>  
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index a834a5b4dfe2..53dfcb495ca0 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -327,7 +327,9 @@ struct fuse_file {
>  enum {
>  	/** Any fops on given ff should fail immediately */
>  	FUSE_S_FAIL_IMMEDIATELY = 0,
> -	FUSE_S_CLOSING		= 1
> +	FUSE_S_CLOSING		= 1,
> +	/** The open path already holds the inode lock (close_wait) */
> +	FUSE_S_INODE_LOCKED	= 2,
>  };
>  
>  /** One input argument of a request */

-- 
Best regards, Pavel Tikhomirov
Senior Software Developer, Virtuozzo.