* [PATCH next] shmem: minor fixes to splice-read implementation
@ 2023-04-17 4:46 Hugh Dickins
2023-04-17 7:18 ` David Howells
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Hugh Dickins @ 2023-04-17 4:46 UTC (permalink / raw)
To: David Howells
Cc: Jens Axboe, Andrew Morton, Matthew Wilcox, Naoya Horiguchi,
David Hildenbrand, Yang Shi, linux-block, linux-fsdevel,
linux-mm
generic_file_splice_read() makes a couple of preliminary checks (for
s_maxbytes and zero len), but shmem_file_splice_read() is called without
those: so check them inside it. (But shmem does not support O_DIRECT,
so no need for that one here - and even if O_DIRECT support were stubbed
in, it would still just be using the page cache.)
HWPoison: my reading of folio_test_hwpoison() is that it only tests the
head page of a large folio, whereas splice_folio_into_pipe() will splice
as much of the folio as it can: so for safety we should also check the
has_hwpoisoned flag, set if any of the folio's pages are hwpoisoned.
(Perhaps that ugliness can be improved at the mm end later.)
The call to splice_zeropage_into_pipe() risked overrunning past EOF:
ask it for "part" not "len".
Fixes: b81d7b89becc ("shmem: Implement splice-read")
Signed-off-by: Hugh Dickins <hughd@google.com>
---
Thank you, David, for attending to tmpfs in your splice update:
yes, I too wish it could have just used the generic, but I'm sure
you're right that there's a number of reasons it needs its own.
mm/shmem.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -2902,6 +2902,11 @@ static ssize_t shmem_file_splice_read(struct file *in, loff_t *ppos,
loff_t isize;
int error = 0;
+ if (unlikely(*ppos >= MAX_LFS_FILESIZE))
+ return 0;
+ if (unlikely(!len))
+ 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);
@@ -2911,7 +2916,8 @@ static ssize_t shmem_file_splice_read(struct file *in, loff_t *ppos,
if (*ppos >= i_size_read(inode))
break;
- error = shmem_get_folio(inode, *ppos / PAGE_SIZE, &folio, SGP_READ);
+ error = shmem_get_folio(inode, *ppos / PAGE_SIZE, &folio,
+ SGP_READ);
if (error) {
if (error == -EINVAL)
error = 0;
@@ -2920,7 +2926,9 @@ static ssize_t shmem_file_splice_read(struct file *in, loff_t *ppos,
if (folio) {
folio_unlock(folio);
- if (folio_test_hwpoison(folio)) {
+ if (folio_test_hwpoison(folio) ||
+ (folio_test_large(folio) &&
+ folio_test_has_hwpoisoned(folio))) {
error = -EIO;
break;
}
@@ -2956,7 +2964,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)
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH next] shmem: minor fixes to splice-read implementation
2023-04-17 4:46 [PATCH next] shmem: minor fixes to splice-read implementation Hugh Dickins
@ 2023-04-17 7:18 ` David Howells
2023-04-17 11:21 ` David Hildenbrand
2023-04-17 13:48 ` Jens Axboe
2 siblings, 0 replies; 5+ messages in thread
From: David Howells @ 2023-04-17 7:18 UTC (permalink / raw)
To: Hugh Dickins
Cc: dhowells, Jens Axboe, Andrew Morton, Matthew Wilcox,
Naoya Horiguchi, David Hildenbrand, Yang Shi, linux-block,
linux-fsdevel, linux-mm
Hugh Dickins <hughd@google.com> wrote:
> + if (unlikely(!len))
> + return 0;
Should this be done in do_splice_to() instead?
Also, is it too late for Jens to fold this into the original patch since it's
not upstream yet?
Otherwise:
Reviewed-by: David Howells <dhowells@redhat.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH next] shmem: minor fixes to splice-read implementation
2023-04-17 4:46 [PATCH next] shmem: minor fixes to splice-read implementation Hugh Dickins
2023-04-17 7:18 ` David Howells
@ 2023-04-17 11:21 ` David Hildenbrand
2023-04-17 13:48 ` Jens Axboe
2 siblings, 0 replies; 5+ messages in thread
From: David Hildenbrand @ 2023-04-17 11:21 UTC (permalink / raw)
To: Hugh Dickins, David Howells
Cc: Jens Axboe, Andrew Morton, Matthew Wilcox, Naoya Horiguchi,
Yang Shi, linux-block, linux-fsdevel, linux-mm
On 17.04.23 06:46, Hugh Dickins wrote:
> generic_file_splice_read() makes a couple of preliminary checks (for
> s_maxbytes and zero len), but shmem_file_splice_read() is called without
> those: so check them inside it. (But shmem does not support O_DIRECT,
> so no need for that one here - and even if O_DIRECT support were stubbed
> in, it would still just be using the page cache.)
>
> HWPoison: my reading of folio_test_hwpoison() is that it only tests the
> head page of a large folio, whereas splice_folio_into_pipe() will splice
> as much of the folio as it can: so for safety we should also check the
> has_hwpoisoned flag, set if any of the folio's pages are hwpoisoned.
> (Perhaps that ugliness can be improved at the mm end later.)
>
> The call to splice_zeropage_into_pipe() risked overrunning past EOF:
> ask it for "part" not "len".
>
> Fixes: b81d7b89becc ("shmem: Implement splice-read")
> Signed-off-by: Hugh Dickins <hughd@google.com>
> ---
> Thank you, David, for attending to tmpfs in your splice update:
> yes, I too wish it could have just used the generic, but I'm sure
> you're right that there's a number of reasons it needs its own.
>
> mm/shmem.c | 14 +++++++++++---
> 1 file changed, 11 insertions(+), 3 deletions(-)
>
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -2902,6 +2902,11 @@ static ssize_t shmem_file_splice_read(struct file *in, loff_t *ppos,
> loff_t isize;
> int error = 0;
>
> + if (unlikely(*ppos >= MAX_LFS_FILESIZE))
> + return 0;
> + if (unlikely(!len))
> + 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);
> @@ -2911,7 +2916,8 @@ static ssize_t shmem_file_splice_read(struct file *in, loff_t *ppos,
> if (*ppos >= i_size_read(inode))
> break;
>
> - error = shmem_get_folio(inode, *ppos / PAGE_SIZE, &folio, SGP_READ);
> + error = shmem_get_folio(inode, *ppos / PAGE_SIZE, &folio,
> + SGP_READ);
> if (error) {
> if (error == -EINVAL)
> error = 0;
> @@ -2920,7 +2926,9 @@ static ssize_t shmem_file_splice_read(struct file *in, loff_t *ppos,
> if (folio) {
> folio_unlock(folio);
>
> - if (folio_test_hwpoison(folio)) {
> + if (folio_test_hwpoison(folio) ||
> + (folio_test_large(folio) &&
> + folio_test_has_hwpoisoned(folio))) {
> error = -EIO;
> break;
> }
> @@ -2956,7 +2964,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)
>
FWIW, looks good to me.
--
Thanks,
David / dhildenb
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH next] shmem: minor fixes to splice-read implementation
2023-04-17 4:46 [PATCH next] shmem: minor fixes to splice-read implementation Hugh Dickins
2023-04-17 7:18 ` David Howells
2023-04-17 11:21 ` David Hildenbrand
@ 2023-04-17 13:48 ` Jens Axboe
2023-06-29 4:42 ` Hugh Dickins
2 siblings, 1 reply; 5+ messages in thread
From: Jens Axboe @ 2023-04-17 13:48 UTC (permalink / raw)
To: David Howells, Hugh Dickins
Cc: Andrew Morton, Matthew Wilcox, Naoya Horiguchi,
David Hildenbrand, Yang Shi, linux-block, linux-fsdevel,
linux-mm
On Sun, 16 Apr 2023 21:46:16 -0700, Hugh Dickins wrote:
> generic_file_splice_read() makes a couple of preliminary checks (for
> s_maxbytes and zero len), but shmem_file_splice_read() is called without
> those: so check them inside it. (But shmem does not support O_DIRECT,
> so no need for that one here - and even if O_DIRECT support were stubbed
> in, it would still just be using the page cache.)
>
> HWPoison: my reading of folio_test_hwpoison() is that it only tests the
> head page of a large folio, whereas splice_folio_into_pipe() will splice
> as much of the folio as it can: so for safety we should also check the
> has_hwpoisoned flag, set if any of the folio's pages are hwpoisoned.
> (Perhaps that ugliness can be improved at the mm end later.)
>
> [...]
Applied, thanks!
[1/1] shmem: minor fixes to splice-read implementation
commit: 72887c976a7c9ee7527f4a2e3d109576efea98ab
Best regards,
--
Jens Axboe
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH next] shmem: minor fixes to splice-read implementation
2023-04-17 13:48 ` Jens Axboe
@ 2023-06-29 4:42 ` Hugh Dickins
0 siblings, 0 replies; 5+ messages in thread
From: Hugh Dickins @ 2023-06-29 4:42 UTC (permalink / raw)
To: Jens Axboe
Cc: David Howells, Hugh Dickins, Andrew Morton, Matthew Wilcox,
Naoya Horiguchi, David Hildenbrand, Yang Shi, linux-block,
linux-fsdevel, linux-mm
On Mon, 17 Apr 2023, Jens Axboe wrote:
> On Sun, 16 Apr 2023 21:46:16 -0700, Hugh Dickins wrote:
> > generic_file_splice_read() makes a couple of preliminary checks (for
> > s_maxbytes and zero len), but shmem_file_splice_read() is called without
> > those: so check them inside it. (But shmem does not support O_DIRECT,
> > so no need for that one here - and even if O_DIRECT support were stubbed
> > in, it would still just be using the page cache.)
> >
> > HWPoison: my reading of folio_test_hwpoison() is that it only tests the
> > head page of a large folio, whereas splice_folio_into_pipe() will splice
> > as much of the folio as it can: so for safety we should also check the
> > has_hwpoisoned flag, set if any of the folio's pages are hwpoisoned.
> > (Perhaps that ugliness can be improved at the mm end later.)
> >
> > [...]
>
> Applied, thanks!
>
> [1/1] shmem: minor fixes to splice-read implementation
> commit: 72887c976a7c9ee7527f4a2e3d109576efea98ab
>
> Best regards,
> --
> Jens Axboe
Thanks, but it then vanished amidst the subsequent splice convulsions,
and I can't quite tell how much of it is still needed - looks like the
!len check is now done in vfs_splice_read(), but I didn't work out
what happened to ppos and s_maxbytes; the hwpoison and "part" mods
still needed, surely?
Hugh
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-06-29 4:42 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-04-17 4:46 [PATCH next] shmem: minor fixes to splice-read implementation Hugh Dickins
2023-04-17 7:18 ` David Howells
2023-04-17 11:21 ` David Hildenbrand
2023-04-17 13:48 ` Jens Axboe
2023-06-29 4:42 ` Hugh Dickins
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).