If vfs_copy_file_range() is called with (flags & COPY_FILE_SPLICE == 0)
and both file_out->f_op->copy_file_range and file_in->f_op->remap_file_range
are NULL, too, the default call to do_splice_direct() cannot be reached.
This patch adds an else clause to make the default reachable in all
cases.
Signed-off-by: Bert Karwatzki <[email protected]>
---
fs/read_write.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/fs/read_write.c b/fs/read_write.c
index e0c2c1b5962b..3599c54bd26d 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1554,6 +1554,8 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
/* fallback to splice */
if (ret <= 0)
splice = true;
+ } else {
+ splice = true;
}
file_end_write(file_out);
--
2.39.2
Since linux-next-20231204 I noticed that it was impossible to start the
game Path of Exile (using the steam client). I bisected the error to
commit 05ee2d85cd4ace5cd37dc24132e3fd7f5142ebef. Reverting this commit
in linux-next-20231204 made the game start again and after inserting
printks into vfs_copy_file_range() I found that steam (via python3)
calls this function with (flags & COPY_FILE_SPLICE == 0),
file_out->f_op->copy_file_range == NULL and
file_in->f_op->remap_file_range == NULL so the default is never reached.
This patch adds a catch all else clause so the default is reached in
all cases. This patch fixes the describe issue with steam and Path of
Exile.
Bert Karwatzki
On Tue, Dec 5, 2023 at 2:16 AM Bert Karwatzki <[email protected]> wrote:
>
> If vfs_copy_file_range() is called with (flags & COPY_FILE_SPLICE == 0)
> and both file_out->f_op->copy_file_range and file_in->f_op->remap_file_range
> are NULL, too, the default call to do_splice_direct() cannot be reached.
> This patch adds an else clause to make the default reachable in all
> cases.
>
> Signed-off-by: Bert Karwatzki <[email protected]>
Hi Bert,
Thank you for testing and reporting this so early!!
I would edit the commit message differently, but anyway, I think that
the fix should be folded into commit 05ee2d85cd4a ("fs: use
do_splice_direct() for nfsd/ksmbd server-side-copy").
Since I end up making a mistake every time I touch this code,
I also added a small edit to your patch below, that should make the logic
more clear to readers. Hopefully, that will help me avoid making a mistake
the next time I touch this code...
Would you mind testing my revised fix, so we can add:
Tested-by: Bert Karwatzki <[email protected]>
when folding it into the original patch?
Thanks,
Amir.
> ---
> fs/read_write.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/fs/read_write.c b/fs/read_write.c
> index e0c2c1b5962b..3599c54bd26d 100644
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -1554,6 +1554,8 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
> /* fallback to splice */
> if (ret <= 0)
> splice = true;
> + } else {
This is logically correct because of the earlier "same sb" check in
generic_copy_file_checks(), but we better spell out the logic here as well:
+ } else if (file_inode(file_in)->i_sb ==
file_inode(file_out)->i_sb) {
+ /* Fallback to splice for same sb copy for
backward compat */
> + splice = true;
> }
>
> file_end_write(file_out);
> --
> 2.39.2
>
> Since linux-next-20231204 I noticed that it was impossible to start the
> game Path of Exile (using the steam client). I bisected the error to
> commit 05ee2d85cd4ace5cd37dc24132e3fd7f5142ebef. Reverting this commit
> in linux-next-20231204 made the game start again and after inserting
> printks into vfs_copy_file_range() I found that steam (via python3)
> calls this function with (flags & COPY_FILE_SPLICE == 0),
> file_out->f_op->copy_file_range == NULL and
> file_in->f_op->remap_file_range == NULL so the default is never reached.
> This patch adds a catch all else clause so the default is reached in
> all cases. This patch fixes the describe issue with steam and Path of
> Exile.
>
> Bert Karwatzki
On Tue, Dec 5, 2023 at 5:45 AM Amir Goldstein <[email protected]> wrote:
>
> On Tue, Dec 5, 2023 at 2:16 AM Bert Karwatzki <[email protected]> wrote:
> >
> > If vfs_copy_file_range() is called with (flags & COPY_FILE_SPLICE == 0)
> > and both file_out->f_op->copy_file_range and file_in->f_op->remap_file_range
> > are NULL, too, the default call to do_splice_direct() cannot be reached.
> > This patch adds an else clause to make the default reachable in all
> > cases.
> >
> > Signed-off-by: Bert Karwatzki <[email protected]>
>
> Hi Bert,
>
> Thank you for testing and reporting this so early!!
>
> I would edit the commit message differently, but anyway, I think that
> the fix should be folded into commit 05ee2d85cd4a ("fs: use
> do_splice_direct() for nfsd/ksmbd server-side-copy").
>
> Since I end up making a mistake every time I touch this code,
> I also added a small edit to your patch below, that should make the logic
> more clear to readers. Hopefully, that will help me avoid making a mistake
> the next time I touch this code...
>
> Would you mind testing my revised fix, so we can add:
> Tested-by: Bert Karwatzki <[email protected]>
> when folding it into the original patch?
>
Attached an even cleaner version of the fix patch for you to test.
I tested fstests check -g copy_range on ext4.
My fault was that I had tested earlier only on xfs and overlayfs
(the two other cases in the if/else if statement).
Thanks,
Amir.
> > ---
> > fs/read_write.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/fs/read_write.c b/fs/read_write.c
> > index e0c2c1b5962b..3599c54bd26d 100644
> > --- a/fs/read_write.c
> > +++ b/fs/read_write.c
> > @@ -1554,6 +1554,8 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
> > /* fallback to splice */
> > if (ret <= 0)
> > splice = true;
> > + } else {
>
> This is logically correct because of the earlier "same sb" check in
> generic_copy_file_checks(), but we better spell out the logic here as well:
>
> + } else if (file_inode(file_in)->i_sb ==
> file_inode(file_out)->i_sb) {
> + /* Fallback to splice for same sb copy for
> backward compat */
>
> > + splice = true;
> > }
> >
> > file_end_write(file_out);
> > --
> > 2.39.2
> >
> > Since linux-next-20231204 I noticed that it was impossible to start the
> > game Path of Exile (using the steam client). I bisected the error to
> > commit 05ee2d85cd4ace5cd37dc24132e3fd7f5142ebef. Reverting this commit
> > in linux-next-20231204 made the game start again and after inserting
> > printks into vfs_copy_file_range() I found that steam (via python3)
> > calls this function with (flags & COPY_FILE_SPLICE == 0),
> > file_out->f_op->copy_file_range == NULL and
> > file_in->f_op->remap_file_range == NULL so the default is never reached.
> > This patch adds a catch all else clause so the default is reached in
> > all cases. This patch fixes the describe issue with steam and Path of
> > Exile.
> >
> > Bert Karwatzki
Am Dienstag, dem 05.12.2023 um 07:01 +0200 schrieb Amir Goldstein:
> On Tue, Dec 5, 2023 at 5:45 AM Amir Goldstein <[email protected]> wrote:
> >
> > On Tue, Dec 5, 2023 at 2:16 AM Bert Karwatzki <[email protected]> wrote:
> > >
> > > If vfs_copy_file_range() is called with (flags & COPY_FILE_SPLICE == 0)
> > > and both file_out->f_op->copy_file_range and file_in->f_op-
> > > >remap_file_range
> > > are NULL, too, the default call to do_splice_direct() cannot be reached.
> > > This patch adds an else clause to make the default reachable in all
> > > cases.
> > >
> > > Signed-off-by: Bert Karwatzki <[email protected]>
> >
> > Hi Bert,
> >
> > Thank you for testing and reporting this so early!!
> >
> > I would edit the commit message differently, but anyway, I think that
> > the fix should be folded into commit 05ee2d85cd4a ("fs: use
> > do_splice_direct() for nfsd/ksmbd server-side-copy").
> >
> > Since I end up making a mistake every time I touch this code,
> > I also added a small edit to your patch below, that should make the logic
> > more clear to readers. Hopefully, that will help me avoid making a mistake
> > the next time I touch this code...
> >
> > Would you mind testing my revised fix, so we can add:
> > Tested-by: Bert Karwatzki <[email protected]>
> > when folding it into the original patch?
> >
>
> Attached an even cleaner version of the fix patch for you to test.
> I tested fstests check -g copy_range on ext4.
> My fault was that I had tested earlier only on xfs and overlayfs
> (the two other cases in the if/else if statement).
>
> Thanks,
> Amir.
>
> > > ---
> > > fs/read_write.c | 2 ++
> > > 1 file changed, 2 insertions(+)
> > >
> > > diff --git a/fs/read_write.c b/fs/read_write.c
> > > index e0c2c1b5962b..3599c54bd26d 100644
> > > --- a/fs/read_write.c
> > > +++ b/fs/read_write.c
> > > @@ -1554,6 +1554,8 @@ ssize_t vfs_copy_file_range(struct file *file_in,
> > > loff_t pos_in,
> > > /* fallback to splice */
> > > if (ret <= 0)
> > > splice = true;
> > > + } else {
> >
> > This is logically correct because of the earlier "same sb" check in
> > generic_copy_file_checks(), but we better spell out the logic here as well:
> >
> > + } else if (file_inode(file_in)->i_sb ==
> > file_inode(file_out)->i_sb) {
> > + /* Fallback to splice for same sb copy for
> > backward compat */
> >
> > > + splice = true;
> > > }
> > >
> > > file_end_write(file_out);
> > > --
> > > 2.39.2
> > >
> > > Since linux-next-20231204 I noticed that it was impossible to start the
> > > game Path of Exile (using the steam client). I bisected the error to
> > > commit 05ee2d85cd4ace5cd37dc24132e3fd7f5142ebef. Reverting this commit
> > > in linux-next-20231204 made the game start again and after inserting
> > > printks into vfs_copy_file_range() I found that steam (via python3)
> > > calls this function with (flags & COPY_FILE_SPLICE == 0),
> > > file_out->f_op->copy_file_range == NULL and
> > > file_in->f_op->remap_file_range == NULL so the default is never reached.
> > > This patch adds a catch all else clause so the default is reached in
> > > all cases. This patch fixes the describe issue with steam and Path of
> > > Exile.
> > >
> > > Bert Karwatzki
Your new patch works fine (I applied it to linux-next-20231204), again tested by
starting Path of Exile via steam from an ext4 filesystem.
Bert Karwatzki