From: Amir Goldstein <amir73il@gmail.com>
To: Miklos Szeredi <mszeredi@redhat.com>
Cc: linux-unionfs@vger.kernel.org, linux-fsdevel@vger.kernel.org,
Christian Brauner <brauner@kernel.org>
Subject: Re: [RFC PATCH] backing-file: clean up the API
Date: Tue, 15 Oct 2024 17:31:59 +0200 [thread overview]
Message-ID: <CAOQ4uxh-3H4QkTEihujFgz53ajeArWH9u_yj4kaWByVJAGmgrw@mail.gmail.com> (raw)
In-Reply-To: <20241015133141.70632-1-mszeredi@redhat.com>
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.
next prev parent reply other threads:[~2024-10-15 15:32 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-10-15 13:31 [RFC PATCH] backing-file: clean up the API Miklos Szeredi
2024-10-15 15:31 ` Amir Goldstein [this message]
2024-10-15 15:48 ` Miklos Szeredi
2024-10-15 18:57 ` Amir Goldstein
2024-10-21 10:44 ` Miklos Szeredi
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAOQ4uxh-3H4QkTEihujFgz53ajeArWH9u_yj4kaWByVJAGmgrw@mail.gmail.com \
--to=amir73il@gmail.com \
--cc=brauner@kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-unionfs@vger.kernel.org \
--cc=mszeredi@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).