| Message ID | 20230727161016.169066-3-dhowells@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | shmem, splice: Fixes for shmem_splice_read() | expand |
On Thu, 27 Jul 2023, David Howells wrote: > Fix shmem_splice_read() to use the inode from in->f_mapping->host rather > than file_inode(in) and to skip the splice if it starts after s_maxbytes, > analogously with fixes to filemap_splice_read(). > > Fixes: bd194b187115 ("shmem: Implement splice-read") > Signed-off-by: David Howells <dhowells@redhat.com> Thanks for trying to keep them in synch, but I already had to look into both of these two "fixes" before sending my patch to Andrew, and neither appears to be needed. Neither of them does any harm either, but I think I'd prefer Andrew to drop this patch from mm-unstable, just because it changes nothing. Comment on each below... > cc: Hugh Dickins <hughd@google.com> > cc: Christoph Hellwig <hch@lst.de> > cc: Jens Axboe <axboe@kernel.dk> > cc: Al Viro <viro@zeniv.linux.org.uk> > cc: John Hubbard <jhubbard@nvidia.com> > cc: David Hildenbrand <david@redhat.com> > cc: Matthew Wilcox <willy@infradead.org> > cc: Chuck Lever <chuck.lever@oracle.com> > cc: linux-block@vger.kernel.org > cc: linux-fsdevel@vger.kernel.org > cc: linux-mm@kvack.org > --- > mm/shmem.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/mm/shmem.c b/mm/shmem.c > index 0164cccdcd71..8a16d4c7092b 100644 > --- a/mm/shmem.c > +++ b/mm/shmem.c > @@ -2780,13 +2780,16 @@ static ssize_t shmem_file_splice_read(struct file *in, loff_t *ppos, > struct pipe_inode_info *pipe, > size_t len, unsigned int flags) > { > - struct inode *inode = file_inode(in); > + struct inode *inode = in->f_mapping->host; Haha, it's years and years since I had to worry about that distinction: I noticed you'd made that change in filemap, and had to refresh old memories, before concluding that this change is not needed. shmem_file_splice_read() is specified only in shmem_file_operations, and shmem_file_operations is connected up only when S_IFREG; so block and char device nodes will not ever arrive at shmem_file_splice_read(). And shmem does not appear to be out of step there: I did not search through many filesystems, but it appeared to be normal that only the regular files reach the splice_read. Which made me wonder whether the file_inode -> f_mapping->host change was appropriate elsewhere. Wouldn't the splice_read always get called on the bd_inode? Ah, perhaps not: init_special_inode() sets i_fop to def_blk_fops (for shmem and everyone else), and that points to filemap_splice_read(), which explains why just that one needs to pivot to f_mapping->host (I think). > struct address_space *mapping = inode->i_mapping; > struct folio *folio = NULL; > size_t total_spliced = 0, used, npages, n, part; > loff_t isize; > int error = 0; > > + if (unlikely(*ppos >= inode->i_sb->s_maxbytes)) > + return 0; > + > /* Work out how much data we can actually add into the pipe */ > used = pipe_occupancy(pipe->head, pipe->tail); > npages = max_t(ssize_t, pipe->max_usage - used, 0); len = min_t(size_t, len, npages * PAGE_SIZE); do { if (*ppos >= i_size_read(inode)) break; I've added to the context there, to show that the very first thing the do loop does is check *ppos against i_size: so there's no need for that preliminary check against s_maxbytes - something would be wrong already if the file has grown beyond s_maxbytes. So, thanks for the patch, but shmem does not need it. Hugh
Hugh Dickins <hughd@google.com> wrote: > Which made me wonder whether the file_inode -> f_mapping->host > change was appropriate elsewhere. It's sometimes confusing. There are several (potentially) different inodes available at some points. David
diff --git a/mm/shmem.c b/mm/shmem.c index 0164cccdcd71..8a16d4c7092b 100644 --- a/mm/shmem.c +++ b/mm/shmem.c @@ -2780,13 +2780,16 @@ static ssize_t shmem_file_splice_read(struct file *in, loff_t *ppos, struct pipe_inode_info *pipe, size_t len, unsigned int flags) { - struct inode *inode = file_inode(in); + struct inode *inode = in->f_mapping->host; struct address_space *mapping = inode->i_mapping; struct folio *folio = NULL; size_t total_spliced = 0, used, npages, n, part; loff_t isize; int error = 0; + if (unlikely(*ppos >= inode->i_sb->s_maxbytes)) + return 0; + /* Work out how much data we can actually add into the pipe */ used = pipe_occupancy(pipe->head, pipe->tail); npages = max_t(ssize_t, pipe->max_usage - used, 0);
Fix shmem_splice_read() to use the inode from in->f_mapping->host rather than file_inode(in) and to skip the splice if it starts after s_maxbytes, analogously with fixes to filemap_splice_read(). Fixes: bd194b187115 ("shmem: Implement splice-read") Signed-off-by: David Howells <dhowells@redhat.com> cc: Hugh Dickins <hughd@google.com> cc: Christoph Hellwig <hch@lst.de> cc: Jens Axboe <axboe@kernel.dk> cc: Al Viro <viro@zeniv.linux.org.uk> cc: John Hubbard <jhubbard@nvidia.com> cc: David Hildenbrand <david@redhat.com> cc: Matthew Wilcox <willy@infradead.org> cc: Chuck Lever <chuck.lever@oracle.com> cc: linux-block@vger.kernel.org cc: linux-fsdevel@vger.kernel.org cc: linux-mm@kvack.org --- mm/shmem.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)