[Devel] [PATCH VZ10 v3] fs/fuse: align fuse_create_open()'s open path with fuse_open()
Kui Liu
kui.liu at virtuozzo.com
Mon Jun 29 14:18:21 MSK 2026
- 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.
I don't think the flag will ever be used in !close_wait path.
- 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?
yeah, it's more appropriate
- 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?
agreed, this could be a risk.
________________________________
From: Pavel Tikhomirov <ptikhomirov at virtuozzo.com>
Sent: 29 June 2026 18:04
To: Kui Liu <kui.liu at virtuozzo.com>; devel at openvz.org <devel at openvz.org>
Cc: Andrey Zaitsev <azaitsev at virtuozzo.com>
Subject: Re: [Devel] [PATCH VZ10 v3] fs/fuse: align fuse_create_open()'s open path with fuse_open()
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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openvz.org/pipermail/devel/attachments/20260629/c193c94e/attachment-0001.html>
More information about the Devel
mailing list