Re: [PATCH v2 1/3] fs: stamp the current time for a stale delegated ctime update

[Jan Kara]

On Wed 02-09-26 14:54:14, Jeff Layton wrote:
> inode_set_ctime_deleg() drops an update that does not advance the ctime.
> That is correct when a client reports a timestamp that it advanced on its
> own. It is wrong when the client moves the timestamp backwards on purpose,
> as utimensat() does.
> 
> Stamp the current time in that case. The ctime still never moves backwards.
> 
> Fixes: 3952f1cbcbc4 ("nfsd: fix SETATTR updates for delegated timestamps")
> Assisted-by: LLM
> Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>

Looks good to me. Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> ---
>  fs/inode.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index ba7da39be4a3..8391814a4016 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -2959,11 +2959,17 @@ EXPORT_SYMBOL(inode_set_ctime_current);
>   * inode attributes, including the mtime. When updating the mtime, update
>   * the ctime to a value at least equal to that.
>   *
> - * This can race with concurrent updates to the inode, in which
> - * case the update is skipped.
> + * The ctime never moves backwards. An @update that does not advance the ctime
> + * records the current time instead, so that the delegated change is still
> + * visible in the ctime.
> + *
> + * This can still race with a concurrent update to the inode. That stamp takes
> + * precedence, and is at least as recent as the one it displaces.
>   *
>   * Note that this works even when multigrain timestamps are not enabled,
>   * so it is used in either case.
> + *
> + * Returns the resulting ctime.
>   */
>  struct timespec64 inode_set_ctime_deleg(struct inode *inode, struct timespec64 update)
>  {
> @@ -2975,9 +2981,12 @@ struct timespec64 inode_set_ctime_deleg(struct inode *inode, struct timespec64 u
>  	cur_ts.tv_nsec = cur & ~I_CTIME_QUERIED;
>  	cur_ts.tv_sec = inode_get_ctime_sec(inode);
>  
> -	/* If the update is older than the existing value, skip it. */
> +	/*
> +	 * The update does not advance the ctime. Stamp the current time, so
> +	 * that the delegated change is still visible in the ctime.
> +	 */
>  	if (timespec64_compare(&update, &cur_ts) <= 0)
> -		return cur_ts;
> +		return inode_set_ctime_current(inode);
>  
>  	ktime_get_coarse_real_ts64_mg(&now);
>  
> 
> -- 
> 2.55.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR