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