On Fri, May 4, 2018 at 4:31 AM, Goldwyn Rodrigues <rgoldwyn@xxxxxxx> wrote: > > > On 05/03/2018 02:57 PM, Amir Goldstein wrote: >> On Thu, May 3, 2018 at 6:26 PM, Goldwyn Rodrigues <rgoldwyn@xxxxxxx> wrote: >>> From: Goldwyn Rodrigues <rgoldwyn@xxxxxxxx> >>> >>> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@xxxxxxxx> >>> --- >>> fs/overlayfs/copy_up.c | 2 +- >>> fs/read_write.c | 10 ++++++---- >>> include/linux/fs.h | 2 ++ >>> 3 files changed, 9 insertions(+), 5 deletions(-) >>> >>> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c >>> index 8bede0742619..6634a85255ae 100644 >>> --- a/fs/overlayfs/copy_up.c >>> +++ b/fs/overlayfs/copy_up.c >>> @@ -175,7 +175,7 @@ static int ovl_copy_up_data(struct path *old, struct path *new, loff_t len) >>> break; >>> } >>> >>> - bytes = do_splice_direct(old_file, &old_pos, >>> + bytes = splice_with_holes(old_file, &old_pos, >>> new_file, &new_pos, >>> this_len, SPLICE_F_MOVE); >> >> >> Add.. you can remove this comment above :) >> /* FIXME: copy up sparse files efficiently */ >> >> For the record, when I added vfs_clone_file_range() above, >> Dave Chinner has suggested to replace the entire block with >> vfs_copy_file_range(), which would do all the fallbacks. >> Since then, vfs_copy_file_range() gained "try to clone first". >> >> There are still differences between the loop in this function and >> the loop in vfs_copy_file_range(). Perhaps the differences could >> be smoothed away, I did not check recently. >> > > There is a difference. copy_file_range(2) or do_splice_direct() can > return short writes. I think this loop is performed to cover short writes. > > I suppose we could remove the clone_file_range() before the loop and > replace the do_splice_direct() with vfs_copy_file_range(). > There are other differences, like file_start_write(). To resolve them would probably require factoring out do_copy_file_range() with an argument to determine whether or not the function will try to copy as much as possible by looping. BTW, It's a bit odd that at the moment SPLICE_F_MOVE is only tested by fuse and fuse cannot be an overlayfs upper fs. Maybe I am missing something. Thanks, Amir.
