Re: [PATCH RFC] fs: don't force i_version increment when timestamps change

[Mateusz Guzik]

On Tue, Aug 27, 2024 at 4:30 PM Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>
> inode_maybe_inc_iversion will increment the i_version if it has been
> queried. We can also set the "force" parameter to force an increment.
> When we originally did this, the idea was to set it to force when we
> were going to be otherwise updating the inode timestamps anyway --
> purely a "might as well" measure.
>
> When we used coarse-grained timestamps exclusively, this would give us
> an extra cmpxchg operation roughly every jiffy when a file is under
> heavy writes. With the advent of multigrain timestamps however, this can
> fire more frequently.
>
> There is no requirement to force an increment to the i_version just
> because a timestamp changed, so stop doing it.
>
> Cc: Mateusz Guzik <mjguzik@xxxxxxxxx>
> Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> ---
> I've not tested this other than for compilation, but it should be fine.
> Mateusz, does this help your workload at all? There may be other places
> where we can just set this to false (maybe even convert some of the
> inode_inc_iversion() calls to this.

This does not make a difference for me since any call to
inode_maybe_inc_iversion is guaranteed to provide a full barrier, this
is at best moving it from the cmpxchg to spelled out smp_mb

As to whether the patch makes sense otherwise I have no idea.

> ---
>  fs/inode.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/inode.c b/fs/inode.c
> index 10c4619faeef..2abd6317839b 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -1962,7 +1962,7 @@ int inode_update_timestamps(struct inode *inode, int flags)
>                         inode_set_mtime_to_ts(inode, now);
>                         updated |= S_MTIME;
>                 }
> -               if (IS_I_VERSION(inode) && inode_maybe_inc_iversion(inode, updated))
> +               if (IS_I_VERSION(inode) && inode_maybe_inc_iversion(inode, false))
>                         updated |= S_VERSION;
>         } else {
>                 now = current_time(inode);
>
> ---
> base-commit: 3e9bff3bbe1355805de919f688bef4baefbfd436
> change-id: 20240827-iversion-afa53f0a070b
>
> Best regards,
> --
> Jeff Layton <jlayton@xxxxxxxxxx>
>


-- 
Mateusz Guzik <mjguzik gmail.com>