Reviewed-by: Pavel Tikhomirov <[email protected]> On 6/29/26 13:26, 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_OPEN_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 <[email protected]> > --- > Changes in v4: > - Rename FUSE_S_INODE_LOCKED to FUSE_S_OPEN_INODE_LOCKED to make clear > the flag applies to the open path. > - Introduce a local lock_inode helper in fuse_open() for guarding > against changes of lock condition from mainline in the future. > > 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 | 78 ++++++++++++++++++++++++++++-------------------- > fs/fuse/fuse_i.h | 4 ++- > 3 files changed, 49 insertions(+), 50 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..8884f5e522db 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_OPEN_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; > @@ -347,6 +387,7 @@ static int fuse_open(struct inode *inode, struct file > *file) > bool is_truncate = (file->f_flags & O_TRUNC) && fc->atomic_o_trunc; > bool is_wb_truncate = is_truncate && fc->writeback_cache; > bool dax_truncate = is_truncate && FUSE_IS_DAX(inode); > + bool lock_inode = is_wb_truncate || dax_truncate; > > if (fuse_is_bad(inode)) > return -EIO; > @@ -362,7 +403,7 @@ static int fuse_open(struct inode *inode, struct file > *file) > if (err) > return err; > > - if (is_wb_truncate || dax_truncate) > + if (lock_inode) > inode_lock(inode); > > if (dax_truncate) { > @@ -378,6 +419,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 (lock_inode && fc->close_wait) > + set_bit(FUSE_S_OPEN_INODE_LOCKED, &ff->ff_state); > err = fuse_finish_open(inode, file); > if (err) > fuse_sync_release(fi, ff, file->f_flags); > @@ -396,39 +440,9 @@ static int fuse_open(struct inode *inode, struct file > *file) > if (dax_truncate) > filemap_invalidate_unlock(inode->i_mapping); > out_inode_unlock: > - if (is_wb_truncate || dax_truncate) > + if (lock_inode) > 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..68e8df33103a 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_OPEN_INODE_LOCKED = 2, > }; > > /** One input argument of a request */
-- Best regards, Pavel Tikhomirov Senior Software Developer, Virtuozzo. _______________________________________________ Devel mailing list [email protected] https://lists.openvz.org/mailman/listinfo/devel
