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

[Amir Goldstein <amir73il@gmail.com>]

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.