[PATCH v2 0/2] FUSE: Implement atomic lookup + open

[PATCH v2 0/2] FUSE: Implement atomic lookup + open

[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 two patches addresses this issue.

First patch handles the case where we open first time a file/dir or create
a file (O_CREAT) 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.

Second 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.

Here is the link to the libfuse pull request which implements atomic open
https://github.com/libfuse/libfuse/pull/644

I am going to post performance results shortly.


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

 fs/fuse/dir.c             | 179 +++++++++++++++++++++++++++++++++-----
 fs/fuse/file.c            |  30 ++++++-
 fs/fuse/fuse_i.h          |  13 ++-
 fs/fuse/inode.c           |   4 +-
 fs/fuse/ioctl.c           |   2 +-
 include/uapi/linux/fuse.h |   2 +
 6 files changed, 204 insertions(+), 26 deletions(-)

-- 
2.17.1

[PATCH v2 1/2] FUSE: Implement atomic lookup + open

[Dharmendra Singh]

From: Dharmendra Singh <dsingh@ddn.com>

There are couple of places in FUSE where we do agressive
lookup.
1) When we go for creating a file (O_CREAT), we do lookup
for non-existent file. It is very much likely that file
does not exists yet as O_CREAT is passed to open(). This
lookup can be avoided and can be performed  as part of
open call into libfuse.

2) When there is normal open for file/dir (dentry is
new/negative). 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.

This lookup + open in single call to libfuse and finally to
USER space has been named as atomic open. It is expected
that USER space open 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>
---
v2 patch includes:
- disabled o-create atomicity when the user space file system 
  does not have an atomic_open implemented. In principle lookups
  for O_CREATE also could be optimized out, but there is a risk
  to break existing fuse file systems. Those file system might 
  not expect open O_CREATE calls for exiting files, as these calls
  had been so far avoided as lookup was done first.

 fs/fuse/dir.c             | 113 +++++++++++++++++++++++++++++++-------
 fs/fuse/fuse_i.h          |   3 +
 fs/fuse/inode.c           |   4 +-
 include/uapi/linux/fuse.h |   2 +
 4 files changed, 101 insertions(+), 21 deletions(-)

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index 656e921f3506..b2613eb87a4e 100644
--- a/fs/fuse/dir.c
+++ b/fs/fuse/dir.c
@@ -516,16 +516,14 @@ 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.
+ * Perform create + open or lookup + open in single call to libfuse
  */
-static int fuse_create_open(struct inode *dir, struct dentry *entry,
-			    struct file *file, unsigned int flags,
-			    umode_t mode)
+static int fuse_atomic_open_common(struct inode *dir, struct dentry *entry,
+				   struct dentry **alias, struct file *file,
+				   unsigned int flags, umode_t mode,
+				   uint32_t opcode)
 {
-	int err;
+	bool create = (opcode == FUSE_CREATE ? true : false);
 	struct inode *inode;
 	struct fuse_mount *fm = get_fuse_mount(dir);
 	FUSE_ARGS(args);
@@ -535,11 +533,16 @@ 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;
+	int err;
+
+	if (alias)
+		*alias = NULL;
 
 	/* Userspace expects S_IFREG in create mode */
-	BUG_ON((mode & S_IFMT) != S_IFREG);
+	BUG_ON(create && (mode & S_IFMT) != S_IFREG);
 
 	forget = fuse_alloc_forget();
 	err = -ENOMEM;
@@ -554,7 +557,13 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
 	if (!fm->fc->dont_mask)
 		mode &= ~current_umask();
 
-	flags &= ~O_NOCTTY;
+	if (!create) {
+		flags = flags & ~(O_CREAT | O_EXCL | O_NOCTTY);
+		if (!fm->fc->atomic_o_trunc)
+			flags &= ~O_TRUNC;
+	} else {
+		flags &= ~O_NOCTTY;
+	}
 	memset(&inarg, 0, sizeof(inarg));
 	memset(&outentry, 0, sizeof(outentry));
 	inarg.flags = flags;
@@ -566,7 +575,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);
@@ -595,8 +604,12 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
 	if (err)
 		goto out_free_ff;
 
+	err = -ENOENT;
+	if (!S_ISDIR(outentry.attr.mode) && !outentry.nodeid)
+		goto out_free_ff;
+
 	err = -EIO;
-	if (!S_ISREG(outentry.attr.mode) || invalid_nodeid(outentry.nodeid) ||
+	if (invalid_nodeid(outentry.nodeid) ||
 	    fuse_invalid_attr(&outentry.attr))
 		goto out_free_ff;
 
@@ -612,10 +625,32 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
 		err = -ENOMEM;
 		goto out_err;
 	}
+	if (!fm->fc->do_atomic_open)
+		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;
+			} else {
+				entry = res;
+				if (alias)
+					*alias = res;
+			}
+		}
+	}
 	kfree(forget);
-	d_instantiate(entry, inode);
 	fuse_change_entry_timeout(entry, &outentry);
-	fuse_dir_changed(dir);
+	if (create)
+		fuse_dir_changed(dir);
 	err = finish_open(file, entry, generic_file_open);
 	if (err) {
 		fi = get_fuse_inode(inode);
@@ -634,20 +669,54 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
 	return err;
 }
 
+/*
+ * Atomic lookup + open
+ */
+
+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_open_common(dir, entry, alias, file,
+				      flags, mode, FUSE_ATOMIC_OPEN);
+	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,
 			    struct file *file, unsigned flags,
 			    umode_t mode)
 {
-	int err;
+	bool create = (flags & O_CREAT) ? true : false;
 	struct fuse_conn *fc = get_fuse_conn(dir);
-	struct dentry *res = NULL;
+	struct dentry *res = NULL, *alias = NULL;
+	int err;
 
 	if (fuse_is_bad(dir))
 		return -EIO;
 
-	if (d_in_lookup(entry)) {
+	/* Atomic lookup + open - dentry might be File or Directory */
+	if (!create) {
+		err = fuse_do_atomic_open(dir, entry, &alias, file, flags, mode);
+		res = alias;
+		if (!err)
+			goto out_dput;
+		else if (err != -ENOSYS)
+			goto no_open;
+	}
+	/* ENOSYS fall back - user space does not have full atomic open.*/
+
+	/* O_CREAT could be optimized already, but we fear to break some
+	 * userspace implementations therefore optimize in case of atomic
+	 * open only.
+	 */
+	if (!fc->do_atomic_open && d_in_lookup(entry))  {
 		res = fuse_lookup(dir, entry, 0);
 		if (IS_ERR(res))
 			return PTR_ERR(res);
@@ -656,7 +725,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 */
@@ -664,8 +733,12 @@ 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);
+	/*
+	 * If the filesystem doesn't support atomic create + open, then fall
+	 * back to separate 'mknod' + 'open' requests.
+	 */
+	err = fuse_atomic_open_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 e8e59fbdefeb..e4dc68a90b28 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;
 
+	/** Does the filesystem support atomic open? */
+	unsigned do_atomic_open:1;
+
 	/** Is opendir/releasedir not implemented by fs? */
 	unsigned no_opendir: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 d6ccee961891..a28dd60078ff 100644
--- a/include/uapi/linux/fuse.h
+++ b/include/uapi/linux/fuse.h
@@ -389,6 +389,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
@@ -537,6 +538,7 @@ enum fuse_opcode {
 	FUSE_SETUPMAPPING	= 48,
 	FUSE_REMOVEMAPPING	= 49,
 	FUSE_SYNCFS		= 50,
+	FUSE_ATOMIC_OPEN        = 51,
 
 	/* CUSE specific operations */
 	CUSE_INIT		= 4096,
-- 
2.17.1

[PATCH v2 2/2] FUSE: 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>
---
v2 patch includes:
- fixed a memory leak in <fuse_atomic_open_common>

 fs/fuse/dir.c    | 82 +++++++++++++++++++++++++++++++++++++++++++-----
 fs/fuse/file.c   | 30 ++++++++++++++++--
 fs/fuse/fuse_i.h | 10 +++++-
 fs/fuse/ioctl.c  |  2 +-
 4 files changed, 111 insertions(+), 13 deletions(-)

diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
index b2613eb87a4e..ea42a9eea924 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,13 +527,50 @@ 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;
+}
+
+
+
 /*
  * Perform create + open or lookup + open in single call to libfuse
  */
-static int fuse_atomic_open_common(struct inode *dir, struct dentry *entry,
-				   struct dentry **alias, struct file *file,
-				   unsigned int flags, umode_t mode,
-				   uint32_t opcode)
+int fuse_atomic_open_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)
 {
 	bool create = (opcode == FUSE_CREATE ? true : false);
 	struct inode *inode;
@@ -536,6 +585,7 @@ static int fuse_atomic_open_common(struct inode *dir, struct dentry *entry,
 	struct dentry *res = NULL;
 	void *security_ctx = NULL;
 	u32 security_ctxlen;
+	u64 attr_version = fuse_get_attr_version(fm->fc);
 	int err;
 
 	if (alias)
@@ -616,6 +666,20 @@ static int fuse_atomic_open_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) {
@@ -652,6 +716,7 @@ static int fuse_atomic_open_common(struct inode *dir, struct dentry *entry,
 	if (create)
 		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);
@@ -682,7 +747,7 @@ static int fuse_do_atomic_open(struct inode *dir, struct dentry *entry,
 
 	if (!fc->do_atomic_open)
 		return -ENOSYS;
-	err = fuse_atomic_open_common(dir, entry, alias, file,
+	err = fuse_atomic_open_common(dir, entry, alias, file, NULL,
 				      flags, mode, FUSE_ATOMIC_OPEN);
 	return err;
 }
@@ -703,7 +768,8 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
 
 	/* Atomic lookup + open - dentry might be File or Directory */
 	if (!create) {
-		err = fuse_do_atomic_open(dir, entry, &alias, file, flags, mode);
+		err = fuse_do_atomic_open(dir, entry, &alias, file,
+					  flags, mode);
 		res = alias;
 		if (!err)
 			goto out_dput;
@@ -737,8 +803,8 @@ static int fuse_atomic_open(struct inode *dir, struct dentry *entry,
 	 * If the filesystem doesn't support atomic create + open, then fall
 	 * back to separate 'mknod' + 'open' requests.
 	 */
-	err = fuse_atomic_open_common(dir, entry, NULL, file, flags, mode,
-				      FUSE_CREATE);
+	err = fuse_atomic_open_common(dir, entry, NULL, file, NULL, flags,
+				      mode, FUSE_CREATE);
 	if (err == -ENOSYS) {
 		fc->no_create = 1;
 		goto mknod;
diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 829094451774..37eebfb90500 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);
+	ret = fuse_atomic_open_common(d_inode_rcu(parent), dentry, NULL, file,
+				      d_inode_rcu(dentry), open_flags, 0,
+				      FUSE_ATOMIC_OPEN);
+	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 e4dc68a90b28..bb3cd0631ff2 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -1011,6 +1011,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_atomic_open_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);
 /**
  * Send RELEASE or RELEASEDIR request
  */
@@ -1314,7 +1321,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 v2 0/2] FUSE: Atomic lookup + open performance numbers

[Dharmendra Singh]

Subject: 'Re: [PATCH v2 0/2] FUSE: Atomic lookup + open performance numbers'

Thanks, Miklos. For measuring the performance, bonnie++ was used over passthrough_ll mount on tmpfs.
When taking numbers on vm, I could see non-deterministic behaviour in the results. Therefore core
binding was used for passthrough_ll and bonnie++, keeping them on separate cores.

Here are the google sheets having performance numbers.
https://docs.google.com/spreadsheets/d/1JRgF8DTR9xk5zz3_azmLcyy5kW3bgjjItmS8CYsAoT4/edit#gid=0
https://docs.google.com/spreadsheets/d/1JRgF8DTR9xk5zz3_azmLcyy5kW3bgjjItmS8CYsAoT4/edit#gid=1833203226

Following are the libfuse patches(commit on March 7 and March 8 in first link) which were used to test
these changes
https://github.com/aakefbs/libfuse/commits/atomic-open-and-no-flush
https://github.com/libfuse/libfuse/pull/644

Parameters used in mounting passthrough_ll:
 numactl --localalloc --physcpubind=16-23 passthrough_ll -f -osource=/tmp/source,allow_other,allow_root,
 cache=never -o max_idle_threads=1 /tmp/dest
     (Here cache=never results in direct-io on the file)

Parameters used in bonnie++:
In sheet 0B:
numactl --localalloc --physcpubind=0-7  bonnie++ -x 4 -q -s0  -d /tmp/dest/ -n 10:0:0:10 -r 0 -u 0 2>/dev/null

in sheet 1B:
numactl --localalloc --physcpubind=0-7 bonnie++ -x 4 -q -s0 -d /tmp/dest/ -n 10:1:1:10 -r 0 -u 0 2>/dev/null

Additional settings done on the testing machine:
cpupower frequency-set -g performance

Running bonnie++ gives us results for Create/s,  Read/s and Delete/s. Below table summarises the numbers
for  these three operations. Please note that for read of 0 bytes, bonnie++ does ops in order of create-open,
close and stat but no atomic open.  Therefore performance results  in the sheet 0B had overhead of extra
stat calls.  Whereas in sheet 1B, we directed bonnie++ to read 1 byte and this triggered atomic open call but
numbers for this run involve overhead for read operation itself instead of just plain open/close.

Here is the table summarising the performance numbers

Table: 0B
                                               Sequential                  |            Random
                                           Creat/s       Read/s    Del/s   |    Creat/s     Read/s      Del/s
Patched Libfuse                                -3.55%    -4.9%    -4.43%   |    -0.4%      -1.6%       -1.0%
Patched Libfuse + No-Flush                     +22.3%    +6%       +5.15%  |    +27.9%     +14.5%       +2.8%
Patched Libfuse + Patched FuseK                +22.9%    +6.1%     +5.3%   |    +28.3%     +14.5%       +2.3%
Patched Libfuse + Patched FuseK + No-Flush     +33.4%    -4.4%     -3.73%  |    +38.8%     -2.5%        -2.0%



 Table: 1B
                                                  Sequential                    |                  Random
                                           Create/s       Read/s       Del/s    |      Create/s     Read/s     Del/s
Patched Libfuse                            -0.22%        -0.35%       -0.7%     |      -0.27%        -0.78%    -2.35%
Patched Libfuse + No-Flush                 +2.5%         +2.6%        -9.6%     |      +2.5%         -8.6%     -6.26%
Patched Libfuse + Patched FuseK            +1.63%        -1.0%        -11.45%   |      +4.48%        -6.84%    -4.0%
Patched Libfuse + Patched FuseK + No-Flush  +32.43%      +26.61%      +076%     |      +33.2%       +14.7%     -0.40%

Here
No-Flush = No flush trigger from fuse kernel into libfuse

In Table 1B, we see 4th row has good improvements for both create and Read whereas Del seems to be almost not
changed. In Table 0B, 4th row we have Read perf reduced, it was found out that this was caused by some changes
in libfuse. So this was fixed and in Table 1B, same row, we can see increased numbers.

In Table 0B, 3rd row, we have good numbers because bonnie++ used 0 bytes to read  and this changed behaviour
and impacted perf whereas for the same row, Table 1B we have reduced numbers because it involved flush
calls for 1 byte from the fuse kernel into libfuse.

These changes are not for fuse kernel/users-space context switches only, but our main goal is to have improvement performance
for network file systems
   - Number network round trips
   - Reduce load on meta servers with thousands of clients

Reduced kernel/userspace context switches is 'just' a side effect.

Thanks,
Dharmendra

Re: [PATCH v2 0/2] FUSE: Implement atomic lookup + open

[Dharmendra Hans]

On Tue, Mar 22, 2022 at 5:22 PM 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 two patches addresses this issue.
>
> First patch handles the case where we open first time a file/dir or create
> a file (O_CREAT) 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.
>
> Second 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.
>
> Here is the link to the libfuse pull request which implements atomic open
> https://github.com/libfuse/libfuse/pull/644
>
> I am going to post performance results shortly.
>
>
> Dharmendra Singh (2):
>   FUSE: Implement atomic lookup + open
>   FUSE: Avoid lookup in d_revalidate()

A gentle reminder to look into the above patch set.

Re: [PATCH v2 0/2] FUSE: Implement atomic lookup + open

[Dharmendra Hans]

On Tue, Mar 29, 2022 at 4:37 PM Dharmendra Hans <dharamhans87@gmail.com> wrote:
>
> On Tue, Mar 22, 2022 at 5:22 PM 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 two patches addresses this issue.
> >
> > First patch handles the case where we open first time a file/dir or create
> > a file (O_CREAT) 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.
> >
> > Second 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.
> >
> > Here is the link to the libfuse pull request which implements atomic open
> > https://github.com/libfuse/libfuse/pull/644
> >
> > I am going to post performance results shortly.
> >
> >
> > Dharmendra Singh (2):
> >   FUSE: Implement atomic lookup + open
> >   FUSE: Avoid lookup in d_revalidate()
>
> A gentle reminder to look into the above patch set.
Sending a gentle reminder again to look into the requested patches.

Re: [PATCH v2 1/2] FUSE: Implement atomic lookup + open

[Miklos Szeredi]

On Tue, 22 Mar 2022 at 12:52, Dharmendra Singh <dharamhans87@gmail.com> wrote:
>
> From: Dharmendra Singh <dsingh@ddn.com>
>
> There are couple of places in FUSE where we do agressive
> lookup.
> 1) When we go for creating a file (O_CREAT), we do lookup
> for non-existent file. It is very much likely that file
> does not exists yet as O_CREAT is passed to open(). This
> lookup can be avoided and can be performed  as part of
> open call into libfuse.
>
> 2) When there is normal open for file/dir (dentry is
> new/negative). 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.
>
> This lookup + open in single call to libfuse and finally to
> USER space has been named as atomic open. It is expected
> that USER space open 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>
> ---
> v2 patch includes:
> - disabled o-create atomicity when the user space file system
>   does not have an atomic_open implemented. In principle lookups
>   for O_CREATE also could be optimized out, but there is a risk
>   to break existing fuse file systems. Those file system might
>   not expect open O_CREATE calls for exiting files, as these calls
>   had been so far avoided as lookup was done first.

So we enabling atomic lookup+create only if FUSE_DO_ATOMIC_OPEN is
set.  This logic is a bit confusing as CREATE is unrelated to
ATOMIC_OPEN.   It would be cleaner to have a separate flag for atomic
lookup+create.  And in fact FUSE_DO_ATOMIC_OPEN could be dropped and
the usual logic of setting fc->no_atomic_open if ENOSYS is returned
could be used instead.

>
>  fs/fuse/dir.c             | 113 +++++++++++++++++++++++++++++++-------
>  fs/fuse/fuse_i.h          |   3 +
>  fs/fuse/inode.c           |   4 +-
>  include/uapi/linux/fuse.h |   2 +
>  4 files changed, 101 insertions(+), 21 deletions(-)
>
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index 656e921f3506..b2613eb87a4e 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -516,16 +516,14 @@ 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.
> + * Perform create + open or lookup + open in single call to libfuse
>   */
> -static int fuse_create_open(struct inode *dir, struct dentry *entry,
> -                           struct file *file, unsigned int flags,
> -                           umode_t mode)
> +static int fuse_atomic_open_common(struct inode *dir, struct dentry *entry,
> +                                  struct dentry **alias, struct file *file,
> +                                  unsigned int flags, umode_t mode,
> +                                  uint32_t opcode)
>  {
> -       int err;
> +       bool create = (opcode == FUSE_CREATE ? true : false);
>         struct inode *inode;
>         struct fuse_mount *fm = get_fuse_mount(dir);
>         FUSE_ARGS(args);
> @@ -535,11 +533,16 @@ 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;
> +       int err;
> +
> +       if (alias)
> +               *alias = NULL;
>
>         /* Userspace expects S_IFREG in create mode */
> -       BUG_ON((mode & S_IFMT) != S_IFREG);
> +       BUG_ON(create && (mode & S_IFMT) != S_IFREG);
>
>         forget = fuse_alloc_forget();
>         err = -ENOMEM;
> @@ -554,7 +557,13 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
>         if (!fm->fc->dont_mask)
>                 mode &= ~current_umask();
>
> -       flags &= ~O_NOCTTY;
> +       if (!create) {
> +               flags = flags & ~(O_CREAT | O_EXCL | O_NOCTTY);

We know O_CREAT and O_EXCL are not set in this case.

> +               if (!fm->fc->atomic_o_trunc)
> +                       flags &= ~O_TRUNC;

I think atomic_open should imply atomic_o_trunc.  Not worth
complicating this further with a separate case.

> +       } else {
> +               flags &= ~O_NOCTTY;
> +       }
>         memset(&inarg, 0, sizeof(inarg));
>         memset(&outentry, 0, sizeof(outentry));
>         inarg.flags = flags;
> @@ -566,7 +575,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);
> @@ -595,8 +604,12 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
>         if (err)
>                 goto out_free_ff;
>
> +       err = -ENOENT;
> +       if (!S_ISDIR(outentry.attr.mode) && !outentry.nodeid)
> +               goto out_free_ff;
> +
>         err = -EIO;
> -       if (!S_ISREG(outentry.attr.mode) || invalid_nodeid(outentry.nodeid) ||
> +       if (invalid_nodeid(outentry.nodeid) ||
>             fuse_invalid_attr(&outentry.attr))
>                 goto out_free_ff;
>
> @@ -612,10 +625,32 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
>                 err = -ENOMEM;
>                 goto out_err;
>         }
> +       if (!fm->fc->do_atomic_open)
> +               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;
> +                       } else {
> +                               entry = res;
> +                               if (alias)
> +                                       *alias = res;
> +                       }
> +               }
> +       }
>         kfree(forget);
> -       d_instantiate(entry, inode);
>         fuse_change_entry_timeout(entry, &outentry);
> -       fuse_dir_changed(dir);
> +       if (create)
> +               fuse_dir_changed(dir);

This will invalidate the parent even if the file was not created.
Userspace will have to indicate whether the file was created or not as
the kernel won't be able to determine this otherwise.  This affects
permission checking as well.

>         err = finish_open(file, entry, generic_file_open);
>         if (err) {
>                 fi = get_fuse_inode(inode);
> @@ -634,20 +669,54 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
>         return err;
>  }
>
> +/*
> + * Atomic lookup + open
> + */
> +
> +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_open_common(dir, entry, alias, file,
> +                                     flags, mode, FUSE_ATOMIC_OPEN);
> +       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,
>                             struct file *file, unsigned flags,
>                             umode_t mode)
>  {
> -       int err;
> +       bool create = (flags & O_CREAT) ? true : false;
>         struct fuse_conn *fc = get_fuse_conn(dir);
> -       struct dentry *res = NULL;
> +       struct dentry *res = NULL, *alias = NULL;
> +       int err;
>
>         if (fuse_is_bad(dir))
>                 return -EIO;
>
> -       if (d_in_lookup(entry)) {
> +       /* Atomic lookup + open - dentry might be File or Directory */
> +       if (!create) {
> +               err = fuse_do_atomic_open(dir, entry, &alias, file, flags, mode);
> +               res = alias;
> +               if (!err)
> +                       goto out_dput;
> +               else if (err != -ENOSYS)
> +                       goto no_open;

The above looks bogus.  On error we just want to return that error,
not finish the open.

> +       }
> +       /* ENOSYS fall back - user space does not have full atomic open.*/
> +
> +       /* O_CREAT could be optimized already, but we fear to break some
> +        * userspace implementations therefore optimize in case of atomic
> +        * open only.
> +        */
> +       if (!fc->do_atomic_open && d_in_lookup(entry))  {
>                 res = fuse_lookup(dir, entry, 0);
>                 if (IS_ERR(res))
>                         return PTR_ERR(res);
> @@ -656,7 +725,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 */
> @@ -664,8 +733,12 @@ 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);
> +       /*
> +        * If the filesystem doesn't support atomic create + open, then fall
> +        * back to separate 'mknod' + 'open' requests.
> +        */
> +       err = fuse_atomic_open_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 e8e59fbdefeb..e4dc68a90b28 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;
>
> +       /** Does the filesystem support atomic open? */
> +       unsigned do_atomic_open:1;
> +
>         /** Is opendir/releasedir not implemented by fs? */
>         unsigned no_opendir: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 d6ccee961891..a28dd60078ff 100644
> --- a/include/uapi/linux/fuse.h
> +++ b/include/uapi/linux/fuse.h
> @@ -389,6 +389,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
> @@ -537,6 +538,7 @@ enum fuse_opcode {
>         FUSE_SETUPMAPPING       = 48,
>         FUSE_REMOVEMAPPING      = 49,
>         FUSE_SYNCFS             = 50,
> +       FUSE_ATOMIC_OPEN        = 51,
>
>         /* CUSE specific operations */
>         CUSE_INIT               = 4096,
> --
> 2.17.1
>

Re: [PATCH v2 1/2] FUSE: Implement atomic lookup + open

[Dharmendra Hans]

On Fri, Apr 22, 2022 at 8:59 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Tue, 22 Mar 2022 at 12:52, Dharmendra Singh <dharamhans87@gmail.com> wrote:
> >
> > From: Dharmendra Singh <dsingh@ddn.com>
> >
> > There are couple of places in FUSE where we do agressive
> > lookup.
> > 1) When we go for creating a file (O_CREAT), we do lookup
> > for non-existent file. It is very much likely that file
> > does not exists yet as O_CREAT is passed to open(). This
> > lookup can be avoided and can be performed  as part of
> > open call into libfuse.
> >
> > 2) When there is normal open for file/dir (dentry is
> > new/negative). 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.
> >
> > This lookup + open in single call to libfuse and finally to
> > USER space has been named as atomic open. It is expected
> > that USER space open 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>
> > ---
> > v2 patch includes:
> > - disabled o-create atomicity when the user space file system
> >   does not have an atomic_open implemented. In principle lookups
> >   for O_CREATE also could be optimized out, but there is a risk
> >   to break existing fuse file systems. Those file system might
> >   not expect open O_CREATE calls for exiting files, as these calls
> >   had been so far avoided as lookup was done first.
>
> So we enabling atomic lookup+create only if FUSE_DO_ATOMIC_OPEN is
> set.  This logic is a bit confusing as CREATE is unrelated to
> ATOMIC_OPEN.   It would be cleaner to have a separate flag for atomic
> lookup+create.  And in fact FUSE_DO_ATOMIC_OPEN could be dropped and
> the usual logic of setting fc->no_atomic_open if ENOSYS is returned
> could be used instead.

I am aware that ATOMIC_OPEN is not directly related to CREATE. But
This is more of feature enabling by using the flag. If we do not
FUSE_DO_ATOMIC_OPEN, CREATE calls would not know that it need to
optimize lookup calls otherwise as we know only from open call that
atomic open is implemented. So workloads or performance measuring
applications such as bonnie++ would not be showing improvements for
CREATE, it would not be making 'open' calls. And it is only in open
calls we set fc->do_atomic_open.

>
> >
> >  fs/fuse/dir.c             | 113 +++++++++++++++++++++++++++++++-------
> >  fs/fuse/fuse_i.h          |   3 +
> >  fs/fuse/inode.c           |   4 +-
> >  include/uapi/linux/fuse.h |   2 +
> >  4 files changed, 101 insertions(+), 21 deletions(-)
> >
> > diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> > index 656e921f3506..b2613eb87a4e 100644
> > --- a/fs/fuse/dir.c
> > +++ b/fs/fuse/dir.c
> > @@ -516,16 +516,14 @@ 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.
> > + * Perform create + open or lookup + open in single call to libfuse
> >   */
> > -static int fuse_create_open(struct inode *dir, struct dentry *entry,
> > -                           struct file *file, unsigned int flags,
> > -                           umode_t mode)
> > +static int fuse_atomic_open_common(struct inode *dir, struct dentry *entry,
> > +                                  struct dentry **alias, struct file *file,
> > +                                  unsigned int flags, umode_t mode,
> > +                                  uint32_t opcode)
> >  {
> > -       int err;
> > +       bool create = (opcode == FUSE_CREATE ? true : false);
> >         struct inode *inode;
> >         struct fuse_mount *fm = get_fuse_mount(dir);
> >         FUSE_ARGS(args);
> > @@ -535,11 +533,16 @@ 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;
> > +       int err;
> > +
> > +       if (alias)
> > +               *alias = NULL;
> >
> >         /* Userspace expects S_IFREG in create mode */
> > -       BUG_ON((mode & S_IFMT) != S_IFREG);
> > +       BUG_ON(create && (mode & S_IFMT) != S_IFREG);
> >
> >         forget = fuse_alloc_forget();
> >         err = -ENOMEM;
> > @@ -554,7 +557,13 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
> >         if (!fm->fc->dont_mask)
> >                 mode &= ~current_umask();
> >
> > -       flags &= ~O_NOCTTY;
> > +       if (!create) {
> > +               flags = flags & ~(O_CREAT | O_EXCL | O_NOCTTY);
>
> We know O_CREAT and O_EXCL are not set in this case.

Would remove it

>
> > +               if (!fm->fc->atomic_o_trunc)
> > +                       flags &= ~O_TRUNC;
>
> I think atomic_open should imply atomic_o_trunc.  Not worth
> complicating this further with a separate case.

 I see. So if atomic open is enabled, we should be truncating file as
part of this atomic open call itself despite  fc->atomic_o_trunc is
set or not. Would make changes here.

>
> > +       } else {
> > +               flags &= ~O_NOCTTY;
> > +       }
> >         memset(&inarg, 0, sizeof(inarg));
> >         memset(&outentry, 0, sizeof(outentry));
> >         inarg.flags = flags;
> > @@ -566,7 +575,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);
> > @@ -595,8 +604,12 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
> >         if (err)
> >                 goto out_free_ff;
> >
> > +       err = -ENOENT;
> > +       if (!S_ISDIR(outentry.attr.mode) && !outentry.nodeid)
> > +               goto out_free_ff;
> > +
> >         err = -EIO;
> > -       if (!S_ISREG(outentry.attr.mode) || invalid_nodeid(outentry.nodeid) ||
> > +       if (invalid_nodeid(outentry.nodeid) ||
> >             fuse_invalid_attr(&outentry.attr))
> >                 goto out_free_ff;
> >
> > @@ -612,10 +625,32 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
> >                 err = -ENOMEM;
> >                 goto out_err;
> >         }
> > +       if (!fm->fc->do_atomic_open)
> > +               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;
> > +                       } else {
> > +                               entry = res;
> > +                               if (alias)
> > +                                       *alias = res;
> > +                       }
> > +               }
> > +       }
> >         kfree(forget);
> > -       d_instantiate(entry, inode);
> >         fuse_change_entry_timeout(entry, &outentry);
> > -       fuse_dir_changed(dir);
> > +       if (create)
> > +               fuse_dir_changed(dir);
>
> This will invalidate the parent even if the file was not created.
> Userspace will have to indicate whether the file was created or not as
> the kernel won't be able to determine this otherwise.  This affects
> permission checking as well.

Thanks, I see. I would check if we can pass a flag from libfuse to
fuse kernel and check here for the same in case file was actually
created or not.

> >         err = finish_open(file, entry, generic_file_open);
> >         if (err) {
> >                 fi = get_fuse_inode(inode);
> > @@ -634,20 +669,54 @@ static int fuse_create_open(struct inode *dir, struct dentry *entry,
> >         return err;
> >  }
> >
> > +/*
> > + * Atomic lookup + open
> > + */
> > +
> > +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_open_common(dir, entry, alias, file,
> > +                                     flags, mode, FUSE_ATOMIC_OPEN);
> > +       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,
> >                             struct file *file, unsigned flags,
> >                             umode_t mode)
> >  {
> > -       int err;
> > +       bool create = (flags & O_CREAT) ? true : false;
> >         struct fuse_conn *fc = get_fuse_conn(dir);
> > -       struct dentry *res = NULL;
> > +       struct dentry *res = NULL, *alias = NULL;
> > +       int err;
> >
> >         if (fuse_is_bad(dir))
> >                 return -EIO;
> >
> > -       if (d_in_lookup(entry)) {
> > +       /* Atomic lookup + open - dentry might be File or Directory */
> > +       if (!create) {
> > +               err = fuse_do_atomic_open(dir, entry, &alias, file, flags, mode);
> > +               res = alias;
> > +               if (!err)
> > +                       goto out_dput;
> > +               else if (err != -ENOSYS)
> > +                       goto no_open;
>
> The above looks bogus.  On error we just want to return that error,
> not finish the open.

Thanks for pointing it out. I would handle error return.

Re: [PATCH v2 1/2] FUSE: Implement atomic lookup + open

[Miklos Szeredi]

On Mon, 25 Apr 2022 at 07:26, Dharmendra Hans <dharamhans87@gmail.com> wrote:
>
> On Fri, Apr 22, 2022 at 8:59 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Tue, 22 Mar 2022 at 12:52, Dharmendra Singh <dharamhans87@gmail.com> wrote:
> > >
> > > From: Dharmendra Singh <dsingh@ddn.com>
> > >
> > > There are couple of places in FUSE where we do agressive
> > > lookup.
> > > 1) When we go for creating a file (O_CREAT), we do lookup
> > > for non-existent file. It is very much likely that file
> > > does not exists yet as O_CREAT is passed to open(). This
> > > lookup can be avoided and can be performed  as part of
> > > open call into libfuse.
> > >
> > > 2) When there is normal open for file/dir (dentry is
> > > new/negative). 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.
> > >
> > > This lookup + open in single call to libfuse and finally to
> > > USER space has been named as atomic open. It is expected
> > > that USER space open 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>
> > > ---
> > > v2 patch includes:
> > > - disabled o-create atomicity when the user space file system
> > >   does not have an atomic_open implemented. In principle lookups
> > >   for O_CREATE also could be optimized out, but there is a risk
> > >   to break existing fuse file systems. Those file system might
> > >   not expect open O_CREATE calls for exiting files, as these calls
> > >   had been so far avoided as lookup was done first.
> >
> > So we enabling atomic lookup+create only if FUSE_DO_ATOMIC_OPEN is
> > set.  This logic is a bit confusing as CREATE is unrelated to
> > ATOMIC_OPEN.   It would be cleaner to have a separate flag for atomic
> > lookup+create.  And in fact FUSE_DO_ATOMIC_OPEN could be dropped and
> > the usual logic of setting fc->no_atomic_open if ENOSYS is returned
> > could be used instead.
>
> I am aware that ATOMIC_OPEN is not directly related to CREATE. But
> This is more of feature enabling by using the flag. If we do not
> FUSE_DO_ATOMIC_OPEN, CREATE calls would not know that it need to
> optimize lookup calls otherwise as we know only from open call that
> atomic open is implemented.

Right.  So because the atomic lookup+crteate would need a new flag to
return whether the file was created or not, this is probably better
implemented as a completely new request type (FUSE_ATOMIC_CREATE?)

No new INIT flags needed at all, since we can use the ENOSYS mechanism
to determine whether the filesystem has atomic open/create ops or not.

Thanks,
Miklos

Re: [PATCH v2 1/2] FUSE: Implement atomic lookup + open

[Dharmendra Hans]

On Mon, Apr 25, 2022 at 1:08 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Mon, 25 Apr 2022 at 07:26, Dharmendra Hans <dharamhans87@gmail.com> wrote:
> >
> > On Fri, Apr 22, 2022 at 8:59 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > >
> > > On Tue, 22 Mar 2022 at 12:52, Dharmendra Singh <dharamhans87@gmail.com> wrote:
> > > >
> > > > From: Dharmendra Singh <dsingh@ddn.com>
> > > >
> > > > There are couple of places in FUSE where we do agressive
> > > > lookup.
> > > > 1) When we go for creating a file (O_CREAT), we do lookup
> > > > for non-existent file. It is very much likely that file
> > > > does not exists yet as O_CREAT is passed to open(). This
> > > > lookup can be avoided and can be performed  as part of
> > > > open call into libfuse.
> > > >
> > > > 2) When there is normal open for file/dir (dentry is
> > > > new/negative). 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.
> > > >
> > > > This lookup + open in single call to libfuse and finally to
> > > > USER space has been named as atomic open. It is expected
> > > > that USER space open 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>
> > > > ---
> > > > v2 patch includes:
> > > > - disabled o-create atomicity when the user space file system
> > > >   does not have an atomic_open implemented. In principle lookups
> > > >   for O_CREATE also could be optimized out, but there is a risk
> > > >   to break existing fuse file systems. Those file system might
> > > >   not expect open O_CREATE calls for exiting files, as these calls
> > > >   had been so far avoided as lookup was done first.
> > >
> > > So we enabling atomic lookup+create only if FUSE_DO_ATOMIC_OPEN is
> > > set.  This logic is a bit confusing as CREATE is unrelated to
> > > ATOMIC_OPEN.   It would be cleaner to have a separate flag for atomic
> > > lookup+create.  And in fact FUSE_DO_ATOMIC_OPEN could be dropped and
> > > the usual logic of setting fc->no_atomic_open if ENOSYS is returned
> > > could be used instead.
> >
> > I am aware that ATOMIC_OPEN is not directly related to CREATE. But
> > This is more of feature enabling by using the flag. If we do not
> > FUSE_DO_ATOMIC_OPEN, CREATE calls would not know that it need to
> > optimize lookup calls otherwise as we know only from open call that
> > atomic open is implemented.
>
> Right.  So because the atomic lookup+crteate would need a new flag to
> return whether the file was created or not, this is probably better
> implemented as a completely new request type (FUSE_ATOMIC_CREATE?)
>
> No new INIT flags needed at all, since we can use the ENOSYS mechanism
> to determine whether the filesystem has atomic open/create ops or not.

Yes, it sounds good to have a separate request type for CREATE. I
would separate out the patch into two for create and open.  Will omit
INIT flags. Also, I would change libfuse code accordingly.

Re: [PATCH v2 1/2] FUSE: Implement atomic lookup + open

[Dharmendra Hans]

On Mon, Apr 25, 2022 at 4:13 PM Dharmendra Hans <dharamhans87@gmail.com> wrote:
>
> On Mon, Apr 25, 2022 at 1:08 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Mon, 25 Apr 2022 at 07:26, Dharmendra Hans <dharamhans87@gmail.com> wrote:
> > >
> > > On Fri, Apr 22, 2022 at 8:59 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > > >
> > > > On Tue, 22 Mar 2022 at 12:52, Dharmendra Singh <dharamhans87@gmail.com> wrote:
> > > > >
> > > > > From: Dharmendra Singh <dsingh@ddn.com>
> > > > >
> > > > > There are couple of places in FUSE where we do agressive
> > > > > lookup.
> > > > > 1) When we go for creating a file (O_CREAT), we do lookup
> > > > > for non-existent file. It is very much likely that file
> > > > > does not exists yet as O_CREAT is passed to open(). This
> > > > > lookup can be avoided and can be performed  as part of
> > > > > open call into libfuse.
> > > > >
> > > > > 2) When there is normal open for file/dir (dentry is
> > > > > new/negative). 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.
> > > > >
> > > > > This lookup + open in single call to libfuse and finally to
> > > > > USER space has been named as atomic open. It is expected
> > > > > that USER space open 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>
> > > > > ---
> > > > > v2 patch includes:
> > > > > - disabled o-create atomicity when the user space file system
> > > > >   does not have an atomic_open implemented. In principle lookups
> > > > >   for O_CREATE also could be optimized out, but there is a risk
> > > > >   to break existing fuse file systems. Those file system might
> > > > >   not expect open O_CREATE calls for exiting files, as these calls
> > > > >   had been so far avoided as lookup was done first.
> > > >
> > > > So we enabling atomic lookup+create only if FUSE_DO_ATOMIC_OPEN is
> > > > set.  This logic is a bit confusing as CREATE is unrelated to
> > > > ATOMIC_OPEN.   It would be cleaner to have a separate flag for atomic
> > > > lookup+create.  And in fact FUSE_DO_ATOMIC_OPEN could be dropped and
> > > > the usual logic of setting fc->no_atomic_open if ENOSYS is returned
> > > > could be used instead.
> > >
> > > I am aware that ATOMIC_OPEN is not directly related to CREATE. But
> > > This is more of feature enabling by using the flag. If we do not
> > > FUSE_DO_ATOMIC_OPEN, CREATE calls would not know that it need to
> > > optimize lookup calls otherwise as we know only from open call that
> > > atomic open is implemented.
> >
> > Right.  So because the atomic lookup+crteate would need a new flag to
> > return whether the file was created or not, this is probably better
> > implemented as a completely new request type (FUSE_ATOMIC_CREATE?)
> >
> > No new INIT flags needed at all, since we can use the ENOSYS mechanism
> > to determine whether the filesystem has atomic open/create ops or not.
>
> Yes, it sounds good to have a separate request type for CREATE. I
> would separate out the patch into two for create and open.  Will omit
> INIT flags. Also, I would change libfuse code accordingly.

Actually when writing the code, I observe that not having INIT flags
works fine for atomic create but it does not work well for atomic
open case considering specially 3rd  patch which optimises
d_revalidate() lookups.
(https://lore.kernel.org/linux-fsdevel/20220322115148.3870-3-dharamhans87@gmail.com/,
 we did not receive any comments on it so far).
So it looks like we need INIT flags in atomic open case at least
considering that 3rd patch would go in as well.