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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux