[PATCH 0/3] FUSE passthrough fixes

[PATCH 0/3] FUSE passthrough fixes

[Amir Goldstein]

Miklos,

While going over the code to prepare for getattr() passthrough I
experienced a WTF moment that resulted in the two fix patches.

Patch 3/3 is included for reference and to give Sweet Tea a starting
point for getattr() passthrough.

What puzzled me is that some ff->iomode state bugs were so blunt that
I needed to figure out how I did not see any WARN_ON in my tests of rc1.
There are different reasons for different types of bugs.

1. For concurrent dio writes without any passthrough open,
fuse_file_cached_io_start() was supposed to hit
WARN_ON(ff->iomode == IOM_UNCACHED) if there is already a dio write
in-flight.

My conclusion is that the set of fstests that run on passthrough_hp,
on my small test VM do not excercise concurrent dio writes.

2. For dio write, where the file was opened passthrough, every write
was going to hit WARN_ON(ff->iomode == IOM_UNCACHED) and also
fuse_file_cached_io_end() was going to set ff->iomode == IOM_NONE
and leak the fuse_backing object.

However, the bug fixed by patch 2/3 made sure that parallel dio write
would always fallback to exclusive dio if file was open with a backing
file.

Testing:

I ran fstests with passthrough_hp with options:
1) FOPEN_PASSTHROUGH
2) FOPEN_DIRECT_IO | FOPEN_PARALLEL_DIRECT_WRITES
3) FOPEN_PASSTHROUGH | FOPEN_DIRECT_IO | FOPEN_PARALLEL_DIRECT_WRITES

Did not observe any regressions (not any improvments) from rc1.

Ran some multi threads aiodio tests with just patch 2/3 and the
assertions in fuse_evict_inode() from patch 3/3.

First two configs did not hit any assertion.
The passthrough+direct_io+parallel_direct_writes config always
hits the assertion in fuse_file_cached_io_start() and always hits
the leaked fuse_backing assertion in fuse_evict_inode().

Bernd do you have different tests to cover concurrent dio writes in
your setup? Any ideas on how to improve the fstests test coverage?

Thanks,
Amir.

Amir Goldstein (3):
  fuse: fix wrong ff->iomode state changes from parallel dio write
  fuse: fix parallel dio write on file open in passthrough mode
  fuse: prepare for long lived reference on backing file

 fs/fuse/file.c   | 18 +++++++-----
 fs/fuse/fuse_i.h | 10 +++++--
 fs/fuse/inode.c  |  7 +++++
 fs/fuse/iomode.c | 73 +++++++++++++++++++++++++++++++++---------------
 4 files changed, 76 insertions(+), 32 deletions(-)

-- 
2.34.1

[PATCH 1/3] fuse: fix wrong ff->iomode state changes from parallel dio write

[Amir Goldstein]

There is a confusion with fuse_file_uncached_io_{start,end} interface.
These helpers do two things when called from passthrough open()/release():
1. Take/drop negative refcount of fi->iocachectr (inode uncached io mode)
2. State change ff->iomode IOM_NONE <-> IOM_UNCACHED (file uncached open)

The calls from parallel dio write path need to take a reference on
fi->iocachectr, but they should not be changing ff->iomode state,
because in this case, the fi->iocachectr reference does not stick around
until file release().

Factor out helpers fuse_inode_uncached_io_{start,end}, to be used from
parallel dio write path and rename fuse_file_*cached_io_{start,end}
helpers to fuse_file_*cached_io_{open,release} to clarify the difference.

Add a check of ff->iomode in mmap(), so that fuse_file_cached_io_open()
is called only on first mmap of direct_io file.

Fixes: 205c1d802683 ("fuse: allow parallel dio writes with FUSE_DIRECT_IO_ALLOW_MMAP")
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/fuse/file.c   | 18 +++++++++------
 fs/fuse/fuse_i.h |  7 +++---
 fs/fuse/inode.c  |  1 +
 fs/fuse/iomode.c | 59 ++++++++++++++++++++++++++++++++----------------
 4 files changed, 56 insertions(+), 29 deletions(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index a56e7bffd000..fcf20b304093 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1362,7 +1362,7 @@ static void fuse_dio_lock(struct kiocb *iocb, struct iov_iter *from,
 			  bool *exclusive)
 {
 	struct inode *inode = file_inode(iocb->ki_filp);
-	struct fuse_file *ff = iocb->ki_filp->private_data;
+	struct fuse_inode *fi = get_fuse_inode(inode);
 
 	*exclusive = fuse_dio_wr_exclusive_lock(iocb, from);
 	if (*exclusive) {
@@ -1377,7 +1377,7 @@ static void fuse_dio_lock(struct kiocb *iocb, struct iov_iter *from,
 		 * have raced, so check it again.
 		 */
 		if (fuse_io_past_eof(iocb, from) ||
-		    fuse_file_uncached_io_start(inode, ff, NULL) != 0) {
+		    fuse_inode_uncached_io_start(fi, NULL) != 0) {
 			inode_unlock_shared(inode);
 			inode_lock(inode);
 			*exclusive = true;
@@ -1388,13 +1388,13 @@ static void fuse_dio_lock(struct kiocb *iocb, struct iov_iter *from,
 static void fuse_dio_unlock(struct kiocb *iocb, bool exclusive)
 {
 	struct inode *inode = file_inode(iocb->ki_filp);
-	struct fuse_file *ff = iocb->ki_filp->private_data;
+	struct fuse_inode *fi = get_fuse_inode(inode);
 
 	if (exclusive) {
 		inode_unlock(inode);
 	} else {
 		/* Allow opens in caching mode after last parallel dio end */
-		fuse_file_uncached_io_end(inode, ff);
+		fuse_inode_uncached_io_end(fi);
 		inode_unlock_shared(inode);
 	}
 }
@@ -2574,10 +2574,14 @@ static int fuse_file_mmap(struct file *file, struct vm_area_struct *vma)
 		 * First mmap of direct_io file enters caching inode io mode.
 		 * Also waits for parallel dio writers to go into serial mode
 		 * (exclusive instead of shared lock).
+		 * After first mmap, the inode stays in caching io mode until
+		 * the direct_io file release.
 		 */
-		rc = fuse_file_cached_io_start(inode, ff);
-		if (rc)
-			return rc;
+		if (READ_ONCE(ff->iomode) != IOM_CACHED) {
+			rc = fuse_file_cached_io_open(inode, ff);
+			if (rc)
+				return rc;
+		}
 	}
 
 	if ((vma->vm_flags & VM_SHARED) && (vma->vm_flags & VM_MAYWRITE))
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index b24084b60864..f23919610313 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -1394,9 +1394,10 @@ int fuse_fileattr_set(struct mnt_idmap *idmap,
 		      struct dentry *dentry, struct fileattr *fa);
 
 /* iomode.c */
-int fuse_file_cached_io_start(struct inode *inode, struct fuse_file *ff);
-int fuse_file_uncached_io_start(struct inode *inode, struct fuse_file *ff, struct fuse_backing *fb);
-void fuse_file_uncached_io_end(struct inode *inode, struct fuse_file *ff);
+int fuse_file_cached_io_open(struct inode *inode, struct fuse_file *ff);
+int fuse_inode_uncached_io_start(struct fuse_inode *fi,
+				 struct fuse_backing *fb);
+void fuse_inode_uncached_io_end(struct fuse_inode *fi);
 
 int fuse_file_io_open(struct file *file, struct inode *inode);
 void fuse_file_io_release(struct fuse_file *ff, struct inode *inode);
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 3a5d88878335..99e44ea7d875 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -175,6 +175,7 @@ static void fuse_evict_inode(struct inode *inode)
 		}
 	}
 	if (S_ISREG(inode->i_mode) && !fuse_is_bad(inode)) {
+		WARN_ON(fi->iocachectr != 0);
 		WARN_ON(!list_empty(&fi->write_files));
 		WARN_ON(!list_empty(&fi->queued_writes));
 	}
diff --git a/fs/fuse/iomode.c b/fs/fuse/iomode.c
index c653ddcf0578..1519f895f0a9 100644
--- a/fs/fuse/iomode.c
+++ b/fs/fuse/iomode.c
@@ -21,12 +21,13 @@ static inline bool fuse_is_io_cache_wait(struct fuse_inode *fi)
 }
 
 /*
- * Start cached io mode.
+ * Called on cached file open() and on first mmap() of direct_io file.
+ * Takes cached_io inode mode reference to be dropped on file release.
  *
  * Blocks new parallel dio writes and waits for the in-progress parallel dio
  * writes to complete.
  */
-int fuse_file_cached_io_start(struct inode *inode, struct fuse_file *ff)
+int fuse_file_cached_io_open(struct inode *inode, struct fuse_file *ff)
 {
 	struct fuse_inode *fi = get_fuse_inode(inode);
 
@@ -56,8 +57,7 @@ int fuse_file_cached_io_start(struct inode *inode, struct fuse_file *ff)
 		return -ETXTBSY;
 	}
 
-	WARN_ON(ff->iomode == IOM_UNCACHED);
-	if (ff->iomode == IOM_NONE) {
+	if (!WARN_ON(ff->iomode != IOM_NONE)) {
 		ff->iomode = IOM_CACHED;
 		if (fi->iocachectr == 0)
 			set_bit(FUSE_I_CACHE_IO_MODE, &fi->state);
@@ -67,10 +67,9 @@ int fuse_file_cached_io_start(struct inode *inode, struct fuse_file *ff)
 	return 0;
 }
 
-static void fuse_file_cached_io_end(struct inode *inode, struct fuse_file *ff)
+static void fuse_file_cached_io_release(struct fuse_file *ff,
+					struct fuse_inode *fi)
 {
-	struct fuse_inode *fi = get_fuse_inode(inode);
-
 	spin_lock(&fi->lock);
 	WARN_ON(fi->iocachectr <= 0);
 	WARN_ON(ff->iomode != IOM_CACHED);
@@ -82,9 +81,8 @@ static void fuse_file_cached_io_end(struct inode *inode, struct fuse_file *ff)
 }
 
 /* Start strictly uncached io mode where cache access is not allowed */
-int fuse_file_uncached_io_start(struct inode *inode, struct fuse_file *ff, struct fuse_backing *fb)
+int fuse_inode_uncached_io_start(struct fuse_inode *fi, struct fuse_backing *fb)
 {
-	struct fuse_inode *fi = get_fuse_inode(inode);
 	struct fuse_backing *oldfb;
 	int err = 0;
 
@@ -99,9 +97,7 @@ int fuse_file_uncached_io_start(struct inode *inode, struct fuse_file *ff, struc
 		err = -ETXTBSY;
 		goto unlock;
 	}
-	WARN_ON(ff->iomode != IOM_NONE);
 	fi->iocachectr--;
-	ff->iomode = IOM_UNCACHED;
 
 	/* fuse inode holds a single refcount of backing file */
 	if (!oldfb) {
@@ -115,15 +111,29 @@ int fuse_file_uncached_io_start(struct inode *inode, struct fuse_file *ff, struc
 	return err;
 }
 
-void fuse_file_uncached_io_end(struct inode *inode, struct fuse_file *ff)
+/* Takes uncached_io inode mode reference to be dropped on file release */
+static int fuse_file_uncached_io_open(struct inode *inode,
+				      struct fuse_file *ff,
+				      struct fuse_backing *fb)
 {
 	struct fuse_inode *fi = get_fuse_inode(inode);
+	int err;
+
+	err = fuse_inode_uncached_io_start(fi, fb);
+	if (err)
+		return err;
+
+	WARN_ON(ff->iomode != IOM_NONE);
+	ff->iomode = IOM_UNCACHED;
+	return 0;
+}
+
+void fuse_inode_uncached_io_end(struct fuse_inode *fi)
+{
 	struct fuse_backing *oldfb = NULL;
 
 	spin_lock(&fi->lock);
 	WARN_ON(fi->iocachectr >= 0);
-	WARN_ON(ff->iomode != IOM_UNCACHED);
-	ff->iomode = IOM_NONE;
 	fi->iocachectr++;
 	if (!fi->iocachectr) {
 		wake_up(&fi->direct_io_waitq);
@@ -134,6 +144,15 @@ void fuse_file_uncached_io_end(struct inode *inode, struct fuse_file *ff)
 		fuse_backing_put(oldfb);
 }
 
+/* Drop uncached_io reference from passthrough open */
+static void fuse_file_uncached_io_release(struct fuse_file *ff,
+					  struct fuse_inode *fi)
+{
+	WARN_ON(ff->iomode != IOM_UNCACHED);
+	ff->iomode = IOM_NONE;
+	fuse_inode_uncached_io_end(fi);
+}
+
 /*
  * Open flags that are allowed in combination with FOPEN_PASSTHROUGH.
  * A combination of FOPEN_PASSTHROUGH and FOPEN_DIRECT_IO means that read/write
@@ -163,7 +182,7 @@ static int fuse_file_passthrough_open(struct inode *inode, struct file *file)
 		return PTR_ERR(fb);
 
 	/* First passthrough file open denies caching inode io mode */
-	err = fuse_file_uncached_io_start(inode, ff, fb);
+	err = fuse_file_uncached_io_open(inode, ff, fb);
 	if (!err)
 		return 0;
 
@@ -216,7 +235,7 @@ int fuse_file_io_open(struct file *file, struct inode *inode)
 	if (ff->open_flags & FOPEN_PASSTHROUGH)
 		err = fuse_file_passthrough_open(inode, file);
 	else
-		err = fuse_file_cached_io_start(inode, ff);
+		err = fuse_file_cached_io_open(inode, ff);
 	if (err)
 		goto fail;
 
@@ -236,8 +255,10 @@ int fuse_file_io_open(struct file *file, struct inode *inode)
 /* No more pending io and no new io possible to inode via open/mmapped file */
 void fuse_file_io_release(struct fuse_file *ff, struct inode *inode)
 {
+	struct fuse_inode *fi = get_fuse_inode(inode);
+
 	/*
-	 * Last parallel dio close allows caching inode io mode.
+	 * Last passthrough file close allows caching inode io mode.
 	 * Last caching file close exits caching inode io mode.
 	 */
 	switch (ff->iomode) {
@@ -245,10 +266,10 @@ void fuse_file_io_release(struct fuse_file *ff, struct inode *inode)
 		/* Nothing to do */
 		break;
 	case IOM_UNCACHED:
-		fuse_file_uncached_io_end(inode, ff);
+		fuse_file_uncached_io_release(ff, fi);
 		break;
 	case IOM_CACHED:
-		fuse_file_cached_io_end(inode, ff);
+		fuse_file_cached_io_release(ff, fi);
 		break;
 	}
 }
-- 
2.34.1

[PATCH 2/3] fuse: fix parallel dio write on file open in passthrough mode

[Amir Goldstein]

Paralle dio write takes a negative refcount of fi->iocachectr and so
does open of file in passthrough mode.

The refcount of passthrough mode is associated with attach/detach
of a fuse_backing object to fuse inode.

For parallel dio write, the backing file is irrelevant, so the call to
fuse_inode_uncached_io_start() passes a NULL fuse_backing object.

Passing a NULL fuse_backing will result in false -EBUSY error if
the file is already open in passthrough mode.

Allow taking negative fi->iocachectr refcount with NULL fuse_backing,
because it does not conflict with an already attached fuse_backing
object.

Fixes: 4a90451bbc7f ("fuse: implement open in passthrough mode")
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/fuse/iomode.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/fuse/iomode.c b/fs/fuse/iomode.c
index 1519f895f0a9..f9e30c4540af 100644
--- a/fs/fuse/iomode.c
+++ b/fs/fuse/iomode.c
@@ -89,7 +89,7 @@ int fuse_inode_uncached_io_start(struct fuse_inode *fi, struct fuse_backing *fb)
 	spin_lock(&fi->lock);
 	/* deny conflicting backing files on same fuse inode */
 	oldfb = fuse_inode_backing(fi);
-	if (oldfb && oldfb != fb) {
+	if (fb && oldfb && oldfb != fb) {
 		err = -EBUSY;
 		goto unlock;
 	}
@@ -100,7 +100,7 @@ int fuse_inode_uncached_io_start(struct fuse_inode *fi, struct fuse_backing *fb)
 	fi->iocachectr--;
 
 	/* fuse inode holds a single refcount of backing file */
-	if (!oldfb) {
+	if (fb && !oldfb) {
 		oldfb = fuse_inode_backing_set(fi, fb);
 		WARN_ON_ONCE(oldfb != NULL);
 	} else {
-- 
2.34.1

[PATCH 3/3] fuse: prepare for long lived reference on backing file

[Amir Goldstein]

Currently backing file is associated to fuse inode on the first
passthrough open of the inode and detached on last passthourgh close.

In preparation for attaching a backing file on lookup, allow attaching
a long lived (single) reference on fuse inode backing file that will be
dropped on fuse inode evict.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/fuse/file.c   |  2 +-
 fs/fuse/fuse_i.h |  5 ++++-
 fs/fuse/inode.c  |  6 ++++++
 fs/fuse/iomode.c | 14 +++++++++++---
 4 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index fcf20b304093..347bae2b287f 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1377,7 +1377,7 @@ static void fuse_dio_lock(struct kiocb *iocb, struct iov_iter *from,
 		 * have raced, so check it again.
 		 */
 		if (fuse_io_past_eof(iocb, from) ||
-		    fuse_inode_uncached_io_start(fi, NULL) != 0) {
+		    fuse_inode_uncached_io_start(fi, NULL, false) != 0) {
 			inode_unlock_shared(inode);
 			inode_lock(inode);
 			*exclusive = true;
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index f23919610313..2f340fd05e8a 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -209,6 +209,8 @@ enum {
 	FUSE_I_BTIME,
 	/* Wants or already has page cache IO */
 	FUSE_I_CACHE_IO_MODE,
+	/* Long lived backing file */
+	FUSE_I_BACKING,
 };
 
 struct fuse_conn;
@@ -1396,7 +1398,8 @@ int fuse_fileattr_set(struct mnt_idmap *idmap,
 /* iomode.c */
 int fuse_file_cached_io_open(struct inode *inode, struct fuse_file *ff);
 int fuse_inode_uncached_io_start(struct fuse_inode *fi,
-				 struct fuse_backing *fb);
+				 struct fuse_backing *fb,
+				 bool keep_fb);
 void fuse_inode_uncached_io_end(struct fuse_inode *fi);
 
 int fuse_file_io_open(struct file *file, struct inode *inode);
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 99e44ea7d875..989a84f6a825 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -175,6 +175,12 @@ static void fuse_evict_inode(struct inode *inode)
 		}
 	}
 	if (S_ISREG(inode->i_mode) && !fuse_is_bad(inode)) {
+		/* fuse inode may have a long lived reference to backing file */
+		if (fuse_inode_backing(fi)) {
+			WARN_ON(!test_bit(FUSE_I_BACKING, &fi->state));
+			fuse_inode_uncached_io_end(fi);
+		}
+
 		WARN_ON(fi->iocachectr != 0);
 		WARN_ON(!list_empty(&fi->write_files));
 		WARN_ON(!list_empty(&fi->queued_writes));
diff --git a/fs/fuse/iomode.c b/fs/fuse/iomode.c
index f9e30c4540af..b1ff43668800 100644
--- a/fs/fuse/iomode.c
+++ b/fs/fuse/iomode.c
@@ -80,8 +80,14 @@ static void fuse_file_cached_io_release(struct fuse_file *ff,
 	spin_unlock(&fi->lock);
 }
 
-/* Start strictly uncached io mode where cache access is not allowed */
-int fuse_inode_uncached_io_start(struct fuse_inode *fi, struct fuse_backing *fb)
+/*
+ * Start strictly uncached io mode where cache access is not allowed.
+ *
+ * With @keep_fb true, the backing file reference is expected to be dropped
+ * on inode evict.
+ */
+int fuse_inode_uncached_io_start(struct fuse_inode *fi, struct fuse_backing *fb,
+				 bool keep_fb)
 {
 	struct fuse_backing *oldfb;
 	int err = 0;
@@ -103,6 +109,8 @@ int fuse_inode_uncached_io_start(struct fuse_inode *fi, struct fuse_backing *fb)
 	if (fb && !oldfb) {
 		oldfb = fuse_inode_backing_set(fi, fb);
 		WARN_ON_ONCE(oldfb != NULL);
+		if (keep_fb)
+			set_bit(FUSE_I_BACKING, &fi->state);
 	} else {
 		fuse_backing_put(fb);
 	}
@@ -119,7 +127,7 @@ static int fuse_file_uncached_io_open(struct inode *inode,
 	struct fuse_inode *fi = get_fuse_inode(inode);
 	int err;
 
-	err = fuse_inode_uncached_io_start(fi, fb);
+	err = fuse_inode_uncached_io_start(fi, fb, false);
 	if (err)
 		return err;
 
-- 
2.34.1

Re: [PATCH 1/3] fuse: fix wrong ff->iomode state changes from parallel dio write

[Miklos Szeredi]

On Sun, 7 Apr 2024 at 17:58, Amir Goldstein <amir73il@gmail.com> wrote:
>
> There is a confusion with fuse_file_uncached_io_{start,end} interface.
> These helpers do two things when called from passthrough open()/release():
> 1. Take/drop negative refcount of fi->iocachectr (inode uncached io mode)
> 2. State change ff->iomode IOM_NONE <-> IOM_UNCACHED (file uncached open)
>
> The calls from parallel dio write path need to take a reference on
> fi->iocachectr, but they should not be changing ff->iomode state,
> because in this case, the fi->iocachectr reference does not stick around
> until file release().

Okay.

>
> Factor out helpers fuse_inode_uncached_io_{start,end}, to be used from
> parallel dio write path and rename fuse_file_*cached_io_{start,end}
> helpers to fuse_file_*cached_io_{open,release} to clarify the difference.
>
> Add a check of ff->iomode in mmap(), so that fuse_file_cached_io_open()
> is called only on first mmap of direct_io file.

Is this supposed to be an optimization?  AFAICS it's wrong, because it
moves the check outside of any relevant locks.


> @@ -56,8 +57,7 @@ int fuse_file_cached_io_start(struct inode *inode, struct fuse_file *ff)
>                 return -ETXTBSY;
>         }
>
> -       WARN_ON(ff->iomode == IOM_UNCACHED);
> -       if (ff->iomode == IOM_NONE) {
> +       if (!WARN_ON(ff->iomode != IOM_NONE)) {

This double negation is ugly.  Just let the compiler optimize away the
second comparison.

Thanks,
Miklos

Re: [PATCH 1/3] fuse: fix wrong ff->iomode state changes from parallel dio write

[Amir Goldstein]

On Tue, Apr 9, 2024 at 4:33 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Sun, 7 Apr 2024 at 17:58, Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > There is a confusion with fuse_file_uncached_io_{start,end} interface.
> > These helpers do two things when called from passthrough open()/release():
> > 1. Take/drop negative refcount of fi->iocachectr (inode uncached io mode)
> > 2. State change ff->iomode IOM_NONE <-> IOM_UNCACHED (file uncached open)
> >
> > The calls from parallel dio write path need to take a reference on
> > fi->iocachectr, but they should not be changing ff->iomode state,
> > because in this case, the fi->iocachectr reference does not stick around
> > until file release().
>
> Okay.
>
> >
> > Factor out helpers fuse_inode_uncached_io_{start,end}, to be used from
> > parallel dio write path and rename fuse_file_*cached_io_{start,end}
> > helpers to fuse_file_*cached_io_{open,release} to clarify the difference.
> >
> > Add a check of ff->iomode in mmap(), so that fuse_file_cached_io_open()
> > is called only on first mmap of direct_io file.
>
> Is this supposed to be an optimization?

No.
The reason I did this is because I wanted to differentiate
the refcount semantics (start/end)
from the state semantics (open/release)
and to make it clearer that there is only one state change
and refcount increment on the first mmap().

> AFAICS it's wrong, because it
> moves the check outside of any relevant locks.
>

Aren't concurrent mmap serialized on some lock?

Anyway, I think that the only "bug" that this can trigger is the
WARN_ON(ff->iomode != IOM_NONE)
so if we ....

>
> > @@ -56,8 +57,7 @@ int fuse_file_cached_io_start(struct inode *inode, struct fuse_file *ff)
> >                 return -ETXTBSY;
> >         }
> >
> > -       WARN_ON(ff->iomode == IOM_UNCACHED);
> > -       if (ff->iomode == IOM_NONE) {
> > +       if (!WARN_ON(ff->iomode != IOM_NONE)) {
>
> This double negation is ugly.  Just let the compiler optimize away the
> second comparison.

...drop this change, we should be good.

If you agree, do you need me to re-post?

Thanks,
Amir.

Re: [PATCH 1/3] fuse: fix wrong ff->iomode state changes from parallel dio write

[Miklos Szeredi]

On Tue, 9 Apr 2024 at 17:10, Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Tue, Apr 9, 2024 at 4:33 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Sun, 7 Apr 2024 at 17:58, Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > There is a confusion with fuse_file_uncached_io_{start,end} interface.
> > > These helpers do two things when called from passthrough open()/release():
> > > 1. Take/drop negative refcount of fi->iocachectr (inode uncached io mode)
> > > 2. State change ff->iomode IOM_NONE <-> IOM_UNCACHED (file uncached open)
> > >
> > > The calls from parallel dio write path need to take a reference on
> > > fi->iocachectr, but they should not be changing ff->iomode state,
> > > because in this case, the fi->iocachectr reference does not stick around
> > > until file release().
> >
> > Okay.
> >
> > >
> > > Factor out helpers fuse_inode_uncached_io_{start,end}, to be used from
> > > parallel dio write path and rename fuse_file_*cached_io_{start,end}
> > > helpers to fuse_file_*cached_io_{open,release} to clarify the difference.
> > >
> > > Add a check of ff->iomode in mmap(), so that fuse_file_cached_io_open()
> > > is called only on first mmap of direct_io file.
> >
> > Is this supposed to be an optimization?
>
> No.
> The reason I did this is because I wanted to differentiate
> the refcount semantics (start/end)
> from the state semantics (open/release)
> and to make it clearer that there is only one state change
> and refcount increment on the first mmap().
>
> > AFAICS it's wrong, because it
> > moves the check outside of any relevant locks.
> >
>
> Aren't concurrent mmap serialized on some lock?

Only on current->mm, which doesn't serialize mmaps of the same file in
different processes.

>
> Anyway, I think that the only "bug" that this can trigger is the
> WARN_ON(ff->iomode != IOM_NONE)
> so if we ....
>
> >
> > > @@ -56,8 +57,7 @@ int fuse_file_cached_io_start(struct inode *inode, struct fuse_file *ff)
> > >                 return -ETXTBSY;
> > >         }
> > >
> > > -       WARN_ON(ff->iomode == IOM_UNCACHED);
> > > -       if (ff->iomode == IOM_NONE) {
> > > +       if (!WARN_ON(ff->iomode != IOM_NONE)) {
> >
> > This double negation is ugly.  Just let the compiler optimize away the
> > second comparison.
>
> ...drop this change, we should be good.
>
> If you agree, do you need me to re-post?

Okay, but then what's the point of the unlocked check?

Thanks,
Miklos

Re: [PATCH 1/3] fuse: fix wrong ff->iomode state changes from parallel dio write

[Amir Goldstein]

On Tue, Apr 9, 2024 at 6:32 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Tue, 9 Apr 2024 at 17:10, Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Tue, Apr 9, 2024 at 4:33 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > >
> > > On Sun, 7 Apr 2024 at 17:58, Amir Goldstein <amir73il@gmail.com> wrote:
> > > >
> > > > There is a confusion with fuse_file_uncached_io_{start,end} interface.
> > > > These helpers do two things when called from passthrough open()/release():
> > > > 1. Take/drop negative refcount of fi->iocachectr (inode uncached io mode)
> > > > 2. State change ff->iomode IOM_NONE <-> IOM_UNCACHED (file uncached open)
> > > >
> > > > The calls from parallel dio write path need to take a reference on
> > > > fi->iocachectr, but they should not be changing ff->iomode state,
> > > > because in this case, the fi->iocachectr reference does not stick around
> > > > until file release().
> > >
> > > Okay.
> > >
> > > >
> > > > Factor out helpers fuse_inode_uncached_io_{start,end}, to be used from
> > > > parallel dio write path and rename fuse_file_*cached_io_{start,end}
> > > > helpers to fuse_file_*cached_io_{open,release} to clarify the difference.
> > > >
> > > > Add a check of ff->iomode in mmap(), so that fuse_file_cached_io_open()
> > > > is called only on first mmap of direct_io file.
> > >
> > > Is this supposed to be an optimization?
> >
> > No.
> > The reason I did this is because I wanted to differentiate
> > the refcount semantics (start/end)
> > from the state semantics (open/release)
> > and to make it clearer that there is only one state change
> > and refcount increment on the first mmap().
> >
> > > AFAICS it's wrong, because it
> > > moves the check outside of any relevant locks.
> > >
> >
> > Aren't concurrent mmap serialized on some lock?
>
> Only on current->mm, which doesn't serialize mmaps of the same file in
> different processes.
>
> >
> > Anyway, I think that the only "bug" that this can trigger is the
> > WARN_ON(ff->iomode != IOM_NONE)
> > so if we ....
> >
> > >
> > > > @@ -56,8 +57,7 @@ int fuse_file_cached_io_start(struct inode *inode, struct fuse_file *ff)
> > > >                 return -ETXTBSY;
> > > >         }
> > > >
> > > > -       WARN_ON(ff->iomode == IOM_UNCACHED);
> > > > -       if (ff->iomode == IOM_NONE) {
> > > > +       if (!WARN_ON(ff->iomode != IOM_NONE)) {
> > >
> > > This double negation is ugly.  Just let the compiler optimize away the
> > > second comparison.
> >
> > ...drop this change, we should be good.
> >
> > If you agree, do you need me to re-post?
>
> Okay, but then what's the point of the unlocked check?

As I wrote, I just did it to emphasize the open-once
semantics.
If you do not like the unlocked check, feel free to remove it.

Thanks,
Amir.

Re: [PATCH 1/3] fuse: fix wrong ff->iomode state changes from parallel dio write

[Amir Goldstein]

On Tue, Apr 9, 2024 at 7:18 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Tue, Apr 9, 2024 at 6:32 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Tue, 9 Apr 2024 at 17:10, Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > On Tue, Apr 9, 2024 at 4:33 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > > >
> > > > On Sun, 7 Apr 2024 at 17:58, Amir Goldstein <amir73il@gmail.com> wrote:
> > > > >
> > > > > There is a confusion with fuse_file_uncached_io_{start,end} interface.
> > > > > These helpers do two things when called from passthrough open()/release():
> > > > > 1. Take/drop negative refcount of fi->iocachectr (inode uncached io mode)
> > > > > 2. State change ff->iomode IOM_NONE <-> IOM_UNCACHED (file uncached open)
> > > > >
> > > > > The calls from parallel dio write path need to take a reference on
> > > > > fi->iocachectr, but they should not be changing ff->iomode state,
> > > > > because in this case, the fi->iocachectr reference does not stick around
> > > > > until file release().
> > > >
> > > > Okay.
> > > >
> > > > >
> > > > > Factor out helpers fuse_inode_uncached_io_{start,end}, to be used from
> > > > > parallel dio write path and rename fuse_file_*cached_io_{start,end}
> > > > > helpers to fuse_file_*cached_io_{open,release} to clarify the difference.
> > > > >
> > > > > Add a check of ff->iomode in mmap(), so that fuse_file_cached_io_open()
> > > > > is called only on first mmap of direct_io file.
> > > >
> > > > Is this supposed to be an optimization?
> > >
> > > No.
> > > The reason I did this is because I wanted to differentiate
> > > the refcount semantics (start/end)
> > > from the state semantics (open/release)
> > > and to make it clearer that there is only one state change
> > > and refcount increment on the first mmap().
> > >
> > > > AFAICS it's wrong, because it
> > > > moves the check outside of any relevant locks.
> > > >
> > >
> > > Aren't concurrent mmap serialized on some lock?
> >
> > Only on current->mm, which doesn't serialize mmaps of the same file in
> > different processes.
> >
> > >
> > > Anyway, I think that the only "bug" that this can trigger is the
> > > WARN_ON(ff->iomode != IOM_NONE)
> > > so if we ....
> > >
> > > >
> > > > > @@ -56,8 +57,7 @@ int fuse_file_cached_io_start(struct inode *inode, struct fuse_file *ff)
> > > > >                 return -ETXTBSY;
> > > > >         }
> > > > >
> > > > > -       WARN_ON(ff->iomode == IOM_UNCACHED);
> > > > > -       if (ff->iomode == IOM_NONE) {
> > > > > +       if (!WARN_ON(ff->iomode != IOM_NONE)) {
> > > >
> > > > This double negation is ugly.  Just let the compiler optimize away the
> > > > second comparison.
> > >
> > > ...drop this change, we should be good.
> > >
> > > If you agree, do you need me to re-post?
> >
> > Okay, but then what's the point of the unlocked check?
>
> As I wrote, I just did it to emphasize the open-once
> semantics.
> If you do not like the unlocked check, feel free to remove it.

Miklos,

I see that you removed the unlocked check in the fix staged for-next.
Please also remove this from commit message:

    Add a check of ff->iomode in mmap(), so that fuse_file_cached_io_open() is
    called only on first mmap of direct_io file.

Thanks,
Amir.

Re: [PATCH 1/3] fuse: fix wrong ff->iomode state changes from parallel dio write

[Miklos Szeredi]

On Sat, 13 Apr 2024 at 08:50, Amir Goldstein <amir73il@gmail.com> wrote:

> I see that you removed the unlocked check in the fix staged for-next.
> Please also remove this from commit message:
>
>     Add a check of ff->iomode in mmap(), so that fuse_file_cached_io_open() is
>     called only on first mmap of direct_io file.

Done.

Thanks,
Miklos