[RFC PATCH] backing-file: clean up the API

[RFC PATCH] backing-file: clean up the API

[Miklos Szeredi]

 - Pass iocb to ctx->end_write() instead of file + pos

 - Get rid of ctx->user_file, which is redundant most of the time

 - Instead pass user_file explicitly to backing_file_splice_read and
   backing_file_splice_write

Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
---
This applies on top of "fs: pass offset and result to backing_file
end_write() callback"

 fs/backing-file.c            | 37 ++++++++++++++++++++----------------
 fs/fuse/passthrough.c        | 21 +++++++-------------
 fs/overlayfs/file.c          | 14 +++++---------
 include/linux/backing-file.h |  9 ++++-----
 4 files changed, 37 insertions(+), 44 deletions(-)

diff --git a/fs/backing-file.c b/fs/backing-file.c
index 09a9be945d45..98f4486cfca9 100644
--- a/fs/backing-file.c
+++ b/fs/backing-file.c
@@ -80,7 +80,7 @@ struct backing_aio {
 	refcount_t ref;
 	struct kiocb *orig_iocb;
 	/* used for aio completion */
-	void (*end_write)(struct file *, loff_t, ssize_t);
+	void (*end_write)(struct kiocb *iocb, ssize_t);
 	struct work_struct work;
 	long res;
 };
@@ -108,10 +108,10 @@ static void backing_aio_cleanup(struct backing_aio *aio, long res)
 	struct kiocb *iocb = &aio->iocb;
 	struct kiocb *orig_iocb = aio->orig_iocb;
 
+	orig_iocb->ki_pos = iocb->ki_pos;
 	if (aio->end_write)
-		aio->end_write(orig_iocb->ki_filp, iocb->ki_pos, res);
+		aio->end_write(orig_iocb, res);
 
-	orig_iocb->ki_pos = iocb->ki_pos;
 	backing_aio_put(aio);
 }
 
@@ -200,7 +200,7 @@ ssize_t backing_file_read_iter(struct file *file, struct iov_iter *iter,
 	revert_creds(old_cred);
 
 	if (ctx->accessed)
-		ctx->accessed(ctx->user_file);
+		ctx->accessed(iocb->ki_filp);
 
 	return ret;
 }
@@ -219,7 +219,7 @@ ssize_t backing_file_write_iter(struct file *file, struct iov_iter *iter,
 	if (!iov_iter_count(iter))
 		return 0;
 
-	ret = file_remove_privs(ctx->user_file);
+	ret = file_remove_privs(iocb->ki_filp);
 	if (ret)
 		return ret;
 
@@ -239,7 +239,7 @@ ssize_t backing_file_write_iter(struct file *file, struct iov_iter *iter,
 
 		ret = vfs_iter_write(file, iter, &iocb->ki_pos, rwf);
 		if (ctx->end_write)
-			ctx->end_write(ctx->user_file, iocb->ki_pos, ret);
+			ctx->end_write(iocb, ret);
 	} else {
 		struct backing_aio *aio;
 
@@ -270,7 +270,7 @@ 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,
+ssize_t backing_file_splice_read(struct file *in, struct file *user_in, loff_t *ppos,
 				 struct pipe_inode_info *pipe, size_t len,
 				 unsigned int flags,
 				 struct backing_file_ctx *ctx)
@@ -286,15 +286,15 @@ ssize_t backing_file_splice_read(struct file *in, loff_t *ppos,
 	revert_creds(old_cred);
 
 	if (ctx->accessed)
-		ctx->accessed(ctx->user_file);
+		ctx->accessed(user_in);
 
 	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 file *out, struct file *user_out, loff_t *ppos,
+				  size_t len, unsigned int flags,
 				  struct backing_file_ctx *ctx)
 {
 	const struct cred *old_cred;
@@ -306,7 +306,7 @@ ssize_t backing_file_splice_write(struct pipe_inode_info *pipe,
 	if (!out->f_op->splice_write)
 		return -EINVAL;
 
-	ret = file_remove_privs(ctx->user_file);
+	ret = file_remove_privs(user_out);
 	if (ret)
 		return ret;
 
@@ -316,8 +316,14 @@ ssize_t backing_file_splice_write(struct pipe_inode_info *pipe,
 	file_end_write(out);
 	revert_creds(old_cred);
 
-	if (ctx->end_write)
-		ctx->end_write(ctx->user_file, ppos ? *ppos : 0, ret);
+	if (ctx->end_write) {
+		struct kiocb iocb;
+
+		init_sync_kiocb(&iocb, out);
+		iocb.ki_pos = *ppos;
+
+		ctx->end_write(&iocb, ret);
+	}
 
 	return ret;
 }
@@ -329,8 +335,7 @@ int backing_file_mmap(struct file *file, struct vm_area_struct *vma,
 	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))
+	if (WARN_ON_ONCE(!(file->f_mode & FMODE_BACKING)))
 		return -EIO;
 
 	if (!file->f_op->mmap)
@@ -343,7 +348,7 @@ int backing_file_mmap(struct file *file, struct vm_area_struct *vma,
 	revert_creds(old_cred);
 
 	if (ctx->accessed)
-		ctx->accessed(ctx->user_file);
+		ctx->accessed(vma->vm_file);
 
 	return ret;
 }
diff --git a/fs/fuse/passthrough.c b/fs/fuse/passthrough.c
index bbac547dfcb3..5c502394a208 100644
--- a/fs/fuse/passthrough.c
+++ b/fs/fuse/passthrough.c
@@ -18,11 +18,11 @@ static void fuse_file_accessed(struct file *file)
 	fuse_invalidate_atime(inode);
 }
 
-static void fuse_passthrough_end_write(struct file *file, loff_t pos, ssize_t ret)
+static void fuse_passthrough_end_write(struct kiocb *iocb, ssize_t ret)
 {
-	struct inode *inode = file_inode(file);
+	struct inode *inode = file_inode(iocb->ki_filp);
 
-	fuse_write_update_attr(inode, pos, ret);
+	fuse_write_update_attr(inode, iocb->ki_pos, ret);
 }
 
 ssize_t fuse_passthrough_read_iter(struct kiocb *iocb, struct iov_iter *iter)
@@ -34,7 +34,6 @@ ssize_t fuse_passthrough_read_iter(struct kiocb *iocb, struct iov_iter *iter)
 	ssize_t ret;
 	struct backing_file_ctx ctx = {
 		.cred = ff->cred,
-		.user_file = file,
 		.accessed = fuse_file_accessed,
 	};
 
@@ -62,7 +61,6 @@ ssize_t fuse_passthrough_write_iter(struct kiocb *iocb,
 	ssize_t ret;
 	struct backing_file_ctx ctx = {
 		.cred = ff->cred,
-		.user_file = file,
 		.end_write = fuse_passthrough_end_write,
 	};
 
@@ -88,15 +86,13 @@ ssize_t fuse_passthrough_splice_read(struct file *in, loff_t *ppos,
 	struct file *backing_file = fuse_file_passthrough(ff);
 	struct backing_file_ctx ctx = {
 		.cred = ff->cred,
-		.user_file = in,
 		.accessed = fuse_file_accessed,
 	};
 
 	pr_debug("%s: backing_file=0x%p, pos=%lld, len=%zu, flags=0x%x\n", __func__,
-		 backing_file, ppos ? *ppos : 0, len, flags);
+		 backing_file, *ppos, len, flags);
 
-	return backing_file_splice_read(backing_file, ppos, pipe, len, flags,
-					&ctx);
+	return backing_file_splice_read(backing_file, in, ppos, pipe, len, flags, &ctx);
 }
 
 ssize_t fuse_passthrough_splice_write(struct pipe_inode_info *pipe,
@@ -109,16 +105,14 @@ ssize_t fuse_passthrough_splice_write(struct pipe_inode_info *pipe,
 	ssize_t ret;
 	struct backing_file_ctx ctx = {
 		.cred = ff->cred,
-		.user_file = out,
 		.end_write = fuse_passthrough_end_write,
 	};
 
 	pr_debug("%s: backing_file=0x%p, pos=%lld, len=%zu, flags=0x%x\n", __func__,
-		 backing_file, ppos ? *ppos : 0, len, flags);
+		 backing_file, *ppos, len, flags);
 
 	inode_lock(inode);
-	ret = backing_file_splice_write(pipe, backing_file, ppos, len, flags,
-					&ctx);
+	ret = backing_file_splice_write(pipe, backing_file, out, ppos, len, flags, &ctx);
 	inode_unlock(inode);
 
 	return ret;
@@ -130,7 +124,6 @@ ssize_t fuse_passthrough_mmap(struct file *file, struct vm_area_struct *vma)
 	struct file *backing_file = fuse_file_passthrough(ff);
 	struct backing_file_ctx ctx = {
 		.cred = ff->cred,
-		.user_file = file,
 		.accessed = fuse_file_accessed,
 	};
 
diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
index 24a36d61bb0c..1debab93e3d6 100644
--- a/fs/overlayfs/file.c
+++ b/fs/overlayfs/file.c
@@ -231,9 +231,9 @@ static void ovl_file_modified(struct file *file)
 	ovl_copyattr(file_inode(file));
 }
 
-static void ovl_file_end_write(struct file *file, loff_t, ssize_t)
+static void ovl_file_end_write(struct kiocb *iocb, ssize_t)
 {
-	ovl_file_modified(file);
+	ovl_file_modified(iocb->ki_filp);
 }
 
 static void ovl_file_accessed(struct file *file)
@@ -271,7 +271,6 @@ static ssize_t ovl_read_iter(struct kiocb *iocb, struct iov_iter *iter)
 	ssize_t ret;
 	struct backing_file_ctx ctx = {
 		.cred = ovl_creds(file_inode(file)->i_sb),
-		.user_file = file,
 		.accessed = ovl_file_accessed,
 	};
 
@@ -298,7 +297,6 @@ static ssize_t ovl_write_iter(struct kiocb *iocb, struct iov_iter *iter)
 	int ifl = iocb->ki_flags;
 	struct backing_file_ctx ctx = {
 		.cred = ovl_creds(inode->i_sb),
-		.user_file = file,
 		.end_write = ovl_file_end_write,
 	};
 
@@ -338,7 +336,6 @@ static ssize_t ovl_splice_read(struct file *in, loff_t *ppos,
 	ssize_t ret;
 	struct backing_file_ctx ctx = {
 		.cred = ovl_creds(file_inode(in)->i_sb),
-		.user_file = in,
 		.accessed = ovl_file_accessed,
 	};
 
@@ -346,7 +343,7 @@ static ssize_t ovl_splice_read(struct file *in, loff_t *ppos,
 	if (ret)
 		return ret;
 
-	ret = backing_file_splice_read(fd_file(real), ppos, pipe, len, flags, &ctx);
+	ret = backing_file_splice_read(fd_file(real), in, ppos, pipe, len, flags, &ctx);
 	fdput(real);
 
 	return ret;
@@ -368,7 +365,6 @@ static ssize_t ovl_splice_write(struct pipe_inode_info *pipe, struct file *out,
 	ssize_t ret;
 	struct backing_file_ctx ctx = {
 		.cred = ovl_creds(inode->i_sb),
-		.user_file = out,
 		.end_write = ovl_file_end_write,
 	};
 
@@ -380,9 +376,10 @@ static ssize_t ovl_splice_write(struct pipe_inode_info *pipe, struct file *out,
 	if (ret)
 		goto out_unlock;
 
-	ret = backing_file_splice_write(pipe, fd_file(real), ppos, len, flags, &ctx);
+	ret = backing_file_splice_write(pipe, fd_file(real), out, ppos, len, flags, &ctx);
 	fdput(real);
 
+
 out_unlock:
 	inode_unlock(inode);
 
@@ -420,7 +417,6 @@ static int ovl_mmap(struct file *file, struct vm_area_struct *vma)
 	struct file *realfile = file->private_data;
 	struct backing_file_ctx ctx = {
 		.cred = ovl_creds(file_inode(file)->i_sb),
-		.user_file = file,
 		.accessed = ovl_file_accessed,
 	};
 
diff --git a/include/linux/backing-file.h b/include/linux/backing-file.h
index 2eed0ffb5e8f..7db9f77281ca 100644
--- a/include/linux/backing-file.h
+++ b/include/linux/backing-file.h
@@ -14,9 +14,8 @@
 
 struct backing_file_ctx {
 	const struct cred *cred;
-	struct file *user_file;
 	void (*accessed)(struct file *);
-	void (*end_write)(struct file *, loff_t, ssize_t);
+	void (*end_write)(struct kiocb *iocb, ssize_t);
 };
 
 struct file *backing_file_open(const struct path *user_path, int flags,
@@ -31,13 +30,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,
+ssize_t backing_file_splice_read(struct file *in, struct file *user_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 file *out, struct file *user_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);
-- 
2.47.0

Re: [RFC PATCH] backing-file: clean up the API

[Amir Goldstein]

On Tue, Oct 15, 2024 at 3:31 PM Miklos Szeredi <mszeredi@redhat.com> wrote:
>
>  - Pass iocb to ctx->end_write() instead of file + pos
>
>  - Get rid of ctx->user_file, which is redundant most of the time
>
>  - Instead pass user_file explicitly to backing_file_splice_read and
>    backing_file_splice_write
>
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> ---
> This applies on top of "fs: pass offset and result to backing_file
> end_write() callback"

For the sake of people who did not see the above:
https://lore.kernel.org/linux-fsdevel/20241014192759.863031-2-amir73il@gmail.com/

If this cleanup is acceptable then perhaps squash it with the above commit?

>
>  fs/backing-file.c            | 37 ++++++++++++++++++++----------------
>  fs/fuse/passthrough.c        | 21 +++++++-------------
>  fs/overlayfs/file.c          | 14 +++++---------
>  include/linux/backing-file.h |  9 ++++-----
>  4 files changed, 37 insertions(+), 44 deletions(-)
>
> diff --git a/fs/backing-file.c b/fs/backing-file.c
> index 09a9be945d45..98f4486cfca9 100644
> --- a/fs/backing-file.c
> +++ b/fs/backing-file.c
> @@ -80,7 +80,7 @@ struct backing_aio {
>         refcount_t ref;
>         struct kiocb *orig_iocb;
>         /* used for aio completion */
> -       void (*end_write)(struct file *, loff_t, ssize_t);
> +       void (*end_write)(struct kiocb *iocb, ssize_t);
>         struct work_struct work;
>         long res;
>  };
> @@ -108,10 +108,10 @@ static void backing_aio_cleanup(struct backing_aio *aio, long res)
>         struct kiocb *iocb = &aio->iocb;
>         struct kiocb *orig_iocb = aio->orig_iocb;
>
> +       orig_iocb->ki_pos = iocb->ki_pos;
>         if (aio->end_write)
> -               aio->end_write(orig_iocb->ki_filp, iocb->ki_pos, res);
> +               aio->end_write(orig_iocb, res);
>
> -       orig_iocb->ki_pos = iocb->ki_pos;
>         backing_aio_put(aio);
>  }
>
> @@ -200,7 +200,7 @@ ssize_t backing_file_read_iter(struct file *file, struct iov_iter *iter,
>         revert_creds(old_cred);
>
>         if (ctx->accessed)
> -               ctx->accessed(ctx->user_file);
> +               ctx->accessed(iocb->ki_filp);
>
>         return ret;
>  }
> @@ -219,7 +219,7 @@ ssize_t backing_file_write_iter(struct file *file, struct iov_iter *iter,
>         if (!iov_iter_count(iter))
>                 return 0;
>
> -       ret = file_remove_privs(ctx->user_file);
> +       ret = file_remove_privs(iocb->ki_filp);
>         if (ret)
>                 return ret;
>
> @@ -239,7 +239,7 @@ ssize_t backing_file_write_iter(struct file *file, struct iov_iter *iter,
>
>                 ret = vfs_iter_write(file, iter, &iocb->ki_pos, rwf);
>                 if (ctx->end_write)
> -                       ctx->end_write(ctx->user_file, iocb->ki_pos, ret);
> +                       ctx->end_write(iocb, ret);
>         } else {
>                 struct backing_aio *aio;
>
> @@ -270,7 +270,7 @@ 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,
> +ssize_t backing_file_splice_read(struct file *in, struct file *user_in, loff_t *ppos,
>                                  struct pipe_inode_info *pipe, size_t len,
>                                  unsigned int flags,
>                                  struct backing_file_ctx *ctx)
> @@ -286,15 +286,15 @@ ssize_t backing_file_splice_read(struct file *in, loff_t *ppos,
>         revert_creds(old_cred);
>
>         if (ctx->accessed)
> -               ctx->accessed(ctx->user_file);
> +               ctx->accessed(user_in);
>
>         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 file *out, struct file *user_out, loff_t *ppos,
> +                                 size_t len, unsigned int flags,
>                                   struct backing_file_ctx *ctx)
>  {
>         const struct cred *old_cred;
> @@ -306,7 +306,7 @@ ssize_t backing_file_splice_write(struct pipe_inode_info *pipe,
>         if (!out->f_op->splice_write)
>                 return -EINVAL;
>
> -       ret = file_remove_privs(ctx->user_file);
> +       ret = file_remove_privs(user_out);
>         if (ret)
>                 return ret;
>
> @@ -316,8 +316,14 @@ ssize_t backing_file_splice_write(struct pipe_inode_info *pipe,
>         file_end_write(out);
>         revert_creds(old_cred);
>
> -       if (ctx->end_write)
> -               ctx->end_write(ctx->user_file, ppos ? *ppos : 0, ret);
> +       if (ctx->end_write) {
> +               struct kiocb iocb;
> +
> +               init_sync_kiocb(&iocb, out);
> +               iocb.ki_pos = *ppos;
> +
> +               ctx->end_write(&iocb, ret);
> +       }
>
>         return ret;
>  }
> @@ -329,8 +335,7 @@ int backing_file_mmap(struct file *file, struct vm_area_struct *vma,
>         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))
> +       if (WARN_ON_ONCE(!(file->f_mode & FMODE_BACKING)))
>                 return -EIO;
>
>         if (!file->f_op->mmap)
> @@ -343,7 +348,7 @@ int backing_file_mmap(struct file *file, struct vm_area_struct *vma,
>         revert_creds(old_cred);
>
>         if (ctx->accessed)
> -               ctx->accessed(ctx->user_file);
> +               ctx->accessed(vma->vm_file);
>
>         return ret;
>  }
> diff --git a/fs/fuse/passthrough.c b/fs/fuse/passthrough.c
> index bbac547dfcb3..5c502394a208 100644
> --- a/fs/fuse/passthrough.c
> +++ b/fs/fuse/passthrough.c
> @@ -18,11 +18,11 @@ static void fuse_file_accessed(struct file *file)
>         fuse_invalidate_atime(inode);
>  }
>
> -static void fuse_passthrough_end_write(struct file *file, loff_t pos, ssize_t ret)
> +static void fuse_passthrough_end_write(struct kiocb *iocb, ssize_t ret)
>  {
> -       struct inode *inode = file_inode(file);
> +       struct inode *inode = file_inode(iocb->ki_filp);
>
> -       fuse_write_update_attr(inode, pos, ret);
> +       fuse_write_update_attr(inode, iocb->ki_pos, ret);
>  }
>
>  ssize_t fuse_passthrough_read_iter(struct kiocb *iocb, struct iov_iter *iter)
> @@ -34,7 +34,6 @@ ssize_t fuse_passthrough_read_iter(struct kiocb *iocb, struct iov_iter *iter)
>         ssize_t ret;
>         struct backing_file_ctx ctx = {
>                 .cred = ff->cred,
> -               .user_file = file,
>                 .accessed = fuse_file_accessed,
>         };
>
> @@ -62,7 +61,6 @@ ssize_t fuse_passthrough_write_iter(struct kiocb *iocb,
>         ssize_t ret;
>         struct backing_file_ctx ctx = {
>                 .cred = ff->cred,
> -               .user_file = file,
>                 .end_write = fuse_passthrough_end_write,
>         };
>
> @@ -88,15 +86,13 @@ ssize_t fuse_passthrough_splice_read(struct file *in, loff_t *ppos,
>         struct file *backing_file = fuse_file_passthrough(ff);
>         struct backing_file_ctx ctx = {
>                 .cred = ff->cred,
> -               .user_file = in,
>                 .accessed = fuse_file_accessed,
>         };
>
>         pr_debug("%s: backing_file=0x%p, pos=%lld, len=%zu, flags=0x%x\n", __func__,
> -                backing_file, ppos ? *ppos : 0, len, flags);
> +                backing_file, *ppos, len, flags);
>
> -       return backing_file_splice_read(backing_file, ppos, pipe, len, flags,
> -                                       &ctx);
> +       return backing_file_splice_read(backing_file, in, ppos, pipe, len, flags, &ctx);
>  }
>
>  ssize_t fuse_passthrough_splice_write(struct pipe_inode_info *pipe,
> @@ -109,16 +105,14 @@ ssize_t fuse_passthrough_splice_write(struct pipe_inode_info *pipe,
>         ssize_t ret;
>         struct backing_file_ctx ctx = {
>                 .cred = ff->cred,
> -               .user_file = out,
>                 .end_write = fuse_passthrough_end_write,
>         };
>
>         pr_debug("%s: backing_file=0x%p, pos=%lld, len=%zu, flags=0x%x\n", __func__,
> -                backing_file, ppos ? *ppos : 0, len, flags);
> +                backing_file, *ppos, len, flags);
>
>         inode_lock(inode);
> -       ret = backing_file_splice_write(pipe, backing_file, ppos, len, flags,
> -                                       &ctx);
> +       ret = backing_file_splice_write(pipe, backing_file, out, ppos, len, flags, &ctx);
>         inode_unlock(inode);
>
>         return ret;
> @@ -130,7 +124,6 @@ ssize_t fuse_passthrough_mmap(struct file *file, struct vm_area_struct *vma)
>         struct file *backing_file = fuse_file_passthrough(ff);
>         struct backing_file_ctx ctx = {
>                 .cred = ff->cred,
> -               .user_file = file,
>                 .accessed = fuse_file_accessed,
>         };
>
> diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
> index 24a36d61bb0c..1debab93e3d6 100644
> --- a/fs/overlayfs/file.c
> +++ b/fs/overlayfs/file.c
> @@ -231,9 +231,9 @@ static void ovl_file_modified(struct file *file)
>         ovl_copyattr(file_inode(file));
>  }
>
> -static void ovl_file_end_write(struct file *file, loff_t, ssize_t)
> +static void ovl_file_end_write(struct kiocb *iocb, ssize_t)
>  {
> -       ovl_file_modified(file);
> +       ovl_file_modified(iocb->ki_filp);
>  }
>
>  static void ovl_file_accessed(struct file *file)
> @@ -271,7 +271,6 @@ static ssize_t ovl_read_iter(struct kiocb *iocb, struct iov_iter *iter)
>         ssize_t ret;
>         struct backing_file_ctx ctx = {
>                 .cred = ovl_creds(file_inode(file)->i_sb),
> -               .user_file = file,
>                 .accessed = ovl_file_accessed,
>         };
>
> @@ -298,7 +297,6 @@ static ssize_t ovl_write_iter(struct kiocb *iocb, struct iov_iter *iter)
>         int ifl = iocb->ki_flags;
>         struct backing_file_ctx ctx = {
>                 .cred = ovl_creds(inode->i_sb),
> -               .user_file = file,
>                 .end_write = ovl_file_end_write,
>         };
>
> @@ -338,7 +336,6 @@ static ssize_t ovl_splice_read(struct file *in, loff_t *ppos,
>         ssize_t ret;
>         struct backing_file_ctx ctx = {
>                 .cred = ovl_creds(file_inode(in)->i_sb),
> -               .user_file = in,
>                 .accessed = ovl_file_accessed,
>         };
>
> @@ -346,7 +343,7 @@ static ssize_t ovl_splice_read(struct file *in, loff_t *ppos,
>         if (ret)
>                 return ret;
>
> -       ret = backing_file_splice_read(fd_file(real), ppos, pipe, len, flags, &ctx);
> +       ret = backing_file_splice_read(fd_file(real), in, ppos, pipe, len, flags, &ctx);
>         fdput(real);
>
>         return ret;
> @@ -368,7 +365,6 @@ static ssize_t ovl_splice_write(struct pipe_inode_info *pipe, struct file *out,
>         ssize_t ret;
>         struct backing_file_ctx ctx = {
>                 .cred = ovl_creds(inode->i_sb),
> -               .user_file = out,
>                 .end_write = ovl_file_end_write,
>         };
>
> @@ -380,9 +376,10 @@ static ssize_t ovl_splice_write(struct pipe_inode_info *pipe, struct file *out,
>         if (ret)
>                 goto out_unlock;
>
> -       ret = backing_file_splice_write(pipe, fd_file(real), ppos, len, flags, &ctx);
> +       ret = backing_file_splice_write(pipe, fd_file(real), out, ppos, len, flags, &ctx);
>         fdput(real);
>
> +
>  out_unlock:
>         inode_unlock(inode);
>
> @@ -420,7 +417,6 @@ static int ovl_mmap(struct file *file, struct vm_area_struct *vma)
>         struct file *realfile = file->private_data;
>         struct backing_file_ctx ctx = {
>                 .cred = ovl_creds(file_inode(file)->i_sb),
> -               .user_file = file,
>                 .accessed = ovl_file_accessed,
>         };
>
> diff --git a/include/linux/backing-file.h b/include/linux/backing-file.h
> index 2eed0ffb5e8f..7db9f77281ca 100644
> --- a/include/linux/backing-file.h
> +++ b/include/linux/backing-file.h
> @@ -14,9 +14,8 @@
>
>  struct backing_file_ctx {
>         const struct cred *cred;
> -       struct file *user_file;
>         void (*accessed)(struct file *);
> -       void (*end_write)(struct file *, loff_t, ssize_t);
> +       void (*end_write)(struct kiocb *iocb, ssize_t);
>  };

This seems wrong to me, that the callback gets an iocb
that was not initialized by the caller of backing_file_*().
It seems better if the callback would get the backing_file_ctx.
We could copy the pos to this ctx if you think this is better.

OTOH, ->user_file pretty much belongs to backing_file_ctx,
even if only used in the io callbacks.

Thanks,
Amir.

Re: [RFC PATCH] backing-file: clean up the API

[Miklos Szeredi]

On Tue, 15 Oct 2024 at 17:32, Amir Goldstein <amir73il@gmail.com> wrote:

> If this cleanup is acceptable then perhaps squash it with the above commit?

I didn't want to do that, because for the backport your version is the
simplest and easiest to review.

> This seems wrong to me, that the callback gets an iocb
> that was not initialized by the caller of backing_file_*().

That's only true for backing_file_splice_write().   Would oassing an
iocb into that function fix your concern?

> It seems better if the callback would get the backing_file_ctx.
> We could copy the pos to this ctx if you think this is better.
>
> OTOH, ->user_file pretty much belongs to backing_file_ctx,
> even if only used in the io callbacks.

I don't like having user_file in there, because it's redundant.
->user_file and iocb->ki_filp *must* be the same, and I don't see how
passing it as an opaque thing back to ->end_write() would make it any
better.

Thanks,
Miklos

Re: [RFC PATCH] backing-file: clean up the API

[Amir Goldstein]

On Tue, Oct 15, 2024 at 5:49 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Tue, 15 Oct 2024 at 17:32, Amir Goldstein <amir73il@gmail.com> wrote:
>
> > If this cleanup is acceptable then perhaps squash it with the above commit?
>
> I didn't want to do that, because for the backport your version is the
> simplest and easiest to review.
>
> > This seems wrong to me, that the callback gets an iocb
> > that was not initialized by the caller of backing_file_*().
>
> That's only true for backing_file_splice_write().   Would passing an
> iocb into that function fix your concern?
>

I suppose that would be better, only then passing iocb and a separate
ppos would be weird, or did you have something else in mind?

But I think that if we aim for a cleaner interface then the arguments of
backing_file_splice_{read,write}() should be similar and so should the
arguments of ->{end_write,access}(), so we should change
->access(file *) to ->end_read(iocb *)

This way we could call fuse_read_update_size() or something
from ->end_read() if we wanted to.

Thanks,
Amir.

Re: [RFC PATCH] backing-file: clean up the API

[Miklos Szeredi]

On Tue, 15 Oct 2024 at 20:57, Amir Goldstein <amir73il@gmail.com> wrote:

> But I think that if we aim for a cleaner interface then the arguments of
> backing_file_splice_{read,write}() should be similar and so should the
> arguments of ->{end_write,access}(), so we should change
> ->access(file *) to ->end_read(iocb *)

Changing ->access() to iocb would mean having to manufature an iocb
for mmap(), which would be unnatural.   So I didn't touch that.

Thanks,
Miklos