* [PATCH v3 0/3] FUSE: Implement atomic lookup + open/create
@ 2022-05-02 5:46 Dharmendra Singh
2022-05-02 5:46 ` [PATCH v3 1/3] FUSE: Implement atomic lookup + create Dharmendra Singh
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Dharmendra Singh @ 2022-05-02 5:46 UTC (permalink / raw)
To: miklos
Cc: Dharmendra Singh, linux-fsdevel, fuse-devel, linux-kernel, bschubert
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3 1/3] FUSE: Implement atomic lookup + create
2022-05-02 5:46 [PATCH v3 0/3] FUSE: Implement atomic lookup + open/create Dharmendra Singh
@ 2022-05-02 5:46 ` Dharmendra Singh
2022-05-04 15:12 ` Vivek Goyal
2022-05-02 5:46 ` [PATCH v3 2/3] Implement atomic lookup + open Dharmendra Singh
2022-05-02 5:46 ` [PATCH v3 3/3] Avoid lookup in d_revalidate() Dharmendra Singh
2 siblings, 1 reply; 9+ messages in thread
From: Dharmendra Singh @ 2022-05-02 5:46 UTC (permalink / raw)
To: miklos
Cc: Dharmendra Singh, linux-fsdevel, fuse-devel, linux-kernel,
bschubert, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v3 2/3] Implement atomic lookup + open
2022-05-02 5:46 [PATCH v3 0/3] FUSE: Implement atomic lookup + open/create Dharmendra Singh
2022-05-02 5:46 ` [PATCH v3 1/3] FUSE: Implement atomic lookup + create Dharmendra Singh
@ 2022-05-02 5:46 ` Dharmendra Singh
2022-05-02 5:46 ` [PATCH v3 3/3] Avoid lookup in d_revalidate() Dharmendra Singh
2 siblings, 0 replies; 9+ messages in thread
From: Dharmendra Singh @ 2022-05-02 5:46 UTC (permalink / raw)
To: miklos
Cc: Dharmendra Singh, linux-fsdevel, fuse-devel, linux-kernel,
bschubert, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH v3 3/3] Avoid lookup in d_revalidate()
2022-05-02 5:46 [PATCH v3 0/3] FUSE: Implement atomic lookup + open/create Dharmendra Singh
2022-05-02 5:46 ` [PATCH v3 1/3] FUSE: Implement atomic lookup + create Dharmendra Singh
2022-05-02 5:46 ` [PATCH v3 2/3] Implement atomic lookup + open Dharmendra Singh
@ 2022-05-02 5:46 ` Dharmendra Singh
2022-05-02 8:02 ` kernel test robot
2022-05-02 9:24 ` kernel test robot
2 siblings, 2 replies; 9+ messages in thread
From: Dharmendra Singh @ 2022-05-02 5:46 UTC (permalink / raw)
To: miklos
Cc: Dharmendra Singh, linux-fsdevel, fuse-devel, linux-kernel,
bschubert, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3 3/3] Avoid lookup in d_revalidate()
2022-05-02 5:46 ` [PATCH v3 3/3] Avoid lookup in d_revalidate() Dharmendra Singh
@ 2022-05-02 8:02 ` kernel test robot
2022-05-02 9:24 ` kernel test robot
1 sibling, 0 replies; 9+ messages in thread
From: kernel test robot @ 2022-05-02 8:02 UTC (permalink / raw)
To: Dharmendra Singh, miklos
Cc: kbuild-all, Dharmendra Singh, linux-fsdevel, fuse-devel,
linux-kernel, bschubert
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 3/3] Avoid lookup in d_revalidate()
2022-05-02 5:46 ` [PATCH v3 3/3] Avoid lookup in d_revalidate() Dharmendra Singh
2022-05-02 8:02 ` kernel test robot
@ 2022-05-02 9:24 ` kernel test robot
1 sibling, 0 replies; 9+ messages in thread
From: kernel test robot @ 2022-05-02 9:24 UTC (permalink / raw)
To: Dharmendra Singh, miklos
Cc: llvm, kbuild-all, Dharmendra Singh, linux-fsdevel, fuse-devel,
linux-kernel, bschubert
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/3] FUSE: Implement atomic lookup + create
2022-05-02 5:46 ` [PATCH v3 1/3] FUSE: Implement atomic lookup + create Dharmendra Singh
@ 2022-05-04 15:12 ` Vivek Goyal
2022-05-05 7:33 ` Dharmendra Hans
0 siblings, 1 reply; 9+ messages in thread
From: Vivek Goyal @ 2022-05-04 15:12 UTC (permalink / raw)
To: Dharmendra Singh
Cc: miklos, linux-fsdevel, fuse-devel, linux-kernel, bschubert,
Dharmendra Singh
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
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/3] FUSE: Implement atomic lookup + create
2022-05-04 15:12 ` Vivek Goyal
@ 2022-05-05 7:33 ` Dharmendra Hans
2022-05-06 20:06 ` Vivek Goyal
0 siblings, 1 reply; 9+ messages in thread
From: Dharmendra Hans @ 2022-05-05 7:33 UTC (permalink / raw)
To: Vivek Goyal
Cc: Miklos Szeredi, linux-fsdevel, fuse-devel, linux-kernel,
Bernd Schubert, Dharmendra Singh
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
> >
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v3 1/3] FUSE: Implement atomic lookup + create
2022-05-05 7:33 ` Dharmendra Hans
@ 2022-05-06 20:06 ` Vivek Goyal
0 siblings, 0 replies; 9+ messages in thread
From: Vivek Goyal @ 2022-05-06 20:06 UTC (permalink / raw)
To: Dharmendra Hans
Cc: Miklos Szeredi, linux-fsdevel, fuse-devel, linux-kernel,
Bernd Schubert, Dharmendra Singh
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
> > >
> >
>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-05-06 20:06 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-02 5:46 [PATCH v3 0/3] FUSE: Implement atomic lookup + open/create Dharmendra Singh
2022-05-02 5:46 ` [PATCH v3 1/3] FUSE: Implement atomic lookup + create Dharmendra Singh
2022-05-04 15:12 ` Vivek Goyal
2022-05-05 7:33 ` Dharmendra Hans
2022-05-06 20:06 ` Vivek Goyal
2022-05-02 5:46 ` [PATCH v3 2/3] Implement atomic lookup + open Dharmendra Singh
2022-05-02 5:46 ` [PATCH v3 3/3] Avoid lookup in d_revalidate() Dharmendra Singh
2022-05-02 8:02 ` kernel test robot
2022-05-02 9:24 ` kernel test robot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).