[PATCH] ovl: copy-up optimization for truncate

[PATCH] ovl: copy-up optimization for truncate

[Chengguang Xu]

Currently copy-up will copy whole lower file to upper
regardless of the data range which is needed for further
operation. This patch avoids unnecessary copy when truncate
size is smaller than the file size.

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

diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 0b2891c6c71e..b426a3f59636 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -866,7 +866,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);
@@ -903,6 +903,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);
@@ -929,7 +931,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);
@@ -962,7 +964,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);
@@ -994,7 +996,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);
 		}
 	}
@@ -1002,12 +1004,12 @@ 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);
+	return ovl_copy_up_flags(dentry, O_WRONLY, size);
 }
 
 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 003cf83bf78a..5eb99e4c3c73 100644
--- a/fs/overlayfs/inode.c
+++ b/fs/overlayfs/inode.c
@@ -44,7 +44,9 @@ int ovl_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
 	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 < i_size_read(d_inode(dentry)) ?
+				attr->ia_size : 0);
 	if (!err) {
 		struct inode *winode = NULL;
 
diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
index 95cff83786a5..1bc17ca87158 100644
--- a/fs/overlayfs/overlayfs.h
+++ b/fs/overlayfs/overlayfs.h
@@ -524,7 +524,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] ovl: copy-up optimization for truncate

[Amir Goldstein]

On Mon, Mar 8, 2021 at 1:25 PM Chengguang Xu <cgxu519@mykernel.net> wrote:
>
> Currently copy-up will copy whole lower file to upper
> regardless of the data range which is needed for further
> operation. This patch avoids unnecessary copy when truncate
> size is smaller than the file size.
>
> Signed-off-by: Chengguang Xu <cgxu519@mykernel.net>

Nice!

Reviewed-by: Amir Goldstein <amir73il@gmail.com>

This reminds me. I think this comment in ovl_nlink_start() can be removed:

         * TODO: implement metadata only index copy up when called with
         *       ovl_copy_up_flags(dentry, O_PATH).

It was added in 2017 and not removed when metacopy was implemented.
Technically, I think that truncate() could also be a metacopy, but it seems
like a corner case that is not worth the trouble...

Thanks,
Amir.


> ---
>  fs/overlayfs/copy_up.c   | 16 +++++++++-------
>  fs/overlayfs/inode.c     |  4 +++-
>  fs/overlayfs/overlayfs.h |  2 +-
>  3 files changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index 0b2891c6c71e..b426a3f59636 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -866,7 +866,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);
> @@ -903,6 +903,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);
> @@ -929,7 +931,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);
> @@ -962,7 +964,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);
> @@ -994,7 +996,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);
>                 }
>         }
> @@ -1002,12 +1004,12 @@ 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);
> +       return ovl_copy_up_flags(dentry, O_WRONLY, size);
>  }
>
>  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 003cf83bf78a..5eb99e4c3c73 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -44,7 +44,9 @@ int ovl_setattr(struct user_namespace *mnt_userns, struct dentry *dentry,
>         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 < i_size_read(d_inode(dentry)) ?
> +                               attr->ia_size : 0);
>         if (!err) {
>                 struct inode *winode = NULL;
>
> diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> index 95cff83786a5..1bc17ca87158 100644
> --- a/fs/overlayfs/overlayfs.h
> +++ b/fs/overlayfs/overlayfs.h
> @@ -524,7 +524,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] ovl: copy-up optimization for truncate

[Miklos Szeredi]

On Mon, Mar 8, 2021 at 12:17 PM Chengguang Xu <cgxu519@mykernel.net> wrote:
>
> Currently copy-up will copy whole lower file to upper
> regardless of the data range which is needed for further
> operation. This patch avoids unnecessary copy when truncate
> size is smaller than the file size.

This doesn't look right.   If copy up succeeds, resulting in a
truncated file, then we should return success there and then.   Doing
the truncate again and failing (unlikely, but I wouldn't think it
impossible) wouldn't be nice.

But need to be careful, because we could possibly have other attribute
change requests besides ATTR_SIZE, in which case optimizing the
truncate away and returning success wouldn't be correct.

Minor issue: this patch doesn't optimize the truncate to zero case.
That's not a bug, but I'm curious if that is an oversight or
deliberate.

Thanks,
Miklos

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

[Chengguang Xu]

 ---- 在 星期一, 2021-03-29 23:13:52 Miklos Szeredi <miklos@szeredi.hu> 撰写 ----
 > On Mon, Mar 8, 2021 at 12:17 PM Chengguang Xu <cgxu519@mykernel.net> wrote:
 > >
 > > Currently copy-up will copy whole lower file to upper
 > > regardless of the data range which is needed for further
 > > operation. This patch avoids unnecessary copy when truncate
 > > size is smaller than the file size.
 > 
 > This doesn't look right.   If copy up succeeds, resulting in a
 > truncated file, then we should return success there and then.   Doing
 > the truncate again and failing (unlikely, but I wouldn't think it
 > impossible) wouldn't be nice.
 > 
 > But need to be careful, because we could possibly have other attribute
 > change requests besides ATTR_SIZE, in which case optimizing the
 > truncate away and returning success wouldn't be correct.

OK, I'll modify in V2.

 > 
 > Minor issue: this patch doesn't optimize the truncate to zero case.
 > That's not a bug, but I'm curious if that is an oversight or
 > deliberate.
 > 

I overlooked that case because all our cases use O_TRUNC flag on open time
when truncate to zero size. How about specify O_TRUNC flag when calling
copy-up function for this case?


Thanks,
Chengguang

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

[Chengguang Xu]

 ---- 在 星期一, 2021-03-29 23:13:52 Miklos Szeredi <miklos@szeredi.hu> 撰写 ----
 > On Mon, Mar 8, 2021 at 12:17 PM Chengguang Xu <cgxu519@mykernel.net> wrote:
 > >
 > > Currently copy-up will copy whole lower file to upper
 > > regardless of the data range which is needed for further
 > > operation. This patch avoids unnecessary copy when truncate
 > > size is smaller than the file size.
 > 
 > This doesn't look right.   If copy up succeeds, resulting in a
 > truncated file, then we should return success there and then.   Doing
 > the truncate again and failing (unlikely, but I wouldn't think it
 > impossible) wouldn't be nice.

Hi Miklos

I noticed a problem here, if we just return success after copy-up then mtime
keeps the same as lower file. I think doing the truncate again would be better
than manually updating the upper file's mtime. What do you think for this?

Thanks,
Chengguang

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

[Miklos Szeredi]

On Thu, Apr 1, 2021 at 1:15 PM Chengguang Xu <cgxu519@mykernel.net> wrote:
>
>  ---- 在 星期一, 2021-03-29 23:13:52 Miklos Szeredi <miklos@szeredi.hu> 撰写 ----
>  > On Mon, Mar 8, 2021 at 12:17 PM Chengguang Xu <cgxu519@mykernel.net> wrote:
>  > >
>  > > Currently copy-up will copy whole lower file to upper
>  > > regardless of the data range which is needed for further
>  > > operation. This patch avoids unnecessary copy when truncate
>  > > size is smaller than the file size.
>  >
>  > This doesn't look right.   If copy up succeeds, resulting in a
>  > truncated file, then we should return success there and then.   Doing
>  > the truncate again and failing (unlikely, but I wouldn't think it
>  > impossible) wouldn't be nice.
>
> Hi Miklos
>
> I noticed a problem here, if we just return success after copy-up then mtime
> keeps the same as lower file. I think doing the truncate again would be better
> than manually updating the upper file's mtime. What do you think for this?

Let's simplify instead:  skip the mtime restore on copy-up.   Not sure
how that's handled on O_TRUNC opens, maybe it's relevant to that case
too.

Thanks,
Miklos

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

[Chengguang Xu]

 ---- 在 星期四, 2021-04-01 19:31:12 Miklos Szeredi <miklos@szeredi.hu> 撰写 ----
 > On Thu, Apr 1, 2021 at 1:15 PM Chengguang Xu <cgxu519@mykernel.net> wrote:
 > >
 > >  ---- 在 星期一, 2021-03-29 23:13:52 Miklos Szeredi <miklos@szeredi.hu> 撰写 ----
 > >  > On Mon, Mar 8, 2021 at 12:17 PM Chengguang Xu <cgxu519@mykernel.net> wrote:
 > >  > >
 > >  > > Currently copy-up will copy whole lower file to upper
 > >  > > regardless of the data range which is needed for further
 > >  > > operation. This patch avoids unnecessary copy when truncate
 > >  > > size is smaller than the file size.
 > >  >
 > >  > This doesn't look right.   If copy up succeeds, resulting in a
 > >  > truncated file, then we should return success there and then.   Doing
 > >  > the truncate again and failing (unlikely, but I wouldn't think it
 > >  > impossible) wouldn't be nice.
 > >
 > > Hi Miklos
 > >
 > > I noticed a problem here, if we just return success after copy-up then mtime
 > > keeps the same as lower file. I think doing the truncate again would be better
 > > than manually updating the upper file's mtime. What do you think for this?
 > 
 > Let's simplify instead:  skip the mtime restore on copy-up.   Not sure
 > how that's handled on O_TRUNC opens, maybe it's relevant to that case
 > too.
 > 

Currently on O_TRUNC open, copy-up(zero size) triggered by vfs_open() then do the truncate(zero size)
operation by handle_truncate() afterwards. mtime will be updated after calling handle_truncate().



Thanks,
Chengguang