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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAGudoHE+PkQp+RVen+0YZ1G6LZfG5aQsq0LrmTP+kZb-6=p2xA@mail.gmail.com>
Date: Wed, 28 Aug 2024 01:38:46 +0200
From: Mateusz Guzik <mjguzik@...il.com>
To: Jeff Layton <jlayton@...nel.org>
Cc: Alexander Viro <viro@...iv.linux.org.uk>, Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>, 
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC] fs: don't force i_version increment when timestamps change

On Tue, Aug 27, 2024 at 4:30 PM Jeff Layton <jlayton@...nel.org> 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@...il.com>
> Signed-off-by: Jeff Layton <jlayton@...nel.org>
> ---
> 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@...nel.org>
>


-- 
Mateusz Guzik <mjguzik gmail.com>