[PATCH v5 0/3] FUSE: Implement atomic lookup + open/create

[PATCH v5 0/3] FUSE: Implement atomic lookup + open/create

[Dharmendra Singh]

In FUSE, as of now, uncached lookups are expensive over the wire.
E.g additional latencies and stressing (meta data) servers from
thousands of clients. These lookup calls possibly can be avoided
in some cases. Incoming three patches address this issue.


Fist patch handles the case where we are creating a file with O_CREAT.
Before we go for file creation, we do a lookup on the file which is most
likely non-existent. After this lookup is done, we again go into libfuse
to create file. Such lookups where file is most likely non-existent, can
be avoided.

Second patch handles the case where we open first time a file/dir
but do a lookup first on it. After lookup is performed we make another
call into libfuse to open the file. Now these two separate calls into
libfuse can be combined and performed as a single call into libfuse.

Here is the link to performance numbers
https://lore.kernel.org/linux-fsdevel/20220322121212.5087-1-dharamhans87@gmail.com/


Dharmendra Singh (3):
  FUSE: Avoid lookups in fuse create
  FUSE: Rename fuse_create_open() to fuse_atomic_common()
  Implement atomic lookup + open

 fs/fuse/dir.c             | 149 +++++++++++++++++++++++++++++++++-----
 fs/fuse/fuse_i.h          |   9 +++
 include/uapi/linux/fuse.h |   5 +-
 3 files changed, 143 insertions(+), 20 deletions(-)

---
v4: Addressed all comments and refactored the code into 3 patches for extended 
    create and atomic open. Dropped the patch for optimizing lookup in
    d_revalidate().
---

[PATCH v5 1/3] FUSE: Avoid lookups in fuse create

[Dharmendra Singh]

From: Dharmendra Singh <dsingh@ddn.com>

When we go for creating a file (O_CREAT), we trigger
a lookup to FUSE USER space. It is very  much likely
that file does not exist yet as O_CREAT is passed to
open(). This extra lookup call can be avoided.

Here is how current fuse create works:

A. Looks up dentry (if d_in_lookup() is set.
B. If dentry is positive or O_CREAT is not set, return.
C. If server supports atomic create + open, use that to create file
   and open it as well.
D. If server does not support atomic create + open, just create file
   using "mknod" and return. VFS will take care of opening the file.

Here is how the proposed patch would work:

A. Skip lookup if extended create is supported and file is being
   created.
B. Remains same. if dentry is positive or O_CREATE is not set, return.
C. If server supports new extended create, use that.
D. If not, if server supports atomic create + open, use that
E. If not, fall back to mknod and do not open file.

(Current code returns file attributes from user space as part of
 reply of FUSE_CREATE call itself.)

It is expected that USER SPACE create the file, open it and fills in
the attributes which are then used to make inode stand/revalidate
in the kernel cache. Also if file was newly created(does not
exist yet by this time) in USER SPACE then it should be indicated
in `struct fuse_file_info` by setting a bit which is again used by
libfuse to send some flags back to fuse kernel to indicate that
that file was newly created. These flags are used by kernel to
indicate changes in parent directory.

Fuse kernel automatically detects if extended create is implemented
by libfuse/USER SPACE or not. And depending upon the outcome of
this check all further creates are decided to be extended create or
the old create way.

If libfuse/USER SPACE has not implemented the extended create operation
then by default behaviour remains same i.e we do not optimize lookup
calls which are triggered before create calls into libfuse.

Signed-off-by: Dharmendra Singh <dsingh@ddn.com>
---
 fs/fuse/dir.c             | 84 +++++++++++++++++++++++++++++++++------
 fs/fuse/fuse_i.h          |  6 +++
 include/uapi/linux/fuse.h |  3 ++
 3 files changed, 81 insertions(+), 12 deletions(-)

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 656e921f3506..ed9da8d6b57b 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -523,7 +523,7 @@ static int get_security_context(struct dentry *entry, umode_t mode,
  */
 static int fuse_create_open(struct inode *dir, struct dentry *entry,
 			    struct file *file, unsigned int flags,
-			    umode_t mode)
+			    umode_t mode, uint32_t opcode)
 {
 	int err;
 	struct inode *inode;
@@ -535,8 +535,10 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
 	struct fuse_entry_out outentry;
 	struct fuse_inode *fi;
 	struct fuse_file *ff;
+	struct dentry *res = NULL;
 	void *security_ctx = NULL;
 	u32 security_ctxlen;
+	bool ext_create = (opcode == FUSE_CREATE_EXT ? true : false);
 
 	/* Userspace expects S_IFREG in create mode */
 	BUG_ON((mode & S_IFMT) != S_IFREG);
@@ -566,7 +568,7 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
 		inarg.open_flags |= FUSE_OPEN_KILL_SUIDGID;
 	}
 
-	args.opcode = FUSE_CREATE;
+	args.opcode = opcode;
 	args.nodeid = get_node_id(dir);
 	args.in_numargs = 2;
 	args.in_args[0].size = sizeof(inarg);
@@ -613,9 +615,37 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
 		goto out_err;
 	}
 	kfree(forget);
-	d_instantiate(entry, inode);
+	/*
+	 * In extended create, fuse_lookup() was skipped, which also uses
+	 * d_splice_alias(). As we come directly here after picking up dentry
+	 * it is very much likely that dentry has DCACHE_PAR_LOOKUP flag set
+	 * on it so call d_splice_alias().
+	 */
+	if (!ext_create && !d_in_lookup(entry))
+		d_instantiate(entry, inode);
+	else {
+		res = d_splice_alias(inode, entry);
+		if (IS_ERR(res)) {
+			/* Close the file in user space, but do not unlink it,
+			 * if it was created - with network file systems other
+			 * clients might have already accessed it.
+			 */
+			fi = get_fuse_inode(inode);
+			fuse_sync_release(fi, ff, flags);
+			fuse_queue_forget(fm->fc, forget, outentry.nodeid, 1);
+			err = PTR_ERR(res);
+			goto out_err;
+		}
+	}
 	fuse_change_entry_timeout(entry, &outentry);
-	fuse_dir_changed(dir);
+	/*
+	 * This should be always set when the file is created, but only
+	 * CREATE_EXT introduced FOPEN_FILE_CREATED to user space.
+	 */
+	if (!ext_create || (outopen.open_flags & FOPEN_FILE_CREATED)) {
+		fuse_dir_changed(dir);
+		file->f_mode |= FMODE_CREATED;
+	}
 	err = finish_open(file, entry, generic_file_open);
 	if (err) {
 		fi = get_fuse_inode(inode);
@@ -634,6 +664,29 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
 	return err;
 }
 
+static int fuse_create_ext(struct inode *dir, struct dentry *entry,
+			   struct file *file, unsigned int flags,
+			   umode_t mode)
+{
+	int err;
+	struct fuse_conn *fc = get_fuse_conn(dir);
+
+	if (fc->no_create_ext)
+		return -ENOSYS;
+
+	err = fuse_create_open(dir, entry, file, flags, mode,
+			       FUSE_CREATE_EXT);
+	/* If ext create is not implemented then indicate in fc so that next
+	 * request falls back to normal create instead of going into libufse and
+	 * returning with -ENOSYS.
+	 */
+	if (err == -ENOSYS) {
+		if (!fc->no_create_ext)
+			fc->no_create_ext = 1;
+	}
+	return err;
+}
+
 static int fuse_mknod(struct user_namespace *, struct inode *, struct dentry *,
 		      umode_t, dev_t);
 static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
@@ -643,29 +696,35 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
 	int err;
 	struct fuse_conn *fc = get_fuse_conn(dir);
 	struct dentry *res = NULL;
+	bool create = flags & O_CREAT ? true : false;
 
 	if (fuse_is_bad(dir))
 		return -EIO;
 
-	if (d_in_lookup(entry)) {
+lookup:
+	if ((!create || fc->no_create_ext) && d_in_lookup(entry)) {
 		res = fuse_lookup(dir, entry, 0);
 		if (IS_ERR(res))
 			return PTR_ERR(res);
-
 		if (res)
 			entry = res;
 	}
-
-	if (!(flags & O_CREAT) || d_really_is_positive(entry))
+	if (!create || d_really_is_positive(entry))
 		goto no_open;
 
-	/* Only creates */
-	file->f_mode |= FMODE_CREATED;
-
 	if (fc->no_create)
 		goto mknod;
 
-	err = fuse_create_open(dir, entry, file, flags, mode);
+	if (!fc->no_create_ext) {
+		err = fuse_create_ext(dir, entry, file, flags, mode);
+		/* If libfuse/user space has not implemented extended create,
+		 * fall back to normal create.
+		 */
+		if (err == -ENOSYS)
+			goto lookup;
+	} else
+		err = fuse_create_open(dir, entry, file, flags, mode,
+				       FUSE_CREATE);
 	if (err == -ENOSYS) {
 		fc->no_create = 1;
 		goto mknod;
@@ -683,6 +742,7 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
 }
 
 /*
+
  * Code shared between mknod, mkdir, symlink and link
  */
 static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index e8e59fbdefeb..266133dcab5e 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -669,6 +669,12 @@ struct fuse_conn {
 	/** Is open/release not implemented by fs? */
 	unsigned no_open:1;
 
+	/*
+	 * Is atomic lookup-create-open(extended create) not implemented
+	 * by fs?
+	 */
+	unsigned no_create_ext:1;
+
 	/** Is opendir/releasedir not implemented by fs? */
 	unsigned no_opendir:1;
 
diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
index d6ccee961891..bebe4be3f1cb 100644
--- a/include/uapi/linux/fuse.h
+++ b/include/uapi/linux/fuse.h
@@ -301,6 +301,7 @@ struct fuse_file_lock {
  * FOPEN_CACHE_DIR: allow caching this directory
  * FOPEN_STREAM: the file is stream-like (no file position at all)
  * FOPEN_NOFLUSH: don't flush data cache on close (unless FUSE_WRITEBACK_CACHE)
+ * FOPEN_FILE_CREATED: the file was actually created
  */
 #define FOPEN_DIRECT_IO		(1 << 0)
 #define FOPEN_KEEP_CACHE	(1 << 1)
@@ -308,6 +309,7 @@ struct fuse_file_lock {
 #define FOPEN_CACHE_DIR		(1 << 3)
 #define FOPEN_STREAM		(1 << 4)
 #define FOPEN_NOFLUSH		(1 << 5)
+#define FOPEN_FILE_CREATED	(1 << 6)
 
 /**
  * INIT request/reply flags
@@ -537,6 +539,7 @@ enum fuse_opcode {
 	FUSE_SETUPMAPPING	= 48,
 	FUSE_REMOVEMAPPING	= 49,
 	FUSE_SYNCFS		= 50,
+	FUSE_CREATE_EXT		= 51,
 
 	/* CUSE specific operations */
 	CUSE_INIT		= 4096,
-- 
2.17.1

Re: [PATCH v5 1/3] FUSE: Avoid lookups in fuse create

[Vivek Goyal]

On Tue, May 17, 2022 at 03:37:42PM +0530, Dharmendra Singh wrote:
> From: Dharmendra Singh <dsingh@ddn.com>
> 
> When we go for creating a file (O_CREAT), we trigger
> a lookup to FUSE USER space. It is very  much likely
> that file does not exist yet as O_CREAT is passed to
> open(). This extra lookup call can be avoided.
> 
> Here is how current fuse create works:
> 
> A. Looks up dentry (if d_in_lookup() is set.
> B. If dentry is positive or O_CREAT is not set, return.
> C. If server supports atomic create + open, use that to create file
>    and open it as well.
> D. If server does not support atomic create + open, just create file
>    using "mknod" and return. VFS will take care of opening the file.
> 
> Here is how the proposed patch would work:
> 
> A. Skip lookup if extended create is supported and file is being
>    created.
> B. Remains same. if dentry is positive or O_CREATE is not set, return.
> C. If server supports new extended create, use that.
> D. If not, if server supports atomic create + open, use that
> E. If not, fall back to mknod and do not open file.
> 
> (Current code returns file attributes from user space as part of
>  reply of FUSE_CREATE call itself.)
> 
> It is expected that USER SPACE create the file, open it and fills in
> the attributes which are then used to make inode stand/revalidate
> in the kernel cache.

Even current FUSE_CREATE command does that. I think we need to make
changelogs little more readable and clear. Something like.

Current FUSE_CREATE command creates and opens a file. Client has
either has a positive dentry or has done lookup to figure out if
file needs to be created or not. Assumption here is that file does
not exist on server and needs to be created.

Now add command FUSE_CREATE_EXT which can return information whether
file was actually created or not. It is possible that file already
exists. Server sets bit FOPEN_FILE_CREATED in fuse_file_info if
file was indeed created.

> Also if file was newly created(does not
> exist yet by this time) in USER SPACE then it should be indicated
> in `struct fuse_file_info` by setting a bit which is again used by
> libfuse to send some flags back to fuse kernel to indicate that
> that file was newly created. These flags are used by kernel to
> indicate changes in parent directory.
> 
> Fuse kernel automatically detects if extended create is implemented
> by libfuse/USER SPACE or not. And depending upon the outcome of
> this check all further creates are decided to be extended create or
> the old create way.
> 
> If libfuse/USER SPACE has not implemented the extended create operation
> then by default behaviour remains same i.e we do not optimize lookup
> calls which are triggered before create calls into libfuse.
> 
> Signed-off-by: Dharmendra Singh <dsingh@ddn.com>
> ---
>  fs/fuse/dir.c             | 84 +++++++++++++++++++++++++++++++++------
>  fs/fuse/fuse_i.h          |  6 +++
>  include/uapi/linux/fuse.h |  3 ++
>  3 files changed, 81 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index 656e921f3506..ed9da8d6b57b 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -523,7 +523,7 @@ static int get_security_context(struct dentry *entry, umode_t mode,
>   */
>  static int fuse_create_open(struct inode *dir, struct dentry *entry,
>  			    struct file *file, unsigned int flags,
> -			    umode_t mode)
> +			    umode_t mode, uint32_t opcode)
>  {
>  	int err;
>  	struct inode *inode;
> @@ -535,8 +535,10 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
>  	struct fuse_entry_out outentry;
>  	struct fuse_inode *fi;
>  	struct fuse_file *ff;
> +	struct dentry *res = NULL;
>  	void *security_ctx = NULL;
>  	u32 security_ctxlen;
> +	bool ext_create = (opcode == FUSE_CREATE_EXT ? true : false);
>  
>  	/* Userspace expects S_IFREG in create mode */
>  	BUG_ON((mode & S_IFMT) != S_IFREG);
> @@ -566,7 +568,7 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
>  		inarg.open_flags |= FUSE_OPEN_KILL_SUIDGID;
>  	}
>  
> -	args.opcode = FUSE_CREATE;
> +	args.opcode = opcode;
>  	args.nodeid = get_node_id(dir);
>  	args.in_numargs = 2;
>  	args.in_args[0].size = sizeof(inarg);
> @@ -613,9 +615,37 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
>  		goto out_err;
>  	}
>  	kfree(forget);
> -	d_instantiate(entry, inode);
> +	/*
> +	 * In extended create, fuse_lookup() was skipped, which also uses
> +	 * d_splice_alias(). As we come directly here after picking up dentry
> +	 * it is very much likely that dentry has DCACHE_PAR_LOOKUP flag set
> +	 * on it so call d_splice_alias().
> +	 */

Ok, following piece of code I don't understand. If we are not using
FUSE_CREATE_EXT, then d_in_lookup() should always be false. Because
in that case we have done lookup() if needed. So what's the point
of following check.
> +	if (!ext_create && !d_in_lookup(entry))
			    ^^^^ This should always be true if
			    ext_create=0.

> +		d_instantiate(entry, inode);
> +	else {
> +		res = d_splice_alias(inode, entry);

To me we have following 3 conditions.

A. if FUSE_CREATE is being used, we have done the lookup if needed. File
   has definitely been created. Just call d_instantiate().

B. If we are using FUSE_CRATE_EXT, then it is possibel that d_in_lookup()
   is true. But it is also possible that dentry is negative. So looks
   you probably want to do this.

   if (!ext_create || !d_in_lookup()) {
   	d_instanatiate()
   } else {
	/* Dentry is in lookup() as well as we used FUSE_CRATE_EXT and
	 * skipped lookup. So use d_splice_alias() instead of
	 * d_instantiate().
	 */
   	d_splice_alias();
   }

Having said that I am not sure if using dentry_spliace_alias() is the
right thing. I think Miklos or somebody else who understands associated
logic better should have a look at it.

I am just trying to reason through the code based on your arguments.

> +		if (IS_ERR(res)) {
> +			/* Close the file in user space, but do not unlink it,
> +			 * if it was created - with network file systems other
> +			 * clients might have already accessed it.
> +			 */
> +			fi = get_fuse_inode(inode);
> +			fuse_sync_release(fi, ff, flags);
> +			fuse_queue_forget(fm->fc, forget, outentry.nodeid, 1);
> +			err = PTR_ERR(res);
> +			goto out_err;
> +		}
> +	}
>  	fuse_change_entry_timeout(entry, &outentry);
> -	fuse_dir_changed(dir);
> +	/*
> +	 * This should be always set when the file is created, but only
> +	 * CREATE_EXT introduced FOPEN_FILE_CREATED to user space.
> +	 */
> +	if (!ext_create || (outopen.open_flags & FOPEN_FILE_CREATED)) {
> +		fuse_dir_changed(dir);
> +		file->f_mode |= FMODE_CREATED;
> +	}
>  	err = finish_open(file, entry, generic_file_open);
>  	if (err) {
>  		fi = get_fuse_inode(inode);
> @@ -634,6 +664,29 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
>  	return err;
>  }
>  
> +static int fuse_create_ext(struct inode *dir, struct dentry *entry,
> +			   struct file *file, unsigned int flags,
> +			   umode_t mode)
> +{
> +	int err;
> +	struct fuse_conn *fc = get_fuse_conn(dir);
> +
> +	if (fc->no_create_ext)
> +		return -ENOSYS;

If we check this in fuse_atomic_open(), then we don't need this check
here. Look at suggested changes below in fuse_atomic_open().

> +
> +	err = fuse_create_open(dir, entry, file, flags, mode,
> +			       FUSE_CREATE_EXT);
> +	/* If ext create is not implemented then indicate in fc so that next
> +	 * request falls back to normal create instead of going into libufse and
> +	 * returning with -ENOSYS.
> +	 */
> +	if (err == -ENOSYS) {
> +		if (!fc->no_create_ext)

Why to check for !fc->no_create_ext.
> +			fc->no_create_ext = 1;
> +	}
> +	return err;
> +}

I think we can completely get rid of fuse_create_ext() function. And
just add a parameter "opcode" to FUSE_CREATE_OPEN and that should
do it.

> +
>  static int fuse_mknod(struct user_namespace *, struct inode *, struct dentry *,
>  		      umode_t, dev_t);
>  static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
> @@ -643,29 +696,35 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
>  	int err;
>  	struct fuse_conn *fc = get_fuse_conn(dir);
>  	struct dentry *res = NULL;
> +	bool create = flags & O_CREAT ? true : false;
>  
>  	if (fuse_is_bad(dir))
>  		return -EIO;
>  
> -	if (d_in_lookup(entry)) {
> +lookup:
> +	if ((!create || fc->no_create_ext) && d_in_lookup(entry)) {
>  		res = fuse_lookup(dir, entry, 0);
>  		if (IS_ERR(res))
>  			return PTR_ERR(res);
> -
>  		if (res)
>  			entry = res;
>  	}
> -
> -	if (!(flags & O_CREAT) || d_really_is_positive(entry))
> +	if (!create || d_really_is_positive(entry))
>  		goto no_open;
>  
> -	/* Only creates */
> -	file->f_mode |= FMODE_CREATED;
> -
>  	if (fc->no_create)

This should be (fc_no_create && fc->no_create_ext)? In theory it is
possible that a new server has FUSE_CREATE_EXT implemented but not
FUSE_CREATE?

If we are expecting FUSE_CREATE_EXT to be a super set of FUSE_CREATE,
then this should be possible. Maybe it is a good idea to not assume
the state of dentry (whether it is in lookup or not) if FUSE_CREATE_EXT 
is being used. And just design FUSE_CREATE_EXT to be superset of
FUSE_CREATE.

>  		goto mknod;
>  
> -	err = fuse_create_open(dir, entry, file, flags, mode);
> +	if (!fc->no_create_ext) {

What happens if dentry is negative here and d_in_lookup(entry) == false?
I mean there is no need to use FUSE_CREATE_EXT in that case necessarily?

But using it does not harm either. Just that we need to then keep track
what's the state of dentry. Whether it was in in lookup or not.


> +		err = fuse_create_ext(dir, entry, file, flags, mode);
> +		/* If libfuse/user space has not implemented extended create,
> +		 * fall back to normal create.
> +		 */
> +		if (err == -ENOSYS)
> +			goto lookup;
> +	} else
> +		err = fuse_create_open(dir, entry, file, flags, mode,
> +				       FUSE_CREATE);

BTW, may be following code structure is better. We can just add a label
for create_ext.

	if (fc->no_create_ext)
		goto create;
	err = fuse_create_ext(dir, entry, file, flags, mode);
	if (err == -ENOSYS) {
		fc->no_create_ext = 1;
		/*
		 * We might have skipped lookup assuming FUSE_CREATE_EXT
		 * is supported. Go through lookup path again if needed.
		 */
		goto lookup;
	}
create:
	err = fuse_create_open(dir, entry, file, flags, mode);

Thanks
Vivek

>  	if (err == -ENOSYS) {
>  		fc->no_create = 1;
>  		goto mknod;
> @@ -683,6 +742,7 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
>  }
>  
>  /*
> +
>   * Code shared between mknod, mkdir, symlink and link
>   */
>  static int create_new_entry(struct fuse_mount *fm, struct fuse_args *args,
> diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
> index e8e59fbdefeb..266133dcab5e 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -669,6 +669,12 @@ struct fuse_conn {
>  	/** Is open/release not implemented by fs? */
>  	unsigned no_open:1;
>  
> +	/*
> +	 * Is atomic lookup-create-open(extended create) not implemented
> +	 * by fs?
> +	 */
> +	unsigned no_create_ext:1;
> +
>  	/** Is opendir/releasedir not implemented by fs? */
>  	unsigned no_opendir:1;
>  
> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> index d6ccee961891..bebe4be3f1cb 100644
> --- a/include/uapi/linux/fuse.h
> +++ b/include/uapi/linux/fuse.h
> @@ -301,6 +301,7 @@ struct fuse_file_lock {
>   * FOPEN_CACHE_DIR: allow caching this directory
>   * FOPEN_STREAM: the file is stream-like (no file position at all)
>   * FOPEN_NOFLUSH: don't flush data cache on close (unless FUSE_WRITEBACK_CACHE)
> + * FOPEN_FILE_CREATED: the file was actually created
>   */
>  #define FOPEN_DIRECT_IO		(1 << 0)
>  #define FOPEN_KEEP_CACHE	(1 << 1)
> @@ -308,6 +309,7 @@ struct fuse_file_lock {
>  #define FOPEN_CACHE_DIR		(1 << 3)
>  #define FOPEN_STREAM		(1 << 4)
>  #define FOPEN_NOFLUSH		(1 << 5)
> +#define FOPEN_FILE_CREATED	(1 << 6)
>  
>  /**
>   * INIT request/reply flags
> @@ -537,6 +539,7 @@ enum fuse_opcode {
>  	FUSE_SETUPMAPPING	= 48,
>  	FUSE_REMOVEMAPPING	= 49,
>  	FUSE_SYNCFS		= 50,
> +	FUSE_CREATE_EXT		= 51,
>  
>  	/* CUSE specific operations */
>  	CUSE_INIT		= 4096,
> -- 
> 2.17.1
> 

Re: [PATCH v5 1/3] FUSE: Avoid lookups in fuse create

[Vivek Goyal]

On Tue, May 17, 2022 at 03:37:42PM +0530, Dharmendra Singh wrote:

[..]
> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> index d6ccee961891..bebe4be3f1cb 100644
> --- a/include/uapi/linux/fuse.h
> +++ b/include/uapi/linux/fuse.h
> @@ -301,6 +301,7 @@ struct fuse_file_lock {
>   * FOPEN_CACHE_DIR: allow caching this directory
>   * FOPEN_STREAM: the file is stream-like (no file position at all)
>   * FOPEN_NOFLUSH: don't flush data cache on close (unless FUSE_WRITEBACK_CACHE)
> + * FOPEN_FILE_CREATED: the file was actually created
>   */
>  #define FOPEN_DIRECT_IO		(1 << 0)
>  #define FOPEN_KEEP_CACHE	(1 << 1)
> @@ -308,6 +309,7 @@ struct fuse_file_lock {
>  #define FOPEN_CACHE_DIR		(1 << 3)
>  #define FOPEN_STREAM		(1 << 4)
>  #define FOPEN_NOFLUSH		(1 << 5)
> +#define FOPEN_FILE_CREATED	(1 << 6)
>  
>  /**
>   * INIT request/reply flags
> @@ -537,6 +539,7 @@ enum fuse_opcode {
>  	FUSE_SETUPMAPPING	= 48,
>  	FUSE_REMOVEMAPPING	= 49,
>  	FUSE_SYNCFS		= 50,
> +	FUSE_CREATE_EXT		= 51,

I am wondering if we really have to introduce a new opcode for this. Both
FUSE_CREATE and FUSE_CREATE_EXT prepare and send fuse_create_in{} and
expect fuse_entry_out and fuse_open_out in response. So no new structures
are being added. Only thing FUSE_CREATE_EXT does extra is that it also
reports back whether file was actually created or not.

May be instead of adding an new fuse_opcode, we could simply add a
new flag which we send in fuse_create_in and that reqeusts to report
if file was created or not. This is along the lines of
FUSE_OPEN_KILL_SUIDGID.

So say, a new flag FUSE_OPEN_REPORT_CREATE flag. Which we will set in
fuse_create_in->open_flags. If file server sees this flag is set, it
knows that it needs to set FOPEN_FILE_CREATED flag in response.

To me creating a new flag FUSE_OPEN_REPORT_CREATE seems better instead
of adding a new opcode.

Thanks
Vivek
>  
>  	/* CUSE specific operations */
>  	CUSE_INIT		= 4096,
> -- 
> 2.17.1
> 

Re: [PATCH v5 1/3] FUSE: Avoid lookups in fuse create

[Vivek Goyal]

On Wed, May 18, 2022 at 01:41:02PM -0400, Vivek Goyal wrote:
> On Tue, May 17, 2022 at 03:37:42PM +0530, Dharmendra Singh wrote:
> 
> [..]
> > diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
> > index d6ccee961891..bebe4be3f1cb 100644
> > --- a/include/uapi/linux/fuse.h
> > +++ b/include/uapi/linux/fuse.h
> > @@ -301,6 +301,7 @@ struct fuse_file_lock {
> >   * FOPEN_CACHE_DIR: allow caching this directory
> >   * FOPEN_STREAM: the file is stream-like (no file position at all)
> >   * FOPEN_NOFLUSH: don't flush data cache on close (unless FUSE_WRITEBACK_CACHE)
> > + * FOPEN_FILE_CREATED: the file was actually created
> >   */
> >  #define FOPEN_DIRECT_IO		(1 << 0)
> >  #define FOPEN_KEEP_CACHE	(1 << 1)
> > @@ -308,6 +309,7 @@ struct fuse_file_lock {
> >  #define FOPEN_CACHE_DIR		(1 << 3)
> >  #define FOPEN_STREAM		(1 << 4)
> >  #define FOPEN_NOFLUSH		(1 << 5)
> > +#define FOPEN_FILE_CREATED	(1 << 6)
> >  
> >  /**
> >   * INIT request/reply flags
> > @@ -537,6 +539,7 @@ enum fuse_opcode {
> >  	FUSE_SETUPMAPPING	= 48,
> >  	FUSE_REMOVEMAPPING	= 49,
> >  	FUSE_SYNCFS		= 50,
> > +	FUSE_CREATE_EXT		= 51,
> 
> I am wondering if we really have to introduce a new opcode for this. Both
> FUSE_CREATE and FUSE_CREATE_EXT prepare and send fuse_create_in{} and
> expect fuse_entry_out and fuse_open_out in response. So no new structures
> are being added. Only thing FUSE_CREATE_EXT does extra is that it also
> reports back whether file was actually created or not.
> 
> May be instead of adding an new fuse_opcode, we could simply add a
> new flag which we send in fuse_create_in and that reqeusts to report
> if file was created or not. This is along the lines of
> FUSE_OPEN_KILL_SUIDGID.
> 
> So say, a new flag FUSE_OPEN_REPORT_CREATE flag. Which we will set in
> fuse_create_in->open_flags. If file server sees this flag is set, it
> knows that it needs to set FOPEN_FILE_CREATED flag in response.
> 
> To me creating a new flag FUSE_OPEN_REPORT_CREATE seems better instead
> of adding a new opcode.

Actually I take that back. If we were to use a flag, then we will have to
do feature negotiation in advance at init time and only then we can set
FUSE_OPEN_REPORT_CREATE. But we are relying on no new feature bit instead
-ENOSYS will be returned if server does not support FUSE_CREATE_EXT.
So adding a new opcode is better.

Thanks
Vivek

Re: [PATCH v5 1/3] FUSE: Avoid lookups in fuse create

[Bernd Schubert]

On 5/18/22 19:44, Vivek Goyal wrote:
> On Wed, May 18, 2022 at 01:41:02PM -0400, Vivek Goyal wrote:
>> On Tue, May 17, 2022 at 03:37:42PM +0530, Dharmendra Singh wrote:
>>
>> [..]
>>> diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
>>> index d6ccee961891..bebe4be3f1cb 100644
>>> --- a/include/uapi/linux/fuse.h
>>> +++ b/include/uapi/linux/fuse.h
>>> @@ -301,6 +301,7 @@ struct fuse_file_lock {
>>>    * FOPEN_CACHE_DIR: allow caching this directory
>>>    * FOPEN_STREAM: the file is stream-like (no file position at all)
>>>    * FOPEN_NOFLUSH: don't flush data cache on close (unless FUSE_WRITEBACK_CACHE)
>>> + * FOPEN_FILE_CREATED: the file was actually created
>>>    */
>>>   #define FOPEN_DIRECT_IO		(1 << 0)
>>>   #define FOPEN_KEEP_CACHE	(1 << 1)
>>> @@ -308,6 +309,7 @@ struct fuse_file_lock {
>>>   #define FOPEN_CACHE_DIR		(1 << 3)
>>>   #define FOPEN_STREAM		(1 << 4)
>>>   #define FOPEN_NOFLUSH		(1 << 5)
>>> +#define FOPEN_FILE_CREATED	(1 << 6)
>>>   
>>>   /**
>>>    * INIT request/reply flags
>>> @@ -537,6 +539,7 @@ enum fuse_opcode {
>>>   	FUSE_SETUPMAPPING	= 48,
>>>   	FUSE_REMOVEMAPPING	= 49,
>>>   	FUSE_SYNCFS		= 50,
>>> +	FUSE_CREATE_EXT		= 51,
>>
>> I am wondering if we really have to introduce a new opcode for this. Both
>> FUSE_CREATE and FUSE_CREATE_EXT prepare and send fuse_create_in{} and
>> expect fuse_entry_out and fuse_open_out in response. So no new structures
>> are being added. Only thing FUSE_CREATE_EXT does extra is that it also
>> reports back whether file was actually created or not.
>>
>> May be instead of adding an new fuse_opcode, we could simply add a
>> new flag which we send in fuse_create_in and that reqeusts to report
>> if file was created or not. This is along the lines of
>> FUSE_OPEN_KILL_SUIDGID.
>>
>> So say, a new flag FUSE_OPEN_REPORT_CREATE flag. Which we will set in
>> fuse_create_in->open_flags. If file server sees this flag is set, it
>> knows that it needs to set FOPEN_FILE_CREATED flag in response.
>>
>> To me creating a new flag FUSE_OPEN_REPORT_CREATE seems better instead
>> of adding a new opcode.
> 
> Actually I take that back. If we were to use a flag, then we will have to
> do feature negotiation in advance at init time and only then we can set
> FUSE_OPEN_REPORT_CREATE. But we are relying on no new feature bit instead
> -ENOSYS will be returned if server does not support FUSE_CREATE_EXT.
> So adding a new opcode is better.

I guess it might work, if a flag is set and also returned (I would then 
call it FUSE_CREATE_EXT) - user space creat would need to set 
FOPEN_FILE_CREATED and that new flag. I just doubt that it simplifies 
things.

Btw, thanks a lot for your thorough reviews! Much appreciated.


Thanks,
Bernd

[PATCH v5 2/3] FUSE: Rename fuse_create_open() to fuse_atomic_common()

[Dharmendra Singh]

This patch just changes function name as it is used in next patch
to make code better readable.

Signed-off-by: Dharmendra Singh <dsingh@ddn.com>
---
 fs/fuse/dir.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index ed9da8d6b57b..517c9add014d 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -521,9 +521,9 @@ static int get_security_context(struct dentry *entry, umode_t mode,
  * If the filesystem doesn't support this, then fall back to separate
  * 'mknod' + 'open' requests.
  */
-static int fuse_create_open(struct inode *dir, struct dentry *entry,
-			    struct file *file, unsigned int flags,
-			    umode_t mode, uint32_t opcode)
+static int fuse_atomic_common(struct inode *dir, struct dentry *entry,
+			      struct file *file, unsigned int flags,
+			      umode_t mode, uint32_t opcode)
 {
 	int err;
 	struct inode *inode;
@@ -674,8 +674,8 @@ static int fuse_create_ext(struct inode *dir, struct dentry *entry,
 	if (fc->no_create_ext)
 		return -ENOSYS;
 
-	err = fuse_create_open(dir, entry, file, flags, mode,
-			       FUSE_CREATE_EXT);
+	err = fuse_atomic_common(dir, entry, file, flags, mode,
+				 FUSE_CREATE_EXT);
 	/* If ext create is not implemented then indicate in fc so that next
 	 * request falls back to normal create instead of going into libufse and
 	 * returning with -ENOSYS.
@@ -723,8 +723,8 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
 		if (err == -ENOSYS)
 			goto lookup;
 	} else
-		err = fuse_create_open(dir, entry, file, flags, mode,
-				       FUSE_CREATE);
+		err = fuse_atomic_common(dir, entry, file, flags, mode,
+					 FUSE_CREATE);
 	if (err == -ENOSYS) {
 		fc->no_create = 1;
 		goto mknod;
-- 
2.17.1

[PATCH v5 3/3] FUSE: Implement atomic lookup + open

[Dharmendra Singh]

We can optimize aggressive lookups which are triggered when
there is normal open for file/dir (dentry is new/negative).

Here in this case since we are anyway going to open the file/dir
with USER SPACE, avoid this separate lookup call into libfuse
and combine it with open call into libfuse.

This lookup + open in single call to libfuse has been named
as atomic open. It is expected that USER SPACE opens the file
and fills in the attributes, which are then used to make inode
stand/revalidate in the kernel cache.

Signed-off-by: Dharmendra Singh <dsingh@ddn.com>
---
 fs/fuse/dir.c             | 109 ++++++++++++++++++++++++++++----------
 fs/fuse/fuse_i.h          |   3 ++
 include/uapi/linux/fuse.h |   2 +-
 3 files changed, 84 insertions(+), 30 deletions(-)

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 517c9add014d..cb99e529b3e9 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -516,14 +516,15 @@ static int get_security_context(struct dentry *entry, umode_t mode,
 }
 
 /*
- * Atomic create+open operation
+ * If user has not implemented create ext or Atomic open + lookup
+ * then fall back to usual Atomic create/open operations.
  *
- * If the filesystem doesn't support this, then fall back to separate
- * 'mknod' + 'open' requests.
+ * If the filesystem doesn't support Atomic create + open, then
+ * fall back to separate 'mknod' + 'open' requests.
  */
 static int fuse_atomic_common(struct inode *dir, struct dentry *entry,
-			      struct file *file, unsigned int flags,
-			      umode_t mode, uint32_t opcode)
+			      struct dentry **alias, struct file *file,
+			      unsigned int flags, umode_t mode, uint32_t opcode)
 {
 	int err;
 	struct inode *inode;
@@ -538,10 +539,21 @@ static int fuse_atomic_common(struct inode *dir, struct dentry *entry,
 	struct dentry *res = NULL;
 	void *security_ctx = NULL;
 	u32 security_ctxlen;
-	bool ext_create = (opcode == FUSE_CREATE_EXT ? true : false);
+	bool simple_create = (opcode == FUSE_CREATE ? true : false);
+	bool create_ops = (simple_create || opcode == FUSE_CREATE_EXT) ?
+			   true : false;
+	bool skipped_lookup = (opcode == FUSE_CREATE_EXT ||
+			       opcode == FUSE_ATOMIC_OPEN) ? true : false;
+
+	if (alias)
+		*alias = NULL;
 
 	/* Userspace expects S_IFREG in create mode */
-	BUG_ON((mode & S_IFMT) != S_IFREG);
+	if (create_ops && (mode & S_IFMT) != S_IFREG) {
+		WARN_ON(1);
+		err = -EINVAL;
+		goto out_err;
+	}
 
 	forget = fuse_alloc_forget();
 	err = -ENOMEM;
@@ -616,33 +628,38 @@ static int fuse_atomic_common(struct inode *dir, struct dentry *entry,
 	}
 	kfree(forget);
 	/*
-	 * In extended create, fuse_lookup() was skipped, which also uses
-	 * d_splice_alias(). As we come directly here after picking up dentry
-	 * it is very much likely that dentry has DCACHE_PAR_LOOKUP flag set
-	 * on it so call d_splice_alias().
+	 * In extended create/atomic open, fuse_lookup() is skipped which also
+	 * uses d_splice_alias(). As we come directly here after picking up
+	 * dentry it is very much likely that dentry has DCACHE_PAR_LOOKUP flag
+	 * set on it so call d_splice_alias().
 	 */
-	if (!ext_create && !d_in_lookup(entry))
-		d_instantiate(entry, inode);
-	else {
+	if (skipped_lookup) {
 		res = d_splice_alias(inode, entry);
-		if (IS_ERR(res)) {
-			/* Close the file in user space, but do not unlink it,
-			 * if it was created - with network file systems other
-			 * clients might have already accessed it.
-			 */
-			fi = get_fuse_inode(inode);
-			fuse_sync_release(fi, ff, flags);
-			fuse_queue_forget(fm->fc, forget, outentry.nodeid, 1);
-			err = PTR_ERR(res);
-			goto out_err;
+		if (res) {
+			if (IS_ERR(res)) {
+				/* Close the file in user space, but do not unlink it,
+				 * if it was created - with network file systems other
+				 * clients might have already accessed it.
+				 */
+				fi = get_fuse_inode(inode);
+				fuse_sync_release(fi, ff, flags);
+				fuse_queue_forget(fm->fc, forget, outentry.nodeid, 1);
+				err = PTR_ERR(res);
+				goto out_err;
+			}
+			entry = res;
+			if (alias)
+				*alias = res;
 		}
-	}
+	} else
+		d_instantiate(entry, inode);
+
 	fuse_change_entry_timeout(entry, &outentry);
 	/*
 	 * This should be always set when the file is created, but only
 	 * CREATE_EXT introduced FOPEN_FILE_CREATED to user space.
 	 */
-	if (!ext_create || (outopen.open_flags & FOPEN_FILE_CREATED)) {
+	if (simple_create || (outopen.open_flags & FOPEN_FILE_CREATED)) {
 		fuse_dir_changed(dir);
 		file->f_mode |= FMODE_CREATED;
 	}
@@ -674,7 +691,7 @@ static int fuse_create_ext(struct inode *dir, struct dentry *entry,
 	if (fc->no_create_ext)
 		return -ENOSYS;
 
-	err = fuse_atomic_common(dir, entry, file, flags, mode,
+	err = fuse_atomic_common(dir, entry, NULL, file, flags, mode,
 				 FUSE_CREATE_EXT);
 	/* If ext create is not implemented then indicate in fc so that next
 	 * request falls back to normal create instead of going into libufse and
@@ -687,6 +704,31 @@ static int fuse_create_ext(struct inode *dir, struct dentry *entry,
 	return err;
 }
 
+static int fuse_do_atomic_open(struct inode *dir, struct dentry *entry,
+				struct dentry **alias, struct file *file,
+				unsigned int flags, umode_t mode)
+{
+	int err;
+	struct fuse_conn *fc = get_fuse_conn(dir);
+
+	if (fc->no_atomic_open)
+		return -ENOSYS;
+
+	err = fuse_atomic_common(dir, entry, alias, file, flags, mode,
+				 FUSE_ATOMIC_OPEN);
+
+	/* Set if atomic open not implemented */
+	if (err == -ENOSYS) {
+		if (!fc->no_atomic_open)
+			fc->no_atomic_open = 1;
+
+	} else if (!fc->atomic_o_trunc) {
+		/* If atomic open is set then imply atomic truncate as well */
+		fc->atomic_o_trunc = 1;
+	}
+	return err;
+}
+
 static int fuse_mknod(struct user_namespace *, struct inode *, struct dentry *,
 		      umode_t, dev_t);
 static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
@@ -695,13 +737,22 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
 {
 	int err;
 	struct fuse_conn *fc = get_fuse_conn(dir);
-	struct dentry *res = NULL;
+	struct dentry *res = NULL, *alias = NULL;
 	bool create = flags & O_CREAT ? true : false;
 
 	if (fuse_is_bad(dir))
 		return -EIO;
 
+	if (!create && !fc->no_atomic_open) {
+		err = fuse_do_atomic_open(dir, entry, &alias,
+					  file, flags, mode);
+		res = alias;
+		if (err != -ENOSYS)
+			goto out_dput;
+	}
+
 lookup:
+	/* Fall back to open- user space does not have full atomic open */
 	if ((!create || fc->no_create_ext) && d_in_lookup(entry)) {
 		res = fuse_lookup(dir, entry, 0);
 		if (IS_ERR(res))
@@ -723,7 +774,7 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
 		if (err == -ENOSYS)
 			goto lookup;
 	} else
-		err = fuse_atomic_common(dir, entry, file, flags, mode,
+		err = fuse_atomic_common(dir, entry, NULL, file, flags, mode,
 					 FUSE_CREATE);
 	if (err == -ENOSYS) {
 		fc->no_create = 1;
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index 266133dcab5e..949c230e14c7 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -675,6 +675,9 @@ struct fuse_conn {
 	 */
 	unsigned no_create_ext:1;
 
+	/** Is atomic open implemented by fs ? */
+	unsigned no_atomic_open : 1;
+
 	/** Is opendir/releasedir not implemented by fs? */
 	unsigned no_opendir:1;
 
diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
index bebe4be3f1cb..f4c94e5bbffc 100644
--- a/include/uapi/linux/fuse.h
+++ b/include/uapi/linux/fuse.h
@@ -540,7 +540,7 @@ enum fuse_opcode {
 	FUSE_REMOVEMAPPING	= 49,
 	FUSE_SYNCFS		= 50,
 	FUSE_CREATE_EXT		= 51,
-
+	FUSE_ATOMIC_OPEN	= 52,
 	/* CUSE specific operations */
 	CUSE_INIT		= 4096,
 
-- 
2.17.1

Re: [PATCH v5 0/3] FUSE: Implement atomic lookup + open/create

[Miklos Szeredi]

On Tue, 17 May 2022 at 12:08, Dharmendra Singh <dharamhans87@gmail.com> wrote:
>
> In FUSE, as of now, uncached lookups are expensive over the wire.
> E.g additional latencies and stressing (meta data) servers from
> thousands of clients. These lookup calls possibly can be avoided
> in some cases. Incoming three patches address this issue.
>
>
> Fist patch handles the case where we are creating a file with O_CREAT.
> Before we go for file creation, we do a lookup on the file which is most
> likely non-existent. After this lookup is done, we again go into libfuse
> to create file. Such lookups where file is most likely non-existent, can
> be avoided.

I'd really like to see a bit wider picture...

We have several cases, first of all let's look at plain O_CREAT
without O_EXCL (assume that there were no changes since the last
lookup for simplicity):

[not cached, negative]
   ->atomic_open()
      LOOKUP
      CREATE

[not cached, positive]
   ->atomic_open()
      LOOKUP
   ->open()
      OPEN

[cached, negative, validity timeout not expired]
   ->d_revalidate()
      return 1
   ->atomic_open()
      CREATE

[cached, negative, validity timeout expired]
   ->d_revalidate()
      return 0
   ->atomic_open()
      LOOKUP
      CREATE

[cached, positive, validity timeout not expired]
   ->d_revalidate()
      return 1
   ->open()
      OPEN

[cached, positive, validity timeout expired]
   ->d_revalidate()
      LOOKUP
      return 1
   ->open()
      OPEN

(Caveat emptor: I'm just looking at the code and haven't actually
tested what happens.)

Apparently in all of these cases we are doing at least one request, so
it would make sense to make them uniform:

[not cached]
   ->atomic_open()
      CREATE_EXT

[cached]
   ->d_revalidate()
      return 0
   ->atomic_open()
      CREATE_EXT

Similarly we can look at the current O_CREAT | O_EXCL cases:

[not cached, negative]
   ->atomic_open()
      LOOKUP
      CREATE

[not cached, positive]
   ->atomic_open()
      LOOKUP
   return -EEXIST

[cached, negative]
   ->d_revalidate()
      return 0 (see LOOKUP_EXCL check)
   ->atomic_open()
      LOOKUP
      CREATE

[cached, positive]
   ->d_revalidate()
      LOOKUP
      return 1
   return -EEXIST

Again we are doing at least one request, so we can unconditionally
replace them with CREATE_EXT like the non-O_EXCL case.


>
> Second patch handles the case where we open first time a file/dir
> but do a lookup first on it. After lookup is performed we make another
> call into libfuse to open the file. Now these two separate calls into
> libfuse can be combined and performed as a single call into libfuse.

And here's my analysis:

[not cached, negative]
   ->lookup()
      LOOKUP
   return -ENOENT

[not cached, positive]
   ->lookup()
      LOOKUP
   ->open()
      OPEN

[cached, negative, validity timeout not expired]
    ->d_revalidate()
       return 1
    return -ENOENT

[cached, negative, validity timeout expired]
   ->d_revalidate()
      return 0
   ->atomic_open()
      LOOKUP
   return -ENOENT

[cached, positive, validity timeout not expired]
   ->d_revalidate()
      return 1
   ->open()
      OPEN

[cached, positive, validity timeout expired]
   ->d_revalidate()
      LOOKUP
      return 1
   ->open()
      OPEN

There's one case were no request is sent:  a valid cached negative
dentry.   Possibly we can also make this uniform, e.g.:

[not cached]
   ->atomic_open()
       OPEN_ATOMIC

[cached, negative, validity timeout not expired]
    ->d_revalidate()
       return 1
    return -ENOENT

[cached, negative, validity timeout expired]
   ->d_revalidate()
      return 0
   ->atomic_open()
      OPEN_ATOMIC

[cached, positive]
   ->d_revalidate()
      return 0
   ->atomic_open()
      OPEN_ATOMIC

It may even make the code simpler to clearly separate the cases where
the atomic variants are supported and when not.  I'd also consider
merging CREATE_EXT into OPEN_ATOMIC, since a filesystem implementing
one will highly likely want to implement the other as well.

Thanks,
Miklos

Re: [PATCH v5 0/3] FUSE: Implement atomic lookup + open/create

[Miklos Szeredi]

On Thu, 19 May 2022 at 11:39, Miklos Szeredi <miklos@szeredi.hu> wrote:

> Apparently in all of these cases we are doing at least one request, so
> it would make sense to make them uniform:
>
> [not cached]
>    ->atomic_open()
>       CREATE_EXT
>
> [cached]
>    ->d_revalidate()
>       return 0

Note to self:  invalidating a valid positive dentry would break things.

Revalidation would need to be moved into ->atomic_open(), which is a
bigger surgery.  Oh well...

Thanks,
Miklos

Re: [PATCH v5 0/3] FUSE: Implement atomic lookup + open/create

[Bernd Schubert]

On 5/19/22 11:39, Miklos Szeredi wrote:
> On Tue, 17 May 2022 at 12:08, Dharmendra Singh <dharamhans87@gmail.com> wrote:
>>
>> In FUSE, as of now, uncached lookups are expensive over the wire.
>> E.g additional latencies and stressing (meta data) servers from
>> thousands of clients. These lookup calls possibly can be avoided
>> in some cases. Incoming three patches address this issue.
>>
>>
>> Fist patch handles the case where we are creating a file with O_CREAT.
>> Before we go for file creation, we do a lookup on the file which is most
>> likely non-existent. After this lookup is done, we again go into libfuse
>> to create file. Such lookups where file is most likely non-existent, can
>> be avoided.
> 
> I'd really like to see a bit wider picture...
> 
> We have several cases, first of all let's look at plain O_CREAT
> without O_EXCL (assume that there were no changes since the last
> lookup for simplicity):
> 
> [not cached, negative]
>     ->atomic_open()
>        LOOKUP
>        CREATE
> 
> [not cached, positive]
>     ->atomic_open()
>        LOOKUP
>     ->open()
>        OPEN
> 
> [cached, negative, validity timeout not expired]
>     ->d_revalidate()
>        return 1
>     ->atomic_open()
>        CREATE
> 
> [cached, negative, validity timeout expired]
>     ->d_revalidate()
>        return 0
>     ->atomic_open()
>        LOOKUP
>        CREATE
> 
> [cached, positive, validity timeout not expired]
>     ->d_revalidate()
>        return 1
>     ->open()
>        OPEN
> 
> [cached, positive, validity timeout expired]
>     ->d_revalidate()
>        LOOKUP
>        return 1
>     ->open()
>        OPEN
> 
> (Caveat emptor: I'm just looking at the code and haven't actually
> tested what happens.)
> 
> Apparently in all of these cases we are doing at least one request, so
> it would make sense to make them uniform:
> 
> [not cached]
>     ->atomic_open()
>        CREATE_EXT
> 
> [cached]
>     ->d_revalidate()
>        return 0
>     ->atomic_open()
>        CREATE_EXT
> 
> Similarly we can look at the current O_CREAT | O_EXCL cases:
> 
> [not cached, negative]
>     ->atomic_open()
>        LOOKUP
>        CREATE
> 
> [not cached, positive]
>     ->atomic_open()
>        LOOKUP
>     return -EEXIST
> 
> [cached, negative]
>     ->d_revalidate()
>        return 0 (see LOOKUP_EXCL check)
>     ->atomic_open()
>        LOOKUP
>        CREATE
> 
> [cached, positive]
>     ->d_revalidate()
>        LOOKUP
>        return 1
>     return -EEXIST
> 
> Again we are doing at least one request, so we can unconditionally
> replace them with CREATE_EXT like the non-O_EXCL case.
> 
> 
>>
>> Second patch handles the case where we open first time a file/dir
>> but do a lookup first on it. After lookup is performed we make another
>> call into libfuse to open the file. Now these two separate calls into
>> libfuse can be combined and performed as a single call into libfuse.
> 
> And here's my analysis:
> 
> [not cached, negative]
>     ->lookup()
>        LOOKUP
>     return -ENOENT
> 
> [not cached, positive]
>     ->lookup()
>        LOOKUP
>     ->open()
>        OPEN
> 
> [cached, negative, validity timeout not expired]
>      ->d_revalidate()
>         return 1
>      return -ENOENT
> 
> [cached, negative, validity timeout expired]
>     ->d_revalidate()
>        return 0
>     ->atomic_open()
>        LOOKUP
>     return -ENOENT
> 
> [cached, positive, validity timeout not expired]
>     ->d_revalidate()
>        return 1
>     ->open()
>        OPEN
> 
> [cached, positive, validity timeout expired]
>     ->d_revalidate()
>        LOOKUP
>        return 1
>     ->open()
>        OPEN
> 
> There's one case were no request is sent:  a valid cached negative
> dentry.   Possibly we can also make this uniform, e.g.:
> 
> [not cached]
>     ->atomic_open()
>         OPEN_ATOMIC
> 
> [cached, negative, validity timeout not expired]
>      ->d_revalidate()
>         return 1
>      return -ENOENT
> 
> [cached, negative, validity timeout expired]
>     ->d_revalidate()
>        return 0
>     ->atomic_open()
>        OPEN_ATOMIC
> 
> [cached, positive]
>     ->d_revalidate()
>        return 0
>     ->atomic_open()
>        OPEN_ATOMIC
> 
> It may even make the code simpler to clearly separate the cases where
> the atomic variants are supported and when not.  I'd also consider
> merging CREATE_EXT into OPEN_ATOMIC, since a filesystem implementing
> one will highly likely want to implement the other as well.


Can you help me a bit to understand what we should change? I had also 
already thought to merge CREATE_EXT and OPEN_ATOMIC - so agreed.
Shall we make the other cases more visible?

Also thanks a lot for you revalidate patch.


Thanks,
Bernd

Thanks,
Bernd

Re: [PATCH v5 0/3] FUSE: Implement atomic lookup + open/create

[Miklos Szeredi]

On Thu, 19 May 2022 at 19:42, Bernd Schubert <bschubert@ddn.com> wrote:

> Can you help me a bit to understand what we should change? I had also
> already thought to merge CREATE_EXT and OPEN_ATOMIC - so agreed.
> Shall we make the other cases more visible?

Make it clear in the code flow if we are using the new request or the
old; e.g. rename current fuse_atomic_open() to fuse_open_nonatomic()
and do

static int fuse_open_atomic(...)
{
    ...
    args.opcode = FUSE_OPEN_ATOMIC;
    ...
    err = fuse_simple_request(...);
    if (err == -ENOSYS)
        goto fallback;
    ...
fallback:
    return fuse_open_nonatomic();
}

static int fuse_atomic_open(...)
{
    if (fc->no_open_atomic)
        return fuse_open_nonatomic();
    else
        return fuse_open_atomic();
}

Also we can tweak fuse_dentry_revalidate() so it always invalidates
negative dentries if the new atomic open is available, and possibly
for positive dentries as well, if the rfc patch makes it.

Thanks,
Miklos

Re: [fuse-devel] [PATCH v5 0/3] FUSE: Implement atomic lookup + open/create

[Bernd Schubert]

On 5/19/22 20:16, Miklos Szeredi wrote:
> On Thu, 19 May 2022 at 19:42, Bernd Schubert <bschubert@ddn.com> wrote:
> 
>> Can you help me a bit to understand what we should change? I had also
>> already thought to merge CREATE_EXT and OPEN_ATOMIC - so agreed.
>> Shall we make the other cases more visible?
> 
> Make it clear in the code flow if we are using the new request or the
> old; e.g. rename current fuse_atomic_open() to fuse_open_nonatomic()
> and do
> 
> static int fuse_open_atomic(...)
> {
>      ...
>      args.opcode = FUSE_OPEN_ATOMIC;
>      ...
>      err = fuse_simple_request(...);
>      if (err == -ENOSYS)
>          goto fallback;
>      ...
> fallback:
>      return fuse_open_nonatomic();
> }
> 
> static int fuse_atomic_open(...)
> {
>      if (fc->no_open_atomic)
>          return fuse_open_nonatomic();
>      else
>          return fuse_open_atomic();
> }
> 
> Also we can tweak fuse_dentry_revalidate() so it always invalidates
> negative dentries if the new atomic open is available, and possibly
> for positive dentries as well, if the rfc patch makes it.

Thank you, we will try to do it like that during the next day.


Thanks,
Bernd

Re: [PATCH v5 0/3] FUSE: Implement atomic lookup + open/create

[Vivek Goyal]

On Thu, May 19, 2022 at 11:39:01AM +0200, Miklos Szeredi wrote:
> On Tue, 17 May 2022 at 12:08, Dharmendra Singh <dharamhans87@gmail.com> wrote:
> >
> > In FUSE, as of now, uncached lookups are expensive over the wire.
> > E.g additional latencies and stressing (meta data) servers from
> > thousands of clients. These lookup calls possibly can be avoided
> > in some cases. Incoming three patches address this issue.
> >
> >
> > Fist patch handles the case where we are creating a file with O_CREAT.
> > Before we go for file creation, we do a lookup on the file which is most
> > likely non-existent. After this lookup is done, we again go into libfuse
> > to create file. Such lookups where file is most likely non-existent, can
> > be avoided.
> 
> I'd really like to see a bit wider picture...
> 
> We have several cases, first of all let's look at plain O_CREAT
> without O_EXCL (assume that there were no changes since the last
> lookup for simplicity):

Hi Miklos,

Thanks for providing this breakup. There are too many cases here and
this data helps a lot with that. I feel this should really be captured
in commit logs to show the current paths and how these have been 
optimized with ATOMIC_OPEN/CREATE_EXT.

> 
> [not cached, negative]
>    ->atomic_open()
>       LOOKUP
>       CREATE
> 
> [not cached, positive]
>    ->atomic_open()
>       LOOKUP
>    ->open()
>       OPEN
> 
> [cached, negative, validity timeout not expired]
>    ->d_revalidate()
>       return 1
>    ->atomic_open()
>       CREATE
> 
> [cached, negative, validity timeout expired]
>    ->d_revalidate()
>       return 0
>    ->atomic_open()
>       LOOKUP
>       CREATE
> 
> [cached, positive, validity timeout not expired]
>    ->d_revalidate()
>       return 1
>    ->open()
>       OPEN
> 
> [cached, positive, validity timeout expired]
>    ->d_revalidate()
>       LOOKUP
>       return 1
>    ->open()
>       OPEN
> 
> (Caveat emptor: I'm just looking at the code and haven't actually
> tested what happens.)
> 
> Apparently in all of these cases we are doing at least one request, so
> it would make sense to make them uniform:
> 
> [not cached]
>    ->atomic_open()
>       CREATE_EXT
> 
> [cached]
>    ->d_revalidate()
>       return 0

So fuse_dentry_revalidate() will return 0 even if timeout has not
expired (if server supports so called atomic_open()).
And that will lead to calling d_invalidate() on existing positive dentry
always. IOW, if I am calling open() on a dentry, dentry will always be
dropped and a new dentry will always be created from ->atomic_open() path,
is that right.

I am not sure what does it mean from VFS perspective to always call
d_invalidate() on a cached positive dentry when open() is called. 

/**
 * d_invalidate - detach submounts, prune dcache, and drop
 * @dentry: dentry to invalidate (aka detach, prune and drop)
 */

Thanks
Vivek

>    ->atomic_open()
>       CREATE_EXT
> 
> Similarly we can look at the current O_CREAT | O_EXCL cases:
> 
> [not cached, negative]
>    ->atomic_open()
>       LOOKUP
>       CREATE
> 
> [not cached, positive]
>    ->atomic_open()
>       LOOKUP
>    return -EEXIST
> 
> [cached, negative]
>    ->d_revalidate()
>       return 0 (see LOOKUP_EXCL check)
>    ->atomic_open()
>       LOOKUP
>       CREATE
> 
> [cached, positive]
>    ->d_revalidate()
>       LOOKUP
>       return 1
>    return -EEXIST
> 
> Again we are doing at least one request, so we can unconditionally
> replace them with CREATE_EXT like the non-O_EXCL case.
> 
> 
> >
> > Second patch handles the case where we open first time a file/dir
> > but do a lookup first on it. After lookup is performed we make another
> > call into libfuse to open the file. Now these two separate calls into
> > libfuse can be combined and performed as a single call into libfuse.
> 
> And here's my analysis:
> 
> [not cached, negative]
>    ->lookup()
>       LOOKUP
>    return -ENOENT
> 
> [not cached, positive]
>    ->lookup()
>       LOOKUP
>    ->open()
>       OPEN
> 
> [cached, negative, validity timeout not expired]
>     ->d_revalidate()
>        return 1
>     return -ENOENT
> 
> [cached, negative, validity timeout expired]
>    ->d_revalidate()
>       return 0
>    ->atomic_open()
>       LOOKUP
>    return -ENOENT
> 
> [cached, positive, validity timeout not expired]
>    ->d_revalidate()
>       return 1
>    ->open()
>       OPEN
> 
> [cached, positive, validity timeout expired]
>    ->d_revalidate()
>       LOOKUP
>       return 1
>    ->open()
>       OPEN
> 
> There's one case were no request is sent:  a valid cached negative
> dentry.   Possibly we can also make this uniform, e.g.:
> 
> [not cached]
>    ->atomic_open()
>        OPEN_ATOMIC
> 
> [cached, negative, validity timeout not expired]
>     ->d_revalidate()
>        return 1
>     return -ENOENT
> 
> [cached, negative, validity timeout expired]
>    ->d_revalidate()
>       return 0
>    ->atomic_open()
>       OPEN_ATOMIC
> 
> [cached, positive]
>    ->d_revalidate()
>       return 0
>    ->atomic_open()
>       OPEN_ATOMIC
> 
> It may even make the code simpler to clearly separate the cases where
> the atomic variants are supported and when not.  I'd also consider
> merging CREATE_EXT into OPEN_ATOMIC, since a filesystem implementing
> one will highly likely want to implement the other as well.
> 
> Thanks,
> Miklos
> 

Re: [PATCH v5 0/3] FUSE: Implement atomic lookup + open/create

[Bernd Schubert]

Hi Miklos,

On 5/19/22 11:39, Miklos Szeredi wrote:
> On Tue, 17 May 2022 at 12:08, Dharmendra Singh <dharamhans87@gmail.com> wrote:
>>
>> In FUSE, as of now, uncached lookups are expensive over the wire.
>> E.g additional latencies and stressing (meta data) servers from
>> thousands of clients. These lookup calls possibly can be avoided
>> in some cases. Incoming three patches address this issue.
>>
>>
>> Fist patch handles the case where we are creating a file with O_CREAT.
>> Before we go for file creation, we do a lookup on the file which is most
>> likely non-existent. After this lookup is done, we again go into libfuse
>> to create file. Such lookups where file is most likely non-existent, can
>> be avoided.
> 
> I'd really like to see a bit wider picture...
> 
> We have several cases, first of all let's look at plain O_CREAT
> without O_EXCL (assume that there were no changes since the last
> lookup for simplicity):
> 
> [not cached, negative]
>     ->atomic_open()
>        LOOKUP
>        CREATE
> 

[...]

> [not cached]
>     ->atomic_open()
>         OPEN_ATOMIC

new patch version is eventually going through xfstests (and it finds 
some issues), but I have a question about wording here. Why 
"OPEN_ATOMIC" and not "ATOMIC_OPEN". Based on your comment  @Dharmendra 
renamed all functions and this fuse op "open atomic" instead of "atomic 
open" - for my non native English this sounds rather weird. At best it 
should be "open atomically"?


Thanks,
Bernd

Re: [PATCH v5 0/3] FUSE: Implement atomic lookup + open/create

[Miklos Szeredi]

On Thu, 1 Jun 2023 at 13:17, Bernd Schubert <bschubert@ddn.com> wrote:
>
> Hi Miklos,
>
> On 5/19/22 11:39, Miklos Szeredi wrote:
> > On Tue, 17 May 2022 at 12:08, Dharmendra Singh <dharamhans87@gmail.com> wrote:
> >>
> >> In FUSE, as of now, uncached lookups are expensive over the wire.
> >> E.g additional latencies and stressing (meta data) servers from
> >> thousands of clients. These lookup calls possibly can be avoided
> >> in some cases. Incoming three patches address this issue.
> >>
> >>
> >> Fist patch handles the case where we are creating a file with O_CREAT.
> >> Before we go for file creation, we do a lookup on the file which is most
> >> likely non-existent. After this lookup is done, we again go into libfuse
> >> to create file. Such lookups where file is most likely non-existent, can
> >> be avoided.
> >
> > I'd really like to see a bit wider picture...
> >
> > We have several cases, first of all let's look at plain O_CREAT
> > without O_EXCL (assume that there were no changes since the last
> > lookup for simplicity):
> >
> > [not cached, negative]
> >     ->atomic_open()
> >        LOOKUP
> >        CREATE
> >
>
> [...]
>
> > [not cached]
> >     ->atomic_open()
> >         OPEN_ATOMIC
>
> new patch version is eventually going through xfstests (and it finds
> some issues), but I have a question about wording here. Why
> "OPEN_ATOMIC" and not "ATOMIC_OPEN". Based on your comment  @Dharmendra
> renamed all functions and this fuse op "open atomic" instead of "atomic
> open" - for my non native English this sounds rather weird. At best it
> should be "open atomically"?

FUSE_OPEN_ATOMIC is a specialization of FUSE_OPEN.  Does that explain
my thinking?

Thanks,
Miklos

Re: [PATCH v5 0/3] FUSE: Implement atomic lookup + open/create

[Bernd Schubert]

On 6/1/23 13:50, Miklos Szeredi wrote:
> On Thu, 1 Jun 2023 at 13:17, Bernd Schubert <bschubert@ddn.com> wrote:
>>
>> Hi Miklos,
>>
>> On 5/19/22 11:39, Miklos Szeredi wrote:
>>> On Tue, 17 May 2022 at 12:08, Dharmendra Singh <dharamhans87@gmail.com> wrote:
>>>>
>>>> In FUSE, as of now, uncached lookups are expensive over the wire.
>>>> E.g additional latencies and stressing (meta data) servers from
>>>> thousands of clients. These lookup calls possibly can be avoided
>>>> in some cases. Incoming three patches address this issue.
>>>>
>>>>
>>>> Fist patch handles the case where we are creating a file with O_CREAT.
>>>> Before we go for file creation, we do a lookup on the file which is most
>>>> likely non-existent. After this lookup is done, we again go into libfuse
>>>> to create file. Such lookups where file is most likely non-existent, can
>>>> be avoided.
>>>
>>> I'd really like to see a bit wider picture...
>>>
>>> We have several cases, first of all let's look at plain O_CREAT
>>> without O_EXCL (assume that there were no changes since the last
>>> lookup for simplicity):
>>>
>>> [not cached, negative]
>>>      ->atomic_open()
>>>         LOOKUP
>>>         CREATE
>>>
>>
>> [...]
>>
>>> [not cached]
>>>      ->atomic_open()
>>>          OPEN_ATOMIC
>>
>> new patch version is eventually going through xfstests (and it finds
>> some issues), but I have a question about wording here. Why
>> "OPEN_ATOMIC" and not "ATOMIC_OPEN". Based on your comment  @Dharmendra
>> renamed all functions and this fuse op "open atomic" instead of "atomic
>> open" - for my non native English this sounds rather weird. At best it
>> should be "open atomically"?
> 
> FUSE_OPEN_ATOMIC is a specialization of FUSE_OPEN.  Does that explain
> my thinking?

Yeah, just the vfs function is also called atomic_open. We now have


static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
                 struct file *file, unsigned flags,
                 umode_t mode)
{
     struct fuse_conn *fc = get_fuse_conn(dir);

     if (fc->no_open_atomic)
         return fuse_open_nonatomic(dir, entry, file, flags, mode);
     else
         return fuse_open_atomic(dir, entry, file, flags, mode);
}


Personally I would use something like _fuse_atomic_open() and 
fuse_create_open() (instead of fuse_open_nonatomic). The order of "open 
atomic" also made it into libfuse and comments - it just sounds a bit 
weird ;) I have to live with it, if you prefer it like this.


Thanks,
Bernd

Re: [PATCH v5 0/3] FUSE: Implement atomic lookup + open/create

[Miklos Szeredi]

On Thu, 1 Jun 2023 at 14:01, Bernd Schubert <bschubert@ddn.com> wrote:
>
> On 6/1/23 13:50, Miklos Szeredi wrote:
> > On Thu, 1 Jun 2023 at 13:17, Bernd Schubert <bschubert@ddn.com> wrote:
> >>
> >> Hi Miklos,
> >>
> >> On 5/19/22 11:39, Miklos Szeredi wrote:
> >>> On Tue, 17 May 2022 at 12:08, Dharmendra Singh <dharamhans87@gmail.com> wrote:
> >>>>
> >>>> In FUSE, as of now, uncached lookups are expensive over the wire.
> >>>> E.g additional latencies and stressing (meta data) servers from
> >>>> thousands of clients. These lookup calls possibly can be avoided
> >>>> in some cases. Incoming three patches address this issue.
> >>>>
> >>>>
> >>>> Fist patch handles the case where we are creating a file with O_CREAT.
> >>>> Before we go for file creation, we do a lookup on the file which is most
> >>>> likely non-existent. After this lookup is done, we again go into libfuse
> >>>> to create file. Such lookups where file is most likely non-existent, can
> >>>> be avoided.
> >>>
> >>> I'd really like to see a bit wider picture...
> >>>
> >>> We have several cases, first of all let's look at plain O_CREAT
> >>> without O_EXCL (assume that there were no changes since the last
> >>> lookup for simplicity):
> >>>
> >>> [not cached, negative]
> >>>      ->atomic_open()
> >>>         LOOKUP
> >>>         CREATE
> >>>
> >>
> >> [...]
> >>
> >>> [not cached]
> >>>      ->atomic_open()
> >>>          OPEN_ATOMIC
> >>
> >> new patch version is eventually going through xfstests (and it finds
> >> some issues), but I have a question about wording here. Why
> >> "OPEN_ATOMIC" and not "ATOMIC_OPEN". Based on your comment  @Dharmendra
> >> renamed all functions and this fuse op "open atomic" instead of "atomic
> >> open" - for my non native English this sounds rather weird. At best it
> >> should be "open atomically"?
> >
> > FUSE_OPEN_ATOMIC is a specialization of FUSE_OPEN.  Does that explain
> > my thinking?
>
> Yeah, just the vfs function is also called atomic_open. We now have
>
>
> static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
>                  struct file *file, unsigned flags,
>                  umode_t mode)
> {
>      struct fuse_conn *fc = get_fuse_conn(dir);
>
>      if (fc->no_open_atomic)
>          return fuse_open_nonatomic(dir, entry, file, flags, mode);
>      else
>          return fuse_open_atomic(dir, entry, file, flags, mode);
> }
>
>
> Personally I would use something like _fuse_atomic_open() and
> fuse_create_open() (instead of fuse_open_nonatomic). The order of "open
> atomic" also made it into libfuse and comments - it just sounds a bit
> weird ;) I have to live with it, if you prefer it like this.

I'd prefer FUSE_OPEN_ATOMIC for the API, but I don't care about
function names, as long as the purpose is clear.

Thanks,
Miklos