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

[PATCH v3 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.

Third patch handles the case when we are opening an already existing file
(positive dentry). Before this open call, we re-validate the inode and
this re-validation does a lookup on the file and verify the inode.
This separate lookup also can be avoided (for non-dir) and combined
with open call into libfuse. After open returns we can revalidate the inode.
This optimisation is performed only when we do not have default permissions
enabled.

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

Dharmendra Singh (3):
  FUSE: Implement atomic lookup + create
  Implement atomic lookup + open
  Avoid lookup in d_revalidate()

 fs/fuse/dir.c             | 210 +++++++++++++++++++++++++++++++++++---
 fs/fuse/file.c            |  30 +++++-
 fs/fuse/fuse_i.h          |  16 ++-
 fs/fuse/inode.c           |   4 +-
 fs/fuse/ioctl.c           |   2 +-
 include/uapi/linux/fuse.h |   5 +
 6 files changed, 245 insertions(+), 22 deletions(-)

-- 
2.17.1

[PATCH v3 1/3] FUSE: Implement atomic lookup + 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 lookup can be avoided and can be performed
as part of create call into libfuse.

This lookup + create in single call to libfuse and finally
to USER SPACE has been named as atomic create. 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 atomic create is implemented
by libfuse/USER SPACE or not. And depending upon the outcome of
this check all further creates are decided to be atomic or non-atomic
creates.

If libfuse/USER SPACE has not implemented the atomic 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             | 82 +++++++++++++++++++++++++++++++++++----
 fs/fuse/fuse_i.h          |  3 ++
 include/uapi/linux/fuse.h |  3 ++
 3 files changed, 81 insertions(+), 7 deletions(-)

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 656e921f3506..cad3322a007f 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 atomic_create = (opcode == FUSE_ATOMIC_CREATE ? 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,44 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
 		goto out_err;
 	}
 	kfree(forget);
-	d_instantiate(entry, inode);
+	/*
+	 * In atomic create, we skipped lookup and it is very much likely that
+	 * dentry has DCACHE_PAR_LOOKUP flag set on it so call d_splice_alias().
+	 * Note: Only REG file is allowed under create/atomic create.
+	 */
+	/* There is special case when at very first call where we check if
+	 * atomic create is implemented by USER SPACE/libfuse or not, we
+	 * skipped lookup. Now, in case where atomic create is not implemented
+	 * underlying, we fall back to FUSE_CREATE. here we are required to handle
+	 * DCACHE_PAR_LOOKUP flag.
+	 */
+	if (!atomic_create && !d_in_lookup(entry) && fm->fc->no_atomic_create)
+		d_instantiate(entry, inode);
+	else {
+		res = d_splice_alias(inode, entry);
+		if (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.
+			  */
+			if (IS_ERR(res)) {
+				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;
+			}
+			/* res is expected to be NULL since its REG file */
+			WARN_ON(res);
+		}
+	}
 	fuse_change_entry_timeout(entry, &outentry);
-	fuse_dir_changed(dir);
+	/*
+	 * In case of atomic create, we want to indicate directory change
+	 * only if USER SPACE actually created the file.
+	 */
+	if (!atomic_create || (outopen.open_flags & FOPEN_FILE_CREATED))
+		fuse_dir_changed(dir);
 	err = finish_open(file, entry, generic_file_open);
 	if (err) {
 		fi = get_fuse_inode(inode);
@@ -634,6 +671,29 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
 	return err;
 }
 
+static int fuse_atomic_create(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_atomic_create)
+		return -ENOSYS;
+
+	err = fuse_create_open(dir, entry, file, flags, mode,
+			       FUSE_ATOMIC_CREATE);
+	/* If atomic 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_atomic_create)
+			fc->no_atomic_create = 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,11 +703,12 @@ 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)) {
+	if ((!create || fc->no_atomic_create) && d_in_lookup(entry)) {
 		res = fuse_lookup(dir, entry, 0);
 		if (IS_ERR(res))
 			return PTR_ERR(res);
@@ -656,7 +717,7 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
 			entry = res;
 	}
 
-	if (!(flags & O_CREAT) || d_really_is_positive(entry))
+	if (!create || d_really_is_positive(entry))
 		goto no_open;
 
 	/* Only creates */
@@ -665,7 +726,13 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
 	if (fc->no_create)
 		goto mknod;
 
-	err = fuse_create_open(dir, entry, file, flags, mode);
+	err = fuse_atomic_create(dir, entry, file, flags, mode);
+	/* Libfuse/user space has not implemented atomic create, therefore
+	 * fall back to normal create.
+	 */
+	if (err == -ENOSYS)
+		err = fuse_create_open(dir, entry, file, flags, mode,
+				       FUSE_CREATE);
 	if (err == -ENOSYS) {
 		fc->no_create = 1;
 		goto mknod;
@@ -683,6 +750,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..d577a591ab16 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -669,6 +669,9 @@ struct fuse_conn {
 	/** Is open/release not implemented by fs? */
 	unsigned no_open:1;
 
+	/** Is atomic create not implemented by fs? */
+	unsigned no_atomic_create: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..e4b56004b148 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_ATOMIC_CREATE	= 51,
 
 	/* CUSE specific operations */
 	CUSE_INIT		= 4096,
-- 
2.17.1

Re: [PATCH v3 1/3] FUSE: Implement atomic lookup + create

[Vivek Goyal]

On Mon, May 02, 2022 at 11:16:26AM +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 lookup can be avoided and can be performed
> as part of create call into libfuse.
> 
> This lookup + create in single call to libfuse and finally
> to USER SPACE has been named as atomic create. 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 atomic create is implemented
> by libfuse/USER SPACE or not. And depending upon the outcome of
> this check all further creates are decided to be atomic or non-atomic
> creates.
> 
> If libfuse/USER SPACE has not implemented the atomic 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             | 82 +++++++++++++++++++++++++++++++++++----
>  fs/fuse/fuse_i.h          |  3 ++
>  include/uapi/linux/fuse.h |  3 ++
>  3 files changed, 81 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index 656e921f3506..cad3322a007f 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 atomic_create = (opcode == FUSE_ATOMIC_CREATE ? 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,44 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
>  		goto out_err;
>  	}
>  	kfree(forget);
> -	d_instantiate(entry, inode);
> +	/*
> +	 * In atomic create, we skipped lookup and it is very much likely that
> +	 * dentry has DCACHE_PAR_LOOKUP flag set on it so call d_splice_alias().
> +	 * Note: Only REG file is allowed under create/atomic create.
> +	 */
> +	/* There is special case when at very first call where we check if
> +	 * atomic create is implemented by USER SPACE/libfuse or not, we
> +	 * skipped lookup. Now, in case where atomic create is not implemented
> +	 * underlying, we fall back to FUSE_CREATE. here we are required to handle
> +	 * DCACHE_PAR_LOOKUP flag.
> +	 */
> +	if (!atomic_create && !d_in_lookup(entry) && fm->fc->no_atomic_create)
> +		d_instantiate(entry, inode);
> +	else {
> +		res = d_splice_alias(inode, entry);
> +		if (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.
> +			  */
> +			if (IS_ERR(res)) {
> +				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;
> +			}
> +			/* res is expected to be NULL since its REG file */
> +			WARN_ON(res);

This WARN_ON(res) is strange. We enter if (res) block only if res is
non null. So effectively we are doing this.

if (res) {
   WARN_ON(res);
}

Will it not trigger all the time?

I think I already asked the question in previous email that what's the
difference between d_instanatiate() and d_splice_alias() and why we
need d_splice_alias() in this case instead.

Thanks
Vivek
> +		}
> +	}
>  	fuse_change_entry_timeout(entry, &outentry);
> -	fuse_dir_changed(dir);
> +	/*
> +	 * In case of atomic create, we want to indicate directory change
> +	 * only if USER SPACE actually created the file.
> +	 */
> +	if (!atomic_create || (outopen.open_flags & FOPEN_FILE_CREATED))
> +		fuse_dir_changed(dir);
>  	err = finish_open(file, entry, generic_file_open);
>  	if (err) {
>  		fi = get_fuse_inode(inode);
> @@ -634,6 +671,29 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
>  	return err;
>  }
>  
> +static int fuse_atomic_create(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_atomic_create)
> +		return -ENOSYS;
> +
> +	err = fuse_create_open(dir, entry, file, flags, mode,
> +			       FUSE_ATOMIC_CREATE);
> +	/* If atomic 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_atomic_create)
> +			fc->no_atomic_create = 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,11 +703,12 @@ 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)) {
> +	if ((!create || fc->no_atomic_create) && d_in_lookup(entry)) {
>  		res = fuse_lookup(dir, entry, 0);
>  		if (IS_ERR(res))
>  			return PTR_ERR(res);
> @@ -656,7 +717,7 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
>  			entry = res;
>  	}
>  
> -	if (!(flags & O_CREAT) || d_really_is_positive(entry))
> +	if (!create || d_really_is_positive(entry))
>  		goto no_open;
>  
>  	/* Only creates */
> @@ -665,7 +726,13 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
>  	if (fc->no_create)
>  		goto mknod;
>  
> -	err = fuse_create_open(dir, entry, file, flags, mode);
> +	err = fuse_atomic_create(dir, entry, file, flags, mode);
> +	/* Libfuse/user space has not implemented atomic create, therefore
> +	 * fall back to normal create.
> +	 */
> +	if (err == -ENOSYS)
> +		err = fuse_create_open(dir, entry, file, flags, mode,
> +				       FUSE_CREATE);
>  	if (err == -ENOSYS) {
>  		fc->no_create = 1;
>  		goto mknod;
> @@ -683,6 +750,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..d577a591ab16 100644
> --- a/fs/fuse/fuse_i.h
> +++ b/fs/fuse/fuse_i.h
> @@ -669,6 +669,9 @@ struct fuse_conn {
>  	/** Is open/release not implemented by fs? */
>  	unsigned no_open:1;
>  
> +	/** Is atomic create not implemented by fs? */
> +	unsigned no_atomic_create: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..e4b56004b148 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_ATOMIC_CREATE	= 51,
>  
>  	/* CUSE specific operations */
>  	CUSE_INIT		= 4096,
> -- 
> 2.17.1
> 

Re: [PATCH v3 1/3] FUSE: Implement atomic lookup + create

[Dharmendra Hans]

On Wed, May 4, 2022 at 8:42 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Mon, May 02, 2022 at 11:16:26AM +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 lookup can be avoided and can be performed
> > as part of create call into libfuse.
> >
> > This lookup + create in single call to libfuse and finally
> > to USER SPACE has been named as atomic create. 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 atomic create is implemented
> > by libfuse/USER SPACE or not. And depending upon the outcome of
> > this check all further creates are decided to be atomic or non-atomic
> > creates.
> >
> > If libfuse/USER SPACE has not implemented the atomic 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             | 82 +++++++++++++++++++++++++++++++++++----
> >  fs/fuse/fuse_i.h          |  3 ++
> >  include/uapi/linux/fuse.h |  3 ++
> >  3 files changed, 81 insertions(+), 7 deletions(-)
> >
> > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> > index 656e921f3506..cad3322a007f 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 atomic_create = (opcode == FUSE_ATOMIC_CREATE ? 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,44 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
> >               goto out_err;
> >       }
> >       kfree(forget);
> > -     d_instantiate(entry, inode);
> > +     /*
> > +      * In atomic create, we skipped lookup and it is very much likely that
> > +      * dentry has DCACHE_PAR_LOOKUP flag set on it so call d_splice_alias().
> > +      * Note: Only REG file is allowed under create/atomic create.
> > +      */
> > +     /* There is special case when at very first call where we check if
> > +      * atomic create is implemented by USER SPACE/libfuse or not, we
> > +      * skipped lookup. Now, in case where atomic create is not implemented
> > +      * underlying, we fall back to FUSE_CREATE. here we are required to handle
> > +      * DCACHE_PAR_LOOKUP flag.
> > +      */
> > +     if (!atomic_create && !d_in_lookup(entry) && fm->fc->no_atomic_create)
> > +             d_instantiate(entry, inode);
> > +     else {
> > +             res = d_splice_alias(inode, entry);
> > +             if (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.
> > +                       */
> > +                     if (IS_ERR(res)) {
> > +                             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;
> > +                     }
> > +                     /* res is expected to be NULL since its REG file */
> > +                     WARN_ON(res);
>
> This WARN_ON(res) is strange. We enter if (res) block only if res is
> non null. So effectively we are doing this.

In create mode, we expect only REG files. It can happen that we
encountered an error on inode for REG files. We are detecting that
case. In all cases res should be null.

>
> if (res) {
>    WARN_ON(res);
> }
>
> Will it not trigger all the time?

No, it will not trigger for REG files and we do not expect spliced dentry here.

>
> I think I already asked the question in previous email that what's the
> difference between d_instanatiate() and d_splice_alias() and why we
> need d_splice_alias() in this case instead.

Since we skipped d_lookup() and came here( whether atomic create is
implemented or not, all cases), dentry can have DCACHE_PAR_LOOKUP flag
on it (This flag prevents parallel dentry creation for same file name
in hash, other dentry insert for same file name in hash if came for
allocation would wait until this flag gets cleared on the dentry).
d_splice_alias() awaknes those sleeping on this flag but
d_instantiate() does nothing related to dentry waiters awake/flag
clear etc. It just links inode with dentry. So we need
d_splice_alias() here instead of d_instantiate().

Please note that we are auto detecting the atomic create so even in
normal create case we would avoid lookup first until
fc->no_atomic_create is set. it is after we confirm with lower layers
that atomic create is not implemented, we do not avoid lookups so
would be calling d_instantiate() instead of d_splice_alias();



> Thanks
> Vivek
> > +             }
> > +     }
> >       fuse_change_entry_timeout(entry, &outentry);
> > -     fuse_dir_changed(dir);
> > +     /*
> > +      * In case of atomic create, we want to indicate directory change
> > +      * only if USER SPACE actually created the file.
> > +      */
> > +     if (!atomic_create || (outopen.open_flags & FOPEN_FILE_CREATED))
> > +             fuse_dir_changed(dir);
> >       err = finish_open(file, entry, generic_file_open);
> >       if (err) {
> >               fi = get_fuse_inode(inode);
> > @@ -634,6 +671,29 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
> >       return err;
> >  }
> >
> > +static int fuse_atomic_create(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_atomic_create)
> > +             return -ENOSYS;
> > +
> > +     err = fuse_create_open(dir, entry, file, flags, mode,
> > +                            FUSE_ATOMIC_CREATE);
> > +     /* If atomic 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_atomic_create)
> > +                     fc->no_atomic_create = 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,11 +703,12 @@ 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)) {
> > +     if ((!create || fc->no_atomic_create) && d_in_lookup(entry)) {
> >               res = fuse_lookup(dir, entry, 0);
> >               if (IS_ERR(res))
> >                       return PTR_ERR(res);
> > @@ -656,7 +717,7 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
> >                       entry = res;
> >       }
> >
> > -     if (!(flags & O_CREAT) || d_really_is_positive(entry))
> > +     if (!create || d_really_is_positive(entry))
> >               goto no_open;
> >
> >       /* Only creates */
> > @@ -665,7 +726,13 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
> >       if (fc->no_create)
> >               goto mknod;
> >
> > -     err = fuse_create_open(dir, entry, file, flags, mode);
> > +     err = fuse_atomic_create(dir, entry, file, flags, mode);
> > +     /* Libfuse/user space has not implemented atomic create, therefore
> > +      * fall back to normal create.
> > +      */
> > +     if (err == -ENOSYS)
> > +             err = fuse_create_open(dir, entry, file, flags, mode,
> > +                                    FUSE_CREATE);
> >       if (err == -ENOSYS) {
> >               fc->no_create = 1;
> >               goto mknod;
> > @@ -683,6 +750,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..d577a591ab16 100644
> > --- a/fs/fuse/fuse_i.h
> > +++ b/fs/fuse/fuse_i.h
> > @@ -669,6 +669,9 @@ struct fuse_conn {
> >       /** Is open/release not implemented by fs? */
> >       unsigned no_open:1;
> >
> > +     /** Is atomic create not implemented by fs? */
> > +     unsigned no_atomic_create: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..e4b56004b148 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_ATOMIC_CREATE      = 51,
> >
> >       /* CUSE specific operations */
> >       CUSE_INIT               = 4096,
> > --
> > 2.17.1
> >
>

Re: [PATCH v3 1/3] FUSE: Implement atomic lookup + create

[Vivek Goyal]

On Thu, May 05, 2022 at 01:03:45PM +0530, Dharmendra Hans wrote:
> On Wed, May 4, 2022 at 8:42 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > On Mon, May 02, 2022 at 11:16:26AM +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 lookup can be avoided and can be performed
> > > as part of create call into libfuse.
> > >
> > > This lookup + create in single call to libfuse and finally
> > > to USER SPACE has been named as atomic create. 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 atomic create is implemented
> > > by libfuse/USER SPACE or not. And depending upon the outcome of
> > > this check all further creates are decided to be atomic or non-atomic
> > > creates.
> > >
> > > If libfuse/USER SPACE has not implemented the atomic 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             | 82 +++++++++++++++++++++++++++++++++++----
> > >  fs/fuse/fuse_i.h          |  3 ++
> > >  include/uapi/linux/fuse.h |  3 ++
> > >  3 files changed, 81 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> > > index 656e921f3506..cad3322a007f 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 atomic_create = (opcode == FUSE_ATOMIC_CREATE ? 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,44 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
> > >               goto out_err;
> > >       }
> > >       kfree(forget);
> > > -     d_instantiate(entry, inode);
> > > +     /*
> > > +      * In atomic create, we skipped lookup and it is very much likely that
> > > +      * dentry has DCACHE_PAR_LOOKUP flag set on it so call d_splice_alias().
> > > +      * Note: Only REG file is allowed under create/atomic create.
> > > +      */
> > > +     /* There is special case when at very first call where we check if
> > > +      * atomic create is implemented by USER SPACE/libfuse or not, we
> > > +      * skipped lookup. Now, in case where atomic create is not implemented
> > > +      * underlying, we fall back to FUSE_CREATE. here we are required to handle
> > > +      * DCACHE_PAR_LOOKUP flag.
> > > +      */
> > > +     if (!atomic_create && !d_in_lookup(entry) && fm->fc->no_atomic_create)
> > > +             d_instantiate(entry, inode);
> > > +     else {
> > > +             res = d_splice_alias(inode, entry);
> > > +             if (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.
> > > +                       */
> > > +                     if (IS_ERR(res)) {
> > > +                             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;
> > > +                     }
> > > +                     /* res is expected to be NULL since its REG file */
> > > +                     WARN_ON(res);
> >
> > This WARN_ON(res) is strange. We enter if (res) block only if res is
> > non null. So effectively we are doing this.
> 
> In create mode, we expect only REG files. It can happen that we
> encountered an error on inode for REG files. We are detecting that
> case. In all cases res should be null.
> 
> >
> > if (res) {
> >    WARN_ON(res);
> > }
> >
> > Will it not trigger all the time?
> 
> No, it will not trigger for REG files and we do not expect spliced dentry here.

Ok, Its little twisted to understand. So you do not expect directories
here. So d_splice_alias() will either reuturn NULL or error.

I guess folllowing kind of construct is more readable.

res = d_splice_alias()
if (res) {
	if (!IS_ERR(res))
		warn(1);
	else {
		/* Do error handling. */
	}
}

> 
> >
> > I think I already asked the question in previous email that what's the
> > difference between d_instanatiate() and d_splice_alias() and why we
> > need d_splice_alias() in this case instead.
> 
> Since we skipped d_lookup() and came here( whether atomic create is
> implemented or not, all cases), dentry can have DCACHE_PAR_LOOKUP flag
> on it (This flag prevents parallel dentry creation for same file name
> in hash, other dentry insert for same file name in hash if came for
> allocation would wait until this flag gets cleared on the dentry).
> d_splice_alias() awaknes those sleeping on this flag but
> d_instantiate() does nothing related to dentry waiters awake/flag
> clear etc. It just links inode with dentry. So we need
> d_splice_alias() here instead of d_instantiate().

Ok. Are there other filesystems in kernel doing the same thing?

I spending some time reading through DCACHE_PAR_LOOKUP logic and
realized that if two lookup are happening in parallel with shared
inode lock held, then one lookup will proceed with DCACHE_PAR_LOOKUP
flag and other will wait on wait queue and will be woken up
when d_lookup_done() is called.

I see that d_splice_alias() does that.

d_splice_alias()
  __d_add()
     if (d_in_lookup(dentry))
         __d_lookup_done(dentry);

Also I was concerned about security_d_instantiate() hook being called
and it looks like that's called as well.

d_splice_alias()
  security_d_instantiate(dentry, inode);

Hmm.., If some other filesystem is doing the same thing it becomes
little easier to believe that all this works and we are not breaking
anything by replacing d_instantiate() with d_splice_alias(). :-)

Thanks
Vivek

> 
> Please note that we are auto detecting the atomic create so even in
> normal create case we would avoid lookup first until
> fc->no_atomic_create is set. it is after we confirm with lower layers
> that atomic create is not implemented, we do not avoid lookups so
> would be calling d_instantiate() instead of d_splice_alias();
> 
> 
> 
> > Thanks
> > Vivek
> > > +             }
> > > +     }
> > >       fuse_change_entry_timeout(entry, &outentry);
> > > -     fuse_dir_changed(dir);
> > > +     /*
> > > +      * In case of atomic create, we want to indicate directory change
> > > +      * only if USER SPACE actually created the file.
> > > +      */
> > > +     if (!atomic_create || (outopen.open_flags & FOPEN_FILE_CREATED))
> > > +             fuse_dir_changed(dir);
> > >       err = finish_open(file, entry, generic_file_open);
> > >       if (err) {
> > >               fi = get_fuse_inode(inode);
> > > @@ -634,6 +671,29 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
> > >       return err;
> > >  }
> > >
> > > +static int fuse_atomic_create(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_atomic_create)
> > > +             return -ENOSYS;
> > > +
> > > +     err = fuse_create_open(dir, entry, file, flags, mode,
> > > +                            FUSE_ATOMIC_CREATE);
> > > +     /* If atomic 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_atomic_create)
> > > +                     fc->no_atomic_create = 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,11 +703,12 @@ 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)) {
> > > +     if ((!create || fc->no_atomic_create) && d_in_lookup(entry)) {
> > >               res = fuse_lookup(dir, entry, 0);
> > >               if (IS_ERR(res))
> > >                       return PTR_ERR(res);
> > > @@ -656,7 +717,7 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
> > >                       entry = res;
> > >       }
> > >
> > > -     if (!(flags & O_CREAT) || d_really_is_positive(entry))
> > > +     if (!create || d_really_is_positive(entry))
> > >               goto no_open;
> > >
> > >       /* Only creates */
> > > @@ -665,7 +726,13 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
> > >       if (fc->no_create)
> > >               goto mknod;
> > >
> > > -     err = fuse_create_open(dir, entry, file, flags, mode);
> > > +     err = fuse_atomic_create(dir, entry, file, flags, mode);
> > > +     /* Libfuse/user space has not implemented atomic create, therefore
> > > +      * fall back to normal create.
> > > +      */
> > > +     if (err == -ENOSYS)
> > > +             err = fuse_create_open(dir, entry, file, flags, mode,
> > > +                                    FUSE_CREATE);
> > >       if (err == -ENOSYS) {
> > >               fc->no_create = 1;
> > >               goto mknod;
> > > @@ -683,6 +750,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..d577a591ab16 100644
> > > --- a/fs/fuse/fuse_i.h
> > > +++ b/fs/fuse/fuse_i.h
> > > @@ -669,6 +669,9 @@ struct fuse_conn {
> > >       /** Is open/release not implemented by fs? */
> > >       unsigned no_open:1;
> > >
> > > +     /** Is atomic create not implemented by fs? */
> > > +     unsigned no_atomic_create: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..e4b56004b148 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_ATOMIC_CREATE      = 51,
> > >
> > >       /* CUSE specific operations */
> > >       CUSE_INIT               = 4096,
> > > --
> > > 2.17.1
> > >
> >
> 

[PATCH v3 2/3] Implement atomic lookup + open

[Dharmendra Singh]

From: Dharmendra Singh <dsingh@ddn.com>

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             | 78 ++++++++++++++++++++++++++++++---------
 fs/fuse/fuse_i.h          |  3 ++
 fs/fuse/inode.c           |  4 +-
 include/uapi/linux/fuse.h |  2 +
 4 files changed, 69 insertions(+), 18 deletions(-)

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index cad3322a007f..6879d3a86796 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -516,18 +516,18 @@ static int get_security_context(struct dentry *entry, umode_t mode,
 }
 
 /*
- * Atomic create+open operation
- *
- * If the filesystem doesn't support this, then fall back to separate
- * 'mknod' + 'open' requests.
+ * This is common function for initiating atomic operations into libfuse.
+ * Currently being used by Atomic create(atomic lookup + create) and
+ * Atomic open(atomic lookup + open).
  */
-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_do_common(struct inode *dir, struct dentry *entry,
+				 struct dentry **alias, struct file *file,
+				 unsigned int flags, umode_t mode, uint32_t opcode)
 {
 	int err;
 	struct inode *inode;
 	struct fuse_mount *fm = get_fuse_mount(dir);
+	struct fuse_conn *fc = get_fuse_conn(dir);
 	FUSE_ARGS(args);
 	struct fuse_forget_link *forget;
 	struct fuse_create_in inarg;
@@ -539,9 +539,13 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
 	void *security_ctx = NULL;
 	u32 security_ctxlen;
 	bool atomic_create = (opcode == FUSE_ATOMIC_CREATE ? true : false);
+	bool create_op = (opcode == FUSE_CREATE ||
+			  opcode == FUSE_ATOMIC_CREATE) ? true : false;
+	if (alias)
+		*alias = NULL;
 
 	/* Userspace expects S_IFREG in create mode */
-	BUG_ON((mode & S_IFMT) != S_IFREG);
+	BUG_ON(create_op && (mode & S_IFMT) != S_IFREG);
 
 	forget = fuse_alloc_forget();
 	err = -ENOMEM;
@@ -553,7 +557,7 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
 	if (!ff)
 		goto out_put_forget_req;
 
-	if (!fm->fc->dont_mask)
+	if (!fc->dont_mask)
 		mode &= ~current_umask();
 
 	flags &= ~O_NOCTTY;
@@ -642,8 +646,9 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
 				err = PTR_ERR(res);
 				goto out_err;
 			}
-			/* res is expected to be NULL since its REG file */
-			WARN_ON(res);
+			entry = res;
+			if (alias)
+				*alias = res;
 		}
 	}
 	fuse_change_entry_timeout(entry, &outentry);
@@ -681,8 +686,8 @@ static int fuse_atomic_create(struct inode *dir, struct dentry *entry,
 	if (fc->no_atomic_create)
 		return -ENOSYS;
 
-	err = fuse_create_open(dir, entry, file, flags, mode,
-			       FUSE_ATOMIC_CREATE);
+	err = fuse_atomic_do_common(dir, entry, NULL, file, flags, mode,
+				    FUSE_ATOMIC_CREATE);
 	/* If atomic 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.
@@ -694,6 +699,29 @@ static int fuse_atomic_create(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->do_atomic_open)
+		return -ENOSYS;
+
+	err = fuse_atomic_do_common(dir, entry, alias, file, flags, mode,
+				    FUSE_ATOMIC_OPEN);
+	/* Atomic open imply atomic trunc as well i.e truncate should be performed
+	 * as part of atomic open call itself.
+	 */
+	if (!fc->atomic_o_trunc) {
+		if (fc->do_atomic_open)
+			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,
@@ -702,12 +730,23 @@ 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) {
+		err = fuse_do_atomic_open(dir, entry, &alias,
+					  file, flags, mode);
+		res = alias;
+		if (!err || err != -ENOSYS)
+			goto out_dput;
+	}
+	 /*
+	  * ENOSYS fall back for open- user space does not have full
+	  * atomic open.
+	  */
 	if ((!create || fc->no_atomic_create) && d_in_lookup(entry)) {
 		res = fuse_lookup(dir, entry, 0);
 		if (IS_ERR(res))
@@ -730,9 +769,14 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
 	/* Libfuse/user space has not implemented atomic create, therefore
 	 * fall back to normal create.
 	 */
-	if (err == -ENOSYS)
-		err = fuse_create_open(dir, entry, file, flags, mode,
-				       FUSE_CREATE);
+	/* Atomic create+open operation
+	 * If the filesystem doesn't support this, then fall back to separate
+	 * 'mknod' + 'open' requests.
+	 */
+	if (err == -ENOSYS) {
+		err = fuse_atomic_do_common(dir, entry, NULL, file, flags,
+					    mode, FUSE_CREATE);
+	}
 	if (err == -ENOSYS) {
 		fc->no_create = 1;
 		goto mknod;
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index d577a591ab16..24793b82303f 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -669,6 +669,9 @@ struct fuse_conn {
 	/** Is open/release not implemented by fs? */
 	unsigned no_open:1;
 
+	/** Is atomic open implemented by fs ? */
+	unsigned do_atomic_open : 1;
+
 	/** Is atomic create not implemented by fs? */
 	unsigned no_atomic_create:1;
 
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index ee846ce371d8..5f667de69115 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -1190,6 +1190,8 @@ static void process_init_reply(struct fuse_mount *fm, struct fuse_args *args,
 				fc->setxattr_ext = 1;
 			if (flags & FUSE_SECURITY_CTX)
 				fc->init_security = 1;
+			if (flags & FUSE_DO_ATOMIC_OPEN)
+				fc->do_atomic_open = 1;
 		} else {
 			ra_pages = fc->max_read / PAGE_SIZE;
 			fc->no_lock = 1;
@@ -1235,7 +1237,7 @@ void fuse_send_init(struct fuse_mount *fm)
 		FUSE_ABORT_ERROR | FUSE_MAX_PAGES | FUSE_CACHE_SYMLINKS |
 		FUSE_NO_OPENDIR_SUPPORT | FUSE_EXPLICIT_INVAL_DATA |
 		FUSE_HANDLE_KILLPRIV_V2 | FUSE_SETXATTR_EXT | FUSE_INIT_EXT |
-		FUSE_SECURITY_CTX;
+		FUSE_SECURITY_CTX | FUSE_DO_ATOMIC_OPEN;
 #ifdef CONFIG_FUSE_DAX
 	if (fm->fc->dax)
 		flags |= FUSE_MAP_ALIGNMENT;
diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
index e4b56004b148..ab91e391241a 100644
--- a/include/uapi/linux/fuse.h
+++ b/include/uapi/linux/fuse.h
@@ -391,6 +391,7 @@ struct fuse_file_lock {
 /* bits 32..63 get shifted down 32 bits into the flags2 field */
 #define FUSE_SECURITY_CTX	(1ULL << 32)
 #define FUSE_HAS_INODE_DAX	(1ULL << 33)
+#define FUSE_DO_ATOMIC_OPEN	(1ULL << 34)
 
 /**
  * CUSE INIT request/reply flags
@@ -540,6 +541,7 @@ enum fuse_opcode {
 	FUSE_REMOVEMAPPING	= 49,
 	FUSE_SYNCFS		= 50,
 	FUSE_ATOMIC_CREATE	= 51,
+	FUSE_ATOMIC_OPEN	= 52,
 
 	/* CUSE specific operations */
 	CUSE_INIT		= 4096,
-- 
2.17.1

[PATCH v3 3/3] Avoid lookup in d_revalidate()

[Dharmendra Singh]

From: Dharmendra Singh <dsingh@ddn.com>

With atomic open + lookup implemented, it is possible
to avoid lookups in FUSE d_revalidate() for objects
other than directories.

If FUSE is mounted with default permissions then this
optimization is not possible as we need to fetch fresh
inode attributes for permission check. This lookup
skipped in d_revalidate() can be performed  as part of
open call into libfuse which is made from fuse_file_open().
And when we return from USER SPACE with file opened and
fresh attributes, we can revalidate the inode.

Signed-off-by: Dharmendra Singh <dsingh@ddn.com>
---
 fs/fuse/dir.c    | 90 +++++++++++++++++++++++++++++++++++++++++-------
 fs/fuse/file.c   | 30 ++++++++++++++--
 fs/fuse/fuse_i.h | 10 +++++-
 fs/fuse/ioctl.c  |  2 +-
 4 files changed, 115 insertions(+), 17 deletions(-)

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 6879d3a86796..0b3e26dfdf29 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -196,6 +196,7 @@ static void fuse_lookup_init(struct fuse_conn *fc, struct fuse_args *args,
  * the lookup once more.  If the lookup results in the same inode,
  * then refresh the attributes, timeouts and mark the dentry valid.
  */
+
 static int fuse_dentry_revalidate(struct dentry *entry, unsigned int flags)
 {
 	struct inode *inode;
@@ -224,6 +225,17 @@ static int fuse_dentry_revalidate(struct dentry *entry, unsigned int flags)
 
 		fm = get_fuse_mount(inode);
 
+		/* If atomic open is supported by FUSE then use this opportunity
+		 * (only for non-dir) to avoid this lookup and combine
+		 * lookup + open into single call.
+		 */
+		if (!fm->fc->default_permissions && fm->fc->do_atomic_open &&
+		    !(flags & (LOOKUP_EXCL | LOOKUP_REVAL)) &&
+		    (flags & LOOKUP_OPEN) && !S_ISDIR(inode->i_mode)) {
+			ret = 1;
+			goto out;
+		}
+
 		forget = fuse_alloc_forget();
 		ret = -ENOMEM;
 		if (!forget)
@@ -515,14 +527,51 @@ static int get_security_context(struct dentry *entry, umode_t mode,
 	return err;
 }
 
+/*
+ * Revalidate the inode after we got fresh attributes from user space.
+ */
+static int fuse_atomic_open_revalidate_inode(struct inode *reval_inode,
+					     struct dentry *entry,
+					     struct fuse_inode *fi,
+					     struct fuse_forget_link *forget,
+					     struct fuse_entry_out *outentry,
+					     u64 attr_version)
+{
+	struct fuse_conn *fc = get_fuse_conn(reval_inode);
+	/* Mode should be other than directory */
+	BUG_ON(S_ISDIR(reval_inode->i_mode));
+
+	if (outentry->nodeid != get_node_id(reval_inode)) {
+		fuse_queue_forget(fc, forget, outentry->nodeid, 1);
+		return -ESTALE;
+	}
+	if (fuse_stale_inode(reval_inode, outentry->generation,
+			     &outentry->attr)) {
+		fuse_make_bad(reval_inode);
+		return -ESTALE;
+	}
+	fi = get_fuse_inode(reval_inode);
+	spin_lock(&fi->lock);
+	fi->nlookup++;
+	spin_unlock(&fi->lock);
+
+	forget_all_cached_acls(reval_inode);
+	fuse_change_attributes(reval_inode, &outentry->attr,
+			       entry_attr_timeout(outentry), attr_version);
+	fuse_change_entry_timeout(entry, outentry);
+	return 0;
+}
+
+
 /*
  * This is common function for initiating atomic operations into libfuse.
  * Currently being used by Atomic create(atomic lookup + create) and
  * Atomic open(atomic lookup + open).
  */
-static int fuse_atomic_do_common(struct inode *dir, struct dentry *entry,
-				 struct dentry **alias, struct file *file,
-				 unsigned int flags, umode_t mode, uint32_t opcode)
+int fuse_do_atomic_common(struct inode *dir, struct dentry *entry,
+			  struct dentry **alias, struct file *file,
+			  struct inode *reval_inode, unsigned int flags,
+			  umode_t mode, uint32_t opcode)
 {
 	int err;
 	struct inode *inode;
@@ -541,6 +590,8 @@ static int fuse_atomic_do_common(struct inode *dir, struct dentry *entry,
 	bool atomic_create = (opcode == FUSE_ATOMIC_CREATE ? true : false);
 	bool create_op = (opcode == FUSE_CREATE ||
 			  opcode == FUSE_ATOMIC_CREATE) ? true : false;
+	u64 attr_version = fuse_get_attr_version(fm->fc);
+
 	if (alias)
 		*alias = NULL;
 
@@ -609,6 +660,20 @@ static int fuse_atomic_do_common(struct inode *dir, struct dentry *entry,
 	ff->fh = outopen.fh;
 	ff->nodeid = outentry.nodeid;
 	ff->open_flags = outopen.open_flags;
+
+	/* Inode revalidation was bypassed previously for type other than
+	 * directories, revalidate now as we got fresh attributes.
+	 */
+	if (reval_inode) {
+		err = fuse_atomic_open_revalidate_inode(reval_inode, entry, fi,
+							forget, &outentry,
+							attr_version);
+		if (err)
+			goto out_free_ff;
+		inode = reval_inode;
+		kfree(forget);
+		goto out_finish_open;
+	}
 	inode = fuse_iget(dir->i_sb, outentry.nodeid, outentry.generation,
 			  &outentry.attr, entry_attr_timeout(&outentry), 0);
 	if (!inode) {
@@ -659,6 +724,7 @@ static int fuse_atomic_do_common(struct inode *dir, struct dentry *entry,
 	if (!atomic_create || (outopen.open_flags & FOPEN_FILE_CREATED))
 		fuse_dir_changed(dir);
 	err = finish_open(file, entry, generic_file_open);
+out_finish_open:
 	if (err) {
 		fi = get_fuse_inode(inode);
 		fuse_sync_release(fi, ff, flags);
@@ -686,7 +752,7 @@ static int fuse_atomic_create(struct inode *dir, struct dentry *entry,
 	if (fc->no_atomic_create)
 		return -ENOSYS;
 
-	err = fuse_atomic_do_common(dir, entry, NULL, file, flags, mode,
+	err = fuse_do_atomic_common(dir, entry, NULL, file, NULL, flags, mode,
 				    FUSE_ATOMIC_CREATE);
 	/* If atomic create is not implemented then indicate in fc so that next
 	 * request falls back to normal create instead of going into libufse and
@@ -699,9 +765,10 @@ static int fuse_atomic_create(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 fuse_do_atomic_open(struct inode *dir, struct dentry *entry,
+			struct dentry **alias, struct file *file,
+			struct inode *reval_inode, unsigned int flags,
+			umode_t mode)
 {
 	int err;
 	struct fuse_conn *fc = get_fuse_conn(dir);
@@ -709,8 +776,8 @@ static int fuse_do_atomic_open(struct inode *dir, struct dentry *entry,
 	if (!fc->do_atomic_open)
 		return -ENOSYS;
 
-	err = fuse_atomic_do_common(dir, entry, alias, file, flags, mode,
-				    FUSE_ATOMIC_OPEN);
+	err = fuse_do_atomic_common(dir, entry, alias, file, reval_inode, flags,
+				    mode, FUSE_ATOMIC_OPEN);
 	/* Atomic open imply atomic trunc as well i.e truncate should be performed
 	 * as part of atomic open call itself.
 	 */
@@ -718,7 +785,6 @@ static int fuse_do_atomic_open(struct inode *dir, struct dentry *entry,
 		if (fc->do_atomic_open)
 			fc->atomic_o_trunc = 1;
 	}
-
 	return err;
 }
 
@@ -738,7 +804,7 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
 
 	if (!create) {
 		err = fuse_do_atomic_open(dir, entry, &alias,
-					  file, flags, mode);
+					  file, NULL, flags, mode);
 		res = alias;
 		if (!err || err != -ENOSYS)
 			goto out_dput;
@@ -774,7 +840,7 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
 	 * 'mknod' + 'open' requests.
 	 */
 	if (err == -ENOSYS) {
-		err = fuse_atomic_do_common(dir, entry, NULL, file, flags,
+		err = fuse_do_atomic_common(dir, entry, NULL, file, NULL, flags,
 					    mode, FUSE_CREATE);
 	}
 	if (err == -ENOSYS) {
diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 829094451774..2b0548163249 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -125,11 +125,15 @@ static void fuse_file_put(struct fuse_file *ff, bool sync, bool isdir)
 }
 
 struct fuse_file *fuse_file_open(struct fuse_mount *fm, u64 nodeid,
-				 unsigned int open_flags, bool isdir)
+				 struct file *file, unsigned int open_flags,
+				 bool isdir)
 {
 	struct fuse_conn *fc = fm->fc;
 	struct fuse_file *ff;
+	struct dentry *dentry = NULL;
+	struct dentry *parent = NULL;
 	int opcode = isdir ? FUSE_OPENDIR : FUSE_OPEN;
+	int ret;
 
 	ff = fuse_file_alloc(fm);
 	if (!ff)
@@ -138,6 +142,11 @@ struct fuse_file *fuse_file_open(struct fuse_mount *fm, u64 nodeid,
 	ff->fh = 0;
 	/* Default for no-open */
 	ff->open_flags = FOPEN_KEEP_CACHE | (isdir ? FOPEN_CACHE_DIR : 0);
+
+	/* For directories we already had lookup */
+	if (!isdir && fc->do_atomic_open && file != NULL)
+		goto revalidate_atomic_open;
+
 	if (isdir ? !fc->no_opendir : !fc->no_open) {
 		struct fuse_open_out outarg;
 		int err;
@@ -164,12 +173,27 @@ struct fuse_file *fuse_file_open(struct fuse_mount *fm, u64 nodeid,
 	ff->nodeid = nodeid;
 
 	return ff;
+
+revalidate_atomic_open:
+	dentry = file->f_path.dentry;
+	/* Get ref on parent */
+	parent = dget_parent(dentry);
+	open_flags &= ~(O_CREAT | O_EXCL | O_NOCTTY);
+	ret = fuse_do_atomic_open(d_inode_rcu(parent), dentry, NULL, file,
+				  d_inode_rcu(dentry), open_flags, 0);
+	dput(parent);
+	if (ret)
+		goto err_out;
+	ff = file->private_data;
+	return ff;
+err_out:
+	return ERR_PTR(ret);
 }
 
 int fuse_do_open(struct fuse_mount *fm, u64 nodeid, struct file *file,
 		 bool isdir)
 {
-	struct fuse_file *ff = fuse_file_open(fm, nodeid, file->f_flags, isdir);
+	struct fuse_file *ff = fuse_file_open(fm, nodeid, file, file->f_flags, isdir);
 
 	if (!IS_ERR(ff))
 		file->private_data = ff;
@@ -252,7 +276,7 @@ int fuse_open_common(struct inode *inode, struct file *file, bool isdir)
 	}
 
 	err = fuse_do_open(fm, get_node_id(inode), file, isdir);
-	if (!err)
+	if (!err && (!fc->do_atomic_open || isdir))
 		fuse_finish_open(inode, file);
 
 out:
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index 24793b82303f..5c83e4249b7e 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -1014,6 +1014,13 @@ void fuse_finish_open(struct inode *inode, struct file *file);
 void fuse_sync_release(struct fuse_inode *fi, struct fuse_file *ff,
 		       unsigned int flags);
 
+/**
+ * Send atomic create + open or lookup + open
+ */
+int fuse_do_atomic_open(struct inode *dir, struct dentry *entry,
+			struct dentry **alias, struct file *file,
+			struct inode *reval_inode, unsigned int flags,
+			umode_t mode);
 /**
  * Send RELEASE or RELEASEDIR request
  */
@@ -1317,7 +1324,8 @@ int fuse_fileattr_set(struct user_namespace *mnt_userns,
 /* file.c */
 
 struct fuse_file *fuse_file_open(struct fuse_mount *fm, u64 nodeid,
-				 unsigned int open_flags, bool isdir);
+				 struct file *file, unsigned int open_flags,
+				 bool isdir);
 void fuse_file_release(struct inode *inode, struct fuse_file *ff,
 		       unsigned int open_flags, fl_owner_t id, bool isdir);
 
diff --git a/fs/fuse/ioctl.c b/fs/fuse/ioctl.c
index fbc09dab1f85..63106a54ba1a 100644
--- a/fs/fuse/ioctl.c
+++ b/fs/fuse/ioctl.c
@@ -408,7 +408,7 @@ static struct fuse_file *fuse_priv_ioctl_prepare(struct inode *inode)
 	if (!S_ISREG(inode->i_mode) && !isdir)
 		return ERR_PTR(-ENOTTY);
 
-	return fuse_file_open(fm, get_node_id(inode), O_RDONLY, isdir);
+	return fuse_file_open(fm, get_node_id(inode), NULL, O_RDONLY, isdir);
 }
 
 static void fuse_priv_ioctl_cleanup(struct inode *inode, struct fuse_file *ff)
-- 
2.17.1

Re: [PATCH v3 3/3] Avoid lookup in d_revalidate()

[kernel test robot]

Hi Dharmendra,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on v5.18-rc5]
[cannot apply to mszeredi-fuse/for-next next-20220429]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Dharmendra-Singh/FUSE-Implement-atomic-lookup-open-create/20220502-134729
base:    672c0c5173427e6b3e2a9bbb7be51ceeec78093a
config: arm-randconfig-p002-20220501 (https://download.01.org/0day-ci/archive/20220502/202205021542.b9f01HaG-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 11.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/a531ce8124c046dffefe5cd731c952c8f7248c5b
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Dharmendra-Singh/FUSE-Implement-atomic-lookup-open-create/20220502-134729
        git checkout a531ce8124c046dffefe5cd731c952c8f7248c5b
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash fs/fuse/
reproduce (cppcheck warning):
        # apt-get install cppcheck
        git checkout a531ce8124c046dffefe5cd731c952c8f7248c5b
        cppcheck --quiet --enable=style,performance,portability --template=gcc FILE

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> fs/fuse/dir.c:571:5: warning: no previous prototype for 'fuse_do_atomic_common' [-Wmissing-prototypes]
     571 | int fuse_do_atomic_common(struct inode *dir, struct dentry *entry,
         |     ^~~~~~~~~~~~~~~~~~~~~


cppcheck possible warnings: (new ones prefixed by >>, may not real problems)

>> fs/fuse/inode.c:411:17: warning: Uninitialized variable: fm_iter->sb [uninitvar]
     if (!fm_iter->sb)
                   ^

vim +/fuse_do_atomic_common +571 fs/fuse/dir.c

   564	
   565	
   566	/*
   567	 * This is common function for initiating atomic operations into libfuse.
   568	 * Currently being used by Atomic create(atomic lookup + create) and
   569	 * Atomic open(atomic lookup + open).
   570	 */
 > 571	int fuse_do_atomic_common(struct inode *dir, struct dentry *entry,
   572				  struct dentry **alias, struct file *file,
   573				  struct inode *reval_inode, unsigned int flags,
   574				  umode_t mode, uint32_t opcode)
   575	{
   576		int err;
   577		struct inode *inode;
   578		struct fuse_mount *fm = get_fuse_mount(dir);
   579		struct fuse_conn *fc = get_fuse_conn(dir);
   580		FUSE_ARGS(args);
   581		struct fuse_forget_link *forget;
   582		struct fuse_create_in inarg;
   583		struct fuse_open_out outopen;
   584		struct fuse_entry_out outentry;
   585		struct fuse_inode *fi;
   586		struct fuse_file *ff;
   587		struct dentry *res = NULL;
   588		void *security_ctx = NULL;
   589		u32 security_ctxlen;
   590		bool atomic_create = (opcode == FUSE_ATOMIC_CREATE ? true : false);
   591		bool create_op = (opcode == FUSE_CREATE ||
   592				  opcode == FUSE_ATOMIC_CREATE) ? true : false;
   593		u64 attr_version = fuse_get_attr_version(fm->fc);
   594	
   595		if (alias)
   596			*alias = NULL;
   597	
   598		/* Userspace expects S_IFREG in create mode */
   599		BUG_ON(create_op && (mode & S_IFMT) != S_IFREG);
   600	
   601		forget = fuse_alloc_forget();
   602		err = -ENOMEM;
   603		if (!forget)
   604			goto out_err;
   605	
   606		err = -ENOMEM;
   607		ff = fuse_file_alloc(fm);
   608		if (!ff)
   609			goto out_put_forget_req;
   610	
   611		if (!fc->dont_mask)
   612			mode &= ~current_umask();
   613	
   614		flags &= ~O_NOCTTY;
   615		memset(&inarg, 0, sizeof(inarg));
   616		memset(&outentry, 0, sizeof(outentry));
   617		inarg.flags = flags;
   618		inarg.mode = mode;
   619		inarg.umask = current_umask();
   620	
   621		if (fm->fc->handle_killpriv_v2 && (flags & O_TRUNC) &&
   622		    !(flags & O_EXCL) && !capable(CAP_FSETID)) {
   623			inarg.open_flags |= FUSE_OPEN_KILL_SUIDGID;
   624		}
   625	
   626		args.opcode = opcode;
   627		args.nodeid = get_node_id(dir);
   628		args.in_numargs = 2;
   629		args.in_args[0].size = sizeof(inarg);
   630		args.in_args[0].value = &inarg;
   631		args.in_args[1].size = entry->d_name.len + 1;
   632		args.in_args[1].value = entry->d_name.name;
   633		args.out_numargs = 2;
   634		args.out_args[0].size = sizeof(outentry);
   635		args.out_args[0].value = &outentry;
   636		args.out_args[1].size = sizeof(outopen);
   637		args.out_args[1].value = &outopen;
   638	
   639		if (fm->fc->init_security) {
   640			err = get_security_context(entry, mode, &security_ctx,
   641						   &security_ctxlen);
   642			if (err)
   643				goto out_put_forget_req;
   644	
   645			args.in_numargs = 3;
   646			args.in_args[2].size = security_ctxlen;
   647			args.in_args[2].value = security_ctx;
   648		}
   649	
   650		err = fuse_simple_request(fm, &args);
   651		kfree(security_ctx);
   652		if (err)
   653			goto out_free_ff;
   654	
   655		err = -EIO;
   656		if (!S_ISREG(outentry.attr.mode) || invalid_nodeid(outentry.nodeid) ||
   657		    fuse_invalid_attr(&outentry.attr))
   658			goto out_free_ff;
   659	
   660		ff->fh = outopen.fh;
   661		ff->nodeid = outentry.nodeid;
   662		ff->open_flags = outopen.open_flags;
   663	
   664		/* Inode revalidation was bypassed previously for type other than
   665		 * directories, revalidate now as we got fresh attributes.
   666		 */
   667		if (reval_inode) {
   668			err = fuse_atomic_open_revalidate_inode(reval_inode, entry, fi,
   669								forget, &outentry,
   670								attr_version);
   671			if (err)
   672				goto out_free_ff;
   673			inode = reval_inode;
   674			kfree(forget);
   675			goto out_finish_open;
   676		}
   677		inode = fuse_iget(dir->i_sb, outentry.nodeid, outentry.generation,
   678				  &outentry.attr, entry_attr_timeout(&outentry), 0);
   679		if (!inode) {
   680			flags &= ~(O_CREAT | O_EXCL | O_TRUNC);
   681			fuse_sync_release(NULL, ff, flags);
   682			fuse_queue_forget(fm->fc, forget, outentry.nodeid, 1);
   683			err = -ENOMEM;
   684			goto out_err;
   685		}
   686		kfree(forget);
   687		/*
   688		 * In atomic create, we skipped lookup and it is very much likely that
   689		 * dentry has DCACHE_PAR_LOOKUP flag set on it so call d_splice_alias().
   690		 * Note: Only REG file is allowed under create/atomic create.
   691		 */
   692		/* There is special case when at very first call where we check if
   693		 * atomic create is implemented by USER SPACE/libfuse or not, we
   694		 * skipped lookup. Now, in case where atomic create is not implemented
   695		 * underlying, we fall back to FUSE_CREATE. here we are required to handle
   696		 * DCACHE_PAR_LOOKUP flag.
   697		 */
   698		if (!atomic_create && !d_in_lookup(entry) && fm->fc->no_atomic_create)
   699			d_instantiate(entry, inode);
   700		else {
   701			res = d_splice_alias(inode, entry);
   702			if (res) {
   703				 /* Close the file in user space, but do not unlink it,
   704				  * if it was created - with network file systems other
   705				  * clients might have already accessed it.
   706				  */
   707				if (IS_ERR(res)) {
   708					fi = get_fuse_inode(inode);
   709					fuse_sync_release(fi, ff, flags);
   710					fuse_queue_forget(fm->fc, forget, outentry.nodeid, 1);
   711					err = PTR_ERR(res);
   712					goto out_err;
   713				}
   714				entry = res;
   715				if (alias)
   716					*alias = res;
   717			}
   718		}
   719		fuse_change_entry_timeout(entry, &outentry);
   720		/*
   721		 * In case of atomic create, we want to indicate directory change
   722		 * only if USER SPACE actually created the file.
   723		 */
   724		if (!atomic_create || (outopen.open_flags & FOPEN_FILE_CREATED))
   725			fuse_dir_changed(dir);
   726		err = finish_open(file, entry, generic_file_open);
   727	out_finish_open:
   728		if (err) {
   729			fi = get_fuse_inode(inode);
   730			fuse_sync_release(fi, ff, flags);
   731		} else {
   732			file->private_data = ff;
   733			fuse_finish_open(inode, file);
   734		}
   735		return err;
   736	
   737	out_free_ff:
   738		fuse_file_free(ff);
   739	out_put_forget_req:
   740		kfree(forget);
   741	out_err:
   742		return err;
   743	}
   744	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH v3 3/3] Avoid lookup in d_revalidate()

[kernel test robot]

Hi Dharmendra,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on v5.18-rc5]
[cannot apply to mszeredi-fuse/for-next next-20220429]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Dharmendra-Singh/FUSE-Implement-atomic-lookup-open-create/20220502-134729
base:    672c0c5173427e6b3e2a9bbb7be51ceeec78093a
config: hexagon-randconfig-r045-20220502 (https://download.01.org/0day-ci/archive/20220502/202205021705.GXaOa8o4-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 09325d36061e42b495d1f4c7e933e260eac260ed)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/a531ce8124c046dffefe5cd731c952c8f7248c5b
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Dharmendra-Singh/FUSE-Implement-atomic-lookup-open-create/20220502-134729
        git checkout a531ce8124c046dffefe5cd731c952c8f7248c5b
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash fs/fuse/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> fs/fuse/dir.c:571:5: warning: no previous prototype for function 'fuse_do_atomic_common' [-Wmissing-prototypes]
   int fuse_do_atomic_common(struct inode *dir, struct dentry *entry,
       ^
   fs/fuse/dir.c:571:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
   int fuse_do_atomic_common(struct inode *dir, struct dentry *entry,
   ^
   static 
>> fs/fuse/dir.c:668:63: warning: variable 'fi' is uninitialized when used here [-Wuninitialized]
                   err = fuse_atomic_open_revalidate_inode(reval_inode, entry, fi,
                                                                               ^~
   fs/fuse/dir.c:585:23: note: initialize the variable 'fi' to silence this warning
           struct fuse_inode *fi;
                                ^
                                 = NULL
   2 warnings generated.


vim +/fuse_do_atomic_common +571 fs/fuse/dir.c

   564	
   565	
   566	/*
   567	 * This is common function for initiating atomic operations into libfuse.
   568	 * Currently being used by Atomic create(atomic lookup + create) and
   569	 * Atomic open(atomic lookup + open).
   570	 */
 > 571	int fuse_do_atomic_common(struct inode *dir, struct dentry *entry,
   572				  struct dentry **alias, struct file *file,
   573				  struct inode *reval_inode, unsigned int flags,
   574				  umode_t mode, uint32_t opcode)
   575	{
   576		int err;
   577		struct inode *inode;
   578		struct fuse_mount *fm = get_fuse_mount(dir);
   579		struct fuse_conn *fc = get_fuse_conn(dir);
   580		FUSE_ARGS(args);
   581		struct fuse_forget_link *forget;
   582		struct fuse_create_in inarg;
   583		struct fuse_open_out outopen;
   584		struct fuse_entry_out outentry;
   585		struct fuse_inode *fi;
   586		struct fuse_file *ff;
   587		struct dentry *res = NULL;
   588		void *security_ctx = NULL;
   589		u32 security_ctxlen;
   590		bool atomic_create = (opcode == FUSE_ATOMIC_CREATE ? true : false);
   591		bool create_op = (opcode == FUSE_CREATE ||
   592				  opcode == FUSE_ATOMIC_CREATE) ? true : false;
   593		u64 attr_version = fuse_get_attr_version(fm->fc);
   594	
   595		if (alias)
   596			*alias = NULL;
   597	
   598		/* Userspace expects S_IFREG in create mode */
   599		BUG_ON(create_op && (mode & S_IFMT) != S_IFREG);
   600	
   601		forget = fuse_alloc_forget();
   602		err = -ENOMEM;
   603		if (!forget)
   604			goto out_err;
   605	
   606		err = -ENOMEM;
   607		ff = fuse_file_alloc(fm);
   608		if (!ff)
   609			goto out_put_forget_req;
   610	
   611		if (!fc->dont_mask)
   612			mode &= ~current_umask();
   613	
   614		flags &= ~O_NOCTTY;
   615		memset(&inarg, 0, sizeof(inarg));
   616		memset(&outentry, 0, sizeof(outentry));
   617		inarg.flags = flags;
   618		inarg.mode = mode;
   619		inarg.umask = current_umask();
   620	
   621		if (fm->fc->handle_killpriv_v2 && (flags & O_TRUNC) &&
   622		    !(flags & O_EXCL) && !capable(CAP_FSETID)) {
   623			inarg.open_flags |= FUSE_OPEN_KILL_SUIDGID;
   624		}
   625	
   626		args.opcode = opcode;
   627		args.nodeid = get_node_id(dir);
   628		args.in_numargs = 2;
   629		args.in_args[0].size = sizeof(inarg);
   630		args.in_args[0].value = &inarg;
   631		args.in_args[1].size = entry->d_name.len + 1;
   632		args.in_args[1].value = entry->d_name.name;
   633		args.out_numargs = 2;
   634		args.out_args[0].size = sizeof(outentry);
   635		args.out_args[0].value = &outentry;
   636		args.out_args[1].size = sizeof(outopen);
   637		args.out_args[1].value = &outopen;
   638	
   639		if (fm->fc->init_security) {
   640			err = get_security_context(entry, mode, &security_ctx,
   641						   &security_ctxlen);
   642			if (err)
   643				goto out_put_forget_req;
   644	
   645			args.in_numargs = 3;
   646			args.in_args[2].size = security_ctxlen;
   647			args.in_args[2].value = security_ctx;
   648		}
   649	
   650		err = fuse_simple_request(fm, &args);
   651		kfree(security_ctx);
   652		if (err)
   653			goto out_free_ff;
   654	
   655		err = -EIO;
   656		if (!S_ISREG(outentry.attr.mode) || invalid_nodeid(outentry.nodeid) ||
   657		    fuse_invalid_attr(&outentry.attr))
   658			goto out_free_ff;
   659	
   660		ff->fh = outopen.fh;
   661		ff->nodeid = outentry.nodeid;
   662		ff->open_flags = outopen.open_flags;
   663	
   664		/* Inode revalidation was bypassed previously for type other than
   665		 * directories, revalidate now as we got fresh attributes.
   666		 */
   667		if (reval_inode) {
 > 668			err = fuse_atomic_open_revalidate_inode(reval_inode, entry, fi,
   669								forget, &outentry,
   670								attr_version);
   671			if (err)
   672				goto out_free_ff;
   673			inode = reval_inode;
   674			kfree(forget);
   675			goto out_finish_open;
   676		}
   677		inode = fuse_iget(dir->i_sb, outentry.nodeid, outentry.generation,
   678				  &outentry.attr, entry_attr_timeout(&outentry), 0);
   679		if (!inode) {
   680			flags &= ~(O_CREAT | O_EXCL | O_TRUNC);
   681			fuse_sync_release(NULL, ff, flags);
   682			fuse_queue_forget(fm->fc, forget, outentry.nodeid, 1);
   683			err = -ENOMEM;
   684			goto out_err;
   685		}
   686		kfree(forget);
   687		/*
   688		 * In atomic create, we skipped lookup and it is very much likely that
   689		 * dentry has DCACHE_PAR_LOOKUP flag set on it so call d_splice_alias().
   690		 * Note: Only REG file is allowed under create/atomic create.
   691		 */
   692		/* There is special case when at very first call where we check if
   693		 * atomic create is implemented by USER SPACE/libfuse or not, we
   694		 * skipped lookup. Now, in case where atomic create is not implemented
   695		 * underlying, we fall back to FUSE_CREATE. here we are required to handle
   696		 * DCACHE_PAR_LOOKUP flag.
   697		 */
   698		if (!atomic_create && !d_in_lookup(entry) && fm->fc->no_atomic_create)
   699			d_instantiate(entry, inode);
   700		else {
   701			res = d_splice_alias(inode, entry);
   702			if (res) {
   703				 /* Close the file in user space, but do not unlink it,
   704				  * if it was created - with network file systems other
   705				  * clients might have already accessed it.
   706				  */
   707				if (IS_ERR(res)) {
   708					fi = get_fuse_inode(inode);
   709					fuse_sync_release(fi, ff, flags);
   710					fuse_queue_forget(fm->fc, forget, outentry.nodeid, 1);
   711					err = PTR_ERR(res);
   712					goto out_err;
   713				}
   714				entry = res;
   715				if (alias)
   716					*alias = res;
   717			}
   718		}
   719		fuse_change_entry_timeout(entry, &outentry);
   720		/*
   721		 * In case of atomic create, we want to indicate directory change
   722		 * only if USER SPACE actually created the file.
   723		 */
   724		if (!atomic_create || (outopen.open_flags & FOPEN_FILE_CREATED))
   725			fuse_dir_changed(dir);
   726		err = finish_open(file, entry, generic_file_open);
   727	out_finish_open:
   728		if (err) {
   729			fi = get_fuse_inode(inode);
   730			fuse_sync_release(fi, ff, flags);
   731		} else {
   732			file->private_data = ff;
   733			fuse_finish_open(inode, file);
   734		}
   735		return err;
   736	
   737	out_free_ff:
   738		fuse_file_free(ff);
   739	out_put_forget_req:
   740		kfree(forget);
   741	out_err:
   742		return err;
   743	}
   744	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp