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

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