[PATCH 0/2] shmem, splice: Fixes for shmem_splice_read()

[PATCH 0/2] shmem, splice: Fixes for shmem_splice_read()

[David Howells]

Hi Hugh,

Here are a couple of fix patches for shmem_splice_read():

 (1) Fix the splicing of a zero_page in place of a missing page.  This should
     only splice in the calculated part of the page and everything to the end
     of the page.

 (2) Apply a couple of fixes already applied to filemap_splice_read(),
     including using in->f_mapping_host rather than file_inode(in) and
     ignoring splices that start at or after s_maxbytes.

I've pushed the patches here also:

	https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=splice-fixes

David

David Howells (2):
  shmem: Fix splice of a missing page
  shmem: Apply a couple of filemap_splice_read() fixes to
    shmem_splice_read()

 mm/shmem.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

[PATCH 1/2] shmem: Fix splice of a missing page

[David Howells]

Fix shmem_splice_read() to splice only part of the partial page at the end
of a splice and not all of it.

This can be seen by splicing from a tmpfs file that's not a multiple of
PAGE_SIZE in size.  Without this fix, it splices extra data to round up to
PAGE_SIZE.

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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/shmem.c b/mm/shmem.c
index 2f2e0e618072..0164cccdcd71 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -2841,7 +2841,7 @@ static ssize_t shmem_file_splice_read(struct file *in, loff_t *ppos,
 			folio_put(folio);
 			folio = NULL;
 		} else {
-			n = splice_zeropage_into_pipe(pipe, *ppos, len);
+			n = splice_zeropage_into_pipe(pipe, *ppos, part);
 		}
 
 		if (!n)

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

[David Howells]

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(-)

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);

Re: [PATCH 1/2] shmem: Fix splice of a missing page

[Andrew Morton]

On Thu, 27 Jul 2023 17:10:15 +0100 David Howells <dhowells@redhat.com> wrote:

> Fix shmem_splice_read() to splice only part of the partial page at the end
> of a splice and not all of it.
> 
> This can be seen by splicing from a tmpfs file that's not a multiple of
> PAGE_SIZE in size.  Without this fix, it splices extra data to round up to
> PAGE_SIZE.

This is already in mm-unstable (and hence linux-next) via Hugh's
"shmem: minor fixes to splice-read implementation"
(https://lkml.kernel.org/r/32c72c9c-72a8-115f-407d-f0148f368@google.com)

Re: [PATCH 1/2] shmem: Fix splice of a missing page

[David Howells]

Andrew Morton <akpm@linux-foundation.org> wrote:

> This is already in mm-unstable (and hence linux-next) via Hugh's
> "shmem: minor fixes to splice-read implementation"
> (https://lkml.kernel.org/r/32c72c9c-72a8-115f-407d-f0148f368@google.com)

And I've already reviewed it:-)

David

Re: [PATCH 1/2] shmem: Fix splice of a missing page

[Hugh Dickins]

On Thu, 27 Jul 2023, David Howells wrote:
> Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > This is already in mm-unstable (and hence linux-next) via Hugh's
> > "shmem: minor fixes to splice-read implementation"
> > (https://lkml.kernel.org/r/32c72c9c-72a8-115f-407d-f0148f368@google.com)
> 
> And I've already reviewed it:-)

I'm not sure whether that ":-)" is implying (good-natured) denial.

You reviewed the original on 17 April, when Jens took it into his tree;
then it vanished in a rewrite, and you didn't respond when I asked about
that on 28 June; then you were Cc'ed when I sent it to Andrew on 23 July
(where I explained about dropping two mods but keeping your Reviewed-by).

This version that Andrew has in mm-unstable includes the hwpoison fix
that we agreed on before, in addition to the len -> part fix.

Hugh

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

[Hugh Dickins]

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

Re: [PATCH 1/2] shmem: Fix splice of a missing page

[David Howells]

Hugh Dickins <hughd@google.com> wrote:

> On Thu, 27 Jul 2023, David Howells wrote:
> > Andrew Morton <akpm@linux-foundation.org> wrote:
> > 
> > > This is already in mm-unstable (and hence linux-next) via Hugh's
> > > "shmem: minor fixes to splice-read implementation"
> > > (https://lkml.kernel.org/r/32c72c9c-72a8-115f-407d-f0148f368@google.com)
> > 
> > And I've already reviewed it:-)
> 
> I'm not sure whether that ":-)" is implying (good-natured) denial.

I've reviewed it, and the review still seems good.

> You reviewed the original on 17 April, when Jens took it into his tree;
> then it vanished in a rewrite, and you didn't respond when I asked about
> that on 28 June;

I missed it in the rush to try and get everything debugged during the merge
window prior to going on holiday.

> then you were Cc'ed when I sent it to Andrew on 23 July (where I explained
> about dropping two mods but keeping your Reviewed-by).

Hmmm...  I don't find that one in my inbox.

> This version that Andrew has in mm-unstable includes the hwpoison fix
> that we agreed on before, in addition to the len -> part fix.

Ok.

David

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

[David Howells]

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