Re: [PATCH 5/9] fs: ext4: set inode timestamps on write

Hi Heinrich,

On 2026-05-18T05:57:19, Heinrich Schuchardt
<[email protected]> wrote:
> fs: ext4: set inode timestamps on write
>
> Replace the hardcoded zero timestamp in ext4fs_write() with the actual
> current time obtained from the RTC when CONFIG_DM_RTC is enabled.
>
> Per the ext2/3/4 specification (ext2 design document, section 4.2):
>   i_mtime  last data modification time
>   i_ctime  last inode change time (the 'c' stands for 'change', not
>            'create'; this is not a creation timestamp)
>   i_atime  last access time
>
> All three fields are set to the same value since writing data modifies
> both the file content (mtime) and the inode metadata (ctime), and any
> write also constitutes an access (atime).  If no RTC is available, the
> timestamp falls back to 2000-01-01 00:00:00 UTC, matching the FAT
> write driver behaviour.
>
> The ext4 i_crtime field (true file creation time, added in ext4) is
> stored in the extra inode area beyond the base 128-byte inode and is
> not modelled by struct ext2_inode, so it cannot be set here.
> [...]
>
> fs/ext4/ext4_write.c | 41 +++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 39 insertions(+), 2 deletions(-)

> diff --git a/fs/ext4/ext4_write.c b/fs/ext4/ext4_write.c
> @@ -21,14 +21,47 @@
> +/**
> + * define EXT4_TIMESTAMP_Y2K - 2000-01-01 00:00:00 UTC as a POSIX timestamp
> + *
> + * Used as a fallback timestamp when no RTC is available.
> + */
> +#define EXT4_TIMESTAMP_Y2K   946684800

'define' is not a valid kernel-doc directive, is it? I suggest a plain
C comment.

Also, the natural sentinel for an unknown ext4 time is 0 (Unix epoch),
which the read path already displays as 1970-01-01; picking Y2K means
files written by U-Boot show 2000 while pre-existing zero-stamped
inodes show 1970, which is jarring. The FAT-parity argument is pretty
weak since FAT cannot represent dates before 1980.

> diff --git a/fs/ext4/ext4_write.c b/fs/ext4/ext4_write.c
> @@ -21,14 +21,47 @@
> +static time_t ext4_current_timestamp(void)
> +{
> +     if (CONFIG_IS_ENABLED(DM_RTC)) {
> +             struct udevice *dev;
> +             struct rtc_time tm;
> +
> +             uclass_first_device(UCLASS_RTC, &dev);
> +             if (!dev)
> +                     goto fallback;
> +             if (dm_rtc_get(dev, &tm))
> +                     goto fallback;
> +             return rtc_mktime(&tm);
> +     }
> +fallback:
> +     return EXT4_TIMESTAMP_Y2K;
> +}

The label sitting outside the if-block and jumped to from inside reads
oddly. Please drop the gotos:

   if (CONFIG_IS_ENABLED(DM_RTC)) {
           struct udevice *dev;
           struct rtc_time tm;

           uclass_first_device(UCLASS_RTC, &dev);
           if (dev && !dm_rtc_get(dev, &tm))
                   return rtc_mktime(&tm);
   }
   return EXT4_TIMESTAMP_Y2K;

Also, this helper sits at the top of a 1000-line file but is only used
at the bottom from ext4fs_write() - please move it nearer to its
caller.

> diff --git a/fs/ext4/ext4_write.c b/fs/ext4/ext4_write.c
> @@ -979,7 +1012,11 @@ int ext4fs_write(const char *fname, const char *buffer,
>       }
>       if (existing_file_inode)
>               free(existing_file_inode);
> -     /* ToDo: Update correct time */
> +     /*
> +      * Note: ext4 i_crtime (true creation time) lives in the extended
> +      * inode area beyond the base 128-byte inode and is not modelled
> +      * by struct ext2_inode, so it cannot be set here.
> +      */
>       file_inode->mtime = cpu_to_le32(timestamp);

The comment describes a field this code does not touch, which is more
confusing than helpful. The i_mtime_extra/i_atime_extra/i_ctime_extra
fields (nanoseconds and the high bits past 2038) have the same
limitation and arguably matter more, since we are truncating the
timestamp to 32 bits via cpu_to_le32(). Drop the comment, or expand it
to mention all the unmodelled time fields.

> diff --git a/fs/ext4/ext4_write.c b/fs/ext4/ext4_write.c
> @@ -854,7 +887,7 @@ int ext4fs_write(const char *fname, const char *buffer,
> -     time_t timestamp = 0;
> +     time_t timestamp = ext4_current_timestamp();

Note the parent directory's mtime/ctime should also change when a file
is created or replaced, but g_parent_inode is written back unchanged a
few lines further down. Not related to this patch, but worth a
follow-up.

Regards,
Simon