diff mbox series

fs: read_write: make default in vfs_copy_file_range() reachable

Message ID 20231205001620.4566-1-spasswolf@web.de (mailing list archive)
State New, archived
Headers show
Series fs: read_write: make default in vfs_copy_file_range() reachable | expand

Commit Message

Bert Karwatzki Dec. 5, 2023, 12:16 a.m. UTC 
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 <spasswolf@web.de>
---
 fs/read_write.c | 2 ++
 1 file changed, 2 insertions(+)

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

Comments

Amir Goldstein Dec. 5, 2023, 3:45 a.m. UTC | #1 
On Tue, Dec 5, 2023 at 2:16 AM Bert Karwatzki <spasswolf@web.de> 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 <spasswolf@web.de>

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 <spasswolf@web.de>
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
Amir Goldstein Dec. 5, 2023, 5:01 a.m. UTC | #2 
On Tue, Dec 5, 2023 at 5:45 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Tue, Dec 5, 2023 at 2:16 AM Bert Karwatzki <spasswolf@web.de> 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 <spasswolf@web.de>
>
> 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 <spasswolf@web.de>
> 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
Bert Karwatzki Dec. 5, 2023, 9:50 a.m. UTC | #3 
Am Dienstag, dem 05.12.2023 um 07:01 +0200 schrieb Amir Goldstein:
> On Tue, Dec 5, 2023 at 5:45 AM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > On Tue, Dec 5, 2023 at 2:16 AM Bert Karwatzki <spasswolf@web.de> 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 <spasswolf@web.de>
> >
> > 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 <spasswolf@web.de>
> > 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
diff mbox series

Patch

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