On Mon 07-08-23 15:38:34, Jeff Layton wrote:
> In future patches we're going to change how the ctime is updated
> to keep track of when it has been queried. The way that the update_time
> operation works (and a lot of its callers) make this difficult, since
> they grab a timestamp early and then pass it down to eventually be
> copied into the inode.
>
> All of the existing update_time callers pass in the result of
> current_time() in some fashion. Drop the "time" parameter from
> generic_update_time, and rework it to fetch its own timestamp.
>
> This change means that an update_time could fetch a different timestamp
> than was seen in inode_needs_update_time. update_time is only ever
> called with one of two flag combinations: Either S_ATIME is set, or
> S_MTIME|S_CTIME|S_VERSION are set.
>
> With this change we now treat the flags argument as an indicator that
> some value needed to be updated when last checked, rather than an
> indication to update specific timestamps.
>
> Rework the logic for updating the timestamps and put it in a new
> inode_update_timestamps helper that other update_time routines can use.
> S_ATIME is as treated as we always have, but if any of the other three
> are set, then we attempt to update all three.
>
> Also, some callers of generic_update_time need to know what timestamps
> were actually updated. Change it to return an S_* flag mask to indicate
> that and rework the callers to expect it.
>
> Signed-off-by: Jeff Layton <jlay...@kernel.org>
Looks good to me. Feel free to add:
Reviewed-by: Jan Kara <j...@suse.cz>
Honza
> ---
> fs/gfs2/inode.c | 3 +-
> fs/inode.c | 84
> +++++++++++++++++++++++++++++++++++++++++------------
> fs/orangefs/inode.c | 3 +-
> fs/ubifs/file.c | 6 ++--
> fs/xfs/xfs_iops.c | 6 ++--
> include/linux/fs.h | 3 +-
> 6 files changed, 80 insertions(+), 25 deletions(-)
>
> diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
> index 200cabf3b393..f1f04557aa21 100644
> --- a/fs/gfs2/inode.c
> +++ b/fs/gfs2/inode.c
> @@ -2155,7 +2155,8 @@ static int gfs2_update_time(struct inode *inode, struct
> timespec64 *time,
> if (error)
> return error;
> }
> - return generic_update_time(inode, time, flags);
> + generic_update_time(inode, flags);
> + return 0;
> }
>
> static const struct inode_operations gfs2_file_iops = {
> diff --git a/fs/inode.c b/fs/inode.c
> index 3fc251bfaf73..e07e45f6cd01 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -1881,29 +1881,76 @@ static int relatime_need_update(struct vfsmount *mnt,
> struct inode *inode,
> return 0;
> }
>
> -int generic_update_time(struct inode *inode, struct timespec64 *time, int
> flags)
> +/**
> + * inode_update_timestamps - update the timestamps on the inode
> + * @inode: inode to be updated
> + * @flags: S_* flags that needed to be updated
> + *
> + * The update_time function is called when an inode's timestamps need to be
> + * updated for a read or write operation. This function handles updating the
> + * actual timestamps. It's up to the caller to ensure that the inode is
> marked
> + * dirty appropriately.
> + *
> + * In the case where any of S_MTIME, S_CTIME, or S_VERSION need to be
> updated,
> + * attempt to update all three of them. S_ATIME updates can be handled
> + * independently of the rest.
> + *
> + * Returns a set of S_* flags indicating which values changed.
> + */
> +int inode_update_timestamps(struct inode *inode, int flags)
> {
> - int dirty_flags = 0;
> + int updated = 0;
> + struct timespec64 now;
> +
> + if (flags & (S_MTIME|S_CTIME|S_VERSION)) {
> + struct timespec64 ctime = inode_get_ctime(inode);
>
> - if (flags & (S_ATIME | S_CTIME | S_MTIME)) {
> - if (flags & S_ATIME)
> - inode->i_atime = *time;
> - if (flags & S_CTIME)
> - inode_set_ctime_to_ts(inode, *time);
> - if (flags & S_MTIME)
> - inode->i_mtime = *time;
> -
> - if (inode->i_sb->s_flags & SB_LAZYTIME)
> - dirty_flags |= I_DIRTY_TIME;
> - else
> - dirty_flags |= I_DIRTY_SYNC;
> + now = inode_set_ctime_current(inode);
> + if (!timespec64_equal(&now, &ctime))
> + updated |= S_CTIME;
> + if (!timespec64_equal(&now, &inode->i_mtime)) {
> + inode->i_mtime = now;
> + updated |= S_MTIME;
> + }
> + if (IS_I_VERSION(inode) && inode_maybe_inc_iversion(inode,
> updated))
> + updated |= S_VERSION;
> + } else {
> + now = current_time(inode);
> }
>
> - if ((flags & S_VERSION) && inode_maybe_inc_iversion(inode, false))
> - dirty_flags |= I_DIRTY_SYNC;
> + if (flags & S_ATIME) {
> + if (!timespec64_equal(&now, &inode->i_atime)) {
> + inode->i_atime = now;
> + updated |= S_ATIME;
> + }
> + }
> + return updated;
> +}
> +EXPORT_SYMBOL(inode_update_timestamps);
> +
> +/**
> + * generic_update_time - update the timestamps on the inode
> + * @inode: inode to be updated
> + * @flags: S_* flags that needed to be updated
> + *
> + * The update_time function is called when an inode's timestamps need to be
> + * updated for a read or write operation. In the case where any of S_MTIME,
> S_CTIME,
> + * or S_VERSION need to be updated we attempt to update all three of them.
> S_ATIME
> + * updates can be handled done independently of the rest.
> + *
> + * Returns a S_* mask indicating which fields were updated.
> + */
> +int generic_update_time(struct inode *inode, int flags)
> +{
> + int updated = inode_update_timestamps(inode, flags);
> + int dirty_flags = 0;
>
> + if (updated & (S_ATIME|S_MTIME|S_CTIME))
> + dirty_flags = inode->i_sb->s_flags & SB_LAZYTIME ? I_DIRTY_TIME
> : I_DIRTY_SYNC;
> + if (updated & S_VERSION)
> + dirty_flags |= I_DIRTY_SYNC;
> __mark_inode_dirty(inode, dirty_flags);
> - return 0;
> + return updated;
> }
> EXPORT_SYMBOL(generic_update_time);
>
> @@ -1915,7 +1962,8 @@ int inode_update_time(struct inode *inode, struct
> timespec64 *time, int flags)
> {
> if (inode->i_op->update_time)
> return inode->i_op->update_time(inode, time, flags);
> - return generic_update_time(inode, time, flags);
> + generic_update_time(inode, flags);
> + return 0;
> }
> EXPORT_SYMBOL(inode_update_time);
>
> diff --git a/fs/orangefs/inode.c b/fs/orangefs/inode.c
> index a52c30e80f45..3afa2a69bc63 100644
> --- a/fs/orangefs/inode.c
> +++ b/fs/orangefs/inode.c
> @@ -903,9 +903,10 @@ int orangefs_permission(struct mnt_idmap *idmap,
> int orangefs_update_time(struct inode *inode, struct timespec64 *time, int
> flags)
> {
> struct iattr iattr;
> +
> gossip_debug(GOSSIP_INODE_DEBUG, "orangefs_update_time: %pU\n",
> get_khandle_from_ino(inode));
> - generic_update_time(inode, time, flags);
> + flags = generic_update_time(inode, flags);
> memset(&iattr, 0, sizeof iattr);
> if (flags & S_ATIME)
> iattr.ia_valid |= ATTR_ATIME;
> diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
> index 436b27d7c58f..df9086b19cd0 100644
> --- a/fs/ubifs/file.c
> +++ b/fs/ubifs/file.c
> @@ -1387,8 +1387,10 @@ int ubifs_update_time(struct inode *inode, struct
> timespec64 *time,
> .dirtied_ino_d = ALIGN(ui->data_len, 8) };
> int err, release;
>
> - if (!IS_ENABLED(CONFIG_UBIFS_ATIME_SUPPORT))
> - return generic_update_time(inode, time, flags);
> + if (!IS_ENABLED(CONFIG_UBIFS_ATIME_SUPPORT)) {
> + generic_update_time(inode, flags);
> + return 0;
> + }
>
> err = ubifs_budget_space(c, &req);
> if (err)
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 3a9363953ef2..731f45391baa 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -1042,8 +1042,10 @@ xfs_vn_update_time(
>
> if (inode->i_sb->s_flags & SB_LAZYTIME) {
> if (!((flags & S_VERSION) &&
> - inode_maybe_inc_iversion(inode, false)))
> - return generic_update_time(inode, now, flags);
> + inode_maybe_inc_iversion(inode, false))) {
> + generic_update_time(inode, flags);
> + return 0;
> + }
>
> /* Capture the iversion update that just occurred */
> log_flags |= XFS_ILOG_CORE;
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 85977cdeda94..bb3c2c4f871f 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2343,7 +2343,8 @@ extern int current_umask(void);
>
> extern void ihold(struct inode * inode);
> extern void iput(struct inode *);
> -extern int generic_update_time(struct inode *, struct timespec64 *, int);
> +int inode_update_timestamps(struct inode *inode, int flags);
> +int generic_update_time(struct inode *, int);
>
> /* /sys/fs */
> extern struct kobject *fs_kobj;
>
> --
> 2.41.0
>
--
Jan Kara <j...@suse.com>
SUSE Labs, CR
