Re: [PATCH 3/3] ovl: Use splice_with_holes in copy_up

[Dave Chinner <david@fromorbit.com>]

On Thu, May 03, 2018 at 10:57:09PM +0300, Amir Goldstein wrote:
> On Thu, May 3, 2018 at 6:26 PM, Goldwyn Rodrigues <rgoldwyn@suse.de> wrote:
> > From: Goldwyn Rodrigues <rgoldwyn@suse.com>
> >
> > Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> > ---
> >  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".

Yup, we want the copy offload infrastructure to be used if at all
possible, so we get consistent behaviour for everyone trying to
optimise copy behaviour.  In this case, I think that it is relevant
that we heard at LSFMM that userspace utils don't want to use
copy_file_range() because it doesn't "optimise for sparse files".

>From that perspective, perhaps this needs "hole preserving copy"
behaviour needs to be moved inside copy-file_range() (and therefore
do_splice_direct()) and triggered by a new flag for
copy_file_range(). e.g. COPY_FILE_SPARSE. That way callers can tell
the kernel they want a sparse copy, and the kernel can attempt that
rather a copy that converts holes to zeros.

In most cases, filesystems that implement efficient offloads already
preserve sparseness, but the do_splice_direct() fallback does not.
If we fix that, then we're a big step closer to getting utilities
like cp and rsync to use copy_file_range() instead of bit shuffling
through userspace to copy data.....

> I am not asking that you do this as part of your work, simply
> pointing out an opportunity.

*nod*

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com