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 */ -- 2.50.1 (Apple Git-155) _______________________________________________ Devel mailing list [email protected] https://lists.openvz.org/mailman/listinfo/devel
