Re: [PATCH 2/2] shmem: Apply a couple of filemap_splice_read() fixes to shmem_splice_read()

[Hugh Dickins <hughd@google.com>]

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