[PATCH v2] ovl: improving copy-up efficiency for big sparse file

[PATCH v2] ovl: improving copy-up efficiency for big sparse file

[Chengguang Xu]

Current copy-up is not efficient for big 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 use case.

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.

Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
---

Hi Miklos, Amir

This is v2 version of hole copy-up improvement which
addressed amir's concerns in previous email.

Could you have a look at this patch?

There is a checkpatch warning but that is
false-positive warning, so you can ignore it.

I've tested the patch with the cases under overlay dir
(include overlay/066) in fstest for xfs/ext4 fstype, 
and passed most of the cases except below.

overlay/045     [not run] fsck.overlay utility required, skipped this test
overlay/046     [not run] fsck.overlay utility required, skipped this test
overlay/056     [not run] fsck.overlay utility required, skipped this test

Above three cases are fsck related cases,
I think they are not important for copy-up.

overlay/061     - output mismatch (see /home/cgxu/git/xfstests-dev/results//overlay/061.out.bad)
    --- tests/overlay/061.out   2019-05-28 09:54:42.320874925 +0800
    +++ /home/cgxu/git/xfstests-dev/results//overlay/061.out.bad        2019-10-30 16:11:50.490848367 +0800
    @@ -1,4 +1,4 @@
     QA output created by 061
    -00000000:  61 61 61 61 61 61 61 61 61 61 61 61 61 61 61 61  aaaaaaaaaaaaaaaa
    +00000000:  54 68 69 73 20 69 73 20 6f 6c 64 20 6e 65 77 73  This.is.old.news
     After mount cycle:
     00000000:  61 61 61 61 61 61 61 61 61 61 61 61 61 61 61 61  aaaaaaaaaaaaaaaa
    ...
    (Run 'diff -u /home/cgxu/git/xfstests-dev/tests/overlay/061.out /home/cgxu/git/xfstests-dev/results//overlay/061.out.bad'  to see the entire diff)

overlay/061 was failed with/without my patch and test results
were just same. have something already broken for the test?


v1->v2:
- Set file size when the hole is in the end of the file.
- Add a code comment for hole copy-up improvement.
- Check SEEK_DATA support before doing hole skip.
- Back to original copy-up when seek data fails(in error case).

 fs/overlayfs/copy_up.c | 78 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 64 insertions(+), 14 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index b801c6353100..7d8a34c480f4 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -116,13 +116,30 @@ int ovl_copy_xattr(struct dentry *old, struct dentry *new)
 	return error;
 }
 
-static int ovl_copy_up_data(struct path *old, struct path *new, loff_t len)
+static int ovl_set_size(struct dentry *upperdentry, struct kstat *stat)
+{
+	struct iattr attr = {
+		.ia_valid = ATTR_SIZE,
+		.ia_size = stat->size,
+	};
+
+	return notify_change(upperdentry, &attr, NULL);
+}
+
+static int ovl_copy_up_data(struct path *old, struct path *new,
+			    struct kstat *stat)
 {
 	struct file *old_file;
 	struct file *new_file;
+	loff_t len = stat->size;
 	loff_t old_pos = 0;
 	loff_t new_pos = 0;
 	loff_t cloned;
+	loff_t old_next_data_pos;
+	loff_t hole_len;
+	bool seek_support = false;
+	bool skip_hole = true;
+	bool set_size = false;
 	int error = 0;
 
 	if (len == 0)
@@ -144,7 +161,12 @@ 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 */
+	/* Check if lower fs supports seek operation */
+	if (old_file->f_mode & FMODE_LSEEK &&
+	    old_file->f_op->llseek) {
+		seek_support = true;
+	}
+
 	while (len) {
 		size_t this_len = OVL_COPY_UP_CHUNK_SIZE;
 		long bytes;
@@ -157,6 +179,38 @@ static int ovl_copy_up_data(struct path *old, struct path *new, loff_t len)
 			break;
 		}
 
+		/*
+		 * Fill zero for hole will cost unnecessary disk space
+		 * and meanwhile slow down the copy-up speed, so we do
+		 * an optimization for hole during copy-up, it relies
+		 * on SEEK_DATA implementation and the hole check is
+		 * aligned to OVL_COPY_UP_CHUNK_SIZE. In other word,
+		 * we do not try to recognize all kind of holes here,
+		 * we just skip big enough hole for simplicity to
+		 * implement. If lower fs does not support SEEK_DATA
+		 * operation, the copy-up will behave as before.
+		 */
+
+		if (seek_support && skip_hole) {
+			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) {
+				set_size = true;
+				break;
+			} else if (old_next_data_pos < 0) {
+				skip_hole = false;
+			}
+		}
+
 		bytes = do_splice_direct(old_file, &old_pos,
 					 new_file, &new_pos,
 					 this_len, SPLICE_F_MOVE);
@@ -168,6 +222,12 @@ static int ovl_copy_up_data(struct path *old, struct path *new, loff_t len)
 
 		len -= bytes;
 	}
+
+	if (!error && set_size) {
+		inode_lock(new->dentry->d_inode);
+		error = ovl_set_size(new->dentry, stat);
+		inode_unlock(new->dentry->d_inode);
+	}
 out:
 	if (!error)
 		error = vfs_fsync(new_file, 0);
@@ -177,16 +237,6 @@ static int ovl_copy_up_data(struct path *old, struct path *new, loff_t len)
 	return error;
 }
 
-static int ovl_set_size(struct dentry *upperdentry, struct kstat *stat)
-{
-	struct iattr attr = {
-		.ia_valid = ATTR_SIZE,
-		.ia_size = stat->size,
-	};
-
-	return notify_change(upperdentry, &attr, NULL);
-}
-
 static int ovl_set_timestamps(struct dentry *upperdentry, struct kstat *stat)
 {
 	struct iattr attr = {
@@ -453,7 +503,7 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
 		upperpath.dentry = temp;
 
 		ovl_path_lowerdata(c->dentry, &datapath);
-		err = ovl_copy_up_data(&datapath, &upperpath, c->stat.size);
+		err = ovl_copy_up_data(&datapath, &upperpath, &c->stat);
 		if (err)
 			return err;
 	}
@@ -757,7 +807,7 @@ static int ovl_copy_up_meta_inode_data(struct ovl_copy_up_ctx *c)
 			goto out;
 	}
 
-	err = ovl_copy_up_data(&datapath, &upperpath, c->stat.size);
+	err = ovl_copy_up_data(&datapath, &upperpath, &c->stat);
 	if (err)
 		goto out_free;
 
-- 
2.20.1

Re: [PATCH v2] ovl: improving copy-up efficiency for big sparse file

[Amir Goldstein]

On Wed, Oct 30, 2019 at 2:45 PM Chengguang Xu <cgxu519@mykernel.net> wrote:
>
> Current copy-up is not efficient for big 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 use case.
>
> 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.
>
> Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
> ---
>
> Hi Miklos, Amir
>
> This is v2 version of hole copy-up improvement which
> addressed amir's concerns in previous email.
>
> Could you have a look at this patch?
>
> There is a checkpatch warning but that is
> false-positive warning, so you can ignore it.
>
> I've tested the patch with the cases under overlay dir
> (include overlay/066) in fstest for xfs/ext4 fstype,
> and passed most of the cases except below.
>
> overlay/045     [not run] fsck.overlay utility required, skipped this test
> overlay/046     [not run] fsck.overlay utility required, skipped this test
> overlay/056     [not run] fsck.overlay utility required, skipped this test

Those are not failures.
You need to install fsck.overlay from
https://github.com/hisilicon/overlayfs-progs
for these tests to run.

>
> Above three cases are fsck related cases,
> I think they are not important for copy-up.
>
> overlay/061     - output mismatch (see /home/cgxu/git/xfstests-dev/results//overlay/061.out.bad)
>     --- tests/overlay/061.out   2019-05-28 09:54:42.320874925 +0800
>     +++ /home/cgxu/git/xfstests-dev/results//overlay/061.out.bad        2019-10-30 16:11:50.490848367 +0800
>     @@ -1,4 +1,4 @@
>      QA output created by 061
>     -00000000:  61 61 61 61 61 61 61 61 61 61 61 61 61 61 61 61  aaaaaaaaaaaaaaaa
>     +00000000:  54 68 69 73 20 69 73 20 6f 6c 64 20 6e 65 77 73  This.is.old.news
>      After mount cycle:
>      00000000:  61 61 61 61 61 61 61 61 61 61 61 61 61 61 61 61  aaaaaaaaaaaaaaaa
>     ...
>     (Run 'diff -u /home/cgxu/git/xfstests-dev/tests/overlay/061.out /home/cgxu/git/xfstests-dev/results//overlay/061.out.bad'  to see the entire diff)
>
> overlay/061 was failed with/without my patch and test results
> were just same. have something already broken for the test?

Yes, overlayfs does not comply with this "posix"' test.
This is why it was removed from the auto and quick groups.

>
>
> v1->v2:
> - Set file size when the hole is in the end of the file.
> - Add a code comment for hole copy-up improvement.
> - Check SEEK_DATA support before doing hole skip.
> - Back to original copy-up when seek data fails(in error case).
>
>  fs/overlayfs/copy_up.c | 78 ++++++++++++++++++++++++++++++++++--------
>  1 file changed, 64 insertions(+), 14 deletions(-)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index b801c6353100..7d8a34c480f4 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -116,13 +116,30 @@ int ovl_copy_xattr(struct dentry *old, struct dentry *new)
>         return error;
>  }
>
> -static int ovl_copy_up_data(struct path *old, struct path *new, loff_t len)
> +static int ovl_set_size(struct dentry *upperdentry, struct kstat *stat)
> +{
> +       struct iattr attr = {
> +               .ia_valid = ATTR_SIZE,
> +               .ia_size = stat->size,
> +       };
> +
> +       return notify_change(upperdentry, &attr, NULL);
> +}
> +
> +static int ovl_copy_up_data(struct path *old, struct path *new,
> +                           struct kstat *stat)
>  {
>         struct file *old_file;
>         struct file *new_file;
> +       loff_t len = stat->size;
>         loff_t old_pos = 0;
>         loff_t new_pos = 0;
>         loff_t cloned;
> +       loff_t old_next_data_pos;
> +       loff_t hole_len;
> +       bool seek_support = false;
> +       bool skip_hole = true;
> +       bool set_size = false;
>         int error = 0;
>
>         if (len == 0)
> @@ -144,7 +161,12 @@ 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 */
> +       /* Check if lower fs supports seek operation */
> +       if (old_file->f_mode & FMODE_LSEEK &&
> +           old_file->f_op->llseek) {
> +               seek_support = true;
> +       }
> +
>         while (len) {
>                 size_t this_len = OVL_COPY_UP_CHUNK_SIZE;
>                 long bytes;
> @@ -157,6 +179,38 @@ static int ovl_copy_up_data(struct path *old, struct path *new, loff_t len)
>                         break;
>                 }
>
> +               /*
> +                * Fill zero for hole will cost unnecessary disk space
> +                * and meanwhile slow down the copy-up speed, so we do
> +                * an optimization for hole during copy-up, it relies
> +                * on SEEK_DATA implementation and the hole check is
> +                * aligned to OVL_COPY_UP_CHUNK_SIZE. In other word,
> +                * we do not try to recognize all kind of holes here,
> +                * we just skip big enough hole for simplicity to
> +                * implement. If lower fs does not support SEEK_DATA
> +                * operation, the copy-up will behave as before.
> +                */
> +
> +               if (seek_support && skip_hole) {
> +                       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;

Use round_down() helper

> +                               old_pos += hole_len;
> +                               new_pos += hole_len;
> +                               len -= hole_len;
> +                               continue;
> +                       } else if (old_next_data_pos == -ENXIO) {
> +                               set_size = true;
> +                               break;
> +                       } else if (old_next_data_pos < 0) {
> +                               skip_hole = false;

Why do you need to use 2 booleans?
You can initialize skip_hole = true only in case of lower
has seek support.

>
> +                       }
> +               }
> +
>                 bytes = do_splice_direct(old_file, &old_pos,
>                                          new_file, &new_pos,
>                                          this_len, SPLICE_F_MOVE);
> @@ -168,6 +222,12 @@ static int ovl_copy_up_data(struct path *old, struct path *new, loff_t len)
>
>                 len -= bytes;
>         }
> +
> +       if (!error && set_size) {
> +               inode_lock(new->dentry->d_inode);
> +               error = ovl_set_size(new->dentry, stat);
> +               inode_unlock(new->dentry->d_inode);
> +       }

I see no reason to repeat this code here.
Two options:
1. always set_size at the end of ovl_copy_up_inode()
    what's the harm in that?
2. set boolean c->set_size here and check it at the end
    of ovl_copy_up_inode() instead of checking c->metacopy


Thanks,
Amir.

Re: [PATCH v2] ovl: improving copy-up efficiency for big sparse file

[Chengguang Xu]

 ---- 在 星期三, 2019-10-30 23:50:13 Amir Goldstein <amir73il@gmail.com> 撰写 ----
 > On Wed, Oct 30, 2019 at 2:45 PM Chengguang Xu <cgxu519@mykernel.net> wrote:
 > >
 > > Current copy-up is not efficient for big 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 use case.
 > >
 > > 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.
 > >
 > > Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
 > > ---
 > >
 > > Hi Miklos, Amir
 > >
 > > This is v2 version of hole copy-up improvement which
 > > addressed amir's concerns in previous email.
 > >
 > > Could you have a look at this patch?
 > >
 > > There is a checkpatch warning but that is
 > > false-positive warning, so you can ignore it.
 > >
 > > I've tested the patch with the cases under overlay dir
 > > (include overlay/066) in fstest for xfs/ext4 fstype,
 > > and passed most of the cases except below.
 > >
 > > overlay/045     [not run] fsck.overlay utility required, skipped this test
 > > overlay/046     [not run] fsck.overlay utility required, skipped this test
 > > overlay/056     [not run] fsck.overlay utility required, skipped this test
 > 
 > Those are not failures.
 > You need to install fsck.overlay from
 > https://github.com/hisilicon/overlayfs-progs
 > for these tests to run.

I'll do.

 > 
 > >
 > > Above three cases are fsck related cases,
 > > I think they are not important for copy-up.
 > >
 > > overlay/061     - output mismatch (see /home/cgxu/git/xfstests-dev/results//overlay/061.out.bad)
 > >     --- tests/overlay/061.out   2019-05-28 09:54:42.320874925 +0800
 > >     +++ /home/cgxu/git/xfstests-dev/results//overlay/061.out.bad        2019-10-30 16:11:50.490848367 +0800
 > >     @@ -1,4 +1,4 @@
 > >      QA output created by 061
 > >     -00000000:  61 61 61 61 61 61 61 61 61 61 61 61 61 61 61 61  aaaaaaaaaaaaaaaa
 > >     +00000000:  54 68 69 73 20 69 73 20 6f 6c 64 20 6e 65 77 73  This.is.old.news
 > >      After mount cycle:
 > >      00000000:  61 61 61 61 61 61 61 61 61 61 61 61 61 61 61 61  aaaaaaaaaaaaaaaa
 > >     ...
 > >     (Run 'diff -u /home/cgxu/git/xfstests-dev/tests/overlay/061.out /home/cgxu/git/xfstests-dev/results//overlay/061.out.bad'  to see the entire diff)
 > >
 > > overlay/061 was failed with/without my patch and test results
 > > were just same. have something already broken for the test?
 > 
 > Yes, overlayfs does not comply with this "posix"' test.
 > This is why it was removed from the auto and quick groups.

So I'm curious what is the purpose for the test?

 > 
 > >
 > >
 > > v1->v2:
 > > - Set file size when the hole is in the end of the file.
 > > - Add a code comment for hole copy-up improvement.
 > > - Check SEEK_DATA support before doing hole skip.
 > > - Back to original copy-up when seek data fails(in error case).
 > >
 > >  fs/overlayfs/copy_up.c | 78 ++++++++++++++++++++++++++++++++++--------
 > >  1 file changed, 64 insertions(+), 14 deletions(-)
 > >
 > > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
 > > index b801c6353100..7d8a34c480f4 100644
 > > --- a/fs/overlayfs/copy_up.c
 > > +++ b/fs/overlayfs/copy_up.c
 > > @@ -116,13 +116,30 @@ int ovl_copy_xattr(struct dentry *old, struct dentry *new)
 > >         return error;
 > >  }
 > >
 > > -static int ovl_copy_up_data(struct path *old, struct path *new, loff_t len)
 > > +static int ovl_set_size(struct dentry *upperdentry, struct kstat *stat)
 > > +{
 > > +       struct iattr attr = {
 > > +               .ia_valid = ATTR_SIZE,
 > > +               .ia_size = stat->size,
 > > +       };
 > > +
 > > +       return notify_change(upperdentry, &attr, NULL);
 > > +}
 > > +
 > > +static int ovl_copy_up_data(struct path *old, struct path *new,
 > > +                           struct kstat *stat)
 > >  {
 > >         struct file *old_file;
 > >         struct file *new_file;
 > > +       loff_t len = stat->size;
 > >         loff_t old_pos = 0;
 > >         loff_t new_pos = 0;
 > >         loff_t cloned;
 > > +       loff_t old_next_data_pos;
 > > +       loff_t hole_len;
 > > +       bool seek_support = false;
 > > +       bool skip_hole = true;
 > > +       bool set_size = false;
 > >         int error = 0;
 > >
 > >         if (len == 0)
 > > @@ -144,7 +161,12 @@ 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 */
 > > +       /* Check if lower fs supports seek operation */
 > > +       if (old_file->f_mode & FMODE_LSEEK &&
 > > +           old_file->f_op->llseek) {
 > > +               seek_support = true;
 > > +       }
 > > +
 > >         while (len) {
 > >                 size_t this_len = OVL_COPY_UP_CHUNK_SIZE;
 > >                 long bytes;
 > > @@ -157,6 +179,38 @@ static int ovl_copy_up_data(struct path *old, struct path *new, loff_t len)
 > >                         break;
 > >                 }
 > >
 > > +               /*
 > > +                * Fill zero for hole will cost unnecessary disk space
 > > +                * and meanwhile slow down the copy-up speed, so we do
 > > +                * an optimization for hole during copy-up, it relies
 > > +                * on SEEK_DATA implementation and the hole check is
 > > +                * aligned to OVL_COPY_UP_CHUNK_SIZE. In other word,
 > > +                * we do not try to recognize all kind of holes here,
 > > +                * we just skip big enough hole for simplicity to
 > > +                * implement. If lower fs does not support SEEK_DATA
 > > +                * operation, the copy-up will behave as before.
 > > +                */
 > > +
 > > +               if (seek_support && skip_hole) {
 > > +                       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;
 > 
 > Use round_down() helper

I'll change the logic of hole detection a bit, so that it could work
more effectively for big continuous hole.


 > 
 > > +                               old_pos += hole_len;
 > > +                               new_pos += hole_len;
 > > +                               len -= hole_len;
 > > +                               continue;
 > > +                       } else if (old_next_data_pos == -ENXIO) {
 > > +                               set_size = true;
 > > +                               break;
 > > +                       } else if (old_next_data_pos < 0) {
 > > +                               skip_hole = false;
 > 
 > Why do you need to use 2 booleans?
 > You can initialize skip_hole = true only in case of lower
 > has seek support.
 > 
 > >
 > > +                       }
 > > +               }
 > > +
 > >                 bytes = do_splice_direct(old_file, &old_pos,
 > >                                          new_file, &new_pos,
 > >                                          this_len, SPLICE_F_MOVE);
 > > @@ -168,6 +222,12 @@ static int ovl_copy_up_data(struct path *old, struct path *new, loff_t len)
 > >
 > >                 len -= bytes;
 > >         }
 > > +
 > > +       if (!error && set_size) {
 > > +               inode_lock(new->dentry->d_inode);
 > > +               error = ovl_set_size(new->dentry, stat);
 > > +               inode_unlock(new->dentry->d_inode);
 > > +       }
 > 
 > I see no reason to repeat this code here.
 > Two options:
 > 1. always set_size at the end of ovl_copy_up_inode()
 >     what's the harm in that?

I think at least it's not suitable for directory.


 > 2. set boolean c->set_size here and check it at the end
 >     of ovl_copy_up_inode() instead of checking c->metacopy
 > 

I don't understand why 'c->set_size' can replace 'c->metacopy',

Thanks,
Chengguang.

Re: [PATCH v2] ovl: improving copy-up efficiency for big sparse file

[Amir Goldstein]

>  > Yes, overlayfs does not comply with this "posix"' test.
>  > This is why it was removed from the auto and quick groups.
>
> So I'm curious what is the purpose for the test?
>

This is a POSIX compliance test.
It is meant to "remind" us that this behavior is not POSIX compliant
and that we should fix it one day...
A bit controversial to have a test like this without a roadmap
when it is going to be fixed in xfstests, but it's there.

>  >
>  > >
>  > >
>  > > v1->v2:
>  > > - Set file size when the hole is in the end of the file.
>  > > - Add a code comment for hole copy-up improvement.
>  > > - Check SEEK_DATA support before doing hole skip.
>  > > - Back to original copy-up when seek data fails(in error case).
>  > >
>  > >  fs/overlayfs/copy_up.c | 78 ++++++++++++++++++++++++++++++++++--------
>  > >  1 file changed, 64 insertions(+), 14 deletions(-)
>  > >
>  > > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
>  > > index b801c6353100..7d8a34c480f4 100644
>  > > --- a/fs/overlayfs/copy_up.c
>  > > +++ b/fs/overlayfs/copy_up.c
>  > > @@ -116,13 +116,30 @@ int ovl_copy_xattr(struct dentry *old, struct dentry *new)
>  > >         return error;
>  > >  }
>  > >
>  > > -static int ovl_copy_up_data(struct path *old, struct path *new, loff_t len)
>  > > +static int ovl_set_size(struct dentry *upperdentry, struct kstat *stat)
>  > > +{
>  > > +       struct iattr attr = {
>  > > +               .ia_valid = ATTR_SIZE,
>  > > +               .ia_size = stat->size,
>  > > +       };
>  > > +
>  > > +       return notify_change(upperdentry, &attr, NULL);
>  > > +}
>  > > +
>  > > +static int ovl_copy_up_data(struct path *old, struct path *new,
>  > > +                           struct kstat *stat)
>  > >  {
>  > >         struct file *old_file;
>  > >         struct file *new_file;
>  > > +       loff_t len = stat->size;
>  > >         loff_t old_pos = 0;
>  > >         loff_t new_pos = 0;
>  > >         loff_t cloned;
>  > > +       loff_t old_next_data_pos;
>  > > +       loff_t hole_len;
>  > > +       bool seek_support = false;
>  > > +       bool skip_hole = true;
>  > > +       bool set_size = false;
>  > >         int error = 0;
>  > >
>  > >         if (len == 0)
>  > > @@ -144,7 +161,12 @@ 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 */
>  > > +       /* Check if lower fs supports seek operation */
>  > > +       if (old_file->f_mode & FMODE_LSEEK &&
>  > > +           old_file->f_op->llseek) {
>  > > +               seek_support = true;
>  > > +       }
>  > > +
>  > >         while (len) {
>  > >                 size_t this_len = OVL_COPY_UP_CHUNK_SIZE;
>  > >                 long bytes;
>  > > @@ -157,6 +179,38 @@ static int ovl_copy_up_data(struct path *old, struct path *new, loff_t len)
>  > >                         break;
>  > >                 }
>  > >
>  > > +               /*
>  > > +                * Fill zero for hole will cost unnecessary disk space
>  > > +                * and meanwhile slow down the copy-up speed, so we do
>  > > +                * an optimization for hole during copy-up, it relies
>  > > +                * on SEEK_DATA implementation and the hole check is
>  > > +                * aligned to OVL_COPY_UP_CHUNK_SIZE. In other word,
>  > > +                * we do not try to recognize all kind of holes here,
>  > > +                * we just skip big enough hole for simplicity to
>  > > +                * implement. If lower fs does not support SEEK_DATA
>  > > +                * operation, the copy-up will behave as before.
>  > > +                */
>  > > +
>  > > +               if (seek_support && skip_hole) {
>  > > +                       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;
>  >
>  > Use round_down() helper
>
> I'll change the logic of hole detection a bit, so that it could work
> more effectively for big continuous hole.

Not sure what you mean.
I meant there is a helper in the kernel you should use
instead of the expression "/ N * N"

>
>
>  >
>  > > +                               old_pos += hole_len;
>  > > +                               new_pos += hole_len;
>  > > +                               len -= hole_len;
>  > > +                               continue;
>  > > +                       } else if (old_next_data_pos == -ENXIO) {
>  > > +                               set_size = true;
>  > > +                               break;
>  > > +                       } else if (old_next_data_pos < 0) {
>  > > +                               skip_hole = false;
>  >
>  > Why do you need to use 2 booleans?
>  > You can initialize skip_hole = true only in case of lower
>  > has seek support.
>  >
>  > >
>  > > +                       }
>  > > +               }
>  > > +
>  > >                 bytes = do_splice_direct(old_file, &old_pos,
>  > >                                          new_file, &new_pos,
>  > >                                          this_len, SPLICE_F_MOVE);
>  > > @@ -168,6 +222,12 @@ static int ovl_copy_up_data(struct path *old, struct path *new, loff_t len)
>  > >
>  > >                 len -= bytes;
>  > >         }
>  > > +
>  > > +       if (!error && set_size) {
>  > > +               inode_lock(new->dentry->d_inode);
>  > > +               error = ovl_set_size(new->dentry, stat);
>  > > +               inode_unlock(new->dentry->d_inode);
>  > > +       }
>  >
>  > I see no reason to repeat this code here.
>  > Two options:
>  > 1. always set_size at the end of ovl_copy_up_inode()
>  >     what's the harm in that?
>
> I think at least it's not suitable for directory.
>
>
>  > 2. set boolean c->set_size here and check it at the end
>  >     of ovl_copy_up_inode() instead of checking c->metacopy
>  >
>
> I don't understand why 'c->set_size' can replace 'c->metacopy',
>

I did not explain myself well.

This should be enough IMO:

@@ -483,7 +483,7 @@ static int ovl_copy_up_inode(struct
ovl_copy_up_ctx *c, struct dentry *temp)
        }

        inode_lock(temp->d_inode);
-       if (c->metacopy)
+       if (S_ISREG(c->stat.mode))
                err = ovl_set_size(temp, &c->stat);
        if (!err)
                err = ovl_set_attr(temp, &c->stat);

There is no special reason IMO to try to spare an unneeded ovl_set_size
if it simplifies the code a bit.

As a matter of fact, I think overlayfs currently does a metacopy
copy up even for files of size 0.
This will cost unneeded code to run during lookup and later
for clearing the metacopy on "data" copy up.
Not sure how much this case is common,
but that's for another patch:

@@ -717,7 +717,7 @@ static int ovl_do_copy_up(struct ovl_copy_up_ctx *c)
        return err;
 }

-static bool ovl_need_meta_copy_up(struct dentry *dentry, umode_t mode,
+static bool ovl_need_meta_copy_up(struct dentry *dentry, struct kstat *stat,
                                  int flags)
 {
        struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
@@ -725,7 +725,7 @@ static bool ovl_need_meta_copy_up(struct dentry
*dentry, umode_t mode,
        if (!ofs->config.metacopy)
                return false;

-       if (!S_ISREG(mode))
+       if (!S_ISREG(stat->mode) || !stat->size)
                return false;

        if (flags && ((OPEN_FMODE(flags) & FMODE_WRITE) || (flags & O_TRUNC)))
@@ -805,7 +805,7 @@ static int ovl_copy_up_one(struct dentry *parent,
struct dentry *dentry,
        if (err)
                return err;

-       ctx.metacopy = ovl_need_meta_copy_up(dentry, ctx.stat.mode, flags);
+       ctx.metacopy = ovl_need_meta_copy_up(dentry, &ctx.stat, flags);

        if (parent) {
                ovl_path_upper(parent, &parentpath);

Thanks,
Amir.

Re: [PATCH v2] ovl: improving copy-up efficiency for big sparse file

[Chengguang Xu]

 ---- 在 星期四, 2019-10-31 14:53:15 Amir Goldstein <amir73il@gmail.com> 撰写 ----
 > >  > Yes, overlayfs does not comply with this "posix"' test.
 > >  > This is why it was removed from the auto and quick groups.
 > >
 > > So I'm curious what is the purpose for the test?
 > >
 > 
 > This is a POSIX compliance test.
 > It is meant to "remind" us that this behavior is not POSIX compliant
 > and that we should fix it one day...
 > A bit controversial to have a test like this without a roadmap
 > when it is going to be fixed in xfstests, but it's there.
 > 
 > >  >
 > >  > >
 > >  > >
 > >  > > v1->v2:
 > >  > > - Set file size when the hole is in the end of the file.
 > >  > > - Add a code comment for hole copy-up improvement.
 > >  > > - Check SEEK_DATA support before doing hole skip.
 > >  > > - Back to original copy-up when seek data fails(in error case).
 > >  > >
 > >  > >  fs/overlayfs/copy_up.c | 78 ++++++++++++++++++++++++++++++++++--------
 > >  > >  1 file changed, 64 insertions(+), 14 deletions(-)
 > >  > >
 > >  > > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
 > >  > > index b801c6353100..7d8a34c480f4 100644
 > >  > > --- a/fs/overlayfs/copy_up.c
 > >  > > +++ b/fs/overlayfs/copy_up.c
 > >  > > @@ -116,13 +116,30 @@ int ovl_copy_xattr(struct dentry *old, struct dentry *new)
 > >  > >         return error;
 > >  > >  }
 > >  > >
 > >  > > -static int ovl_copy_up_data(struct path *old, struct path *new, loff_t len)
 > >  > > +static int ovl_set_size(struct dentry *upperdentry, struct kstat *stat)
 > >  > > +{
 > >  > > +       struct iattr attr = {
 > >  > > +               .ia_valid = ATTR_SIZE,
 > >  > > +               .ia_size = stat->size,
 > >  > > +       };
 > >  > > +
 > >  > > +       return notify_change(upperdentry, &attr, NULL);
 > >  > > +}
 > >  > > +
 > >  > > +static int ovl_copy_up_data(struct path *old, struct path *new,
 > >  > > +                           struct kstat *stat)
 > >  > >  {
 > >  > >         struct file *old_file;
 > >  > >         struct file *new_file;
 > >  > > +       loff_t len = stat->size;
 > >  > >         loff_t old_pos = 0;
 > >  > >         loff_t new_pos = 0;
 > >  > >         loff_t cloned;
 > >  > > +       loff_t old_next_data_pos;
 > >  > > +       loff_t hole_len;
 > >  > > +       bool seek_support = false;
 > >  > > +       bool skip_hole = true;
 > >  > > +       bool set_size = false;
 > >  > >         int error = 0;
 > >  > >
 > >  > >         if (len == 0)
 > >  > > @@ -144,7 +161,12 @@ 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 */
 > >  > > +       /* Check if lower fs supports seek operation */
 > >  > > +       if (old_file->f_mode & FMODE_LSEEK &&
 > >  > > +           old_file->f_op->llseek) {
 > >  > > +               seek_support = true;
 > >  > > +       }
 > >  > > +
 > >  > >         while (len) {
 > >  > >                 size_t this_len = OVL_COPY_UP_CHUNK_SIZE;
 > >  > >                 long bytes;
 > >  > > @@ -157,6 +179,38 @@ static int ovl_copy_up_data(struct path *old, struct path *new, loff_t len)
 > >  > >                         break;
 > >  > >                 }
 > >  > >
 > >  > > +               /*
 > >  > > +                * Fill zero for hole will cost unnecessary disk space
 > >  > > +                * and meanwhile slow down the copy-up speed, so we do
 > >  > > +                * an optimization for hole during copy-up, it relies
 > >  > > +                * on SEEK_DATA implementation and the hole check is
 > >  > > +                * aligned to OVL_COPY_UP_CHUNK_SIZE. In other word,
 > >  > > +                * we do not try to recognize all kind of holes here,
 > >  > > +                * we just skip big enough hole for simplicity to
 > >  > > +                * implement. If lower fs does not support SEEK_DATA
 > >  > > +                * operation, the copy-up will behave as before.
 > >  > > +                */
 > >  > > +
 > >  > > +               if (seek_support && skip_hole) {
 > >  > > +                       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;
 > >  >
 > >  > Use round_down() helper
 > >
 > > I'll change the logic of hole detection a bit, so that it could work
 > > more effectively for big continuous hole.
 > 
 > Not sure what you mean.
 > I meant there is a helper in the kernel you should use
 > instead of the expression "/ N * N"

I'm going to change to like below, so we don't need to round down
to CHUNK_SIZE anymore.


+                * Detail logic of hole detection as below:
+                * When we detect next data position is larger than current
+                * position we will skip that hole, otherwise we copy
+                * data in the size of OVL_COPY_UP_CHUNK_SIZE. Actually,
+                * it may not recognize all kind of holes and sometimes
+                * only skips partial of hole area. However, it will be
+                * enough for most of the use cases.
+                */
+
+               if (skip_hole) {
+                       old_next_data_pos = vfs_llseek(old_file,
+                                               old_pos, SEEK_DATA);
+                       if (old_next_data_pos > old_pos) {
+                               hole_len = old_next_data_pos - old_pos;
+                               old_pos += hole_len;
+                               new_pos += hole_len;
+                               len -= hole_len;
+                               continue;



 > 
 > >
 > >
 > >  >
 > >  > > +                               old_pos += hole_len;
 > >  > > +                               new_pos += hole_len;
 > >  > > +                               len -= hole_len;
 > >  > > +                               continue;
 > >  > > +                       } else if (old_next_data_pos == -ENXIO) {
 > >  > > +                               set_size = true;
 > >  > > +                               break;
 > >  > > +                       } else if (old_next_data_pos < 0) {
 > >  > > +                               skip_hole = false;
 > >  >
 > >  > Why do you need to use 2 booleans?
 > >  > You can initialize skip_hole = true only in case of lower
 > >  > has seek support.
 > >  >
 > >  > >
 > >  > > +                       }
 > >  > > +               }
 > >  > > +
 > >  > >                 bytes = do_splice_direct(old_file, &old_pos,
 > >  > >                                          new_file, &new_pos,
 > >  > >                                          this_len, SPLICE_F_MOVE);
 > >  > > @@ -168,6 +222,12 @@ static int ovl_copy_up_data(struct path *old, struct path *new, loff_t len)
 > >  > >
 > >  > >                 len -= bytes;
 > >  > >         }
 > >  > > +
 > >  > > +       if (!error && set_size) {
 > >  > > +               inode_lock(new->dentry->d_inode);
 > >  > > +               error = ovl_set_size(new->dentry, stat);
 > >  > > +               inode_unlock(new->dentry->d_inode);
 > >  > > +       }
 > >  >
 > >  > I see no reason to repeat this code here.
 > >  > Two options:
 > >  > 1. always set_size at the end of ovl_copy_up_inode()
 > >  >     what's the harm in that?
 > >
 > > I think at least it's not suitable for directory.
 > >
 > >
 > >  > 2. set boolean c->set_size here and check it at the end
 > >  >     of ovl_copy_up_inode() instead of checking c->metacopy
 > >  >
 > >
 > > I don't understand why 'c->set_size' can replace 'c->metacopy',
 > >
 > 
 > I did not explain myself well.
 > 
 > This should be enough IMO:
 > 
 > @@ -483,7 +483,7 @@ static int ovl_copy_up_inode(struct
 > ovl_copy_up_ctx *c, struct dentry *temp)
 >         }
 > 
 >         inode_lock(temp->d_inode);
 > -       if (c->metacopy)
 > +       if (S_ISREG(c->stat.mode))
 >                 err = ovl_set_size(temp, &c->stat);
 >         if (!err)
 >                 err = ovl_set_attr(temp, &c->stat);
 > 
 > There is no special reason IMO to try to spare an unneeded ovl_set_size
 > if it simplifies the code a bit.

We can try this but I'm afraid that someone could complain
we do unnecessary ovl_set_size() in the case of full copy-up
or data-end file's copy-up.


 > 
 > As a matter of fact, I think overlayfs currently does a metacopy
 > copy up even for files of size 0.
 > This will cost unneeded code to run during lookup and later
 > for clearing the metacopy on "data" copy up.
 > Not sure how much this case is common,
 > but that's for another patch:
 > 
 > @@ -717,7 +717,7 @@ static int ovl_do_copy_up(struct ovl_copy_up_ctx *c)
 >         return err;
 >  }
 > 
 > -static bool ovl_need_meta_copy_up(struct dentry *dentry, umode_t mode,
 > +static bool ovl_need_meta_copy_up(struct dentry *dentry, struct kstat *stat,
 >                                   int flags)
 >  {
 >         struct ovl_fs *ofs = dentry->d_sb->s_fs_info;
 > @@ -725,7 +725,7 @@ static bool ovl_need_meta_copy_up(struct dentry
 > *dentry, umode_t mode,
 >         if (!ofs->config.metacopy)
 >                 return false;
 > 
 > -       if (!S_ISREG(mode))
 > +       if (!S_ISREG(stat->mode) || !stat->size)
 >                 return false;
 > 
 >         if (flags && ((OPEN_FMODE(flags) & FMODE_WRITE) || (flags & O_TRUNC)))
 > @@ -805,7 +805,7 @@ static int ovl_copy_up_one(struct dentry *parent,
 > struct dentry *dentry,
 >         if (err)
 >                 return err;
 > 
 > -       ctx.metacopy = ovl_need_meta_copy_up(dentry, ctx.stat.mode, flags);
 > +       ctx.metacopy = ovl_need_meta_copy_up(dentry, &ctx.stat, flags);
 > 
 >         if (parent) {
 >                 ovl_path_upper(parent, &parentpath);
 > 
 
Make sense to me.

Thanks,
Chengguang

Re: [PATCH v2] ovl: improving copy-up efficiency for big sparse file

[Amir Goldstein]

>  > I did not explain myself well.
>  >
>  > This should be enough IMO:
>  >
>  > @@ -483,7 +483,7 @@ static int ovl_copy_up_inode(struct
>  > ovl_copy_up_ctx *c, struct dentry *temp)
>  >         }
>  >
>  >         inode_lock(temp->d_inode);
>  > -       if (c->metacopy)
>  > +       if (S_ISREG(c->stat.mode))
>  >                 err = ovl_set_size(temp, &c->stat);
>  >         if (!err)
>  >                 err = ovl_set_attr(temp, &c->stat);
>  >
>  > There is no special reason IMO to try to spare an unneeded ovl_set_size
>  > if it simplifies the code a bit.
>
> We can try this but I'm afraid that someone could complain
> we do unnecessary ovl_set_size() in the case of full copy-up
> or data-end file's copy-up.
>
>

There is no one to complain.
The cost of ovl_set_size() is insignificant compared to the cost of
copying data (unless I am missing something).
Please post a version as above and if Miklos finds it a problem,
we can add a boolean c->should_set_size to the copy up context, initialize
it: c->should_set_size = (S_ISREG(c->stat.mode) && c->stat.size)
and set it to false in case all data was copied.
I think that won't be necessary though.

Thanks,
Amir.

Re: [PATCH v2] ovl: improving copy-up efficiency for big sparse file

[Chengguang Xu]

 ---- 在 星期四, 2019-10-31 17:06:24 Amir Goldstein <amir73il@gmail.com> 撰写 ----
 > >  > I did not explain myself well.
 > >  >
 > >  > This should be enough IMO:
 > >  >
 > >  > @@ -483,7 +483,7 @@ static int ovl_copy_up_inode(struct
 > >  > ovl_copy_up_ctx *c, struct dentry *temp)
 > >  >         }
 > >  >
 > >  >         inode_lock(temp->d_inode);
 > >  > -       if (c->metacopy)
 > >  > +       if (S_ISREG(c->stat.mode))
 > >  >                 err = ovl_set_size(temp, &c->stat);
 > >  >         if (!err)
 > >  >                 err = ovl_set_attr(temp, &c->stat);
 > >  >
 > >  > There is no special reason IMO to try to spare an unneeded ovl_set_size
 > >  > if it simplifies the code a bit.
 > >
 > > We can try this but I'm afraid that someone could complain
 > > we do unnecessary ovl_set_size() in the case of full copy-up
 > > or data-end file's copy-up.
 > >
 > >
 > 
 > There is no one to complain.
 > The cost of ovl_set_size() is insignificant compared to the cost of
 > copying data (unless I am missing something).
 > Please post a version as above and if Miklos finds it a problem,
 > we can add a boolean c->should_set_size to the copy up context, initialize
 > it: c->should_set_size = (S_ISREG(c->stat.mode) && c->stat.size)
 > and set it to false in case all data was copied.
 > I think that won't be necessary though.
 > 

I forgot to mention that there are two callers of  ovl_copy_up_data()
and ovl_copy_up_meta_inode_data() even don't have logic to set size.
So do you still think set size in ovl_copy_up_inode() is simpler than
set size inside ovl_copy_up_data()?

Thanks,
Chengguang

Re: [PATCH v2] ovl: improving copy-up efficiency for big sparse file

[Amir Goldstein]

On Thu, Oct 31, 2019 at 11:20 AM Chengguang Xu <cgxu519@mykernel.net> wrote:
>
>  ---- 在 星期四, 2019-10-31 17:06:24 Amir Goldstein <amir73il@gmail.com> 撰写 ----
>  > >  > I did not explain myself well.
>  > >  >
>  > >  > This should be enough IMO:
>  > >  >
>  > >  > @@ -483,7 +483,7 @@ static int ovl_copy_up_inode(struct
>  > >  > ovl_copy_up_ctx *c, struct dentry *temp)
>  > >  >         }
>  > >  >
>  > >  >         inode_lock(temp->d_inode);
>  > >  > -       if (c->metacopy)
>  > >  > +       if (S_ISREG(c->stat.mode))
>  > >  >                 err = ovl_set_size(temp, &c->stat);
>  > >  >         if (!err)
>  > >  >                 err = ovl_set_attr(temp, &c->stat);
>  > >  >
>  > >  > There is no special reason IMO to try to spare an unneeded ovl_set_size
>  > >  > if it simplifies the code a bit.
>  > >
>  > > We can try this but I'm afraid that someone could complain
>  > > we do unnecessary ovl_set_size() in the case of full copy-up
>  > > or data-end file's copy-up.
>  > >
>  > >
>  >
>  > There is no one to complain.
>  > The cost of ovl_set_size() is insignificant compared to the cost of
>  > copying data (unless I am missing something).
>  > Please post a version as above and if Miklos finds it a problem,
>  > we can add a boolean c->should_set_size to the copy up context, initialize
>  > it: c->should_set_size = (S_ISREG(c->stat.mode) && c->stat.size)
>  > and set it to false in case all data was copied.
>  > I think that won't be necessary though.
>  >
>
> I forgot to mention that there are two callers of  ovl_copy_up_data()
> and ovl_copy_up_meta_inode_data() even don't have logic to set size.

Because set_size was already done during meta copy up and
"data copy up" does not need to change the size again.

> So do you still think set size in ovl_copy_up_inode() is simpler than
> set size inside ovl_copy_up_data()?
>

Yes. One line change is simpler.

Thanks,
Amir.

Re: [PATCH v2] ovl: improving copy-up efficiency for big sparse file

[Vivek Goyal]

On Thu, Oct 31, 2019 at 08:53:15AM +0200, Amir Goldstein wrote:
[..]
> >  > > +
> >  > > +       if (!error && set_size) {
> >  > > +               inode_lock(new->dentry->d_inode);
> >  > > +               error = ovl_set_size(new->dentry, stat);
> >  > > +               inode_unlock(new->dentry->d_inode);
> >  > > +       }
> >  >
> >  > I see no reason to repeat this code here.
> >  > Two options:
> >  > 1. always set_size at the end of ovl_copy_up_inode()
> >  >     what's the harm in that?
> >
> > I think at least it's not suitable for directory.
> >
> >
> >  > 2. set boolean c->set_size here and check it at the end
> >  >     of ovl_copy_up_inode() instead of checking c->metacopy
> >  >
> >
> > I don't understand why 'c->set_size' can replace 'c->metacopy',
> >
> 
> I did not explain myself well.
> 
> This should be enough IMO:
> 
> @@ -483,7 +483,7 @@ static int ovl_copy_up_inode(struct
> ovl_copy_up_ctx *c, struct dentry *temp)
>         }
> 
>         inode_lock(temp->d_inode);
> -       if (c->metacopy)
> +       if (S_ISREG(c->stat.mode))
>                 err = ovl_set_size(temp, &c->stat);

Hi Amir,

Why do we need this change. c->metacopy is set only for regular files.

ovl_need_meta_copy_up() {
        if (!S_ISREG(mode))
                return false;
}

Even if there is a reason, this change should be part of a separate patch.
What connection does it have to skip holes while copying up.

Thanks
Vivek

Re: [PATCH v2] ovl: improving copy-up efficiency for big sparse file

[Miklos Szeredi]

On Thu, Oct 31, 2019 at 2:20 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Thu, Oct 31, 2019 at 08:53:15AM +0200, Amir Goldstein wrote:

> >
> > @@ -483,7 +483,7 @@ static int ovl_copy_up_inode(struct
> > ovl_copy_up_ctx *c, struct dentry *temp)
> >         }
> >
> >         inode_lock(temp->d_inode);
> > -       if (c->metacopy)
> > +       if (S_ISREG(c->stat.mode))
> >                 err = ovl_set_size(temp, &c->stat);
>
> Hi Amir,
>
> Why do we need this change. c->metacopy is set only for regular files.
>
> ovl_need_meta_copy_up() {
>         if (!S_ISREG(mode))
>                 return false;
> }
>
> Even if there is a reason, this change should be part of a separate patch.
> What connection does it have to skip holes while copying up.

That's a fix only if the copied-up file ends in a hole, which was not
the case before this patch.   But harmless, and makes sense generally
(i.e. file is truncated to final size *before* data is copied up).

Thanks,
Miklos

Re: [PATCH v2] ovl: improving copy-up efficiency for big sparse file

[Chengguang Xu]

 ---- 在 星期四, 2019-10-31 14:53:15 Amir Goldstein <amir73il@gmail.com> 撰写 ----
 > >  > Yes, overlayfs does not comply with this "posix"' test.
 > >  > This is why it was removed from the auto and quick groups.
 > >
 > > So I'm curious what is the purpose for the test?
 > >
 > 
 > This is a POSIX compliance test.
 > It is meant to "remind" us that this behavior is not POSIX compliant
 > and that we should fix it one day...
 > A bit controversial to have a test like this without a roadmap
 > when it is going to be fixed in xfstests, but it's there.

I haven't checked carefully for the detail but It seems  feasible if we copy-up lower file  during mmap regardless of ro/rw mode.
Is it acceptable  by slightly changing copy-up assumption to fulfill POSIX compliance? Or we just wait for a better solution?

Thanks,
Chengguang

Re: [PATCH v2] ovl: improving copy-up efficiency for big sparse file

[Amir Goldstein]

On Sun, Nov 3, 2019 at 2:43 PM Chengguang Xu <cgxu519@mykernel.net> wrote:
>
>  ---- 在 星期四, 2019-10-31 14:53:15 Amir Goldstein <amir73il@gmail.com> 撰写 ----
>  > >  > Yes, overlayfs does not comply with this "posix"' test.
>  > >  > This is why it was removed from the auto and quick groups.
>  > >
>  > > So I'm curious what is the purpose for the test?
>  > >
>  >
>  > This is a POSIX compliance test.
>  > It is meant to "remind" us that this behavior is not POSIX compliant
>  > and that we should fix it one day...
>  > A bit controversial to have a test like this without a roadmap
>  > when it is going to be fixed in xfstests, but it's there.
>
> I haven't checked carefully for the detail but It seems  feasible if we copy-up lower file  during mmap regardless of ro/rw mode.
> Is it acceptable  by slightly changing copy-up assumption to fulfill POSIX compliance? Or we just wait for a better solution?
>

That was attempted in the past.
It's complicated ;-)

Cheers,
Amir.