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@xxxxxxxxxx>
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@xxxxxxxxxx>
> cc: Christoph Hellwig <hch@xxxxxx>
> cc: Jens Axboe <axboe@xxxxxxxxx>
> cc: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> cc: John Hubbard <jhubbard@xxxxxxxxxx>
> cc: David Hildenbrand <david@xxxxxxxxxx>
> cc: Matthew Wilcox <willy@xxxxxxxxxxxxx>
> cc: Chuck Lever <chuck.lever@xxxxxxxxxx>
> cc: linux-block@xxxxxxxxxxxxxxx
> cc: linux-fsdevel@xxxxxxxxxxxxxxx
> cc: linux-mm@xxxxxxxxx
> ---
> 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
