| Message ID | 20240827-iversion-v1-1-b46a2b612400@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] fs: don't force i_version increment when timestamps change | expand |
On Tue, Aug 27, 2024 at 4:30 PM Jeff Layton <jlayton@kernel.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@gmail.com> > Signed-off-by: Jeff Layton <jlayton@kernel.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@kernel.org> >
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);
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@gmail.com> Signed-off-by: Jeff Layton <jlayton@kernel.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. --- fs/inode.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- base-commit: 3e9bff3bbe1355805de919f688bef4baefbfd436 change-id: 20240827-iversion-afa53f0a070b Best regards,