On Thu, Nov 30, 2023 at 3:18 PM Christian Brauner <brauner@xxxxxxxxxx> wrote:
>
> On Wed, Nov 29, 2023 at 10:07:08PM +0200, Amir Goldstein wrote:
> > The new helper is meant to be called from context of ->copy_file_range()
> > methods instead of do_splice_direct().
> >
> > Currently, the only difference is that do_splice_copy_file_range() does
> > not take a splice flags argument and it asserts that file_start_write()
> > was called.
> >
> > Soon, do_splice_direct() will be called without file_start_write() held.
> >
> > Use the new helper from __ceph_copy_file_range(), that was incorrectly
> > passing the copy_file_range() flags argument as splice flags argument
> > to do_splice_direct(). the value of flags was 0, so no actual bug fix.
> >
> > Move the definition of both helpers to linux/splice.h.
> >
> > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> > ---
> > fs/ceph/file.c | 9 ++---
> > fs/read_write.c | 6 ++--
> > fs/splice.c | 82 ++++++++++++++++++++++++++++++------------
> > include/linux/fs.h | 2 --
> > include/linux/splice.h | 13 ++++---
> > 5 files changed, 75 insertions(+), 37 deletions(-)
> >
> > diff --git a/fs/ceph/file.c b/fs/ceph/file.c
> > index 3b5aae29e944..7c2db78e2c6e 100644
> > --- a/fs/ceph/file.c
> > +++ b/fs/ceph/file.c
> > @@ -12,6 +12,7 @@
> > #include <linux/falloc.h>
> > #include <linux/iversion.h>
> > #include <linux/ktime.h>
> > +#include <linux/splice.h>
> >
> > #include "super.h"
> > #include "mds_client.h"
> > @@ -3010,8 +3011,8 @@ static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off,
> > * {read,write}_iter, which will get caps again.
> > */
> > put_rd_wr_caps(src_ci, src_got, dst_ci, dst_got);
> > - ret = do_splice_direct(src_file, &src_off, dst_file,
> > - &dst_off, src_objlen, flags);
> > + ret = do_splice_copy_file_range(src_file, &src_off, dst_file,
> > + &dst_off, src_objlen);
> > /* Abort on short copies or on error */
> > if (ret < (long)src_objlen) {
> > doutc(cl, "Failed partial copy (%zd)\n", ret);
> > @@ -3065,8 +3066,8 @@ static ssize_t __ceph_copy_file_range(struct file *src_file, loff_t src_off,
> > */
> > if (len && (len < src_ci->i_layout.object_size)) {
> > doutc(cl, "Final partial copy of %zu bytes\n", len);
> > - bytes = do_splice_direct(src_file, &src_off, dst_file,
> > - &dst_off, len, flags);
> > + bytes = do_splice_copy_file_range(src_file, &src_off, dst_file,
> > + &dst_off, len);
> > if (bytes > 0)
> > ret += bytes;
> > else
> > diff --git a/fs/read_write.c b/fs/read_write.c
> > index f791555fa246..555514cdad53 100644
> > --- a/fs/read_write.c
> > +++ b/fs/read_write.c
> > @@ -1423,10 +1423,8 @@ ssize_t generic_copy_file_range(struct file *file_in, loff_t pos_in,
> > struct file *file_out, loff_t pos_out,
> > size_t len, unsigned int flags)
> > {
>
> Hm, the low-level helper takes a @flags argument but it's completely
> ignored. I think that helper should remove it or it should check:
>
> if (flags)
> return -EINVAL;
>
It's a good point.
The upstream code and in this v1, generic_copy_file_range() can actually
be called with flag COPY_FILE_SPLICE, but it is a mistake that
I fixed it in my branch for v2, so in v2 I can add this check.
> in case it's ever called from codepaths where @flags hasn't been
> sanitized imho.
>
> > - lockdep_assert(file_write_started(file_out));
> > -
> > - return do_splice_direct(file_in, &pos_in, file_out, &pos_out,
> > - len > MAX_RW_COUNT ? MAX_RW_COUNT : len, 0);
> > + return do_splice_copy_file_range(file_in, &pos_in, file_out, &pos_out,
> > + len > MAX_RW_COUNT ? MAX_RW_COUNT : len);
>
> clamp(len, 0, MAX_RW_COUNT)
>
It is a low level helper, so I don't want to worry about negative len value.
Already changed to min_t(size_t, len, MAX_RW_COUNT) in my branch.
Thanks!
Amir.
