($INBOX_DIR/description missing)
 help / color / Atom feed
* [PATCH] ovl: copy-up optimization for truncate
@ 2021-03-08 11:17 Chengguang Xu
  2021-03-08 12:09 ` Amir Goldstein
  2021-03-29 15:13 ` Miklos Szeredi
  0 siblings, 2 replies; 7+ messages in thread
From: Chengguang Xu @ 2021-03-08 11:17 UTC (permalink / raw)
  To: miklos; +Cc: linux-unionfs, 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



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] ovl: copy-up optimization for truncate
  2021-03-08 11:17 [PATCH] ovl: copy-up optimization for truncate Chengguang Xu
@ 2021-03-08 12:09 ` Amir Goldstein
  2021-03-29 15:13 ` Miklos Szeredi
  1 sibling, 0 replies; 7+ messages in thread
From: Amir Goldstein @ 2021-03-08 12:09 UTC (permalink / raw)
  To: Chengguang Xu; +Cc: Miklos Szeredi, overlayfs

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
>
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] ovl: copy-up optimization for truncate
  2021-03-08 11:17 [PATCH] ovl: copy-up optimization for truncate Chengguang Xu
  2021-03-08 12:09 ` Amir Goldstein
@ 2021-03-29 15:13 ` Miklos Szeredi
  2021-03-30  7:17   ` Chengguang Xu
  2021-04-01 11:15   ` Chengguang Xu
  1 sibling, 2 replies; 7+ messages in thread
From: Miklos Szeredi @ 2021-03-29 15:13 UTC (permalink / raw)
  To: Chengguang Xu; +Cc: overlayfs

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] ovl: copy-up optimization for truncate
  2021-03-29 15:13 ` Miklos Szeredi
@ 2021-03-30  7:17   ` Chengguang Xu
  2021-04-01 11:15   ` Chengguang Xu
  1 sibling, 0 replies; 7+ messages in thread
From: Chengguang Xu @ 2021-03-30  7:17 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: overlayfs

 ---- 在 星期一, 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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] ovl: copy-up optimization for truncate
  2021-03-29 15:13 ` Miklos Szeredi
  2021-03-30  7:17   ` Chengguang Xu
@ 2021-04-01 11:15   ` Chengguang Xu
  2021-04-01 11:31     ` Miklos Szeredi
  1 sibling, 1 reply; 7+ messages in thread
From: Chengguang Xu @ 2021-04-01 11:15 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: overlayfs

 ---- 在 星期一, 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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] ovl: copy-up optimization for truncate
  2021-04-01 11:15   ` Chengguang Xu
@ 2021-04-01 11:31     ` Miklos Szeredi
  2021-04-01 12:39       ` Chengguang Xu
  0 siblings, 1 reply; 7+ messages in thread
From: Miklos Szeredi @ 2021-04-01 11:31 UTC (permalink / raw)
  To: Chengguang Xu; +Cc: overlayfs

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] ovl: copy-up optimization for truncate
  2021-04-01 11:31     ` Miklos Szeredi
@ 2021-04-01 12:39       ` Chengguang Xu
  0 siblings, 0 replies; 7+ messages in thread
From: Chengguang Xu @ 2021-04-01 12:39 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: overlayfs

 ---- 在 星期四, 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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-08 11:17 [PATCH] ovl: copy-up optimization for truncate Chengguang Xu
2021-03-08 12:09 ` Amir Goldstein
2021-03-29 15:13 ` Miklos Szeredi
2021-03-30  7:17   ` Chengguang Xu
2021-04-01 11:15   ` Chengguang Xu
2021-04-01 11:31     ` Miklos Szeredi
2021-04-01 12:39       ` Chengguang Xu

($INBOX_DIR/description missing)

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-unionfs/0 linux-unionfs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-unionfs linux-unionfs/ https://lore.kernel.org/linux-unionfs \
		linux-unionfs@vger.kernel.org
	public-inbox-index linux-unionfs

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-unionfs


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git