* [RFC][PATCH 1/4] fs: prepare for stackable filesystems backing file helpers
2023-12-21 9:54 [RFC][PATCH 0/4] Intruduce stacking filesystem vfs helpers Amir Goldstein
@ 2023-12-21 9:54 ` Amir Goldstein
2023-12-21 9:54 ` [RFC][PATCH 2/4] fs: factor out backing_file_{read,write}_iter() helpers Amir Goldstein
` (3 subsequent siblings)
4 siblings, 0 replies; 13+ messages in thread
From: Amir Goldstein @ 2023-12-21 9:54 UTC (permalink / raw)
To: Christian Brauner, Miklos Szeredi; +Cc: linux-fsdevel, linux-unionfs
In preparation for factoring out some backing file io helpers from
overlayfs, move backing_file_open() into a new file fs/backing-file.c
and header.
Add a MAINTAINERS entry for stackable filesystems and add a Kconfig
FS_STACK which stackable filesystems need to select.
For now, the backing_file struct, the backing_file alloc/free functions
and the backing_file_real_path() accessor remain internal to file_table.c.
We may change that in the future.
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
MAINTAINERS | 9 +++++++
fs/Kconfig | 4 +++
fs/Makefile | 1 +
fs/backing-file.c | 48 ++++++++++++++++++++++++++++++++++++
fs/open.c | 38 ----------------------------
fs/overlayfs/Kconfig | 1 +
fs/overlayfs/file.c | 1 +
include/linux/backing-file.h | 17 +++++++++++++
include/linux/fs.h | 3 ---
9 files changed, 81 insertions(+), 41 deletions(-)
create mode 100644 fs/backing-file.c
create mode 100644 include/linux/backing-file.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 788be9ab5b73..9115a4f0dec7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8186,6 +8186,15 @@ S: Supported
F: fs/iomap/
F: include/linux/iomap.h
+FILESYSTEMS [STACKABLE]
+M: Miklos Szeredi <miklos@szeredi.hu>
+M: Amir Goldstein <amir73il@gmail.com>
+L: linux-fsdevel@vger.kernel.org
+L: linux-unionfs@vger.kernel.org
+S: Maintained
+F: fs/backing-file.c
+F: include/linux/backing-file.h
+
FINTEK F75375S HARDWARE MONITOR AND FAN CONTROLLER DRIVER
M: Riku Voipio <riku.voipio@iki.fi>
L: linux-hwmon@vger.kernel.org
diff --git a/fs/Kconfig b/fs/Kconfig
index fd1f655b4f1f..c47fa4eb9282 100644
--- a/fs/Kconfig
+++ b/fs/Kconfig
@@ -18,6 +18,10 @@ config VALIDATE_FS_PARSER
config FS_IOMAP
bool
+# Stackable filesystems
+config FS_STACK
+ bool
+
config BUFFER_HEAD
bool
diff --git a/fs/Makefile b/fs/Makefile
index 75522f88e763..a6962c588962 100644
--- a/fs/Makefile
+++ b/fs/Makefile
@@ -39,6 +39,7 @@ obj-$(CONFIG_COMPAT_BINFMT_ELF) += compat_binfmt_elf.o
obj-$(CONFIG_BINFMT_ELF_FDPIC) += binfmt_elf_fdpic.o
obj-$(CONFIG_BINFMT_FLAT) += binfmt_flat.o
+obj-$(CONFIG_FS_STACK) += backing-file.o
obj-$(CONFIG_FS_MBCACHE) += mbcache.o
obj-$(CONFIG_FS_POSIX_ACL) += posix_acl.o
obj-$(CONFIG_NFS_COMMON) += nfs_common/
diff --git a/fs/backing-file.c b/fs/backing-file.c
new file mode 100644
index 000000000000..04b33036f709
--- /dev/null
+++ b/fs/backing-file.c
@@ -0,0 +1,48 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Common helpers for stackable filesystems and backing files.
+ *
+ * Copyright (C) 2023 CTERA Networks.
+ */
+
+#include <linux/fs.h>
+#include <linux/backing-file.h>
+
+#include "internal.h"
+
+/**
+ * backing_file_open - open a backing file for kernel internal use
+ * @user_path: path that the user reuqested to open
+ * @flags: open flags
+ * @real_path: path of the backing file
+ * @cred: credentials for open
+ *
+ * Open a backing file for a stackable filesystem (e.g., overlayfs).
+ * @user_path may be on the stackable filesystem and @real_path on the
+ * underlying filesystem. In this case, we want to be able to return the
+ * @user_path of the stackable filesystem. This is done by embedding the
+ * returned file into a container structure that also stores the stacked
+ * file's path, which can be retrieved using backing_file_user_path().
+ */
+struct file *backing_file_open(const struct path *user_path, int flags,
+ const struct path *real_path,
+ const struct cred *cred)
+{
+ struct file *f;
+ int error;
+
+ f = alloc_empty_backing_file(flags, cred);
+ if (IS_ERR(f))
+ return f;
+
+ path_get(user_path);
+ *backing_file_user_path(f) = *user_path;
+ error = vfs_open(real_path, f);
+ if (error) {
+ fput(f);
+ f = ERR_PTR(error);
+ }
+
+ return f;
+}
+EXPORT_SYMBOL_GPL(backing_file_open);
diff --git a/fs/open.c b/fs/open.c
index d877228d5939..a75054237437 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -1184,44 +1184,6 @@ struct file *kernel_file_open(const struct path *path, int flags,
}
EXPORT_SYMBOL_GPL(kernel_file_open);
-/**
- * backing_file_open - open a backing file for kernel internal use
- * @user_path: path that the user reuqested to open
- * @flags: open flags
- * @real_path: path of the backing file
- * @cred: credentials for open
- *
- * Open a backing file for a stackable filesystem (e.g., overlayfs).
- * @user_path may be on the stackable filesystem and @real_path on the
- * underlying filesystem. In this case, we want to be able to return the
- * @user_path of the stackable filesystem. This is done by embedding the
- * returned file into a container structure that also stores the stacked
- * file's path, which can be retrieved using backing_file_user_path().
- */
-struct file *backing_file_open(const struct path *user_path, int flags,
- const struct path *real_path,
- const struct cred *cred)
-{
- struct file *f;
- int error;
-
- f = alloc_empty_backing_file(flags, cred);
- if (IS_ERR(f))
- return f;
-
- path_get(user_path);
- *backing_file_user_path(f) = *user_path;
- f->f_path = *real_path;
- error = do_dentry_open(f, d_inode(real_path->dentry), NULL);
- if (error) {
- fput(f);
- f = ERR_PTR(error);
- }
-
- return f;
-}
-EXPORT_SYMBOL_GPL(backing_file_open);
-
#define WILL_CREATE(flags) (flags & (O_CREAT | __O_TMPFILE))
#define O_PATH_FLAGS (O_DIRECTORY | O_NOFOLLOW | O_PATH | O_CLOEXEC)
diff --git a/fs/overlayfs/Kconfig b/fs/overlayfs/Kconfig
index fec5020c3495..2ac67e04a6fb 100644
--- a/fs/overlayfs/Kconfig
+++ b/fs/overlayfs/Kconfig
@@ -1,6 +1,7 @@
# SPDX-License-Identifier: GPL-2.0-only
config OVERLAY_FS
tristate "Overlay filesystem support"
+ select FS_STACK
select EXPORTFS
help
An overlay filesystem combines two filesystems - an 'upper' filesystem
diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
index 4e46420c8fdd..a6da3eaf6d4f 100644
--- a/fs/overlayfs/file.c
+++ b/fs/overlayfs/file.c
@@ -13,6 +13,7 @@
#include <linux/security.h>
#include <linux/mm.h>
#include <linux/fs.h>
+#include <linux/backing-file.h>
#include "overlayfs.h"
#include "../internal.h" /* for sb_init_dio_done_wq */
diff --git a/include/linux/backing-file.h b/include/linux/backing-file.h
new file mode 100644
index 000000000000..55c9e804f780
--- /dev/null
+++ b/include/linux/backing-file.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Common helpers for stackable filesystems and backing files.
+ *
+ * Copyright (C) 2023 CTERA Networks.
+ */
+
+#ifndef _LINUX_BACKING_FILE_H
+#define _LINUX_BACKING_FILE_H
+
+#include <linux/file.h>
+
+struct file *backing_file_open(const struct path *user_path, int flags,
+ const struct path *real_path,
+ const struct cred *cred);
+
+#endif /* _LINUX_BACKING_FILE_H */
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 900d0cd55b50..db5d07e6e02e 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2575,9 +2575,6 @@ struct file *dentry_open(const struct path *path, int flags,
const struct cred *creds);
struct file *dentry_create(const struct path *path, int flags, umode_t mode,
const struct cred *cred);
-struct file *backing_file_open(const struct path *user_path, int flags,
- const struct path *real_path,
- const struct cred *cred);
struct path *backing_file_user_path(struct file *f);
/*
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* [RFC][PATCH 2/4] fs: factor out backing_file_{read,write}_iter() helpers
2023-12-21 9:54 [RFC][PATCH 0/4] Intruduce stacking filesystem vfs helpers Amir Goldstein
2023-12-21 9:54 ` [RFC][PATCH 1/4] fs: prepare for stackable filesystems backing file helpers Amir Goldstein
@ 2023-12-21 9:54 ` Amir Goldstein
2023-12-21 9:54 ` [RFC][PATCH 3/4] fs: factor out backing_file_splice_{read,write}() helpers Amir Goldstein
` (2 subsequent siblings)
4 siblings, 0 replies; 13+ messages in thread
From: Amir Goldstein @ 2023-12-21 9:54 UTC (permalink / raw)
To: Christian Brauner, Miklos Szeredi; +Cc: linux-fsdevel, linux-unionfs
Overlayfs submits files io to backing files on other filesystems.
Factor out some common helpers to perform io to backing files, into
fs/backing-file.c.
Suggested-by: Miklos Szeredi <miklos@szeredi.hu>
Link: https://lore.kernel.org/r/CAJfpeguhmZbjP3JLqtUy0AdWaHOkAPWeP827BBWwRFEAUgnUcQ@mail.gmail.com
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
fs/backing-file.c | 204 +++++++++++++++++++++++++++++++++++
fs/overlayfs/file.c | 188 +++-----------------------------
fs/overlayfs/overlayfs.h | 8 +-
fs/overlayfs/super.c | 11 +-
include/linux/backing-file.h | 15 +++
5 files changed, 241 insertions(+), 185 deletions(-)
diff --git a/fs/backing-file.c b/fs/backing-file.c
index 04b33036f709..c1976ef5c210 100644
--- a/fs/backing-file.c
+++ b/fs/backing-file.c
@@ -2,6 +2,9 @@
/*
* Common helpers for stackable filesystems and backing files.
*
+ * Forked from fs/overlayfs/file.c.
+ *
+ * Copyright (C) 2017 Red Hat, Inc.
* Copyright (C) 2023 CTERA Networks.
*/
@@ -46,3 +49,204 @@ struct file *backing_file_open(const struct path *user_path, int flags,
return f;
}
EXPORT_SYMBOL_GPL(backing_file_open);
+
+struct backing_aio {
+ struct kiocb iocb;
+ refcount_t ref;
+ struct kiocb *orig_iocb;
+ /* used for aio completion */
+ void (*end_write)(struct file *);
+ struct work_struct work;
+ long res;
+};
+
+static struct kmem_cache *backing_aio_cachep;
+
+#define BACKING_IOCB_MASK \
+ (IOCB_NOWAIT | IOCB_HIPRI | IOCB_DSYNC | IOCB_SYNC | IOCB_APPEND)
+
+static rwf_t iocb_to_rw_flags(int flags)
+{
+ return (__force rwf_t)(flags & BACKING_IOCB_MASK);
+}
+
+static void backing_aio_put(struct backing_aio *aio)
+{
+ if (refcount_dec_and_test(&aio->ref)) {
+ fput(aio->iocb.ki_filp);
+ kmem_cache_free(backing_aio_cachep, aio);
+ }
+}
+
+static void backing_aio_cleanup(struct backing_aio *aio, long res)
+{
+ struct kiocb *iocb = &aio->iocb;
+ struct kiocb *orig_iocb = aio->orig_iocb;
+
+ if (aio->end_write)
+ aio->end_write(orig_iocb->ki_filp);
+
+ orig_iocb->ki_pos = iocb->ki_pos;
+ backing_aio_put(aio);
+}
+
+static void backing_aio_rw_complete(struct kiocb *iocb, long res)
+{
+ struct backing_aio *aio = container_of(iocb, struct backing_aio, iocb);
+ struct kiocb *orig_iocb = aio->orig_iocb;
+
+ if (iocb->ki_flags & IOCB_WRITE)
+ kiocb_end_write(iocb);
+
+ backing_aio_cleanup(aio, res);
+ orig_iocb->ki_complete(orig_iocb, res);
+}
+
+static void backing_aio_complete_work(struct work_struct *work)
+{
+ struct backing_aio *aio = container_of(work, struct backing_aio, work);
+
+ backing_aio_rw_complete(&aio->iocb, aio->res);
+}
+
+static void backing_aio_queue_completion(struct kiocb *iocb, long res)
+{
+ struct backing_aio *aio = container_of(iocb, struct backing_aio, iocb);
+
+ /*
+ * Punt to a work queue to serialize updates of mtime/size.
+ */
+ aio->res = res;
+ INIT_WORK(&aio->work, backing_aio_complete_work);
+ queue_work(file_inode(aio->orig_iocb->ki_filp)->i_sb->s_dio_done_wq,
+ &aio->work);
+}
+
+static int backing_aio_init_wq(struct kiocb *iocb)
+{
+ struct super_block *sb = file_inode(iocb->ki_filp)->i_sb;
+
+ if (sb->s_dio_done_wq)
+ return 0;
+
+ return sb_init_dio_done_wq(sb);
+}
+
+
+ssize_t backing_file_read_iter(struct file *file, struct iov_iter *iter,
+ struct kiocb *iocb, int flags,
+ struct backing_file_ctx *ctx)
+{
+ struct backing_aio *aio = NULL;
+ const struct cred *old_cred;
+ ssize_t ret;
+
+ if (!iov_iter_count(iter))
+ return 0;
+
+ if (iocb->ki_flags & IOCB_DIRECT &&
+ !(file->f_mode & FMODE_CAN_ODIRECT))
+ return -EINVAL;
+
+ old_cred = override_creds(ctx->cred);
+ if (is_sync_kiocb(iocb)) {
+ rwf_t rwf = iocb_to_rw_flags(flags);
+
+ ret = vfs_iter_read(file, iter, &iocb->ki_pos, rwf);
+ } else {
+ ret = -ENOMEM;
+ aio = kmem_cache_zalloc(backing_aio_cachep, GFP_KERNEL);
+ if (!aio)
+ goto out;
+
+ aio->orig_iocb = iocb;
+ kiocb_clone(&aio->iocb, iocb, get_file(file));
+ aio->iocb.ki_complete = backing_aio_rw_complete;
+ refcount_set(&aio->ref, 2);
+ ret = vfs_iocb_iter_read(file, &aio->iocb, iter);
+ backing_aio_put(aio);
+ if (ret != -EIOCBQUEUED)
+ backing_aio_cleanup(aio, ret);
+ }
+out:
+ revert_creds(old_cred);
+
+ if (ctx->accessed)
+ ctx->accessed(ctx->user_file);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(backing_file_read_iter);
+
+ssize_t backing_file_write_iter(struct file *file, struct iov_iter *iter,
+ struct kiocb *iocb, int flags,
+ struct backing_file_ctx *ctx)
+{
+ const struct cred *old_cred;
+ ssize_t ret;
+
+ if (!iov_iter_count(iter))
+ return 0;
+
+ ret = file_remove_privs(ctx->user_file);
+ if (ret)
+ return ret;
+
+ if (iocb->ki_flags & IOCB_DIRECT &&
+ !(file->f_mode & FMODE_CAN_ODIRECT))
+ return -EINVAL;
+
+ /*
+ * Stacked filesystems don't support deferred completions, don't copy
+ * this property in case it is set by the issuer.
+ */
+ flags &= ~IOCB_DIO_CALLER_COMP;
+
+ old_cred = override_creds(ctx->cred);
+ if (is_sync_kiocb(iocb)) {
+ rwf_t rwf = iocb_to_rw_flags(flags);
+
+ ret = vfs_iter_write(file, iter, &iocb->ki_pos, rwf);
+ if (ctx->end_write)
+ ctx->end_write(ctx->user_file);
+ } else {
+ struct backing_aio *aio;
+
+ ret = backing_aio_init_wq(iocb);
+ if (ret)
+ goto out;
+
+ ret = -ENOMEM;
+ aio = kmem_cache_zalloc(backing_aio_cachep, GFP_KERNEL);
+ if (!aio)
+ goto out;
+
+ aio->orig_iocb = iocb;
+ aio->end_write = ctx->end_write;
+ kiocb_clone(&aio->iocb, iocb, get_file(file));
+ aio->iocb.ki_flags = flags;
+ aio->iocb.ki_complete = backing_aio_queue_completion;
+ refcount_set(&aio->ref, 2);
+ ret = vfs_iocb_iter_write(file, &aio->iocb, iter);
+ backing_aio_put(aio);
+ if (ret != -EIOCBQUEUED)
+ backing_aio_cleanup(aio, ret);
+ }
+out:
+ revert_creds(old_cred);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(backing_file_write_iter);
+
+static int __init backing_aio_init(void)
+{
+ backing_aio_cachep = kmem_cache_create("backing_aio",
+ sizeof(struct backing_aio),
+ 0, SLAB_HWCACHE_ALIGN, NULL);
+ if (!backing_aio_cachep)
+ return -ENOMEM;
+
+ return 0;
+}
+fs_initcall(backing_aio_init);
diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
index a6da3eaf6d4f..1b578cb27a26 100644
--- a/fs/overlayfs/file.c
+++ b/fs/overlayfs/file.c
@@ -16,19 +16,6 @@
#include <linux/backing-file.h>
#include "overlayfs.h"
-#include "../internal.h" /* for sb_init_dio_done_wq */
-
-struct ovl_aio_req {
- struct kiocb iocb;
- refcount_t ref;
- struct kiocb *orig_iocb;
- /* used for aio completion */
- struct work_struct work;
- long res;
-};
-
-static struct kmem_cache *ovl_aio_request_cachep;
-
static char ovl_whatisit(struct inode *inode, struct inode *realinode)
{
if (realinode != ovl_inode_upper(inode))
@@ -275,84 +262,16 @@ static void ovl_file_accessed(struct file *file)
touch_atime(&file->f_path);
}
-#define OVL_IOCB_MASK \
- (IOCB_NOWAIT | IOCB_HIPRI | IOCB_DSYNC | IOCB_SYNC | IOCB_APPEND)
-
-static rwf_t iocb_to_rw_flags(int flags)
-{
- return (__force rwf_t)(flags & OVL_IOCB_MASK);
-}
-
-static inline void ovl_aio_put(struct ovl_aio_req *aio_req)
-{
- if (refcount_dec_and_test(&aio_req->ref)) {
- fput(aio_req->iocb.ki_filp);
- kmem_cache_free(ovl_aio_request_cachep, aio_req);
- }
-}
-
-static void ovl_aio_cleanup_handler(struct ovl_aio_req *aio_req)
-{
- struct kiocb *iocb = &aio_req->iocb;
- struct kiocb *orig_iocb = aio_req->orig_iocb;
-
- if (iocb->ki_flags & IOCB_WRITE)
- ovl_file_modified(orig_iocb->ki_filp);
-
- orig_iocb->ki_pos = iocb->ki_pos;
- ovl_aio_put(aio_req);
-}
-
-static void ovl_aio_rw_complete(struct kiocb *iocb, long res)
-{
- struct ovl_aio_req *aio_req = container_of(iocb,
- struct ovl_aio_req, iocb);
- struct kiocb *orig_iocb = aio_req->orig_iocb;
-
- if (iocb->ki_flags & IOCB_WRITE)
- kiocb_end_write(iocb);
-
- ovl_aio_cleanup_handler(aio_req);
- orig_iocb->ki_complete(orig_iocb, res);
-}
-
-static void ovl_aio_complete_work(struct work_struct *work)
-{
- struct ovl_aio_req *aio_req = container_of(work,
- struct ovl_aio_req, work);
-
- ovl_aio_rw_complete(&aio_req->iocb, aio_req->res);
-}
-
-static void ovl_aio_queue_completion(struct kiocb *iocb, long res)
-{
- struct ovl_aio_req *aio_req = container_of(iocb,
- struct ovl_aio_req, iocb);
- struct kiocb *orig_iocb = aio_req->orig_iocb;
-
- /*
- * Punt to a work queue to serialize updates of mtime/size.
- */
- aio_req->res = res;
- INIT_WORK(&aio_req->work, ovl_aio_complete_work);
- queue_work(file_inode(orig_iocb->ki_filp)->i_sb->s_dio_done_wq,
- &aio_req->work);
-}
-
-static int ovl_init_aio_done_wq(struct super_block *sb)
-{
- if (sb->s_dio_done_wq)
- return 0;
-
- return sb_init_dio_done_wq(sb);
-}
-
static ssize_t ovl_read_iter(struct kiocb *iocb, struct iov_iter *iter)
{
struct file *file = iocb->ki_filp;
struct fd real;
- const struct cred *old_cred;
ssize_t ret;
+ struct backing_file_ctx ctx = {
+ .cred = ovl_creds(file_inode(file)->i_sb),
+ .user_file = file,
+ .accessed = ovl_file_accessed,
+ };
if (!iov_iter_count(iter))
return 0;
@@ -361,37 +280,8 @@ static ssize_t ovl_read_iter(struct kiocb *iocb, struct iov_iter *iter)
if (ret)
return ret;
- ret = -EINVAL;
- if (iocb->ki_flags & IOCB_DIRECT &&
- !(real.file->f_mode & FMODE_CAN_ODIRECT))
- goto out_fdput;
-
- old_cred = ovl_override_creds(file_inode(file)->i_sb);
- if (is_sync_kiocb(iocb)) {
- rwf_t rwf = iocb_to_rw_flags(iocb->ki_flags);
-
- ret = vfs_iter_read(real.file, iter, &iocb->ki_pos, rwf);
- } else {
- struct ovl_aio_req *aio_req;
-
- ret = -ENOMEM;
- aio_req = kmem_cache_zalloc(ovl_aio_request_cachep, GFP_KERNEL);
- if (!aio_req)
- goto out;
-
- aio_req->orig_iocb = iocb;
- kiocb_clone(&aio_req->iocb, iocb, get_file(real.file));
- aio_req->iocb.ki_complete = ovl_aio_rw_complete;
- refcount_set(&aio_req->ref, 2);
- ret = vfs_iocb_iter_read(real.file, &aio_req->iocb, iter);
- ovl_aio_put(aio_req);
- if (ret != -EIOCBQUEUED)
- ovl_aio_cleanup_handler(aio_req);
- }
-out:
- revert_creds(old_cred);
- ovl_file_accessed(file);
-out_fdput:
+ ret = backing_file_read_iter(real.file, iter, iocb, iocb->ki_flags,
+ &ctx);
fdput(real);
return ret;
@@ -402,9 +292,13 @@ static ssize_t ovl_write_iter(struct kiocb *iocb, struct iov_iter *iter)
struct file *file = iocb->ki_filp;
struct inode *inode = file_inode(file);
struct fd real;
- const struct cred *old_cred;
ssize_t ret;
int ifl = iocb->ki_flags;
+ struct backing_file_ctx ctx = {
+ .cred = ovl_creds(inode->i_sb),
+ .user_file = file,
+ .end_write = ovl_file_modified,
+ };
if (!iov_iter_count(iter))
return 0;
@@ -412,19 +306,11 @@ static ssize_t ovl_write_iter(struct kiocb *iocb, struct iov_iter *iter)
inode_lock(inode);
/* Update mode */
ovl_copyattr(inode);
- ret = file_remove_privs(file);
- if (ret)
- goto out_unlock;
ret = ovl_real_fdget(file, &real);
if (ret)
goto out_unlock;
- ret = -EINVAL;
- if (iocb->ki_flags & IOCB_DIRECT &&
- !(real.file->f_mode & FMODE_CAN_ODIRECT))
- goto out_fdput;
-
if (!ovl_should_sync(OVL_FS(inode->i_sb)))
ifl &= ~(IOCB_DSYNC | IOCB_SYNC);
@@ -433,39 +319,7 @@ static ssize_t ovl_write_iter(struct kiocb *iocb, struct iov_iter *iter)
* this property in case it is set by the issuer.
*/
ifl &= ~IOCB_DIO_CALLER_COMP;
-
- old_cred = ovl_override_creds(file_inode(file)->i_sb);
- if (is_sync_kiocb(iocb)) {
- rwf_t rwf = iocb_to_rw_flags(ifl);
-
- ret = vfs_iter_write(real.file, iter, &iocb->ki_pos, rwf);
- /* Update size */
- ovl_file_modified(file);
- } else {
- struct ovl_aio_req *aio_req;
-
- ret = ovl_init_aio_done_wq(inode->i_sb);
- if (ret)
- goto out;
-
- ret = -ENOMEM;
- aio_req = kmem_cache_zalloc(ovl_aio_request_cachep, GFP_KERNEL);
- if (!aio_req)
- goto out;
-
- aio_req->orig_iocb = iocb;
- kiocb_clone(&aio_req->iocb, iocb, get_file(real.file));
- aio_req->iocb.ki_flags = ifl;
- aio_req->iocb.ki_complete = ovl_aio_queue_completion;
- refcount_set(&aio_req->ref, 2);
- ret = vfs_iocb_iter_write(real.file, &aio_req->iocb, iter);
- ovl_aio_put(aio_req);
- if (ret != -EIOCBQUEUED)
- ovl_aio_cleanup_handler(aio_req);
- }
-out:
- revert_creds(old_cred);
-out_fdput:
+ ret = backing_file_write_iter(real.file, iter, iocb, ifl, &ctx);
fdput(real);
out_unlock:
@@ -777,19 +631,3 @@ const struct file_operations ovl_file_operations = {
.copy_file_range = ovl_copy_file_range,
.remap_file_range = ovl_remap_file_range,
};
-
-int __init ovl_aio_request_cache_init(void)
-{
- ovl_aio_request_cachep = kmem_cache_create("ovl_aio_req",
- sizeof(struct ovl_aio_req),
- 0, SLAB_HWCACHE_ALIGN, NULL);
- if (!ovl_aio_request_cachep)
- return -ENOMEM;
-
- return 0;
-}
-
-void ovl_aio_request_cache_destroy(void)
-{
- kmem_cache_destroy(ovl_aio_request_cachep);
-}
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 05c3dd597fa8..5ba11eb43767 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -425,6 +425,12 @@ int ovl_want_write(struct dentry *dentry);
void ovl_drop_write(struct dentry *dentry);
struct dentry *ovl_workdir(struct dentry *dentry);
const struct cred *ovl_override_creds(struct super_block *sb);
+
+static inline const struct cred *ovl_creds(struct super_block *sb)
+{
+ return OVL_FS(sb)->creator_cred;
+}
+
int ovl_can_decode_fh(struct super_block *sb);
struct dentry *ovl_indexdir(struct super_block *sb);
bool ovl_index_all(struct super_block *sb);
@@ -837,8 +843,6 @@ struct dentry *ovl_create_temp(struct ovl_fs *ofs, struct dentry *workdir,
/* file.c */
extern const struct file_operations ovl_file_operations;
-int __init ovl_aio_request_cache_init(void);
-void ovl_aio_request_cache_destroy(void);
int ovl_real_fileattr_get(const struct path *realpath, struct fileattr *fa);
int ovl_real_fileattr_set(const struct path *realpath, struct fileattr *fa);
int ovl_fileattr_get(struct dentry *dentry, struct fileattr *fa);
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 1d6b98dd9003..8ac4726795ce 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -1500,14 +1500,10 @@ static int __init ovl_init(void)
if (ovl_inode_cachep == NULL)
return -ENOMEM;
- err = ovl_aio_request_cache_init();
- if (!err) {
- err = register_filesystem(&ovl_fs_type);
- if (!err)
- return 0;
+ err = register_filesystem(&ovl_fs_type);
+ if (!err)
+ return 0;
- ovl_aio_request_cache_destroy();
- }
kmem_cache_destroy(ovl_inode_cachep);
return err;
@@ -1523,7 +1519,6 @@ static void __exit ovl_exit(void)
*/
rcu_barrier();
kmem_cache_destroy(ovl_inode_cachep);
- ovl_aio_request_cache_destroy();
}
module_init(ovl_init);
diff --git a/include/linux/backing-file.h b/include/linux/backing-file.h
index 55c9e804f780..0648d548a418 100644
--- a/include/linux/backing-file.h
+++ b/include/linux/backing-file.h
@@ -9,9 +9,24 @@
#define _LINUX_BACKING_FILE_H
#include <linux/file.h>
+#include <linux/uio.h>
+#include <linux/fs.h>
+
+struct backing_file_ctx {
+ const struct cred *cred;
+ struct file *user_file;
+ void (*accessed)(struct file *);
+ void (*end_write)(struct file *);
+};
struct file *backing_file_open(const struct path *user_path, int flags,
const struct path *real_path,
const struct cred *cred);
+ssize_t backing_file_read_iter(struct file *file, struct iov_iter *iter,
+ struct kiocb *iocb, int flags,
+ struct backing_file_ctx *ctx);
+ssize_t backing_file_write_iter(struct file *file, struct iov_iter *iter,
+ struct kiocb *iocb, int flags,
+ struct backing_file_ctx *ctx);
#endif /* _LINUX_BACKING_FILE_H */
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* [RFC][PATCH 3/4] fs: factor out backing_file_splice_{read,write}() helpers
2023-12-21 9:54 [RFC][PATCH 0/4] Intruduce stacking filesystem vfs helpers Amir Goldstein
2023-12-21 9:54 ` [RFC][PATCH 1/4] fs: prepare for stackable filesystems backing file helpers Amir Goldstein
2023-12-21 9:54 ` [RFC][PATCH 2/4] fs: factor out backing_file_{read,write}_iter() helpers Amir Goldstein
@ 2023-12-21 9:54 ` Amir Goldstein
2023-12-21 9:54 ` [RFC][PATCH 4/4] fs: factor out backing_file_mmap() helper Amir Goldstein
2023-12-22 12:44 ` [RFC][PATCH 0/4] Intruduce stacking filesystem vfs helpers Christian Brauner
4 siblings, 0 replies; 13+ messages in thread
From: Amir Goldstein @ 2023-12-21 9:54 UTC (permalink / raw)
To: Christian Brauner, Miklos Szeredi; +Cc: linux-fsdevel, linux-unionfs
There is not much in those helpers, but it makes sense to have them
logically next to the backing_file_{read,write}_iter() helpers as they
may grow more common logic in the future.
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
fs/backing-file.c | 45 ++++++++++++++++++++++++++++++++++++
fs/overlayfs/file.c | 33 +++++++++++---------------
include/linux/backing-file.h | 8 +++++++
3 files changed, 66 insertions(+), 20 deletions(-)
diff --git a/fs/backing-file.c b/fs/backing-file.c
index c1976ef5c210..46488de821a2 100644
--- a/fs/backing-file.c
+++ b/fs/backing-file.c
@@ -10,6 +10,7 @@
#include <linux/fs.h>
#include <linux/backing-file.h>
+#include <linux/splice.h>
#include "internal.h"
@@ -239,6 +240,50 @@ ssize_t backing_file_write_iter(struct file *file, struct iov_iter *iter,
}
EXPORT_SYMBOL_GPL(backing_file_write_iter);
+ssize_t backing_file_splice_read(struct file *in, loff_t *ppos,
+ struct pipe_inode_info *pipe, size_t len,
+ unsigned int flags,
+ struct backing_file_ctx *ctx)
+{
+ const struct cred *old_cred;
+ ssize_t ret;
+
+ old_cred = override_creds(ctx->cred);
+ ret = vfs_splice_read(in, ppos, pipe, len, flags);
+ revert_creds(old_cred);
+
+ if (ctx->accessed)
+ ctx->accessed(ctx->user_file);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(backing_file_splice_read);
+
+ssize_t backing_file_splice_write(struct pipe_inode_info *pipe,
+ struct file *out, loff_t *ppos, size_t len,
+ unsigned int flags,
+ struct backing_file_ctx *ctx)
+{
+ const struct cred *old_cred;
+ ssize_t ret;
+
+ ret = file_remove_privs(ctx->user_file);
+ if (ret)
+ return ret;
+
+ old_cred = override_creds(ctx->cred);
+ file_start_write(out);
+ ret = iter_file_splice_write(pipe, out, ppos, len, flags);
+ file_end_write(out);
+ revert_creds(old_cred);
+
+ if (ctx->end_write)
+ ctx->end_write(ctx->user_file);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(backing_file_splice_write);
+
static int __init backing_aio_init(void)
{
backing_aio_cachep = kmem_cache_create("backing_aio",
diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
index 1b578cb27a26..69b52d2f9c74 100644
--- a/fs/overlayfs/file.c
+++ b/fs/overlayfs/file.c
@@ -9,7 +9,6 @@
#include <linux/xattr.h>
#include <linux/uio.h>
#include <linux/uaccess.h>
-#include <linux/splice.h>
#include <linux/security.h>
#include <linux/mm.h>
#include <linux/fs.h>
@@ -332,20 +331,21 @@ static ssize_t ovl_splice_read(struct file *in, loff_t *ppos,
struct pipe_inode_info *pipe, size_t len,
unsigned int flags)
{
- const struct cred *old_cred;
struct fd real;
ssize_t ret;
+ struct backing_file_ctx ctx = {
+ .cred = ovl_creds(file_inode(in)->i_sb),
+ .user_file = in,
+ .accessed = ovl_file_accessed,
+ };
ret = ovl_real_fdget(in, &real);
if (ret)
return ret;
- old_cred = ovl_override_creds(file_inode(in)->i_sb);
- ret = vfs_splice_read(real.file, ppos, pipe, len, flags);
- revert_creds(old_cred);
- ovl_file_accessed(in);
-
+ ret = backing_file_splice_read(real.file, ppos, pipe, len, flags, &ctx);
fdput(real);
+
return ret;
}
@@ -361,30 +361,23 @@ static ssize_t ovl_splice_write(struct pipe_inode_info *pipe, struct file *out,
loff_t *ppos, size_t len, unsigned int flags)
{
struct fd real;
- const struct cred *old_cred;
struct inode *inode = file_inode(out);
ssize_t ret;
+ struct backing_file_ctx ctx = {
+ .cred = ovl_creds(inode->i_sb),
+ .user_file = out,
+ .end_write = ovl_file_modified,
+ };
inode_lock(inode);
/* Update mode */
ovl_copyattr(inode);
- ret = file_remove_privs(out);
- if (ret)
- goto out_unlock;
ret = ovl_real_fdget(out, &real);
if (ret)
goto out_unlock;
- old_cred = ovl_override_creds(inode->i_sb);
- file_start_write(real.file);
-
- ret = iter_file_splice_write(pipe, real.file, ppos, len, flags);
-
- file_end_write(real.file);
- /* Update size */
- ovl_file_modified(out);
- revert_creds(old_cred);
+ ret = backing_file_splice_write(pipe, real.file, ppos, len, flags, &ctx);
fdput(real);
out_unlock:
diff --git a/include/linux/backing-file.h b/include/linux/backing-file.h
index 0648d548a418..0546d5b1c9f5 100644
--- a/include/linux/backing-file.h
+++ b/include/linux/backing-file.h
@@ -28,5 +28,13 @@ ssize_t backing_file_read_iter(struct file *file, struct iov_iter *iter,
ssize_t backing_file_write_iter(struct file *file, struct iov_iter *iter,
struct kiocb *iocb, int flags,
struct backing_file_ctx *ctx);
+ssize_t backing_file_splice_read(struct file *in, loff_t *ppos,
+ struct pipe_inode_info *pipe, size_t len,
+ unsigned int flags,
+ struct backing_file_ctx *ctx);
+ssize_t backing_file_splice_write(struct pipe_inode_info *pipe,
+ struct file *out, loff_t *ppos, size_t len,
+ unsigned int flags,
+ struct backing_file_ctx *ctx);
#endif /* _LINUX_BACKING_FILE_H */
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* [RFC][PATCH 4/4] fs: factor out backing_file_mmap() helper
2023-12-21 9:54 [RFC][PATCH 0/4] Intruduce stacking filesystem vfs helpers Amir Goldstein
` (2 preceding siblings ...)
2023-12-21 9:54 ` [RFC][PATCH 3/4] fs: factor out backing_file_splice_{read,write}() helpers Amir Goldstein
@ 2023-12-21 9:54 ` Amir Goldstein
2023-12-22 12:54 ` Christian Brauner
2023-12-22 12:44 ` [RFC][PATCH 0/4] Intruduce stacking filesystem vfs helpers Christian Brauner
4 siblings, 1 reply; 13+ messages in thread
From: Amir Goldstein @ 2023-12-21 9:54 UTC (permalink / raw)
To: Christian Brauner, Miklos Szeredi; +Cc: linux-fsdevel, linux-unionfs
Assert that the file object is allocated in a backing_file container
so that file_user_path() could be used to display the user path and
not the backing file's path in /proc/<pid>/maps.
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
fs/backing-file.c | 27 +++++++++++++++++++++++++++
fs/overlayfs/file.c | 23 ++++++-----------------
include/linux/backing-file.h | 2 ++
3 files changed, 35 insertions(+), 17 deletions(-)
diff --git a/fs/backing-file.c b/fs/backing-file.c
index 46488de821a2..1ad8c252ec8d 100644
--- a/fs/backing-file.c
+++ b/fs/backing-file.c
@@ -11,6 +11,7 @@
#include <linux/fs.h>
#include <linux/backing-file.h>
#include <linux/splice.h>
+#include <linux/mm.h>
#include "internal.h"
@@ -284,6 +285,32 @@ ssize_t backing_file_splice_write(struct pipe_inode_info *pipe,
}
EXPORT_SYMBOL_GPL(backing_file_splice_write);
+int backing_file_mmap(struct file *file, struct vm_area_struct *vma,
+ struct backing_file_ctx *ctx)
+{
+ const struct cred *old_cred;
+ int ret;
+
+ if (WARN_ON_ONCE(!(file->f_mode & FMODE_BACKING)) ||
+ WARN_ON_ONCE(ctx->user_file != vma->vm_file))
+ return -EIO;
+
+ if (!file->f_op->mmap)
+ return -ENODEV;
+
+ vma_set_file(vma, file);
+
+ old_cred = override_creds(ctx->cred);
+ ret = call_mmap(vma->vm_file, vma);
+ revert_creds(old_cred);
+
+ if (ctx->accessed)
+ ctx->accessed(ctx->user_file);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(backing_file_mmap);
+
static int __init backing_aio_init(void)
{
backing_aio_cachep = kmem_cache_create("backing_aio",
diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
index 69b52d2f9c74..05536964d37f 100644
--- a/fs/overlayfs/file.c
+++ b/fs/overlayfs/file.c
@@ -10,7 +10,6 @@
#include <linux/uio.h>
#include <linux/uaccess.h>
#include <linux/security.h>
-#include <linux/mm.h>
#include <linux/fs.h>
#include <linux/backing-file.h>
#include "overlayfs.h"
@@ -415,23 +414,13 @@ static int ovl_fsync(struct file *file, loff_t start, loff_t end, int datasync)
static int ovl_mmap(struct file *file, struct vm_area_struct *vma)
{
struct file *realfile = file->private_data;
- const struct cred *old_cred;
- int ret;
-
- if (!realfile->f_op->mmap)
- return -ENODEV;
-
- if (WARN_ON(file != vma->vm_file))
- return -EIO;
-
- vma_set_file(vma, realfile);
-
- old_cred = ovl_override_creds(file_inode(file)->i_sb);
- ret = call_mmap(vma->vm_file, vma);
- revert_creds(old_cred);
- ovl_file_accessed(file);
+ struct backing_file_ctx ctx = {
+ .cred = ovl_creds(file_inode(file)->i_sb),
+ .user_file = file,
+ .accessed = ovl_file_accessed,
+ };
- return ret;
+ return backing_file_mmap(realfile, vma, &ctx);
}
static long ovl_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
diff --git a/include/linux/backing-file.h b/include/linux/backing-file.h
index 0546d5b1c9f5..3f1fe1774f1b 100644
--- a/include/linux/backing-file.h
+++ b/include/linux/backing-file.h
@@ -36,5 +36,7 @@ ssize_t backing_file_splice_write(struct pipe_inode_info *pipe,
struct file *out, loff_t *ppos, size_t len,
unsigned int flags,
struct backing_file_ctx *ctx);
+int backing_file_mmap(struct file *file, struct vm_area_struct *vma,
+ struct backing_file_ctx *ctx);
#endif /* _LINUX_BACKING_FILE_H */
--
2.34.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [RFC][PATCH 4/4] fs: factor out backing_file_mmap() helper
2023-12-21 9:54 ` [RFC][PATCH 4/4] fs: factor out backing_file_mmap() helper Amir Goldstein
@ 2023-12-22 12:54 ` Christian Brauner
2023-12-23 6:54 ` Amir Goldstein
0 siblings, 1 reply; 13+ messages in thread
From: Christian Brauner @ 2023-12-22 12:54 UTC (permalink / raw)
To: Amir Goldstein; +Cc: Miklos Szeredi, linux-fsdevel, linux-unionfs
On Thu, Dec 21, 2023 at 11:54:10AM +0200, Amir Goldstein wrote:
> Assert that the file object is allocated in a backing_file container
> so that file_user_path() could be used to display the user path and
> not the backing file's path in /proc/<pid>/maps.
>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
> fs/backing-file.c | 27 +++++++++++++++++++++++++++
> fs/overlayfs/file.c | 23 ++++++-----------------
> include/linux/backing-file.h | 2 ++
> 3 files changed, 35 insertions(+), 17 deletions(-)
>
> diff --git a/fs/backing-file.c b/fs/backing-file.c
> index 46488de821a2..1ad8c252ec8d 100644
> --- a/fs/backing-file.c
> +++ b/fs/backing-file.c
> @@ -11,6 +11,7 @@
> #include <linux/fs.h>
> #include <linux/backing-file.h>
> #include <linux/splice.h>
> +#include <linux/mm.h>
>
> #include "internal.h"
>
> @@ -284,6 +285,32 @@ ssize_t backing_file_splice_write(struct pipe_inode_info *pipe,
> }
> EXPORT_SYMBOL_GPL(backing_file_splice_write);
>
> +int backing_file_mmap(struct file *file, struct vm_area_struct *vma,
> + struct backing_file_ctx *ctx)
> +{
> + const struct cred *old_cred;
> + int ret;
> +
> + if (WARN_ON_ONCE(!(file->f_mode & FMODE_BACKING)) ||
Couldn't that WARN_ON_ONCE() be in every one of these helpers in this
series? IOW, when would you ever want to use a backing_file_*() helper
on a non-backing file?
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [RFC][PATCH 4/4] fs: factor out backing_file_mmap() helper
2023-12-22 12:54 ` Christian Brauner
@ 2023-12-23 6:54 ` Amir Goldstein
2023-12-23 6:56 ` Amir Goldstein
0 siblings, 1 reply; 13+ messages in thread
From: Amir Goldstein @ 2023-12-23 6:54 UTC (permalink / raw)
To: Christian Brauner; +Cc: Miklos Szeredi, linux-fsdevel, linux-unionfs
On Fri, Dec 22, 2023 at 2:54 PM Christian Brauner <brauner@kernel.org> wrote:
>
> On Thu, Dec 21, 2023 at 11:54:10AM +0200, Amir Goldstein wrote:
> > Assert that the file object is allocated in a backing_file container
> > so that file_user_path() could be used to display the user path and
> > not the backing file's path in /proc/<pid>/maps.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> > fs/backing-file.c | 27 +++++++++++++++++++++++++++
> > fs/overlayfs/file.c | 23 ++++++-----------------
> > include/linux/backing-file.h | 2 ++
> > 3 files changed, 35 insertions(+), 17 deletions(-)
> >
> > diff --git a/fs/backing-file.c b/fs/backing-file.c
> > index 46488de821a2..1ad8c252ec8d 100644
> > --- a/fs/backing-file.c
> > +++ b/fs/backing-file.c
> > @@ -11,6 +11,7 @@
> > #include <linux/fs.h>
> > #include <linux/backing-file.h>
> > #include <linux/splice.h>
> > +#include <linux/mm.h>
> >
> > #include "internal.h"
> >
> > @@ -284,6 +285,32 @@ ssize_t backing_file_splice_write(struct pipe_inode_info *pipe,
> > }
> > EXPORT_SYMBOL_GPL(backing_file_splice_write);
> >
> > +int backing_file_mmap(struct file *file, struct vm_area_struct *vma,
> > + struct backing_file_ctx *ctx)
> > +{
> > + const struct cred *old_cred;
> > + int ret;
> > +
> > + if (WARN_ON_ONCE(!(file->f_mode & FMODE_BACKING)) ||
>
> Couldn't that WARN_ON_ONCE() be in every one of these helpers in this
> series? IOW, when would you ever want to use a backing_file_*() helper
> on a non-backing file?
AFAIK, the call chain below backing_file_splice*() and backing_file_*_iter()
helpers never end up accessing file_user_path() or assuming that fd of file
is installed in fd table, so there is no strong reason to enforce this with an
assertion.
We can do it for clarity of semantics, in case one of the call chains will
start assuming a struct backing_file in the future. WDIT?
Thanks,
Amir.
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [RFC][PATCH 4/4] fs: factor out backing_file_mmap() helper
2023-12-23 6:54 ` Amir Goldstein
@ 2023-12-23 6:56 ` Amir Goldstein
2023-12-23 13:04 ` Christian Brauner
0 siblings, 1 reply; 13+ messages in thread
From: Amir Goldstein @ 2023-12-23 6:56 UTC (permalink / raw)
To: Christian Brauner; +Cc: Miklos Szeredi, linux-fsdevel, linux-unionfs
On Sat, Dec 23, 2023 at 8:54 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Fri, Dec 22, 2023 at 2:54 PM Christian Brauner <brauner@kernel.org> wrote:
> >
> > On Thu, Dec 21, 2023 at 11:54:10AM +0200, Amir Goldstein wrote:
> > > Assert that the file object is allocated in a backing_file container
> > > so that file_user_path() could be used to display the user path and
> > > not the backing file's path in /proc/<pid>/maps.
> > >
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > ---
> > > fs/backing-file.c | 27 +++++++++++++++++++++++++++
> > > fs/overlayfs/file.c | 23 ++++++-----------------
> > > include/linux/backing-file.h | 2 ++
> > > 3 files changed, 35 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/fs/backing-file.c b/fs/backing-file.c
> > > index 46488de821a2..1ad8c252ec8d 100644
> > > --- a/fs/backing-file.c
> > > +++ b/fs/backing-file.c
> > > @@ -11,6 +11,7 @@
> > > #include <linux/fs.h>
> > > #include <linux/backing-file.h>
> > > #include <linux/splice.h>
> > > +#include <linux/mm.h>
> > >
> > > #include "internal.h"
> > >
> > > @@ -284,6 +285,32 @@ ssize_t backing_file_splice_write(struct pipe_inode_info *pipe,
> > > }
> > > EXPORT_SYMBOL_GPL(backing_file_splice_write);
> > >
> > > +int backing_file_mmap(struct file *file, struct vm_area_struct *vma,
> > > + struct backing_file_ctx *ctx)
> > > +{
> > > + const struct cred *old_cred;
> > > + int ret;
> > > +
> > > + if (WARN_ON_ONCE(!(file->f_mode & FMODE_BACKING)) ||
> >
> > Couldn't that WARN_ON_ONCE() be in every one of these helpers in this
> > series? IOW, when would you ever want to use a backing_file_*() helper
> > on a non-backing file?
>
> AFAIK, the call chain below backing_file_splice*() and backing_file_*_iter()
> helpers never end up accessing file_user_path() or assuming that fd of file
> is installed in fd table, so there is no strong reason to enforce this with an
> assertion.
>
> We can do it for clarity of semantics, in case one of the call chains will
> start assuming a struct backing_file in the future. WDIT?
Doh! WDYT?
Thanks,
Amir.
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [RFC][PATCH 4/4] fs: factor out backing_file_mmap() helper
2023-12-23 6:56 ` Amir Goldstein
@ 2023-12-23 13:04 ` Christian Brauner
2023-12-23 14:28 ` Amir Goldstein
0 siblings, 1 reply; 13+ messages in thread
From: Christian Brauner @ 2023-12-23 13:04 UTC (permalink / raw)
To: Amir Goldstein; +Cc: Miklos Szeredi, linux-fsdevel, linux-unionfs
On Sat, Dec 23, 2023 at 08:56:08AM +0200, Amir Goldstein wrote:
> On Sat, Dec 23, 2023 at 8:54 AM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Fri, Dec 22, 2023 at 2:54 PM Christian Brauner <brauner@kernel.org> wrote:
> > >
> > > On Thu, Dec 21, 2023 at 11:54:10AM +0200, Amir Goldstein wrote:
> > > > Assert that the file object is allocated in a backing_file container
> > > > so that file_user_path() could be used to display the user path and
> > > > not the backing file's path in /proc/<pid>/maps.
> > > >
> > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > > ---
> > > > fs/backing-file.c | 27 +++++++++++++++++++++++++++
> > > > fs/overlayfs/file.c | 23 ++++++-----------------
> > > > include/linux/backing-file.h | 2 ++
> > > > 3 files changed, 35 insertions(+), 17 deletions(-)
> > > >
> > > > diff --git a/fs/backing-file.c b/fs/backing-file.c
> > > > index 46488de821a2..1ad8c252ec8d 100644
> > > > --- a/fs/backing-file.c
> > > > +++ b/fs/backing-file.c
> > > > @@ -11,6 +11,7 @@
> > > > #include <linux/fs.h>
> > > > #include <linux/backing-file.h>
> > > > #include <linux/splice.h>
> > > > +#include <linux/mm.h>
> > > >
> > > > #include "internal.h"
> > > >
> > > > @@ -284,6 +285,32 @@ ssize_t backing_file_splice_write(struct pipe_inode_info *pipe,
> > > > }
> > > > EXPORT_SYMBOL_GPL(backing_file_splice_write);
> > > >
> > > > +int backing_file_mmap(struct file *file, struct vm_area_struct *vma,
> > > > + struct backing_file_ctx *ctx)
> > > > +{
> > > > + const struct cred *old_cred;
> > > > + int ret;
> > > > +
> > > > + if (WARN_ON_ONCE(!(file->f_mode & FMODE_BACKING)) ||
> > >
> > > Couldn't that WARN_ON_ONCE() be in every one of these helpers in this
> > > series? IOW, when would you ever want to use a backing_file_*() helper
> > > on a non-backing file?
> >
> > AFAIK, the call chain below backing_file_splice*() and backing_file_*_iter()
> > helpers never end up accessing file_user_path() or assuming that fd of file
> > is installed in fd table, so there is no strong reason to enforce this with an
> > assertion.
Yeah, but you do use an override_cred() call and you do have that
backing_file_ctx. It smells like a bug if anyone would pass in a
non-backing file.
> >
> > We can do it for clarity of semantics, in case one of the call chains will
> > start assuming a struct backing_file in the future. WDIT?
>
> Doh! WDYT?
I'd add it as the whole series is predicated on this being used for
backing files.
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [RFC][PATCH 4/4] fs: factor out backing_file_mmap() helper
2023-12-23 13:04 ` Christian Brauner
@ 2023-12-23 14:28 ` Amir Goldstein
0 siblings, 0 replies; 13+ messages in thread
From: Amir Goldstein @ 2023-12-23 14:28 UTC (permalink / raw)
To: Christian Brauner; +Cc: Miklos Szeredi, linux-fsdevel, linux-unionfs
On Sat, Dec 23, 2023 at 3:04 PM Christian Brauner <brauner@kernel.org> wrote:
>
> On Sat, Dec 23, 2023 at 08:56:08AM +0200, Amir Goldstein wrote:
> > On Sat, Dec 23, 2023 at 8:54 AM Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > On Fri, Dec 22, 2023 at 2:54 PM Christian Brauner <brauner@kernel.org> wrote:
> > > >
> > > > On Thu, Dec 21, 2023 at 11:54:10AM +0200, Amir Goldstein wrote:
> > > > > Assert that the file object is allocated in a backing_file container
> > > > > so that file_user_path() could be used to display the user path and
> > > > > not the backing file's path in /proc/<pid>/maps.
> > > > >
> > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > > > ---
> > > > > fs/backing-file.c | 27 +++++++++++++++++++++++++++
> > > > > fs/overlayfs/file.c | 23 ++++++-----------------
> > > > > include/linux/backing-file.h | 2 ++
> > > > > 3 files changed, 35 insertions(+), 17 deletions(-)
> > > > >
> > > > > diff --git a/fs/backing-file.c b/fs/backing-file.c
> > > > > index 46488de821a2..1ad8c252ec8d 100644
> > > > > --- a/fs/backing-file.c
> > > > > +++ b/fs/backing-file.c
> > > > > @@ -11,6 +11,7 @@
> > > > > #include <linux/fs.h>
> > > > > #include <linux/backing-file.h>
> > > > > #include <linux/splice.h>
> > > > > +#include <linux/mm.h>
> > > > >
> > > > > #include "internal.h"
> > > > >
> > > > > @@ -284,6 +285,32 @@ ssize_t backing_file_splice_write(struct pipe_inode_info *pipe,
> > > > > }
> > > > > EXPORT_SYMBOL_GPL(backing_file_splice_write);
> > > > >
> > > > > +int backing_file_mmap(struct file *file, struct vm_area_struct *vma,
> > > > > + struct backing_file_ctx *ctx)
> > > > > +{
> > > > > + const struct cred *old_cred;
> > > > > + int ret;
> > > > > +
> > > > > + if (WARN_ON_ONCE(!(file->f_mode & FMODE_BACKING)) ||
> > > >
> > > > Couldn't that WARN_ON_ONCE() be in every one of these helpers in this
> > > > series? IOW, when would you ever want to use a backing_file_*() helper
> > > > on a non-backing file?
> > >
> > > AFAIK, the call chain below backing_file_splice*() and backing_file_*_iter()
> > > helpers never end up accessing file_user_path() or assuming that fd of file
> > > is installed in fd table, so there is no strong reason to enforce this with an
> > > assertion.
>
> Yeah, but you do use an override_cred() call and you do have that
> backing_file_ctx. It smells like a bug if anyone would pass in a
> non-backing file.
>
> > >
> > > We can do it for clarity of semantics, in case one of the call chains will
> > > start assuming a struct backing_file in the future. WDIT?
> >
> > Doh! WDYT?
>
> I'd add it as the whole series is predicated on this being used for
> backing files.
Sure. Will do.
Thanks,
Amir.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC][PATCH 0/4] Intruduce stacking filesystem vfs helpers
2023-12-21 9:54 [RFC][PATCH 0/4] Intruduce stacking filesystem vfs helpers Amir Goldstein
` (3 preceding siblings ...)
2023-12-21 9:54 ` [RFC][PATCH 4/4] fs: factor out backing_file_mmap() helper Amir Goldstein
@ 2023-12-22 12:44 ` Christian Brauner
2023-12-23 8:07 ` Amir Goldstein
4 siblings, 1 reply; 13+ messages in thread
From: Christian Brauner @ 2023-12-22 12:44 UTC (permalink / raw)
To: Amir Goldstein; +Cc: Miklos Szeredi, linux-fsdevel, linux-unionfs
> If I do that, would you preffer to take these patches via the vfs tree
I would prefer if you:
* Add the vfs infrastructure stuff on top of what's in vfs.file.
There's also currently a conflict between this series and what's in there.
* Pull vfs.file into overlayfs.
* Port overlayfs to the new infrastructure.
io_uring already depends on vfs.file as well.
If this is straightforward I can include it in v6.8. The VFS prs will go
out the week before January 7.
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [RFC][PATCH 0/4] Intruduce stacking filesystem vfs helpers
2023-12-22 12:44 ` [RFC][PATCH 0/4] Intruduce stacking filesystem vfs helpers Christian Brauner
@ 2023-12-23 8:07 ` Amir Goldstein
2023-12-23 13:11 ` Christian Brauner
0 siblings, 1 reply; 13+ messages in thread
From: Amir Goldstein @ 2023-12-23 8:07 UTC (permalink / raw)
To: Christian Brauner; +Cc: Miklos Szeredi, linux-fsdevel, linux-unionfs
On Fri, Dec 22, 2023 at 2:44 PM Christian Brauner <brauner@kernel.org> wrote:
>
> > If I do that, would you preffer to take these patches via the vfs tree
>
> I would prefer if you:
>
> * Add the vfs infrastructure stuff on top of what's in vfs.file.
> There's also currently a conflict between this series and what's in there.
I did not notice any actual conflicts with vfs.file.
They do change the same files, but nothing that git can't handle.
Specifically, FMODE_BACKING was excepted from the fput()
changes, so also no logic changes that I noticed.
The only conflict I know of is with the vfs.rw branch,
the move of *_start_write() into *__iter_write(), therefore,
these patches are already based on top of vfs.rw.
I've just pushed branch backing_file rebased over both
vfs.rw and vfs.file to:
https://github.com/amir73il/linux/commits/backing_file
Started to run overlayfs tests to see if vfs.file has unforeseen impact
that I missed in review.
> * Pull vfs.file into overlayfs.
> * Port overlayfs to the new infrastructure.
>
Wait, do you mean add the backing_file_*() helpers
and only then convert overlayfs to use them?
I think that would be harder to review (also in retrospect)
so the "factor out ... helper" patches that move code from
overlayfs to fs/backing_file.c are easier to follow.
Or did you mean something else?
> io_uring already depends on vfs.file as well.
>
> If this is straightforward I can include it in v6.8. The VFS prs will go
> out the week before January 7.
Well, unless I misunderstood you, that was straightforward.
The only complexity is the order and dependency among the PRs.
If I am not mistaken, backing_file could be applied directly on top of
vfs.rw and sent right after it, or along with it (via your tree)?
If I am not mistaken, backing_file is independent of vfs.file, but surely
it could be sent after vfs.file.
The changes I currently have in overlayfs-next for 6.8 are very mild
and do not conflict with any of the aforementioned work.
If you prefer that I send the PR for backing_file via overlayfs tree,
I can do that, even in the second part of the merge window, after
vfs.file and vfs.rw are merged, but in that case, I would like to be
able to treat vfs.rw and stable from here on, so that I can pull it
into overlayfs-next and put backing_file to soak in linux-next.
Let me know how you want to deal with that.
Thanks,
Amir.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [RFC][PATCH 0/4] Intruduce stacking filesystem vfs helpers
2023-12-23 8:07 ` Amir Goldstein
@ 2023-12-23 13:11 ` Christian Brauner
0 siblings, 0 replies; 13+ messages in thread
From: Christian Brauner @ 2023-12-23 13:11 UTC (permalink / raw)
To: Amir Goldstein; +Cc: Miklos Szeredi, linux-fsdevel, linux-unionfs
On Sat, Dec 23, 2023 at 10:07:11AM +0200, Amir Goldstein wrote:
> On Fri, Dec 22, 2023 at 2:44 PM Christian Brauner <brauner@kernel.org> wrote:
> >
> > > If I do that, would you preffer to take these patches via the vfs tree
> >
> > I would prefer if you:
> >
> > * Add the vfs infrastructure stuff on top of what's in vfs.file.
> > There's also currently a conflict between this series and what's in there.
>
> I did not notice any actual conflicts with vfs.file.
> They do change the same files, but nothing that git can't handle.
> Specifically, FMODE_BACKING was excepted from the fput()
> changes, so also no logic changes that I noticed.
Huh, let me retry while writing this mail:
✓ [PATCH RFC 1/4] fs: prepare for stackable filesystems backing file helpers
+ Link: https://lore.kernel.org/r/20231221095410.801061-2-amir73il@gmail.com
+ Signed-off-by: Christian Brauner <brauner@kernel.org>
✓ [PATCH RFC 2/4] fs: factor out backing_file_{read,write}_iter() helpers
+ Link: https://lore.kernel.org/r/20231221095410.801061-3-amir73il@gmail.com
+ Signed-off-by: Christian Brauner <brauner@kernel.org>
✓ [PATCH RFC 3/4] fs: factor out backing_file_splice_{read,write}() helpers
+ Link: https://lore.kernel.org/r/20231221095410.801061-4-amir73il@gmail.com
+ Signed-off-by: Christian Brauner <brauner@kernel.org>
✓ [PATCH RFC 4/4] fs: factor out backing_file_mmap() helper
+ Link: https://lore.kernel.org/r/20231221095410.801061-5-amir73il@gmail.com
+ Signed-off-by: Christian Brauner <brauner@kernel.org>
---
✓ Signed: DKIM/gmail.com
---
Total patches: 4
---
Applying: fs: prepare for stackable filesystems backing file helpers
Applying: fs: factor out backing_file_{read,write}_iter() helpers
Patch failed at 0002 fs: factor out backing_file_{read,write}_iter() helpers
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
error: sha1 information is lacking or useless (fs/overlayfs/file.c).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Ok, I see that I didn't include your branch so git got confused by
missing overlayfs changes that you have in your tree and thus failed to
apply here.
>
> The only conflict I know of is with the vfs.rw branch,
> the move of *_start_write() into *__iter_write(), therefore,
> these patches are already based on top of vfs.rw.
Aha, there we go.
>
> I've just pushed branch backing_file rebased over both
> vfs.rw and vfs.file to:
> https://github.com/amir73il/linux/commits/backing_file
>
> Started to run overlayfs tests to see if vfs.file has unforeseen impact
> that I missed in review.
>
> > * Pull vfs.file into overlayfs.
> > * Port overlayfs to the new infrastructure.
> >
>
> Wait, do you mean add the backing_file_*() helpers
> and only then convert overlayfs to use them?
>
> I think that would be harder to review (also in retrospect)
> so the "factor out ... helper" patches that move code from
> overlayfs to fs/backing_file.c are easier to follow.
It depends. Here I think it's a bit similar to David's netfs library
stuff where a bunch of infra is added. Then the conversion is on top of
that. However, I have no mandatory rule here.
>
> Or did you mean something else?
>
> > io_uring already depends on vfs.file as well.
> >
> > If this is straightforward I can include it in v6.8. The VFS prs will go
> > out the week before January 7.
>
> Well, unless I misunderstood you, that was straightforward.
> The only complexity is the order and dependency among the PRs.
>
> If I am not mistaken, backing_file could be applied directly on top of
> vfs.rw and sent right after it, or along with it (via your tree)?
Along with it.
^ permalink raw reply [flat|nested] 13+ messages in thread