---- 在 星期二, 2019-10-15 22:26:35 Amir Goldstein <amir73il@xxxxxxxxx> 撰写 ----
> On Tue, Oct 15, 2019 at 2:38 PM Chengguang Xu <cgxu519@xxxxxxxxxxxx> wrote:
> >
> > ---- 在 星期五, 2019-10-04 21:20:30 Chengguang Xu <cgxu519@xxxxxxxxxxxx> 撰写 ----
> > > Current copy-up is not efficient for sparse file,
> > > It's not only slow but also wasting more disk space
> > > when the target lower file has huge hole inside.
> > > This patch tries to recognize file hole and skip it
> > > during copy-up.
> > >
> > > In detail, this optimization checks the hole according
> > > to copy-up chunk size so it may not recognize all kind
> > > of holes in the file. However, it is easy to implement
> > > and will be enough for most of the time.
>
> I must say I do not see how aligning to copy-up chunk size
> simplifies the change. Why is that more complicated?
Hi Amir,
Thanks for your feedback.
I would like to say if we do not align to copy-up chunk size then we have to recognize exact size of
every hole in the lower file and that may need relying on both SEEK_HOLE and SEEK_DATA methods.
However, currently SEEK_HOLE implementation seems not so reliable. Look deep into the code,
generic_file_llseek() could not work correctly for SEEK_HOLE in our check, so it means for filesystems
which don't have their own version of f_op->llseek function will get problem.
>
> if (old_next_data_pos >= old_pos) {
> hole_len = old_next_data_pos - old_pos;
> ...
>
> It can still copy hole up to this_len, because there is no
> SEEK_HOLE, so you can document that.
I'll do.
>
> > >
> > > Additionally, this optimization relies on lseek(2)
> > > SEEK_DATA implementation, so for some specific
> > > filesystems which do not support this feature
> > > will behave as before on copy-up.
> > >
>
> I am not sure if we support any lower fs with no f_op->llseek
> in that case, copy up will not behave as before - it will return
> -ESPIPE and will be a regression.
We need adding a check for lower fs llseek support and catching
llseek error for safety. If we notice the optimization could not work
then we back to original copy-up behavior.
>
> > > Signed-off-by: Chengguang Xu <cgxu519@xxxxxxxxxxxx>
> > > ---
> >
> > Any better idea or suggestion for this?
>
> This change should be accompanied with proper xfstests examining
> all sorts of sparse files.
> See overlay/001 and _run_seek_sanity_test for inspiration.
>
> Perhaps you can run all _run_seek_sanity_test tests on
> lower fs, then mount overlay. copy up all the sanity test
> files and then check something???
>
Yeah, thanks for your suggestion, I'll do more work on testing.
Thanks,
Chengguang
>
>
> >
> >
> > > fs/overlayfs/copy_up.c | 15 ++++++++++++++-
> > > 1 file changed, 14 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> > > index b801c6353100..028033c9f021 100644
> > > --- a/fs/overlayfs/copy_up.c
> > > +++ b/fs/overlayfs/copy_up.c
> > > @@ -144,10 +144,11 @@ static int ovl_copy_up_data(struct path *old, struct path *new, loff_t len)
> > > goto out;
> > > /* Couldn't clone, so now we try to copy the data */
> > >
> > > - /* FIXME: copy up sparse files efficiently */
> > > while (len) {
> > > size_t this_len = OVL_COPY_UP_CHUNK_SIZE;
> > > long bytes;
> > > + loff_t old_next_data_pos;
> > > + loff_t hole_len;
> > >
> > > if (len < this_len)
> > > this_len = len;
> > > @@ -157,6 +158,18 @@ static int ovl_copy_up_data(struct path *old, struct path *new, loff_t len)
> > > break;
> > > }
> > >
> > > + old_next_data_pos = vfs_llseek(old_file, old_pos, SEEK_DATA);
> > > + if (old_next_data_pos >= old_pos + OVL_COPY_UP_CHUNK_SIZE) {
> > > + hole_len = (old_next_data_pos - old_pos) /
> > > + OVL_COPY_UP_CHUNK_SIZE * OVL_COPY_UP_CHUNK_SIZE;
> > > + old_pos += hole_len;
> > > + new_pos += hole_len;
> > > + len -= hole_len;
> > > + continue;
> > > + } else if (old_next_data_pos == -ENXIO) {
> > > + break;
> > > + }
> > > +
> > > bytes = do_splice_direct(old_file, &old_pos,
> > > new_file, &new_pos,
> > > this_len, SPLICE_F_MOVE);
> > > --
>
