On Wed, Apr 7, 2021 at 12:04 AM Chengguang Xu <cgxu519@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> ---
> 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.
