| Message ID | 20200701200951.3603160-18-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/23] cachefiles: switch to kernel_write | expand |
On Wed, Jul 01, 2020 at 10:09:45PM +0200, Christoph Hellwig wrote:
> This will allow proc files to implement iter read semantics.
*UGH*
You are introducing file_operations with both ->read() and ->read_iter();
worse, in some cases they are not equivalent. Sure, ->read() takes
precedence right now, but... why not a separate file_operations for
->read_iter-capable files?
I really hate the fallbacks of that sort - they tend to be brittle
as hell. And while we are at it, I'm not sure that your iter_read()
has good cause to be non-static.
On Wed, Jul 01, 2020 at 10:27:51PM +0100, Al Viro wrote: > On Wed, Jul 01, 2020 at 10:09:45PM +0200, Christoph Hellwig wrote: > > This will allow proc files to implement iter read semantics. > > *UGH* > > You are introducing file_operations with both ->read() and ->read_iter(); > worse, in some cases they are not equivalent. Sure, ->read() takes > precedence right now, but... why not a separate file_operations for > ->read_iter-capable files? I looked at that initially. We'd need to more instances as there already are two due to compat stuff. If that is preferably I can switch to that version. > I really hate the fallbacks of that sort - they tend to be brittle > as hell. And while we are at it, I'm not sure that your iter_read() > has good cause to be non-static. The other user of it is seq_file, which as-is should go away, but will probably keep the occasional version of it in the caller. I just got really tired of reimplementing it a few times.
On Thu, Jul 02, 2020 at 07:18:11AM +0200, Christoph Hellwig wrote: > On Wed, Jul 01, 2020 at 10:27:51PM +0100, Al Viro wrote: > > On Wed, Jul 01, 2020 at 10:09:45PM +0200, Christoph Hellwig wrote: > > > This will allow proc files to implement iter read semantics. > > > > *UGH* > > > > You are introducing file_operations with both ->read() and ->read_iter(); > > worse, in some cases they are not equivalent. Sure, ->read() takes > > precedence right now, but... why not a separate file_operations for > > ->read_iter-capable files? > > I looked at that initially. We'd need to more instances as there > already are two due to compat stuff. If that is preferably I can > switch to that version. > > > I really hate the fallbacks of that sort - they tend to be brittle > > as hell. And while we are at it, I'm not sure that your iter_read() > > has good cause to be non-static. > > The other user of it is seq_file, which as-is should go away, but > will probably keep the occasional version of it in the caller. I just > got really tired of reimplementing it a few times. I've force puhed a new version to the existing location: git://git.infradead.org/users/hch/misc.git set_fs-rw That gets uses separate ops in proc after a few preparational cleanups and rid of the iter_read() patch entirely. Let me know what you think, I don't really want to send the whole patch bomb again already. Gitweb is also avaiable here: http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/set_fs-rw
diff --git a/fs/proc/inode.c b/fs/proc/inode.c index 28d6105e908e4c..fa86619cebc2be 100644 --- a/fs/proc/inode.c +++ b/fs/proc/inode.c @@ -297,6 +297,29 @@ static loff_t proc_reg_llseek(struct file *file, loff_t offset, int whence) return rv; } +static ssize_t pde_read_iter(struct proc_dir_entry *pde, struct kiocb *iocb, + struct iov_iter *iter) +{ + if (!pde->proc_ops->proc_read_iter) + return -EINVAL; + return pde->proc_ops->proc_read_iter(iocb, iter); +} + +static ssize_t proc_reg_read_iter(struct kiocb *iocb, struct iov_iter *iter) +{ + struct proc_dir_entry *pde = PDE(file_inode(iocb->ki_filp)); + ssize_t ret; + + if (pde_is_permanent(pde)) + return pde_read_iter(pde, iocb, iter); + + if (!use_pde(pde)) + return -EIO; + ret = pde_read_iter(pde, iocb, iter); + unuse_pde(pde); + return ret; +} + static ssize_t pde_read(struct proc_dir_entry *pde, struct file *file, char __user *buf, size_t count, loff_t *ppos) { typeof_member(struct proc_ops, proc_read) read; @@ -312,6 +335,9 @@ static ssize_t proc_reg_read(struct file *file, char __user *buf, size_t count, struct proc_dir_entry *pde = PDE(file_inode(file)); ssize_t rv = -EIO; + if (pde->proc_ops->proc_read_iter) + return iter_read(file, buf, count, ppos, proc_reg_read_iter); + if (pde_is_permanent(pde)) { return pde_read(pde, file, buf, count, ppos); } else if (use_pde(pde)) { @@ -569,6 +595,7 @@ static int proc_reg_release(struct inode *inode, struct file *file) static const struct file_operations proc_reg_file_ops = { .llseek = proc_reg_llseek, .read = proc_reg_read, + .read_iter = proc_reg_read_iter, .write = proc_reg_write, .poll = proc_reg_poll, .unlocked_ioctl = proc_reg_unlocked_ioctl, @@ -585,6 +612,7 @@ static const struct file_operations proc_reg_file_ops = { static const struct file_operations proc_reg_file_ops_no_compat = { .llseek = proc_reg_llseek, .read = proc_reg_read, + .read_iter = proc_reg_read_iter, .write = proc_reg_write, .poll = proc_reg_poll, .unlocked_ioctl = proc_reg_unlocked_ioctl, diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h index d1eed1b4365172..97b3f5f06db9d8 100644 --- a/include/linux/proc_fs.h +++ b/include/linux/proc_fs.h @@ -30,6 +30,7 @@ struct proc_ops { unsigned int proc_flags; int (*proc_open)(struct inode *, struct file *); ssize_t (*proc_read)(struct file *, char __user *, size_t, loff_t *); + ssize_t (*proc_read_iter)(struct kiocb *, struct iov_iter *); ssize_t (*proc_write)(struct file *, const char __user *, size_t, loff_t *); loff_t (*proc_lseek)(struct file *, loff_t, int); int (*proc_release)(struct inode *, struct file *);
This will allow proc files to implement iter read semantics. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/proc/inode.c | 28 ++++++++++++++++++++++++++++ include/linux/proc_fs.h | 1 + 2 files changed, 29 insertions(+)