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

[Liu Kui]

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 <kui.liu at virtuozzo.com>
---
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)