[PATCH v2 1/3] ovl: do not restore mtime on copy-up for regular file

[PATCH v2 1/3] ovl: do not restore mtime on copy-up for regular file

[Chengguang Xu]

In order to simplify truncate operation on the file which
only has lower, we skip restoring mtime on copy-up for
regular file.

Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
---
 fs/overlayfs/copy_up.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 0fed532efa68..8b92b3ba3c46 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -241,12 +241,17 @@ static int ovl_set_size(struct dentry *upperdentry, struct kstat *stat)
 
 static int ovl_set_timestamps(struct dentry *upperdentry, struct kstat *stat)
 {
-	struct iattr attr = {
-		.ia_valid =
-		     ATTR_ATIME | ATTR_MTIME | ATTR_ATIME_SET | ATTR_MTIME_SET,
-		.ia_atime = stat->atime,
-		.ia_mtime = stat->mtime,
-	};
+	struct iattr attr;
+
+	if (S_ISREG(upperdentry->d_inode->i_mode)) {
+		attr.ia_valid = ATTR_ATIME | ATTR_ATIME_SET;
+		attr.ia_atime = stat->atime;
+	} else {
+		attr.ia_valid = ATTR_ATIME | ATTR_MTIME |
+				ATTR_ATIME_SET | ATTR_MTIME_SET;
+		attr.ia_atime = stat->atime;
+		attr.ia_mtime = stat->mtime;
+	}
 
 	return notify_change(upperdentry, &attr, NULL);
 }
-- 
2.27.0

[PATCH v2 2/3] ovl: check actual copy-up size

[Chengguang Xu]

In order to simplify truncate operation on the file which
only has lower, we allow specifying larger size than lower
file when calling ovl_copy_up_data(), so we should check
actual copy size carefully before doing copy-up.

Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
---
 fs/overlayfs/copy_up.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 8b92b3ba3c46..a1a9a150405a 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -156,6 +156,9 @@ static int ovl_copy_up_data(struct ovl_fs *ofs, struct path *old,
 		goto out_fput;
 	}
 
+	len = (len <= i_size_read(file_inode(old_file))) ? len :
+				i_size_read(file_inode(old_file));
+
 	/* Try to use clone_file_range to clone up within the same fs */
 	cloned = do_clone_file_range(old_file, 0, new_file, 0, len, 0);
 	if (cloned == len)
-- 
2.27.0

[PATCH v2 3/3] ovl: copy-up optimization for truncate

[Chengguang Xu]

Currently truncate operation on the file which only has
lower will copy-up whole lower file and calling truncate(2)
on upper file. It is not efficient for the case which
truncates to much smaller size than lower file. This patch
tries to avoid unnecessary data copy and truncate operation
after copy-up.

Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
---
 fs/overlayfs/copy_up.c   | 18 +++++++++++-------
 fs/overlayfs/inode.c     |  9 ++++++++-
 fs/overlayfs/overlayfs.h |  2 +-
 3 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index a1a9a150405a..331cc32eac95 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -874,7 +874,7 @@ static int ovl_copy_up_meta_inode_data(struct ovl_copy_up_ctx *c)
 }
 
 static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
-			   int flags)
+			   int flags, loff_t size)
 {
 	int err;
 	DEFINE_DELAYED_CALL(done);
@@ -911,6 +911,8 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
 	/* maybe truncate regular file. this has no effect on dirs */
 	if (flags & O_TRUNC)
 		ctx.stat.size = 0;
+	if (size)
+		ctx.stat.size = size;
 
 	if (S_ISLNK(ctx.stat.mode)) {
 		ctx.link = vfs_get_link(ctx.lowerpath.dentry, &done);
@@ -937,7 +939,7 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
 	return err;
 }
 
-static int ovl_copy_up_flags(struct dentry *dentry, int flags)
+static int ovl_copy_up_flags(struct dentry *dentry, int flags, loff_t size)
 {
 	int err = 0;
 	const struct cred *old_cred = ovl_override_creds(dentry->d_sb);
@@ -970,7 +972,7 @@ static int ovl_copy_up_flags(struct dentry *dentry, int flags)
 			next = parent;
 		}
 
-		err = ovl_copy_up_one(parent, next, flags);
+		err = ovl_copy_up_one(parent, next, flags, size);
 
 		dput(parent);
 		dput(next);
@@ -1002,7 +1004,7 @@ int ovl_maybe_copy_up(struct dentry *dentry, int flags)
 	if (ovl_open_need_copy_up(dentry, flags)) {
 		err = ovl_want_write(dentry);
 		if (!err) {
-			err = ovl_copy_up_flags(dentry, flags);
+			err = ovl_copy_up_flags(dentry, flags, 0);
 			ovl_drop_write(dentry);
 		}
 	}
@@ -1010,12 +1012,14 @@ int ovl_maybe_copy_up(struct dentry *dentry, int flags)
 	return err;
 }
 
-int ovl_copy_up_with_data(struct dentry *dentry)
+int ovl_copy_up_with_data(struct dentry *dentry, loff_t size)
 {
-	return ovl_copy_up_flags(dentry, O_WRONLY);
+	if (size)
+		return ovl_copy_up_flags(dentry, O_WRONLY, size);
+	return  ovl_copy_up_flags(dentry, O_TRUNC | O_WRONLY, 0);
 }
 
 int ovl_copy_up(struct dentry *dentry)
 {
-	return ovl_copy_up_flags(dentry, 0);
+	return ovl_copy_up_flags(dentry, 0, 0);
 }
diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
index cf41bcb664bc..92f274844947 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -43,13 +43,20 @@ int ovl_setattr(struct dentry *dentry, struct iattr *attr)
 	if (!full_copy_up)
 		err = ovl_copy_up(dentry);
 	else
-		err = ovl_copy_up_with_data(dentry);
+		err = ovl_copy_up_with_data(dentry, attr->ia_size);
 	if (!err) {
 		struct inode *winode = NULL;
 
 		upperdentry = ovl_dentry_upper(dentry);
 
 		if (attr->ia_valid & ATTR_SIZE) {
+			if (full_copy_up && !(attr->ia_valid & ~ATTR_SIZE)) {
+				inode_lock(upperdentry->d_inode);
+				ovl_copyattr(upperdentry->d_inode, dentry->d_inode);
+				inode_unlock(upperdentry->d_inode);
+				goto out_drop_write;
+			}
+
 			winode = d_inode(upperdentry);
 			err = get_write_access(winode);
 			if (err)
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index cb4e2d60ecf9..efd0ec9bd3b7 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -513,7 +513,7 @@ long ovl_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
 
 /* copy_up.c */
 int ovl_copy_up(struct dentry *dentry);
-int ovl_copy_up_with_data(struct dentry *dentry);
+int ovl_copy_up_with_data(struct dentry *dentry, loff_t size);
 int ovl_maybe_copy_up(struct dentry *dentry, int flags);
 int ovl_copy_xattr(struct super_block *sb, struct dentry *old,
 		   struct dentry *new);
-- 
2.27.0

Re: [PATCH v2 1/3] ovl: do not restore mtime on copy-up for regular file

[Amir Goldstein]

On Wed, Apr 7, 2021 at 12:04 AM Chengguang Xu <cgxu519@mykernel.net> wrote:
>
> In order to simplify truncate operation on the file which
> only has lower, we skip restoring mtime on copy-up for
> regular file.
>
> Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
> ---
>  fs/overlayfs/copy_up.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index 0fed532efa68..8b92b3ba3c46 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -241,12 +241,17 @@ static int ovl_set_size(struct dentry *upperdentry, struct kstat *stat)
>
>  static int ovl_set_timestamps(struct dentry *upperdentry, struct kstat *stat)
>  {
> -       struct iattr attr = {
> -               .ia_valid =
> -                    ATTR_ATIME | ATTR_MTIME | ATTR_ATIME_SET | ATTR_MTIME_SET,
> -               .ia_atime = stat->atime,
> -               .ia_mtime = stat->mtime,
> -       };
> +       struct iattr attr;
> +
> +       if (S_ISREG(upperdentry->d_inode->i_mode)) {
> +               attr.ia_valid = ATTR_ATIME | ATTR_ATIME_SET;
> +               attr.ia_atime = stat->atime;
> +       } else {
> +               attr.ia_valid = ATTR_ATIME | ATTR_MTIME |
> +                               ATTR_ATIME_SET | ATTR_MTIME_SET;
> +               attr.ia_atime = stat->atime;
> +               attr.ia_mtime = stat->mtime;
> +       }

Nit: IMO it would look nicer with:
if (!S_ISREG(stat->mode)) {
               attr.ia_valid |= ATTR_MTIME | ATTR_MTIME_SET;
               attr.ia_mtime = stat->mtime;
}

But generally, this logic looks a bit weird in a function named
ovl_set_timestamps().

When you look at the 3 callers of ovl_set_timestamps(), two of
them do it for a directory and one is in ovl_set_attr() where there
are several other open coded calls to notify_change(), so I
wonder if this logic shouldn't be open coded in ovl_set_attr()
as well?

Thanks,
Amir.

Re: [PATCH v2 2/3] ovl: check actual copy-up size

[Amir Goldstein]

On Wed, Apr 7, 2021 at 12:04 AM Chengguang Xu <cgxu519@mykernel.net> wrote:
>
> In order to simplify truncate operation on the file which
> only has lower, we allow specifying larger size than lower
> file when calling ovl_copy_up_data(), so we should check
> actual copy size carefully before doing copy-up.
>
> Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
> ---
>  fs/overlayfs/copy_up.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index 8b92b3ba3c46..a1a9a150405a 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -156,6 +156,9 @@ static int ovl_copy_up_data(struct ovl_fs *ofs, struct path *old,
>                 goto out_fput;
>         }
>
> +       len = (len <= i_size_read(file_inode(old_file))) ? len :
> +                               i_size_read(file_inode(old_file));

use min() please.

Thanks,
Amir.

Re: [PATCH v2 3/3] ovl: copy-up optimization for truncate

[Amir Goldstein]

On Wed, Apr 7, 2021 at 12:04 AM Chengguang Xu <cgxu519@mykernel.net> wrote:
>
> Currently truncate operation on the file which only has
> lower will copy-up whole lower file and calling truncate(2)
> on upper file. It is not efficient for the case which
> truncates to much smaller size than lower file. This patch
> tries to avoid unnecessary data copy and truncate operation
> after copy-up.
>
> Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
> ---
>  fs/overlayfs/copy_up.c   | 18 +++++++++++-------
>  fs/overlayfs/inode.c     |  9 ++++++++-
>  fs/overlayfs/overlayfs.h |  2 +-
>  3 files changed, 20 insertions(+), 9 deletions(-)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index a1a9a150405a..331cc32eac95 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -874,7 +874,7 @@ static int ovl_copy_up_meta_inode_data(struct ovl_copy_up_ctx *c)
>  }
>
>  static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
> -                          int flags)
> +                          int flags, loff_t size)
>  {
>         int err;
>         DEFINE_DELAYED_CALL(done);
> @@ -911,6 +911,8 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
>         /* maybe truncate regular file. this has no effect on dirs */
>         if (flags & O_TRUNC)
>                 ctx.stat.size = 0;
> +       if (size)
> +               ctx.stat.size = size;

Not sure about this, but *maybe* instead we re-interpret O_TRUNC
internally as "either O_TRUNC or truncate()" and then:
         if (flags & O_TRUNC)
                 ctx.stat.size = size;

It would simplify the logic in ovl_copy_up_with_data().
If you do that, put a comment to clarify that special meaning.

>
>         if (S_ISLNK(ctx.stat.mode)) {
>                 ctx.link = vfs_get_link(ctx.lowerpath.dentry, &done);
> @@ -937,7 +939,7 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
>         return err;
>  }
>
> -static int ovl_copy_up_flags(struct dentry *dentry, int flags)
> +static int ovl_copy_up_flags(struct dentry *dentry, int flags, loff_t size)
>  {
>         int err = 0;
>         const struct cred *old_cred = ovl_override_creds(dentry->d_sb);
> @@ -970,7 +972,7 @@ static int ovl_copy_up_flags(struct dentry *dentry, int flags)
>                         next = parent;
>                 }
>
> -               err = ovl_copy_up_one(parent, next, flags);
> +               err = ovl_copy_up_one(parent, next, flags, size);
>
>                 dput(parent);
>                 dput(next);
> @@ -1002,7 +1004,7 @@ int ovl_maybe_copy_up(struct dentry *dentry, int flags)
>         if (ovl_open_need_copy_up(dentry, flags)) {
>                 err = ovl_want_write(dentry);
>                 if (!err) {
> -                       err = ovl_copy_up_flags(dentry, flags);
> +                       err = ovl_copy_up_flags(dentry, flags, 0);
>                         ovl_drop_write(dentry);
>                 }
>         }
> @@ -1010,12 +1012,14 @@ int ovl_maybe_copy_up(struct dentry *dentry, int flags)
>         return err;
>  }
>
> -int ovl_copy_up_with_data(struct dentry *dentry)
> +int ovl_copy_up_with_data(struct dentry *dentry, loff_t size)
>  {
> -       return ovl_copy_up_flags(dentry, O_WRONLY);
> +       if (size)
> +               return ovl_copy_up_flags(dentry, O_WRONLY, size);
> +       return  ovl_copy_up_flags(dentry, O_TRUNC | O_WRONLY, 0);

Best get rid of this helper and put this logic in ovl_setattr(). see below.

>  }
>
>  int ovl_copy_up(struct dentry *dentry)
>  {
> -       return ovl_copy_up_flags(dentry, 0);
> +       return ovl_copy_up_flags(dentry, 0, 0);
>  }
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index cf41bcb664bc..92f274844947 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -43,13 +43,20 @@ int ovl_setattr(struct dentry *dentry, struct iattr *attr)
>         if (!full_copy_up)
>                 err = ovl_copy_up(dentry);
>         else
> -               err = ovl_copy_up_with_data(dentry);
> +               err = ovl_copy_up_with_data(dentry, attr->ia_size);

You do not know that ia_size is valid here.
Instead of using this if/else and full_copy_up var, use vars 'flags'
and 'size' and call ovl_copy_up_flags().
Instead of full_copy_up = true, set flags and size.
Then you may also remove ovl_copy_up_with_data() which has no other
callers.

>         if (!err) {
>                 struct inode *winode = NULL;
>
>                 upperdentry = ovl_dentry_upper(dentry);
>
>                 if (attr->ia_valid & ATTR_SIZE) {
> +                       if (full_copy_up && !(attr->ia_valid & ~ATTR_SIZE)) {
> +                               inode_lock(upperdentry->d_inode);
> +                               ovl_copyattr(upperdentry->d_inode, dentry->d_inode);
> +                               inode_unlock(upperdentry->d_inode);

All that this is saving is an extra notify_change() call and I am not sure it is
worth the special casing.

Also, I think that is a bug and would make xfstest overlay/013 fail.
When lower file is being executed, its true that we copy up anyway
and that it is safe to do that, but test and applications expect to get
ETXTBSY error all the same.

Please run xfstests at least the generic/quick and overlay/quick groups
with -overlay.

There are a lot of generic and overlay tests that do truncate(size), but it's
hard to say if test coverage for your change is sufficient, so unless you find
tests that cover it, you may need to write a specialized overlay truncate test.

Thanks,
Amir.

Re: [PATCH v2 3/3] ovl: copy-up optimization for truncate

[Chengguang Xu]

 ---- 在 星期三, 2021-04-07 15:52:15 Amir Goldstein <amir73il@gmail.com> 撰写 ----
 > On Wed, Apr 7, 2021 at 12:04 AM Chengguang Xu <cgxu519@mykernel.net> wrote:
 > >
 > > Currently truncate operation on the file which only has
 > > lower will copy-up whole lower file and calling truncate(2)
 > > on upper file. It is not efficient for the case which
 > > truncates to much smaller size than lower file. This patch
 > > tries to avoid unnecessary data copy and truncate operation
 > > after copy-up.
 > >
 > > Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
 > > ---
 > >  fs/overlayfs/copy_up.c   | 18 +++++++++++-------
 > >  fs/overlayfs/inode.c     |  9 ++++++++-
 > >  fs/overlayfs/overlayfs.h |  2 +-
 > >  3 files changed, 20 insertions(+), 9 deletions(-)
 > >
 > > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
 > > index a1a9a150405a..331cc32eac95 100644
 > > --- a/fs/overlayfs/copy_up.c
 > > +++ b/fs/overlayfs/copy_up.c
 > > @@ -874,7 +874,7 @@ static int ovl_copy_up_meta_inode_data(struct ovl_copy_up_ctx *c)
 > >  }
 > >
 > >  static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
 > > -                          int flags)
 > > +                          int flags, loff_t size)
 > >  {
 > >         int err;
 > >         DEFINE_DELAYED_CALL(done);
 > > @@ -911,6 +911,8 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
 > >         /* maybe truncate regular file. this has no effect on dirs */
 > >         if (flags & O_TRUNC)
 > >                 ctx.stat.size = 0;
 > > +       if (size)
 > > +               ctx.stat.size = size;
 > 
 > Not sure about this, but *maybe* instead we re-interpret O_TRUNC
 > internally as "either O_TRUNC or truncate()" and then:
 >          if (flags & O_TRUNC)
 >                  ctx.stat.size = size;
 > 
 > It would simplify the logic in ovl_copy_up_with_data().
 > If you do that, put a comment to clarify that special meaning.
 > 
 > >
 > >         if (S_ISLNK(ctx.stat.mode)) {
 > >                 ctx.link = vfs_get_link(ctx.lowerpath.dentry, &done);
 > > @@ -937,7 +939,7 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
 > >         return err;
 > >  }
 > >
 > > -static int ovl_copy_up_flags(struct dentry *dentry, int flags)
 > > +static int ovl_copy_up_flags(struct dentry *dentry, int flags, loff_t size)
 > >  {
 > >         int err = 0;
 > >         const struct cred *old_cred = ovl_override_creds(dentry->d_sb);
 > > @@ -970,7 +972,7 @@ static int ovl_copy_up_flags(struct dentry *dentry, int flags)
 > >                         next = parent;
 > >                 }
 > >
 > > -               err = ovl_copy_up_one(parent, next, flags);
 > > +               err = ovl_copy_up_one(parent, next, flags, size);
 > >
 > >                 dput(parent);
 > >                 dput(next);
 > > @@ -1002,7 +1004,7 @@ int ovl_maybe_copy_up(struct dentry *dentry, int flags)
 > >         if (ovl_open_need_copy_up(dentry, flags)) {
 > >                 err = ovl_want_write(dentry);
 > >                 if (!err) {
 > > -                       err = ovl_copy_up_flags(dentry, flags);
 > > +                       err = ovl_copy_up_flags(dentry, flags, 0);
 > >                         ovl_drop_write(dentry);
 > >                 }
 > >         }
 > > @@ -1010,12 +1012,14 @@ int ovl_maybe_copy_up(struct dentry *dentry, int flags)
 > >         return err;
 > >  }
 > >
 > > -int ovl_copy_up_with_data(struct dentry *dentry)
 > > +int ovl_copy_up_with_data(struct dentry *dentry, loff_t size)
 > >  {
 > > -       return ovl_copy_up_flags(dentry, O_WRONLY);
 > > +       if (size)
 > > +               return ovl_copy_up_flags(dentry, O_WRONLY, size);
 > > +       return  ovl_copy_up_flags(dentry, O_TRUNC | O_WRONLY, 0);
 > 
 > Best get rid of this helper and put this logic in ovl_setattr(). see below.
 > 
 > >  }
 > >
 > >  int ovl_copy_up(struct dentry *dentry)
 > >  {
 > > -       return ovl_copy_up_flags(dentry, 0);
 > > +       return ovl_copy_up_flags(dentry, 0, 0);
 > >  }
 > > diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
 > > index cf41bcb664bc..92f274844947 100644
 > > --- a/fs/overlayfs/inode.c
 > > +++ b/fs/overlayfs/inode.c
 > > @@ -43,13 +43,20 @@ int ovl_setattr(struct dentry *dentry, struct iattr *attr)
 > >         if (!full_copy_up)
 > >                 err = ovl_copy_up(dentry);
 > >         else
 > > -               err = ovl_copy_up_with_data(dentry);
 > > +               err = ovl_copy_up_with_data(dentry, attr->ia_size);
 > 
 > You do not know that ia_size is valid here.

I think we don't have to worry about validation of ia_size here,
vfs layer has already done simple check for specified size and upper fs
will return error when we set invalid file size after copy-up. Am I missing
something?


 > Instead of using this if/else and full_copy_up var, use vars 'flags'
 > and 'size' and call ovl_copy_up_flags().
 > Instead of full_copy_up = true, set flags and size.
 > Then you may also remove ovl_copy_up_with_data() which has no other
 > callers.
 > 
 > >         if (!err) {
 > >                 struct inode *winode = NULL;
 > >
 > >                 upperdentry = ovl_dentry_upper(dentry);
 > >
 > >                 if (attr->ia_valid & ATTR_SIZE) {
 > > +                       if (full_copy_up && !(attr->ia_valid & ~ATTR_SIZE)) {
 > > +                               inode_lock(upperdentry->d_inode);
 > > +                               ovl_copyattr(upperdentry->d_inode, dentry->d_inode);
 > > +                               inode_unlock(upperdentry->d_inode);
 > 
 > All that this is saving is an extra notify_change() call and I am not sure it is
 > worth the special casing.
 > 
 > Also, I think that is a bug and would make xfstest overlay/013 fail.

I ran testcases in overlay directory and didn't find failure related to this change.
However, generic/313 failed unexpectedly, the reason is I used full_copy_up var
wrongly, for the file which has upper still needs to go through notify_change().

By the way, I don't fully understand calling copy-up function(ovl_copy_up() or ovl_copy_up_with_data())
even for the file which has upper. Maybe it's better to optimize this part first in separated patch.


 > When lower file is being executed, its true that we copy up anyway
 > and that it is safe to do that, but test and applications expect to get
 > ETXTBSY error all the same.

Actually we have already do the check and return ETXTBSY error, see below.

err = -ETXTBSY;
if (atomic_read(&realinode->i_writecount) < 0)
        goto out_drop_write;


Thanks,
Chengguang

 > 
 > Please run xfstests at least the generic/quick and overlay/quick groups
 > with -overlay.
 > 
 > There are a lot of generic and overlay tests that do truncate(size), but it's
 > hard to say if test coverage for your change is sufficient, so unless you find
 > tests that cover it, you may need to write a specialized overlay truncate test.
 > 
 > Thanks,
 > Amir.
 > 

Re: [PATCH v2 3/3] ovl: copy-up optimization for truncate

[Amir Goldstein]

On Thu, Apr 8, 2021 at 4:23 PM Chengguang Xu <cgxu519@mykernel.net> wrote:
>
>  ---- 在 星期三, 2021-04-07 15:52:15 Amir Goldstein <amir73il@gmail.com> 撰写 ----
>  > On Wed, Apr 7, 2021 at 12:04 AM Chengguang Xu <cgxu519@mykernel.net> wrote:
>  > >
>  > > Currently truncate operation on the file which only has
>  > > lower will copy-up whole lower file and calling truncate(2)
>  > > on upper file. It is not efficient for the case which
>  > > truncates to much smaller size than lower file. This patch
>  > > tries to avoid unnecessary data copy and truncate operation
>  > > after copy-up.
>  > >
>  > > Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
>  > > ---
>  > >  fs/overlayfs/copy_up.c   | 18 +++++++++++-------
>  > >  fs/overlayfs/inode.c     |  9 ++++++++-
>  > >  fs/overlayfs/overlayfs.h |  2 +-
>  > >  3 files changed, 20 insertions(+), 9 deletions(-)
>  > >
>  > > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
>  > > index a1a9a150405a..331cc32eac95 100644
>  > > --- a/fs/overlayfs/copy_up.c
>  > > +++ b/fs/overlayfs/copy_up.c
>  > > @@ -874,7 +874,7 @@ static int ovl_copy_up_meta_inode_data(struct ovl_copy_up_ctx *c)
>  > >  }
>  > >
>  > >  static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
>  > > -                          int flags)
>  > > +                          int flags, loff_t size)
>  > >  {
>  > >         int err;
>  > >         DEFINE_DELAYED_CALL(done);
>  > > @@ -911,6 +911,8 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
>  > >         /* maybe truncate regular file. this has no effect on dirs */
>  > >         if (flags & O_TRUNC)
>  > >                 ctx.stat.size = 0;
>  > > +       if (size)
>  > > +               ctx.stat.size = size;
>  >
>  > Not sure about this, but *maybe* instead we re-interpret O_TRUNC
>  > internally as "either O_TRUNC or truncate()" and then:
>  >          if (flags & O_TRUNC)
>  >                  ctx.stat.size = size;
>  >
>  > It would simplify the logic in ovl_copy_up_with_data().
>  > If you do that, put a comment to clarify that special meaning.
>  >
>  > >
>  > >         if (S_ISLNK(ctx.stat.mode)) {
>  > >                 ctx.link = vfs_get_link(ctx.lowerpath.dentry, &done);
>  > > @@ -937,7 +939,7 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
>  > >         return err;
>  > >  }
>  > >
>  > > -static int ovl_copy_up_flags(struct dentry *dentry, int flags)
>  > > +static int ovl_copy_up_flags(struct dentry *dentry, int flags, loff_t size)
>  > >  {
>  > >         int err = 0;
>  > >         const struct cred *old_cred = ovl_override_creds(dentry->d_sb);
>  > > @@ -970,7 +972,7 @@ static int ovl_copy_up_flags(struct dentry *dentry, int flags)
>  > >                         next = parent;
>  > >                 }
>  > >
>  > > -               err = ovl_copy_up_one(parent, next, flags);
>  > > +               err = ovl_copy_up_one(parent, next, flags, size);
>  > >
>  > >                 dput(parent);
>  > >                 dput(next);
>  > > @@ -1002,7 +1004,7 @@ int ovl_maybe_copy_up(struct dentry *dentry, int flags)
>  > >         if (ovl_open_need_copy_up(dentry, flags)) {
>  > >                 err = ovl_want_write(dentry);
>  > >                 if (!err) {
>  > > -                       err = ovl_copy_up_flags(dentry, flags);
>  > > +                       err = ovl_copy_up_flags(dentry, flags, 0);
>  > >                         ovl_drop_write(dentry);
>  > >                 }
>  > >         }
>  > > @@ -1010,12 +1012,14 @@ int ovl_maybe_copy_up(struct dentry *dentry, int flags)
>  > >         return err;
>  > >  }
>  > >
>  > > -int ovl_copy_up_with_data(struct dentry *dentry)
>  > > +int ovl_copy_up_with_data(struct dentry *dentry, loff_t size)
>  > >  {
>  > > -       return ovl_copy_up_flags(dentry, O_WRONLY);
>  > > +       if (size)
>  > > +               return ovl_copy_up_flags(dentry, O_WRONLY, size);
>  > > +       return  ovl_copy_up_flags(dentry, O_TRUNC | O_WRONLY, 0);
>  >
>  > Best get rid of this helper and put this logic in ovl_setattr(). see below.
>  >
>  > >  }
>  > >
>  > >  int ovl_copy_up(struct dentry *dentry)
>  > >  {
>  > > -       return ovl_copy_up_flags(dentry, 0);
>  > > +       return ovl_copy_up_flags(dentry, 0, 0);
>  > >  }
>  > > diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
>  > > index cf41bcb664bc..92f274844947 100644
>  > > --- a/fs/overlayfs/inode.c
>  > > +++ b/fs/overlayfs/inode.c
>  > > @@ -43,13 +43,20 @@ int ovl_setattr(struct dentry *dentry, struct iattr *attr)
>  > >         if (!full_copy_up)
>  > >                 err = ovl_copy_up(dentry);
>  > >         else
>  > > -               err = ovl_copy_up_with_data(dentry);
>  > > +               err = ovl_copy_up_with_data(dentry, attr->ia_size);
>  >
>  > You do not know that ia_size is valid here.
>
> I think we don't have to worry about validation of ia_size here,
> vfs layer has already done simple check for specified size and upper fs
> will return error when we set invalid file size after copy-up. Am I missing
> something?
>

ovl_setattr() will be called from any number of places where ia_size has
uninitialized value, such as vfs_utimes().

You are not allowed to access it without checking
(attr->ia_valid & ATTR_SIZE) which here above you don't.

>
>  > Instead of using this if/else and full_copy_up var, use vars 'flags'
>  > and 'size' and call ovl_copy_up_flags().
>  > Instead of full_copy_up = true, set flags and size.
>  > Then you may also remove ovl_copy_up_with_data() which has no other
>  > callers.
>  >
>  > >         if (!err) {
>  > >                 struct inode *winode = NULL;
>  > >
>  > >                 upperdentry = ovl_dentry_upper(dentry);
>  > >
>  > >                 if (attr->ia_valid & ATTR_SIZE) {
>  > > +                       if (full_copy_up && !(attr->ia_valid & ~ATTR_SIZE)) {
>  > > +                               inode_lock(upperdentry->d_inode);
>  > > +                               ovl_copyattr(upperdentry->d_inode, dentry->d_inode);
>  > > +                               inode_unlock(upperdentry->d_inode);
>  >
>  > All that this is saving is an extra notify_change() call and I am not sure it is
>  > worth the special casing.
>  >
>  > Also, I think that is a bug and would make xfstest overlay/013 fail.
>
> I ran testcases in overlay directory and didn't find failure related to this change.
> However, generic/313 failed unexpectedly, the reason is I used full_copy_up var
> wrongly, for the file which has upper still needs to go through notify_change().
>
> By the way, I don't fully understand calling copy-up function(ovl_copy_up() or ovl_copy_up_with_data())
> even for the file which has upper. Maybe it's better to optimize this part first in separated patch.
>

There is a difference between "has upper" and "has upper data".
It's related to metacopy.

>
>  > When lower file is being executed, its true that we copy up anyway
>  > and that it is safe to do that, but test and applications expect to get
>  > ETXTBSY error all the same.
>
> Actually we have already do the check and return ETXTBSY error, see below.
>
> err = -ETXTBSY;
> if (atomic_read(&realinode->i_writecount) < 0)
>         goto out_drop_write;
>

Yes, my point exactly. Your code does goto out_drop_write; before that check
so it will skip it.

Thanks,
Amir.

Re: [PATCH v2 3/3] ovl: copy-up optimization for truncate

[Chengguang Xu]

 ---- 在 星期四, 2021-04-08 22:40:47 Amir Goldstein <amir73il@gmail.com> 撰写 ----
 > On Thu, Apr 8, 2021 at 4:23 PM Chengguang Xu <cgxu519@mykernel.net> wrote:
 > >
 > >  ---- 在 星期三, 2021-04-07 15:52:15 Amir Goldstein <amir73il@gmail.com> 撰写 ----
 > >  > On Wed, Apr 7, 2021 at 12:04 AM Chengguang Xu <cgxu519@mykernel.net> wrote:
 > >  > >
 > >  > > Currently truncate operation on the file which only has
 > >  > > lower will copy-up whole lower file and calling truncate(2)
 > >  > > on upper file. It is not efficient for the case which
 > >  > > truncates to much smaller size than lower file. This patch
 > >  > > tries to avoid unnecessary data copy and truncate operation
 > >  > > after copy-up.
 > >  > >
 > >  > > Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
 > >  > > ---
 > >  > >  fs/overlayfs/copy_up.c   | 18 +++++++++++-------
 > >  > >  fs/overlayfs/inode.c     |  9 ++++++++-
 > >  > >  fs/overlayfs/overlayfs.h |  2 +-
 > >  > >  3 files changed, 20 insertions(+), 9 deletions(-)
 > >  > >
 > >  > > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
 > >  > > index a1a9a150405a..331cc32eac95 100644
 > >  > > --- a/fs/overlayfs/copy_up.c
 > >  > > +++ b/fs/overlayfs/copy_up.c
 > >  > > @@ -874,7 +874,7 @@ static int ovl_copy_up_meta_inode_data(struct ovl_copy_up_ctx *c)
 > >  > >  }
 > >  > >
 > >  > >  static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
 > >  > > -                          int flags)
 > >  > > +                          int flags, loff_t size)
 > >  > >  {
 > >  > >         int err;
 > >  > >         DEFINE_DELAYED_CALL(done);
 > >  > > @@ -911,6 +911,8 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
 > >  > >         /* maybe truncate regular file. this has no effect on dirs */
 > >  > >         if (flags & O_TRUNC)
 > >  > >                 ctx.stat.size = 0;
 > >  > > +       if (size)
 > >  > > +               ctx.stat.size = size;
 > >  >
 > >  > Not sure about this, but *maybe* instead we re-interpret O_TRUNC
 > >  > internally as "either O_TRUNC or truncate()" and then:
 > >  >          if (flags & O_TRUNC)
 > >  >                  ctx.stat.size = size;
 > >  >
 > >  > It would simplify the logic in ovl_copy_up_with_data().
 > >  > If you do that, put a comment to clarify that special meaning.
 > >  >
 > >  > >
 > >  > >         if (S_ISLNK(ctx.stat.mode)) {
 > >  > >                 ctx.link = vfs_get_link(ctx.lowerpath.dentry, &done);
 > >  > > @@ -937,7 +939,7 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
 > >  > >         return err;
 > >  > >  }
 > >  > >
 > >  > > -static int ovl_copy_up_flags(struct dentry *dentry, int flags)
 > >  > > +static int ovl_copy_up_flags(struct dentry *dentry, int flags, loff_t size)
 > >  > >  {
 > >  > >         int err = 0;
 > >  > >         const struct cred *old_cred = ovl_override_creds(dentry->d_sb);
 > >  > > @@ -970,7 +972,7 @@ static int ovl_copy_up_flags(struct dentry *dentry, int flags)
 > >  > >                         next = parent;
 > >  > >                 }
 > >  > >
 > >  > > -               err = ovl_copy_up_one(parent, next, flags);
 > >  > > +               err = ovl_copy_up_one(parent, next, flags, size);
 > >  > >
 > >  > >                 dput(parent);
 > >  > >                 dput(next);
 > >  > > @@ -1002,7 +1004,7 @@ int ovl_maybe_copy_up(struct dentry *dentry, int flags)
 > >  > >         if (ovl_open_need_copy_up(dentry, flags)) {
 > >  > >                 err = ovl_want_write(dentry);
 > >  > >                 if (!err) {
 > >  > > -                       err = ovl_copy_up_flags(dentry, flags);
 > >  > > +                       err = ovl_copy_up_flags(dentry, flags, 0);
 > >  > >                         ovl_drop_write(dentry);
 > >  > >                 }
 > >  > >         }
 > >  > > @@ -1010,12 +1012,14 @@ int ovl_maybe_copy_up(struct dentry *dentry, int flags)
 > >  > >         return err;
 > >  > >  }
 > >  > >
 > >  > > -int ovl_copy_up_with_data(struct dentry *dentry)
 > >  > > +int ovl_copy_up_with_data(struct dentry *dentry, loff_t size)
 > >  > >  {
 > >  > > -       return ovl_copy_up_flags(dentry, O_WRONLY);
 > >  > > +       if (size)
 > >  > > +               return ovl_copy_up_flags(dentry, O_WRONLY, size);
 > >  > > +       return  ovl_copy_up_flags(dentry, O_TRUNC | O_WRONLY, 0);
 > >  >
 > >  > Best get rid of this helper and put this logic in ovl_setattr(). see below.
 > >  >
 > >  > >  }
 > >  > >
 > >  > >  int ovl_copy_up(struct dentry *dentry)
 > >  > >  {
 > >  > > -       return ovl_copy_up_flags(dentry, 0);
 > >  > > +       return ovl_copy_up_flags(dentry, 0, 0);
 > >  > >  }
 > >  > > diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
 > >  > > index cf41bcb664bc..92f274844947 100644
 > >  > > --- a/fs/overlayfs/inode.c
 > >  > > +++ b/fs/overlayfs/inode.c
 > >  > > @@ -43,13 +43,20 @@ int ovl_setattr(struct dentry *dentry, struct iattr *attr)
 > >  > >         if (!full_copy_up)
 > >  > >                 err = ovl_copy_up(dentry);
 > >  > >         else
 > >  > > -               err = ovl_copy_up_with_data(dentry);
 > >  > > +               err = ovl_copy_up_with_data(dentry, attr->ia_size);
 > >  >
 > >  > You do not know that ia_size is valid here.
 > >
 > > I think we don't have to worry about validation of ia_size here,
 > > vfs layer has already done simple check for specified size and upper fs
 > > will return error when we set invalid file size after copy-up. Am I missing
 > > something?
 > >
 > 
 > ovl_setattr() will be called from any number of places where ia_size has
 > uninitialized value, such as vfs_utimes().
 > 
 > You are not allowed to access it without checking
 > (attr->ia_valid & ATTR_SIZE) which here above you don't.
 > 

Currently, (attr->ia_valid & ATTR_SIZE) is equal to var full_copy_up,
so the size will be valid in this case.

 > >
 > >  > Instead of using this if/else and full_copy_up var, use vars 'flags'
 > >  > and 'size' and call ovl_copy_up_flags().
 > >  > Instead of full_copy_up = true, set flags and size.
 > >  > Then you may also remove ovl_copy_up_with_data() which has no other
 > >  > callers.
 > >  >
 > >  > >         if (!err) {
 > >  > >                 struct inode *winode = NULL;
 > >  > >
 > >  > >                 upperdentry = ovl_dentry_upper(dentry);
 > >  > >
 > >  > >                 if (attr->ia_valid & ATTR_SIZE) {
 > >  > > +                       if (full_copy_up && !(attr->ia_valid & ~ATTR_SIZE)) {
 > >  > > +                               inode_lock(upperdentry->d_inode);
 > >  > > +                               ovl_copyattr(upperdentry->d_inode, dentry->d_inode);
 > >  > > +                               inode_unlock(upperdentry->d_inode);
 > >  >
 > >  > All that this is saving is an extra notify_change() call and I am not sure it is
 > >  > worth the special casing.
 > >  >
 > >  > Also, I think that is a bug and would make xfstest overlay/013 fail.
 > >
 > > I ran testcases in overlay directory and didn't find failure related to this change.
 > > However, generic/313 failed unexpectedly, the reason is I used full_copy_up var
 > > wrongly, for the file which has upper still needs to go through notify_change().
 > >
 > > By the way, I don't fully understand calling copy-up function(ovl_copy_up() or ovl_copy_up_with_data())
 > > even for the file which has upper. Maybe it's better to optimize this part first in separated patch.
 > >
 > 
 > There is a difference between "has upper" and "has upper data".
 > It's related to metacopy.
 > 

I see, my point here is we can skip calling copy-up function for files which already have upper data.



 > >
 > >  > When lower file is being executed, its true that we copy up anyway
 > >  > and that it is safe to do that, but test and applications expect to get
 > >  > ETXTBSY error all the same.
 > >
 > > Actually we have already do the check and return ETXTBSY error, see below.
 > >
 > > err = -ETXTBSY;
 > > if (atomic_read(&realinode->i_writecount) < 0)
 > >         goto out_drop_write;
 > >
 > 
 > Yes, my point exactly. Your code does goto out_drop_write; before that check
 > so it will skip it.
 > 

No, if you look at the code more closely , you'll find the goto in my code is actually after the check, :-).

Thanks,
Chengguang


 

Re: [PATCH v2 3/3] ovl: copy-up optimization for truncate

[Amir Goldstein]

On Fri, Apr 9, 2021 at 6:00 AM Chengguang Xu <cgxu519@mykernel.net> wrote:
>
>  ---- 在 星期四, 2021-04-08 22:40:47 Amir Goldstein <amir73il@gmail.com> 撰写 ----
>  > On Thu, Apr 8, 2021 at 4:23 PM Chengguang Xu <cgxu519@mykernel.net> wrote:
>  > >
>  > >  ---- 在 星期三, 2021-04-07 15:52:15 Amir Goldstein <amir73il@gmail.com> 撰写 ----
>  > >  > On Wed, Apr 7, 2021 at 12:04 AM Chengguang Xu <cgxu519@mykernel.net> wrote:
>  > >  > >
>  > >  > > Currently truncate operation on the file which only has
>  > >  > > lower will copy-up whole lower file and calling truncate(2)
>  > >  > > on upper file. It is not efficient for the case which
>  > >  > > truncates to much smaller size than lower file. This patch
>  > >  > > tries to avoid unnecessary data copy and truncate operation
>  > >  > > after copy-up.
>  > >  > >
>  > >  > > Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>
>  > >  > > ---
>  > >  > >  fs/overlayfs/copy_up.c   | 18 +++++++++++-------
>  > >  > >  fs/overlayfs/inode.c     |  9 ++++++++-
>  > >  > >  fs/overlayfs/overlayfs.h |  2 +-
>  > >  > >  3 files changed, 20 insertions(+), 9 deletions(-)
>  > >  > >
>  > >  > > diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
>  > >  > > index a1a9a150405a..331cc32eac95 100644
>  > >  > > --- a/fs/overlayfs/copy_up.c
>  > >  > > +++ b/fs/overlayfs/copy_up.c
>  > >  > > @@ -874,7 +874,7 @@ static int ovl_copy_up_meta_inode_data(struct ovl_copy_up_ctx *c)
>  > >  > >  }
>  > >  > >
>  > >  > >  static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
>  > >  > > -                          int flags)
>  > >  > > +                          int flags, loff_t size)
>  > >  > >  {
>  > >  > >         int err;
>  > >  > >         DEFINE_DELAYED_CALL(done);
>  > >  > > @@ -911,6 +911,8 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
>  > >  > >         /* maybe truncate regular file. this has no effect on dirs */
>  > >  > >         if (flags & O_TRUNC)
>  > >  > >                 ctx.stat.size = 0;
>  > >  > > +       if (size)
>  > >  > > +               ctx.stat.size = size;
>  > >  >
>  > >  > Not sure about this, but *maybe* instead we re-interpret O_TRUNC
>  > >  > internally as "either O_TRUNC or truncate()" and then:
>  > >  >          if (flags & O_TRUNC)
>  > >  >                  ctx.stat.size = size;
>  > >  >
>  > >  > It would simplify the logic in ovl_copy_up_with_data().
>  > >  > If you do that, put a comment to clarify that special meaning.
>  > >  >
>  > >  > >
>  > >  > >         if (S_ISLNK(ctx.stat.mode)) {
>  > >  > >                 ctx.link = vfs_get_link(ctx.lowerpath.dentry, &done);
>  > >  > > @@ -937,7 +939,7 @@ static int ovl_copy_up_one(struct dentry *parent, struct dentry *dentry,
>  > >  > >         return err;
>  > >  > >  }
>  > >  > >
>  > >  > > -static int ovl_copy_up_flags(struct dentry *dentry, int flags)
>  > >  > > +static int ovl_copy_up_flags(struct dentry *dentry, int flags, loff_t size)
>  > >  > >  {
>  > >  > >         int err = 0;
>  > >  > >         const struct cred *old_cred = ovl_override_creds(dentry->d_sb);
>  > >  > > @@ -970,7 +972,7 @@ static int ovl_copy_up_flags(struct dentry *dentry, int flags)
>  > >  > >                         next = parent;
>  > >  > >                 }
>  > >  > >
>  > >  > > -               err = ovl_copy_up_one(parent, next, flags);
>  > >  > > +               err = ovl_copy_up_one(parent, next, flags, size);
>  > >  > >
>  > >  > >                 dput(parent);
>  > >  > >                 dput(next);
>  > >  > > @@ -1002,7 +1004,7 @@ int ovl_maybe_copy_up(struct dentry *dentry, int flags)
>  > >  > >         if (ovl_open_need_copy_up(dentry, flags)) {
>  > >  > >                 err = ovl_want_write(dentry);
>  > >  > >                 if (!err) {
>  > >  > > -                       err = ovl_copy_up_flags(dentry, flags);
>  > >  > > +                       err = ovl_copy_up_flags(dentry, flags, 0);
>  > >  > >                         ovl_drop_write(dentry);
>  > >  > >                 }
>  > >  > >         }
>  > >  > > @@ -1010,12 +1012,14 @@ int ovl_maybe_copy_up(struct dentry *dentry, int flags)
>  > >  > >         return err;
>  > >  > >  }
>  > >  > >
>  > >  > > -int ovl_copy_up_with_data(struct dentry *dentry)
>  > >  > > +int ovl_copy_up_with_data(struct dentry *dentry, loff_t size)
>  > >  > >  {
>  > >  > > -       return ovl_copy_up_flags(dentry, O_WRONLY);
>  > >  > > +       if (size)
>  > >  > > +               return ovl_copy_up_flags(dentry, O_WRONLY, size);
>  > >  > > +       return  ovl_copy_up_flags(dentry, O_TRUNC | O_WRONLY, 0);
>  > >  >
>  > >  > Best get rid of this helper and put this logic in ovl_setattr(). see below.
>  > >  >
>  > >  > >  }
>  > >  > >
>  > >  > >  int ovl_copy_up(struct dentry *dentry)
>  > >  > >  {
>  > >  > > -       return ovl_copy_up_flags(dentry, 0);
>  > >  > > +       return ovl_copy_up_flags(dentry, 0, 0);
>  > >  > >  }
>  > >  > > diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
>  > >  > > index cf41bcb664bc..92f274844947 100644
>  > >  > > --- a/fs/overlayfs/inode.c
>  > >  > > +++ b/fs/overlayfs/inode.c
>  > >  > > @@ -43,13 +43,20 @@ int ovl_setattr(struct dentry *dentry, struct iattr *attr)
>  > >  > >         if (!full_copy_up)
>  > >  > >                 err = ovl_copy_up(dentry);
>  > >  > >         else
>  > >  > > -               err = ovl_copy_up_with_data(dentry);
>  > >  > > +               err = ovl_copy_up_with_data(dentry, attr->ia_size);
>  > >  >
>  > >  > You do not know that ia_size is valid here.
>  > >
>  > > I think we don't have to worry about validation of ia_size here,
>  > > vfs layer has already done simple check for specified size and upper fs
>  > > will return error when we set invalid file size after copy-up. Am I missing
>  > > something?
>  > >
>  >
>  > ovl_setattr() will be called from any number of places where ia_size has
>  > uninitialized value, such as vfs_utimes().
>  >
>  > You are not allowed to access it without checking
>  > (attr->ia_valid & ATTR_SIZE) which here above you don't.
>  >
>
> Currently, (attr->ia_valid & ATTR_SIZE) is equal to var full_copy_up,
> so the size will be valid in this case.

Right. I still prefer killing the ovl_copy_up_with_data() helper.
It hurts more than it helps at this point IMO.

>
>  > >
>  > >  > Instead of using this if/else and full_copy_up var, use vars 'flags'
>  > >  > and 'size' and call ovl_copy_up_flags().
>  > >  > Instead of full_copy_up = true, set flags and size.
>  > >  > Then you may also remove ovl_copy_up_with_data() which has no other
>  > >  > callers.
>  > >  >
>  > >  > >         if (!err) {
>  > >  > >                 struct inode *winode = NULL;
>  > >  > >
>  > >  > >                 upperdentry = ovl_dentry_upper(dentry);
>  > >  > >
>  > >  > >                 if (attr->ia_valid & ATTR_SIZE) {
>  > >  > > +                       if (full_copy_up && !(attr->ia_valid & ~ATTR_SIZE)) {
>  > >  > > +                               inode_lock(upperdentry->d_inode);
>  > >  > > +                               ovl_copyattr(upperdentry->d_inode, dentry->d_inode);
>  > >  > > +                               inode_unlock(upperdentry->d_inode);
>  > >  >
>  > >  > All that this is saving is an extra notify_change() call and I am not sure it is
>  > >  > worth the special casing.
>  > >  >
>  > >  > Also, I think that is a bug and would make xfstest overlay/013 fail.
>  > >
>  > > I ran testcases in overlay directory and didn't find failure related to this change.
>  > > However, generic/313 failed unexpectedly, the reason is I used full_copy_up var
>  > > wrongly, for the file which has upper still needs to go through notify_change().
>  > >
>  > > By the way, I don't fully understand calling copy-up function(ovl_copy_up() or ovl_copy_up_with_data())
>  > > even for the file which has upper. Maybe it's better to optimize this part first in separated patch.
>  > >
>  >
>  > There is a difference between "has upper" and "has upper data".
>  > It's related to metacopy.
>  >
>
> I see, my point here is we can skip calling copy-up function for files which already have upper data.
>

ovl_copy_up() already does all those optimizations internally.
We can pre-check upper data if it helps anything.

>
>
>  > >
>  > >  > When lower file is being executed, its true that we copy up anyway
>  > >  > and that it is safe to do that, but test and applications expect to get
>  > >  > ETXTBSY error all the same.
>  > >
>  > > Actually we have already do the check and return ETXTBSY error, see below.
>  > >
>  > > err = -ETXTBSY;
>  > > if (atomic_read(&realinode->i_writecount) < 0)
>  > >         goto out_drop_write;
>  > >
>  >
>  > Yes, my point exactly. Your code does goto out_drop_write; before that check
>  > so it will skip it.
>  >
>
> No, if you look at the code more closely , you'll find the goto in my code is actually after the check, :-).
>

I don't see it?? I see get_write_access() check after your goto.
What are we missing?
Anyway, I hold my opinion that the optimization of skipping notify_change()
has little benefit and is a potential for bugs, even if the specific
bug I pointed
out is not real.

Thanks,
Amir.

Re: [PATCH v2 3/3] ovl: copy-up optimization for truncate

[Miklos Szeredi]

On Fri, Apr 9, 2021 at 7:50 AM Amir Goldstein <amir73il@gmail.com> wrote:

> Anyway, I hold my opinion that the optimization of skipping notify_change()
> has little benefit and is a potential for bugs, even if the specific
> bug I pointed
> out is not real.

I can agree with that.

At this point the only sane way I can see this optimization be
implemented is to split the copy-up into a preparation and a commit
phase:

1) copy to tmpfile (can optimize to truncated size)
2) call notify_change() on the tmpfile to truncate
3) if successful, link the tmpfile into upper

With that the truncate still remains a single atomic operation.
Only slight difference is that now ctime will not match mtime after a
copy-up/truncate operation, but I guess that's not something anybody
should be dependent on.

Thanks,
Miklos