| Message ID | 20231122122715.2561213-6-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Tidy up file permission hooks | expand |
> +ssize_t __kernel_write_iter(struct file *file, struct iov_iter *from, > + loff_t *pos); > +ssize_t do_iter_writev(struct file *file, struct iov_iter *iter, loff_t *ppos, > + rwf_t flags); So I first stumbled on the name because I kinda hate do_* functions, especially if they are not static, and then went down a little rathole: - first obviously the name, based on the other functions it probably should be in the __kernel_* namespace unless I'm missing something. - second can you add a little comment when it is to be used? Our maze of read and write helpers is becomeing a little too confusing even for someone who thought he knew the code (and wrote some if it). - can we just split do_iter_readv_writev instead of adding a wrapper Yes, that'll duplicate a little boiler plate code, but it'll make things much easier to follow. (- same probably for do_loop_readv_writev, although not directly relevant to this series)
On Thu, Nov 23, 2023 at 9:47 AM Christoph Hellwig <hch@infradead.org> wrote: > > > +ssize_t __kernel_write_iter(struct file *file, struct iov_iter *from, > > + loff_t *pos); > > +ssize_t do_iter_writev(struct file *file, struct iov_iter *iter, loff_t *ppos, > > + rwf_t flags); > > So I first stumbled on the name because I kinda hate do_* functions, > especially if they are not static, and then went down a little rathole: > > > - first obviously the name, based on the other functions it probably > should be in the __kernel_* namespace unless I'm missing something. Not sure about the best name. I just derived the name from do_iter_readv_writev(), which would be the name of this helper if we split up do_iter_readv_writev() as you suggested. > - second can you add a little comment when it is to be used? Our > maze of read and write helpers is becoming a little too confusing > even for someone who thought he knew the code (and wrote some if it). I could try, but I can't say that I know how to document the maze. I was trying to make small changes that make the maze a bit more consistent - after my series, in most cases we have: vfs_XXX() rw_verify_area() file_start_write() do_XXX() But obviously, there is still a wide variety of do_* helpers with different conventions. > - can we just split do_iter_readv_writev instead of adding a wrapper > Yes, that'll duplicate a little boiler plate code, but it'll make > things much easier to follow. > (- same probably for do_loop_readv_writev, although not directly > relevant to this series) > We can do that, it's easy, but I also got opposite comments from Christian about opportunities to factor out more common code, so I will do it if there is consensus. Thanks, Amir.
On Thu, Nov 23, 2023 at 01:20:13PM +0200, Amir Goldstein wrote: > > - first obviously the name, based on the other functions it probably > > should be in the __kernel_* namespace unless I'm missing something. > > Not sure about the best name. > I just derived the name from do_iter_readv_writev(), which would be the > name of this helper if we split up do_iter_readv_writev() as you suggested. Well, I don't think do_iter_readv_writev is a particular good name even now, but certainly not once it is more than just a static helper with two callers. I can't say __kernel is an all that great name either, but it seems to match the existing ones. That being said - it just is a tiny wrapper anyway, what about just open coding it instead of bike shedding? See below for a patch, this actually seems pretty reasonable and very readable. --- diff --git a/fs/splice.c b/fs/splice.c index d983d375ff1130..982a0872fa03e9 100644 --- a/fs/splice.c +++ b/fs/splice.c @@ -684,6 +684,7 @@ iter_file_splice_write(struct pipe_inode_info *pipe, struct file *out, splice_from_pipe_begin(&sd); while (sd.total_len) { + struct kiocb kiocb; struct iov_iter from; unsigned int head, tail, mask; size_t left; @@ -733,7 +734,10 @@ iter_file_splice_write(struct pipe_inode_info *pipe, struct file *out, } iov_iter_bvec(&from, ITER_SOURCE, array, n, sd.total_len - left); - ret = vfs_iter_write(out, &from, &sd.pos, 0); + init_sync_kiocb(&kiocb, out); + kiocb.ki_pos = sd.pos; + ret = out->f_op->write_iter(&kiocb, &from); + sd.pos = kiocb.ki_pos; if (ret <= 0) break;
On Thu, Nov 23, 2023 at 5:14 PM Christoph Hellwig <hch@infradead.org> wrote: > > On Thu, Nov 23, 2023 at 01:20:13PM +0200, Amir Goldstein wrote: > > > - first obviously the name, based on the other functions it probably > > > should be in the __kernel_* namespace unless I'm missing something. > > > > Not sure about the best name. > > I just derived the name from do_iter_readv_writev(), which would be the > > name of this helper if we split up do_iter_readv_writev() as you suggested. > > Well, I don't think do_iter_readv_writev is a particular good name > even now, but certainly not once it is more than just a static helper > with two callers. > > I can't say __kernel is an all that great name either, but it seems > to match the existing ones. > > That being said - it just is a tiny wrapper anyway, what about just > open coding it instead of bike shedding? See below for a patch, > this actually seems pretty reasonable and very readable. > Heh! avoiding bike shedding is a worthy cause :) It works for me. I will let Christian decide if he prefers this over the existing small helper with meaningless name. > --- > diff --git a/fs/splice.c b/fs/splice.c > index d983d375ff1130..982a0872fa03e9 100644 > --- a/fs/splice.c > +++ b/fs/splice.c > @@ -684,6 +684,7 @@ iter_file_splice_write(struct pipe_inode_info *pipe, struct file *out, > > splice_from_pipe_begin(&sd); > while (sd.total_len) { > + struct kiocb kiocb; > struct iov_iter from; > unsigned int head, tail, mask; > size_t left; > @@ -733,7 +734,10 @@ iter_file_splice_write(struct pipe_inode_info *pipe, struct file *out, > } > > iov_iter_bvec(&from, ITER_SOURCE, array, n, sd.total_len - left); > - ret = vfs_iter_write(out, &from, &sd.pos, 0); > + init_sync_kiocb(&kiocb, out); > + kiocb.ki_pos = sd.pos; > + ret = out->f_op->write_iter(&kiocb, &from); > + sd.pos = kiocb.ki_pos; > if (ret <= 0) > break; > Are we open coding call_write_iter() now? Is that a trend that I am not aware of? Thanks, Amir.
> Are we open coding call_write_iter() now? > Is that a trend that I am not aware of? I thought we finally agreed on killing the damn thing. I was about to get ready to send a series to kill after wading through the read/write helpers again..
> > diff --git a/fs/splice.c b/fs/splice.c > > index d983d375ff1130..982a0872fa03e9 100644 > > --- a/fs/splice.c > > +++ b/fs/splice.c > > @@ -684,6 +684,7 @@ iter_file_splice_write(struct pipe_inode_info *pipe, struct file *out, > > > > splice_from_pipe_begin(&sd); > > while (sd.total_len) { > > + struct kiocb kiocb; > > struct iov_iter from; > > unsigned int head, tail, mask; > > size_t left; > > @@ -733,7 +734,10 @@ iter_file_splice_write(struct pipe_inode_info *pipe, struct file *out, > > } > > > > iov_iter_bvec(&from, ITER_SOURCE, array, n, sd.total_len - left); > > - ret = vfs_iter_write(out, &from, &sd.pos, 0); > > + init_sync_kiocb(&kiocb, out); > > + kiocb.ki_pos = sd.pos; > > + ret = out->f_op->write_iter(&kiocb, &from); > > + sd.pos = kiocb.ki_pos; > > if (ret <= 0) > > break; > > > > Are we open coding call_write_iter() now? > Is that a trend that I am not aware of? I'll fold that in as-is but I'll use call_write_iter() for now. We can remove that later. For now consistency matters more.
On Thu, Nov 23, 2023 at 6:22 PM Christian Brauner <brauner@kernel.org> wrote: > > > > diff --git a/fs/splice.c b/fs/splice.c > > > index d983d375ff1130..982a0872fa03e9 100644 > > > --- a/fs/splice.c > > > +++ b/fs/splice.c > > > @@ -684,6 +684,7 @@ iter_file_splice_write(struct pipe_inode_info *pipe, struct file *out, > > > > > > splice_from_pipe_begin(&sd); > > > while (sd.total_len) { > > > + struct kiocb kiocb; > > > struct iov_iter from; > > > unsigned int head, tail, mask; > > > size_t left; > > > @@ -733,7 +734,10 @@ iter_file_splice_write(struct pipe_inode_info *pipe, struct file *out, > > > } > > > > > > iov_iter_bvec(&from, ITER_SOURCE, array, n, sd.total_len - left); > > > - ret = vfs_iter_write(out, &from, &sd.pos, 0); > > > + init_sync_kiocb(&kiocb, out); > > > + kiocb.ki_pos = sd.pos; > > > + ret = out->f_op->write_iter(&kiocb, &from); > > > + sd.pos = kiocb.ki_pos; > > > if (ret <= 0) > > > break; > > > > > > > Are we open coding call_write_iter() now? > > Is that a trend that I am not aware of? > > I'll fold that in as-is but I'll use call_write_iter() for now. > We can remove that later. For now consistency matters more. Stating the obvious - please don't forget to edit the commit message removing mention of the helper. Thanks, Amir.
> Stating the obvious - please don't forget to edit the commit message > removing mention of the helper. Done. See vfs.rw.
diff --git a/fs/internal.h b/fs/internal.h index 58e43341aebf..c114b85e27a7 100644 --- a/fs/internal.h +++ b/fs/internal.h @@ -298,7 +298,13 @@ static inline ssize_t do_get_acl(struct mnt_idmap *idmap, } #endif -ssize_t __kernel_write_iter(struct file *file, struct iov_iter *from, loff_t *pos); +/* + * fs/read_write.c + */ +ssize_t __kernel_write_iter(struct file *file, struct iov_iter *from, + loff_t *pos); +ssize_t do_iter_writev(struct file *file, struct iov_iter *iter, loff_t *ppos, + rwf_t flags); /* * fs/attr.c diff --git a/fs/read_write.c b/fs/read_write.c index 4771701c896b..313f7eaaa9a7 100644 --- a/fs/read_write.c +++ b/fs/read_write.c @@ -739,6 +739,17 @@ static ssize_t do_iter_readv_writev(struct file *filp, struct iov_iter *iter, return ret; } +/* + * Low-level helpers don't perform rw sanity checks. + * The caller is responsible for that. + */ +ssize_t do_iter_writev(struct file *filp, struct iov_iter *iter, loff_t *ppos, + rwf_t flags) +{ + return do_iter_readv_writev(filp, iter, ppos, WRITE, flags); +} + + /* Do it by hand, with file-ops */ static ssize_t do_loop_readv_writev(struct file *filp, struct iov_iter *iter, loff_t *ppos, int type, rwf_t flags) diff --git a/fs/splice.c b/fs/splice.c index d4fdd44c0b32..decbace5d812 100644 --- a/fs/splice.c +++ b/fs/splice.c @@ -673,10 +673,13 @@ iter_file_splice_write(struct pipe_inode_info *pipe, struct file *out, .u.file = out, }; int nbufs = pipe->max_usage; - struct bio_vec *array = kcalloc(nbufs, sizeof(struct bio_vec), - GFP_KERNEL); + struct bio_vec *array; ssize_t ret; + if (!out->f_op->write_iter) + return -EINVAL; + + array = kcalloc(nbufs, sizeof(struct bio_vec), GFP_KERNEL); if (unlikely(!array)) return -ENOMEM; @@ -733,7 +736,7 @@ iter_file_splice_write(struct pipe_inode_info *pipe, struct file *out, } iov_iter_bvec(&from, ITER_SOURCE, array, n, sd.total_len - left); - ret = vfs_iter_write(out, &from, &sd.pos, 0); + ret = do_iter_writev(out, &from, &sd.pos, 0); if (ret <= 0) break;