fs: rework and optimize i_version handling in filesystems
Subsystems (15)
Jeff Layton
maintainerv3 cover fs: rework and optimize i_version handling in filesystems
From: Jeff Layton <[email protected]>
v3:
- move i_version handling functions to new header file
- document that the kernel-managed i_version implementation will appear to increase over time
- fix inode_cmp_iversion to handle wraparound correctly
v2:
- xfs should use inode_peek_iversion instead of inode_peek_iversion_raw
- rework file_update_time patch
- don't dirty inode when only S_ATIME is set and SB_LAZYTIME is enabled
- better comments and documentation
tl;dr: I think we can greatly reduce the cost of the inode->i_version counter, by exploiting the fact that we don't need to increment it if no one is looking at it. We can also clean up the code to prepare to eventually expose this value via statx().
Note that this set relies on a few patches that are in other trees. The full stack that I've been testing with is here:
https://git.kernel.org/pub/scm/linux/kernel/git/jlayton/linux.git/log/?h=iversion
The inode->i_version field is supposed to be a value that changes whenever there is any data or metadata change to the inode. Some filesystems use it internally to detect directory changes during readdir. knfsd will use it if the filesystem has MS_I_VERSION set. IMA will also use it to optimize away some remeasurement if it's available. NFS and AFS just use it to store an opaque change attribute from the server.
Only btrfs, ext4, and xfs increment it for data changes. Because of this, these filesystems must log the inode to disk whenever the i_version counter changes. That has a non-zero performance impact, especially on write-heavy workloads, because we end up dirtying the inode metadata on every write, not just when the times change.
It turns out though that none of these users of i_version require that it change on every change to the file. The only real requirement is that it be different if something changed since the last time we queried for it.
If we keep track of when something queries the value, we can avoid bumping the counter and an on-disk update when nothing else has changed if no one has queried it since it was last incremented.
This patchset changes the code to only bump the i_version counter when it's strictly necessary, or when we're updating the inode metadata anyway (e.g. when times change).
It takes the approach of converting the existing accessors of i_version to use a new API, while leaving the underlying implementation mostly the same. The last patch then converts the existing implementation to keep track of whether the value has been queried since it was last incremented. It then uses that to avoid incrementing the counter when it can.
With this, we reduce inode metadata updates across all 3 filesystems down to roughly the frequency of the timestamp granularity, particularly when it's not being queried (the vastly common case).
I can see measurable performance gains on xfs and ext4 with iversion enabled, when streaming small (4k) I/Os.
btrfs shows some slight gain in testing, but not quite the magnitude that xfs and ext4 show. I'm not sure why yet and would appreciate some input from btrfs folks.
My goal is to get this into linux-next fairly soon. If it shows no problems then we can look at merging it for 4.16, or 4.17 if all of the prequisite patches are not yet merged.
Jeff Layton (19):
fs: new API for handling inode->i_version
fs: don't take the i_lock in inode_inc_iversion
fat: convert to new i_version API
affs: convert to new i_version API
afs: convert to new i_version API
btrfs: convert to new i_version API
exofs: switch to new i_version API
ext2: convert to new i_version API
ext4: convert to new i_version API
nfs: convert to new i_version API
nfsd: convert to new i_version API
ocfs2: convert to new i_version API
ufs: use new i_version API
xfs: convert to new i_version API
IMA: switch IMA over to new i_version API
fs: only set S_VERSION when updating times if necessary
xfs: avoid setting XFS_ILOG_CORE if i_version doesn't need
incrementing
btrfs: only dirty the inode in btrfs_update_time if something was
changed
fs: handle inode->i_version more efficiently
fs/affs/amigaffs.c | 5 +- fs/affs/dir.c | 5 +- fs/affs/super.c | 3 +- fs/afs/fsclient.c | 3 +- fs/afs/inode.c | 5 +- fs/btrfs/delayed-inode.c | 7 +- fs/btrfs/file.c | 1 + fs/btrfs/inode.c | 12 +- fs/btrfs/ioctl.c | 1 + fs/btrfs/tree-log.c | 4 +- fs/btrfs/xattr.c | 1 + fs/exofs/dir.c | 9 +- fs/exofs/super.c | 3 +- fs/ext2/dir.c | 9 +- fs/ext2/super.c | 5 +- fs/ext4/dir.c | 9 +- fs/ext4/inline.c | 7 +- fs/ext4/inode.c | 13 +- fs/ext4/ioctl.c | 3 +- fs/ext4/namei.c | 5 +- fs/ext4/super.c | 3 +- fs/ext4/xattr.c | 5 +- fs/fat/dir.c | 3 +- fs/fat/inode.c | 9 +- fs/fat/namei_msdos.c | 7 +- fs/fat/namei_vfat.c | 22 +-- fs/inode.c | 20 ++- fs/nfs/delegation.c | 3 +- fs/nfs/fscache-index.c | 5 +- fs/nfs/inode.c | 18 +-- fs/nfs/nfs4proc.c | 10 +- fs/nfs/nfstrace.h | 5 +- fs/nfs/write.c | 8 +- fs/nfsd/nfsfh.h | 3 +- fs/ocfs2/dir.c | 15 +- fs/ocfs2/inode.c | 3 +- fs/ocfs2/namei.c | 3 +- fs/ocfs2/quota_global.c | 3 +- fs/ufs/dir.c | 9 +- fs/ufs/inode.c | 3 +- fs/ufs/super.c | 3 +- fs/xfs/libxfs/xfs_inode_buf.c | 7 +- fs/xfs/xfs_icache.c | 5 +- fs/xfs/xfs_inode.c | 3 +- fs/xfs/xfs_inode_item.c | 3 +- fs/xfs/xfs_trans_inode.c | 16 ++- include/linux/fs.h | 17 +-- include/linux/iversion.h | 286 ++++++++++++++++++++++++++++++++++++++ security/integrity/ima/ima_api.c | 3 +- security/integrity/ima/ima_main.c | 3 +- 50 files changed, 475 insertions(+), 138 deletions(-) create mode 100644 include/linux/iversion.h
Signature · 1 line
2.14.3
Jeff Layton
maintainerPATCH v3 1/19 fs: new API for handling inode->i_version
From: Jeff Layton <[email protected]>
Add a documentation blob that explains what the i_version field is, how it is expected to work, and how it is currently implemented by various filesystems.
We already have inode_inc_iversion. Add several other functions for manipulating and accessing the i_version counter. For now, the implementation is trivial and basically works the way that all of the open-coded i_version accesses work today.
Future patches will convert existing users of i_version to use the new API, and then convert the backend implementation to do things more efficiently.
--- include/linux/fs.h | 15 ---- include/linux/iversion.h | 205 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 205 insertions(+), 15 deletions(-) create mode 100644 include/linux/iversion.h
include/linux/fs.h +0 -15
@@ -2036,21 +2036,6 @@ static inline void inode_dec_link_count(struct inode *inode)mark_inode_dirty (inode); } -/** -* inode_inc_iversion - increments i_version -* @inode: inode that need to be updated -* -* Every time the inode is modified, the i_version field will be incremented. -* The filesystem has to be mounted with i_version flag -*/ - -static inline void inode_inc_iversion (struct inode *inode) -{ -spin_lock (&inode->i_lock); - inode->i_version++; -spin_unlock (&inode->i_lock); -} -enum file_time_flags {S_ATIME =1 ,S_MTIME =2 ,
include/linux/iversion.h new file +205 -0
205 lines not on this page · raw message
Signature · 1 line
2.14.3
Jeff Layton
maintainerRe: [PATCH v3 01/19] fs: new API for handling inode->i_version
Quoted 35 lines · Jeff Layton
From: Jeff Layton <[email protected]>
Add a documentation blob that explains what the
i_versionfield is, how it is expected to work, and how it is currently implemented by various filesystems.We already have
inode_inc_iversion. Add several other functions for manipulating and accessing thei_versioncounter. For now, the implementation is trivial and basically works the way that all of the open-codedi_versionaccesses work today.Future patches will convert existing users of
i_versionto use the new API, and then convert the backend implementation to do things more efficiently.Signed-off-by Jeff Layton <[email protected]>--- include/linux/fs.h | 15 ---- include/linux/iversion.h | 205 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 205 insertions(+), 15 deletions(-) create mode 100644 include/linux/iversion.hinclude/linux/fs.h +0
-5@@ -2036,21 +2036,6 @@ static inline void inode_dec_link_count(struct inode *inode)mark_inode_dirty (inode); } -/** -* inode_inc_iversion - increments i_version -* @inode: inode that need to be updated -* -* Every time the inode is modified, the i_version field will be incremented.
include/linux/fs.h static inline void inode_dec_link_count(struct inode *inode) · line 2053
Quoted 10 lines
include/linux/fs.h +0
-10-* The filesystem has to be mounted with i_version flag -*/ - -static inline void inode_inc_iversion (struct inode *inode) -{ -spin_lock (&inode->i_lock); - inode->i_version++; -spin_unlock (&inode->i_lock); -} -
Note that this patch is broken, as I neglected to include linux/iversion.h in the files with the existing callers of inode_inc_iversion. Fixed in my in-tree version.
Quoted 214 lines
include/linux/fs.h +0
-0enum file_time_flags {S_ATIME =1 ,S_MTIME =2 ,include/linux/iversion.h new file +205
-0@@ -0,0 +1,205 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef _LINUX_IVERSION_H +#define _LINUX_IVERSION_H + +#include <linux/fs.h> + +/* +* The change attribute (i_version) is mandated by NFSv4 and is mostly for +* knfsd, but is also used for other purposes (e.g. IMA). The i_version must +* appear different to observers if there was a change to the inode's data or +* metadata since it was last queried. +* +* It should be considered an opaque value by observers. If it remains the same +* since it was last checked, then nothing has changed in the inode. If it's +* different then something has changed. Observers cannot infer anything about +* the nature or magnitude of the changes from the value, only that the inode +* has changed in some fashion. +* +* Not all filesystems properly implement the i_version counter. Subsystems that +* want to use i_version field on an inode should first check whether the +* filesystem sets the SB_I_VERSION flag (usually via the IS_I_VERSION macro). +* +* Those that set SB_I_VERSION will automatically have their i_version counter +* incremented on writes to normal files. If the SB_I_VERSION is not set, then +* the VFS will not touch it on writes, and the filesystem can use it how it +* wishes. Note that the filesystem is always responsible for updating the +* i_version on namespace changes in directories (mkdir, rmdir, unlink, etc.). +* We consider these sorts of filesystems to have a kernel-managed i_version. +* +* Note that some filesystems (e.g. NFS and AFS) just use the field to store +* a server-provided value (for the most part). For that reason, those +* filesystems do not set SB_I_VERSION. These filesystems are considered to +* have a self-managed i_version. +*/ + +/** +* inode_set_iversion_raw - set i_version to the specified raw value +* @inode: inode to set +* @new: new i_version value to set +* +* Set @inode's i_version field to @new. This function is for use by +* filesystems that self-manage the i_version. +* +* For example, the NFS client stores its NFSv4 change attribute in this way, +* and the AFS client stores the data_version from the server here. +*/ +static inline void +inode_set_iversion_raw (struct inode *inode,const u64 new) +{ + inode->i_version = new; +} + +/** +* inode_set_iversion - set i_version to a particular value +* @inode: inode to set +* @new: new i_version value to set +* +* Set @inode's i_version field to @new. This function is for filesystems with +* a kernel-managed i_version. +* +* For now, this just does the same thing as the _raw variant. +*/ +static inline void +inode_set_iversion (struct inode *inode,const u64 new) +{ +inode_set_iversion_raw (inode, new); +} + +/** +* inode_set_iversion_queried - set i_version to a particular value and set +* flag to indicate that it has been viewed +* @inode: inode to set +* @new: new i_version value to set +* +* When loading in an i_version value from a backing store, we typically don't +* know whether it was previously viewed before being stored or not. Thus, we +* must assume that it was, to ensure that any changes will result in the +* value changing. +* +* This function will set the inode's i_version, and possibly flag the value +* as if it has already been viewed at least once. +* +* For now, this just does what inode_set_iversion does. +*/ +static inline void +inode_set_iversion_queried (struct inode *inode,const u64 new) +{ +inode_set_iversion (inode, new); +} + +/** +* inode_maybe_inc_iversion - increments i_version +* @inode: inode with the i_version that should be updated +* @force: increment the counter even if it's not necessary +* +* Every time the inode is modified, the i_version field must be seen to have +* changed by any observer. +* +* In this implementation, we always increment it after taking the i_lock to +* ensure that we don't race with other incrementors. +* +* Returns true if counter was bumped, and false if it wasn't. +*/ +static inline bool +inode_maybe_inc_iversion (struct inode *inode,bool force) +{ +spin_lock (&inode->i_lock); + inode->i_version++; +spin_unlock (&inode->i_lock); +return true; +} + +/** +* inode_inc_iversion - forcibly increment i_version +* @inode: inode that needs to be updated +* +* Forcbily increment the i_version field. This always results in a change to +* the observable value. +*/ +static inline void +inode_inc_iversion (struct inode *inode) +{ +inode_maybe_inc_iversion (inode, true); +} + +/** +* inode_iversion_need_inc - is the i_version in need of being incremented? +* @inode: inode to check +* +* Returns whether the inode->i_version counter needs incrementing on the next +* change. +* +* For now, we assume that it always does. +*/ +static inline bool +inode_iversion_need_inc (struct inode *inode) +{ +return true; +} + +/** +* inode_peek_iversion_raw - grab a "raw" iversion value +* @inode: inode from which i_version should be read +* +* Grab a "raw" inode->i_version value and return it. The i_version is not +* flagged or converted in any way. This is mostly used to access a self-managed +* i_version. +* +* With those filesystems, we want to treat the i_version as an entirely +* opaque value. +*/ +static inline u64 +inode_peek_iversion_raw (const struct inode *inode) +{ +return inode->i_version; +} + +/** +* inode_peek_iversion - read i_version without flagging it to be incremented +* @inode: inode from which i_version should be read +* +* Read the inode i_version counter for an inode without registering it as a +* query. +* +* This is typically used by local filesystems that need to store an i_version +* on disk. In that situation, it's not necessary to flag it as having been +* viewed, as the result won't be used to gauge changes from that point. +*/ +static inline u64 +inode_peek_iversion (const struct inode *inode) +{ +return inode_peek_iversion_raw (inode); +} + +/** +* inode_query_iversion - read i_version for later use +* @inode: inode from which i_version should be read +* +* Read the inode i_version counter. This should be used by callers that wish +* to store the returned i_version for later comparison. This will guarantee +* that a later query of the i_version will result in a different value if +* anything has changed. +* +* This implementation just does a peek. +*/ +static inline u64 +inode_query_iversion (struct inode *inode) +{ +return inode_peek_iversion (inode); +} + +/** +* inode_cmp_iversion - check whether the i_version counter has changed +* @inode: inode to check +* @old: old value to check against its i_version +* +* Compare an i_version counter with a previous one. Returns 0 if they are +* the same or non-zero if they are different. +*/ +static inline s64 +inode_cmp_iversion (const struct inode *inode,const u64 old) +{ +return (s64 )inode_peek_iversion (inode) - (s64 )old; +} +#endif
Signature · 1 line
Jeff Layton <[email protected]>
Jeff Layton
maintainerPATCH v3 2/19 fs: don't take the i_lock in inode_inc_iversion
In reply to Jeff LaytonFrom: Jeff Layton <[email protected]>
The rationale for taking the i_lock when incrementing this value is lost in antiquity. The readers of the field don't take it (at least not universally), so my assumption is that it was only done here to serialize incrementors.
If that is indeed the case, then we can drop the i_lock from this codepath and treat it as a atomic64_t for the purposes of incrementing it. This allows us to use inode_inc_iversion without any danger of lock inversion.
Note that the read side is not fetched atomically with this change. The assumption here is that that is not a critical issue since the i_version is not fully synchronized with anything else anyway.
--- include/linux/iversion.h | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
include/linux/iversion.h +4 -3
@@ -104,12 +104,13 @@ inode_set_iversion_queried(struct inode *inode, const u64 new)static inline bool inode_maybe_inc_iversion (struct inode *inode,bool force) { -spin_lock (&inode->i_lock); - inode->i_version++; -spin_unlock (&inode->i_lock); +atomic64_t *ivp = (atomic64_t *)&inode->i_version; + +atomic64_inc (ivp);return true; } +/** * inode_inc_iversion - forcibly increment i_version * @inode: inode that needs to be updated
Signature · 1 line
2.14.3
Jeff Layton
maintainerPATCH v3 3/19 fat: convert to new i_version API
In reply to Jeff LaytonFrom: Jeff Layton <[email protected]>
--- fs/fat/dir.c | 3 ++- fs/fat/inode.c | 9 +++++---- fs/fat/namei_msdos.c | 7 ++++--- fs/fat/namei_vfat.c | 22 +++++++++++----------- 4 files changed, 22 insertions(+), 19 deletions(-)
fs/fat/dir.c +2 -1
@@ -16,6 +16,7 @@#include <linux/slab.h> #include <linux/compat.h> #include <linux/uaccess.h> +#include <linux/iversion.h> #include "fat.h" /*
@@ -1055,7 +1056,7 @@ int fat_remove_entries(struct inode *dir, struct fat_slot_info *sinfo)brelse (bh);if (err)return err; - dir->i_version++; +inode_inc_iversion (dir);if (nr_slots) {/*
fs/fat/inode.c +5 -4
@@ -20,6 +20,7 @@#include <linux/blkdev.h> #include <linux/backing-dev.h> #include <asm/unaligned.h> +#include <linux/iversion.h> #include "fat.h" #ifndef CONFIG_FAT_DEFAULT_IOCHARSET
@@ -507,7 +508,7 @@ int fat_fill_inode(struct inode *inode, struct msdos_dir_entry *de)MSDOS_I (inode)->i_pos =0 ; inode->i_uid = sbi->options.fs_uid; inode->i_gid = sbi->options.fs_gid; - inode->i_version++; +inode_inc_iversion (inode); inode->i_generation =get_seconds ();if ((de->attr &ATTR_DIR ) && !IS_FREE (de->name)) {
@@ -590,7 +591,7 @@ struct inode *fat_build_inode(struct super_block *sb, goto out; } inode->i_ino =iunique (sb,MSDOS_ROOT_INO ); - inode->i_version =1 ; +inode_set_iversion (inode,1 ); err =fat_fill_inode (inode, de);if (err) {iput (inode);
@@ -1377,7 +1378,7 @@ static int fat_read_root(struct inode *inode)MSDOS_I (inode)->i_pos =MSDOS_ROOT_INO ; inode->i_uid = sbi->options.fs_uid; inode->i_gid = sbi->options.fs_gid; - inode->i_version++; +inode_inc_iversion (inode); inode->i_generation =0 ; inode->i_mode =fat_make_mode (sbi,ATTR_DIR ,S_IRWXUGO ); inode->i_op = sbi->dir_ops;
@@ -1828,7 +1829,7 @@ int fat_fill_super(struct super_block *sb, void *data, int silent, int isvfat,if (!root_inode) goto out_fail; root_inode->i_ino =MSDOS_ROOT_INO ; - root_inode->i_version =1 ; +inode_set_iversion (root_inode,1 ); error =fat_read_root (root_inode);if (error <0 ) {iput (root_inode);
fs/fat/namei_msdos.c +4 -3
@@ -7,6 +7,7 @@*/ #include <linux/module.h> +#include <linux/iversion.h> #include "fat.h" /* Characters that are undesirable in an MS-DOS file name */
@@ -480,7 +481,7 @@ static int do_msdos_rename(struct inode *old_dir, unsigned char *old_name, }else mark_inode_dirty (old_inode); - old_dir->i_version++; +inode_inc_iversion (old_dir); old_dir->i_ctime = old_dir->i_mtime =current_time (old_dir);if (IS_DIRSYNC (old_dir)) (void )fat_sync_inode (old_dir);
@@ -508,7 +509,7 @@ static int do_msdos_rename(struct inode *old_dir, unsigned char *old_name, goto out; new_i_pos = sinfo.i_pos; } - new_dir->i_version++; +inode_inc_iversion (new_dir);fat_detach (old_inode);fat_attach (old_inode, new_i_pos);
@@ -540,7 +541,7 @@ static int do_msdos_rename(struct inode *old_dir, unsigned char *old_name, old_sinfo.bh =NULL ;if (err) goto error_dotdot; - old_dir->i_version++; +inode_inc_iversion (old_dir); old_dir->i_ctime = old_dir->i_mtime = ts;if (IS_DIRSYNC (old_dir)) (void )fat_sync_inode (old_dir);
fs/fat/namei_vfat.c +11 -11
@@ -20,7 +20,7 @@#include <linux/slab.h> #include <linux/namei.h> #include <linux/kernel.h> - +#include <linux/iversion.h> #include "fat.h" static inline unsigned long vfat_d_version (struct dentry *dentry)
@@ -46,7 +46,7 @@ static int vfat_revalidate_shortname(struct dentry *dentry) {int ret =1 ;spin_lock (&dentry->d_lock); -if (vfat_d_version (dentry) !=d_inode (dentry->d_parent)->i_version) +if (inode_cmp_iversion (d_inode (dentry->d_parent),vfat_d_version (dentry))) ret =0 ;spin_unlock (&dentry->d_lock);return ret;
@@ -759,7 +759,7 @@ static struct dentry *vfat_lookup(struct inode *dir, struct dentry *dentry, out:mutex_unlock (&MSDOS_SB (sb)->s_lock);if (!inode) -vfat_d_version_set (dentry, dir->i_version); +vfat_d_version_set (dentry,inode_query_iversion (dir));return d_splice_alias (inode, dentry); error:mutex_unlock (&MSDOS_SB (sb)->s_lock);
@@ -781,7 +781,7 @@ static int vfat_create(struct inode *dir, struct dentry *dentry, umode_t mode, err =vfat_add_entry (dir, &dentry->d_name,0 ,0 , &ts, &sinfo);if (err) goto out; - dir->i_version++; +inode_inc_iversion (dir); inode =fat_build_inode (sb, sinfo.de, sinfo.i_pos);brelse (sinfo.bh);
@@ -789,7 +789,7 @@ static int vfat_create(struct inode *dir, struct dentry *dentry, umode_t mode, err =PTR_ERR (inode); goto out; } - inode->i_version++; +inode_inc_iversion (inode); inode->i_mtime = inode->i_atime = inode->i_ctime = ts;/* timestamp is already written, so mark_inode_dirty() is unneeded. */
@@ -823,7 +823,7 @@ static int vfat_rmdir(struct inode *dir, struct dentry *dentry)clear_nlink (inode); inode->i_mtime = inode->i_atime =current_time (inode);fat_detach (inode); -vfat_d_version_set (dentry, dir->i_version); +vfat_d_version_set (dentry,inode_query_iversion (dir)); out:mutex_unlock (&MSDOS_SB (sb)->s_lock);
@@ -849,7 +849,7 @@ static int vfat_unlink(struct inode *dir, struct dentry *dentry)clear_nlink (inode); inode->i_mtime = inode->i_atime =current_time (inode);fat_detach (inode); -vfat_d_version_set (dentry, dir->i_version); +vfat_d_version_set (dentry,inode_query_iversion (dir)); out:mutex_unlock (&MSDOS_SB (sb)->s_lock);
@@ -875,7 +875,7 @@ static int vfat_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode) err =vfat_add_entry (dir, &dentry->d_name,1 , cluster, &ts, &sinfo);if (err) goto out_free; - dir->i_version++; +inode_inc_iversion (dir);inc_nlink (dir); inode =fat_build_inode (sb, sinfo.de, sinfo.i_pos);
@@ -885,7 +885,7 @@ static int vfat_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode)/* the directory was completed, just return a error */ goto out; } - inode->i_version++; +inode_inc_iversion (inode);set_nlink (inode,2 ); inode->i_mtime = inode->i_atime = inode->i_ctime = ts;/* timestamp is already written, so mark_inode_dirty() is unneeded. */
@@ -951,7 +951,7 @@ static int vfat_rename(struct inode *old_dir, struct dentry *old_dentry, goto out; new_i_pos = sinfo.i_pos; } - new_dir->i_version++; +inode_inc_iversion (new_dir);fat_detach (old_inode);fat_attach (old_inode, new_i_pos);
@@ -979,7 +979,7 @@ static int vfat_rename(struct inode *old_dir, struct dentry *old_dentry, old_sinfo.bh =NULL ;if (err) goto error_dotdot; - old_dir->i_version++; +inode_inc_iversion (old_dir); old_dir->i_ctime = old_dir->i_mtime = ts;if (IS_DIRSYNC (old_dir)) (void )fat_sync_inode (old_dir);
Signature · 1 line
2.14.3
Jeff Layton
maintainerPATCH v3 4/19 affs: convert to new i_version API
In reply to Jeff LaytonFrom: Jeff Layton <[email protected]>
--- fs/affs/amigaffs.c | 5 +++-- fs/affs/dir.c | 5 +++-- fs/affs/super.c | 3 ++- 3 files changed, 8 insertions(+), 5 deletions(-)
fs/affs/amigaffs.c +3 -2
@@ -10,6 +10,7 @@*/ #include <linux/math64.h> +#include <linux/iversion.h> #include "affs.h" /*
@@ -60,7 +61,7 @@ affs_insert_hash(struct inode *dir, struct buffer_head *bh)affs_brelse (dir_bh); dir->i_mtime = dir->i_ctime =current_time (dir); - dir->i_version++; +inode_inc_iversion (dir);mark_inode_dirty (dir);return 0 ;
@@ -114,7 +115,7 @@ affs_remove_hash(struct inode *dir, struct buffer_head *rem_bh)affs_brelse (bh); dir->i_mtime = dir->i_ctime =current_time (dir); - dir->i_version++; +inode_inc_iversion (dir);mark_inode_dirty (dir);return retval;
fs/affs/dir.c +3 -2
@@ -14,6 +14,7 @@* */ +#include <linux/iversion.h> #include "affs.h" static int affs_readdir (struct file *,struct dir_context *);
@@ -80,7 +81,7 @@ affs_readdir(struct file *file, struct dir_context *ctx)* we can jump directly to where we left off. */ ino = (u32 )(long )file->private_data; -if (ino && file->f_version == inode->i_version) { +if (ino &&inode_cmp_iversion (inode, file->f_version) ==0 ) {pr_debug ("readdir() left off=%d\n" , ino); goto inside; }
@@ -130,7 +131,7 @@ affs_readdir(struct file *file, struct dir_context *ctx) }while (ino); } done: - file->f_version = inode->i_version; + file->f_version =inode_query_iversion (inode); file->private_data = (void *)(long )ino;affs_brelse (fh_bh);
fs/affs/super.c +2 -1
@@ -21,6 +21,7 @@#include <linux/writeback.h> #include <linux/blkdev.h> #include <linux/seq_file.h> +#include <linux/iversion.h> #include "affs.h" static int affs_statfs (struct dentry *dentry,struct kstatfs *buf);
@@ -102,7 +103,7 @@ static struct inode *affs_alloc_inode(struct super_block *sb)if (!i)return NULL ; - i->vfs_inode.i_version =1 ; +inode_set_iversion (&i->vfs_inode,1 ); i->i_lc =NULL ; i->i_ext_bh =NULL ; i->i_pa_cnt =0 ;
Signature · 1 line
2.14.3
Jeff Layton
maintainerPATCH v3 5/19 afs: convert to new i_version API
In reply to Jeff LaytonFrom: Jeff Layton <[email protected]>
For AFS, it's generally treated as an opaque value, so we use the *_raw variants of the API here.
Note that AFS has quite a different definition for this counter. AFS only increments it on changes to the data, not for the metadata. We'll need to reconcile that somehow if we ever want to present this to userspace via statx.
--- fs/afs/fsclient.c | 3 ++- fs/afs/inode.c | 5 +++-- 2 files changed, 5 insertions(+), 3 deletions(-)
fs/afs/fsclient.c +2 -1
@@ -13,6 +13,7 @@#include <linux/slab.h> #include <linux/sched.h> #include <linux/circ_buf.h> +#include <linux/iversion.h> #include "internal.h" #include "afs_fs.h"
@@ -124,7 +125,7 @@ static void xdr_decode_AFSFetchStatus(const __be32 **_bp, vnode->vfs_inode.i_ctime.tv_sec = status->mtime_client; vnode->vfs_inode.i_mtime = vnode->vfs_inode.i_ctime; vnode->vfs_inode.i_atime = vnode->vfs_inode.i_ctime; - vnode->vfs_inode.i_version = data_version; +inode_set_iversion_raw (&vnode->vfs_inode, data_version); } expected_version = status->data_version;
fs/afs/inode.c +3 -2
@@ -21,6 +21,7 @@#include <linux/sched.h> #include <linux/mount.h> #include <linux/namei.h> +#include <linux/iversion.h> #include "internal.h" static const struct inode_operations afs_symlink_inode_operations = {
@@ -89,7 +90,7 @@ static int afs_inode_map_status(struct afs_vnode *vnode, struct key *key) inode->i_atime = inode->i_mtime = inode->i_ctime; inode->i_blocks =0 ; inode->i_generation = vnode->fid.unique; - inode->i_version = vnode->status.data_version; +inode_set_iversion_raw (inode, vnode->status.data_version); inode->i_mapping->a_ops = &afs_fs_aops;read_sequnlock_excl (&vnode->cb_lock);
@@ -218,7 +219,7 @@ struct inode *afs_iget_autocell(struct inode *dir, const char *dev_name, inode->i_ctime.tv_nsec =0 ; inode->i_atime = inode->i_mtime = inode->i_ctime; inode->i_blocks =0 ; - inode->i_version =0 ; +inode_set_iversion_raw (inode,0 ); inode->i_generation =0 ;set_bit (AFS_VNODE_PSEUDODIR , &vnode->flags);
Signature · 1 line
2.14.3
Jeff Layton
maintainerPATCH v3 6/19 btrfs: convert to new i_version API
In reply to Jeff LaytonFrom: Jeff Layton <[email protected]>
--- fs/btrfs/delayed-inode.c | 7 +++++-- fs/btrfs/file.c | 1 + fs/btrfs/inode.c | 7 +++++-- fs/btrfs/ioctl.c | 1 + fs/btrfs/tree-log.c | 4 +++- fs/btrfs/xattr.c | 1 + 6 files changed, 16 insertions(+), 5 deletions(-)
fs/btrfs/delayed-inode.c +5 -2
@@ -18,6 +18,7 @@*/ #include <linux/slab.h> +#include <linux/iversion.h> #include "delayed-inode.h" #include "disk-io.h" #include "transaction.h"
@@ -1700,7 +1701,8 @@ static void fill_stack_inode_item(struct btrfs_trans_handle *trans,btrfs_set_stack_inode_nbytes (inode_item,inode_get_bytes (inode));btrfs_set_stack_inode_generation (inode_item,BTRFS_I (inode)->generation); -btrfs_set_stack_inode_sequence (inode_item, inode->i_version); +btrfs_set_stack_inode_sequence (inode_item, +inode_peek_iversion (inode));btrfs_set_stack_inode_transid (inode_item, trans->transid);btrfs_set_stack_inode_rdev (inode_item, inode->i_rdev);btrfs_set_stack_inode_flags (inode_item,BTRFS_I (inode)->flags);
@@ -1754,7 +1756,8 @@ int btrfs_fill_inode(struct inode *inode, u32 *rdev)BTRFS_I (inode)->generation =btrfs_stack_inode_generation (inode_item);BTRFS_I (inode)->last_trans =btrfs_stack_inode_transid (inode_item); - inode->i_version =btrfs_stack_inode_sequence (inode_item); +inode_set_iversion_queried (inode, +btrfs_stack_inode_sequence (inode_item)); inode->i_rdev =0 ; *rdev =btrfs_stack_inode_rdev (inode_item);BTRFS_I (inode)->flags =btrfs_stack_inode_flags (inode_item);
fs/btrfs/file.c +1 -0
@@ -31,6 +31,7 @@#include <linux/slab.h> #include <linux/btrfs.h> #include <linux/uio.h> +#include <linux/iversion.h> #include "ctree.h" #include "disk-io.h" #include "transaction.h"
fs/btrfs/inode.c +5 -2
@@ -43,6 +43,7 @@#include <linux/posix_acl_xattr.h> #include <linux/uio.h> #include <linux/magic.h> +#include <linux/iversion.h> #include "ctree.h" #include "disk-io.h" #include "transaction.h"
@@ -3777,7 +3778,8 @@ static int btrfs_read_locked_inode(struct inode *inode)BTRFS_I (inode)->generation =btrfs_inode_generation (leaf, inode_item);BTRFS_I (inode)->last_trans =btrfs_inode_transid (leaf, inode_item); - inode->i_version =btrfs_inode_sequence (leaf, inode_item); +inode_set_iversion_queried (inode, +btrfs_inode_sequence (leaf, inode_item)); inode->i_generation =BTRFS_I (inode)->generation; inode->i_rdev =0 ; rdev =btrfs_inode_rdev (leaf, inode_item);
@@ -3945,7 +3947,8 @@ static void fill_inode_item(struct btrfs_trans_handle *trans, &token);btrfs_set_token_inode_generation (leaf, item,BTRFS_I (inode)->generation, &token); -btrfs_set_token_inode_sequence (leaf, item, inode->i_version, &token); +btrfs_set_token_inode_sequence (leaf, item,inode_peek_iversion (inode), + &token);btrfs_set_token_inode_transid (leaf, item, trans->transid, &token);btrfs_set_token_inode_rdev (leaf, item, inode->i_rdev, &token);btrfs_set_token_inode_flags (leaf, item,BTRFS_I (inode)->flags, &token);
fs/btrfs/ioctl.c +1 -0
@@ -43,6 +43,7 @@#include <linux/uuid.h> #include <linux/btrfs.h> #include <linux/uaccess.h> +#include <linux/iversion.h> #include "ctree.h" #include "disk-io.h" #include "transaction.h"
fs/btrfs/tree-log.c +3 -1
@@ -20,6 +20,7 @@#include <linux/slab.h> #include <linux/blkdev.h> #include <linux/list_sort.h> +#include <linux/iversion.h> #include "tree-log.h" #include "disk-io.h" #include "locking.h"
@@ -3609,7 +3610,8 @@ static void fill_inode_item(struct btrfs_trans_handle *trans,btrfs_set_token_inode_nbytes (leaf, item,inode_get_bytes (inode), &token); -btrfs_set_token_inode_sequence (leaf, item, inode->i_version, &token); +btrfs_set_token_inode_sequence (leaf, item, +inode_peek_iversion (inode), &token);btrfs_set_token_inode_transid (leaf, item, trans->transid, &token);btrfs_set_token_inode_rdev (leaf, item, inode->i_rdev, &token);btrfs_set_token_inode_flags (leaf, item,BTRFS_I (inode)->flags, &token);
fs/btrfs/xattr.c +1 -0
@@ -23,6 +23,7 @@#include <linux/xattr.h> #include <linux/security.h> #include <linux/posix_acl_xattr.h> +#include <linux/iversion.h> #include "ctree.h" #include "btrfs_inode.h" #include "transaction.h"
Signature · 6 lines
2.14.3 -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Jeff Layton
maintainerPATCH v3 7/19 exofs: switch to new i_version API
In reply to Jeff LaytonFrom: Jeff Layton <[email protected]>
--- fs/exofs/dir.c | 9 +++++---- fs/exofs/super.c | 3 ++- 2 files changed, 7 insertions(+), 5 deletions(-)
fs/exofs/dir.c +5 -4
@@ -31,6 +31,7 @@* Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA */ +#include <linux/iversion.h> #include "exofs.h" static inline unsigned exofs_chunk_size (struct inode *inode)
@@ -60,7 +61,7 @@ static int exofs_commit_chunk(struct page *page, loff_t pos, unsigned len)struct inode *dir = mapping->host;int err =0 ; - dir->i_version++; +inode_inc_iversion (dir);if (!PageUptodate (page))SetPageUptodate (page);
@@ -241,7 +242,7 @@ exofs_readdir(struct file *file, struct dir_context *ctx)unsigned long n = pos >>PAGE_SHIFT ;unsigned long npages =dir_pages (inode);unsigned chunk_mask = ~(exofs_chunk_size (inode)-1 ); -int need_revalidate = (file->f_version != inode->i_version); +bool need_revalidate =inode_cmp_iversion (inode, file->f_version);if (pos > inode->i_size -EXOFS_DIR_REC_LEN (1 ))return 0 ;
@@ -264,8 +265,8 @@ exofs_readdir(struct file *file, struct dir_context *ctx) chunk_mask); ctx->pos = (n<<PAGE_SHIFT ) + offset; } - file->f_version = inode->i_version; - need_revalidate =0 ; + file->f_version =inode_query_iversion (inode); + need_revalidate = false; } de = (struct exofs_dir_entry *)(kaddr + offset); limit = kaddr + exofs_last_byte(inode, n) -
fs/exofs/super.c +2 -1
@@ -38,6 +38,7 @@#include <linux/module.h> #include <linux/exportfs.h> #include <linux/slab.h> +#include <linux/iversion.h> #include "exofs.h"
@@ -159,7 +160,7 @@ static struct inode *exofs_alloc_inode(struct super_block *sb)if (!oi)return NULL ; - oi->vfs_inode.i_version =1 ; +inode_set_iversion (&oi->vfs_inode,1 );return &oi->vfs_inode; }
Signature · 1 line
2.14.3
Jeff Layton
maintainerPATCH v3 8/19 ext2: convert to new i_version API
In reply to Jeff LaytonFrom: Jeff Layton <[email protected]>
--- fs/ext2/dir.c | 9 +++++---- fs/ext2/super.c | 5 +++-- 2 files changed, 8 insertions(+), 6 deletions(-)
fs/ext2/dir.c +5 -4
@@ -26,6 +26,7 @@#include <linux/buffer_head.h> #include <linux/pagemap.h> #include <linux/swap.h> +#include <linux/iversion.h> typedef struct ext2_dir_entry_2 ext2_dirent ;
@@ -92,7 +93,7 @@ static int ext2_commit_chunk(struct page *page, loff_t pos, unsigned len)struct inode *dir = mapping->host;int err =0 ; - dir->i_version++; +inode_inc_iversion (dir);block_write_end (NULL , mapping, pos, len, len, page,NULL );if (pos+len > dir->i_size) {
@@ -293,7 +294,7 @@ ext2_readdir(struct file *file, struct dir_context *ctx)unsigned long npages =dir_pages (inode);unsigned chunk_mask = ~(ext2_chunk_size (inode)-1 );unsigned char *types =NULL ; -int need_revalidate = file->f_version != inode->i_version; +bool need_revalidate =inode_cmp_iversion (inode, file->f_version);if (pos > inode->i_size -EXT2_DIR_REC_LEN (1 ))return 0 ;
@@ -319,8 +320,8 @@ ext2_readdir(struct file *file, struct dir_context *ctx) offset =ext2_validate_entry (kaddr, offset, chunk_mask); ctx->pos = (n<<PAGE_SHIFT ) + offset; } - file->f_version = inode->i_version; - need_revalidate =0 ; + file->f_version =inode_query_iversion (inode); + need_revalidate = false; } de = (ext2_dirent *)(kaddr+offset); limit = kaddr +ext2_last_byte (inode, n) -EXT2_DIR_REC_LEN (1 );
fs/ext2/super.c +3 -2
@@ -33,6 +33,7 @@#include <linux/quotaops.h> #include <linux/uaccess.h> #include <linux/dax.h> +#include <linux/iversion.h> #include "ext2.h" #include "xattr.h" #include "acl.h"
@@ -184,7 +185,7 @@ static struct inode *ext2_alloc_inode(struct super_block *sb)if (!ei)return NULL ; ei->i_block_alloc_info =NULL ; - ei->vfs_inode.i_version =1 ; +inode_set_iversion (&ei->vfs_inode,1 );#ifdef CONFIG_QUOTA memset (&ei->i_dquot,0 ,sizeof (ei->i_dquot));#endif
@@ -1569,7 +1570,7 @@ static ssize_t ext2_quota_write(struct super_block *sb, int type,return err;if (inode->i_size < off+len-towrite)i_size_write (inode, off+len-towrite); - inode->i_version++; +inode_inc_iversion (inode); inode->i_mtime = inode->i_ctime =current_time (inode);mark_inode_dirty (inode);return len - towrite;
Signature · 1 line
2.14.3
Jeff Layton
maintainerPATCH v3 9/19 ext4: convert to new i_version API
In reply to Jeff LaytonFrom: Jeff Layton <[email protected]>
--- fs/ext4/dir.c | 9 +++++---- fs/ext4/inline.c | 7 ++++--- fs/ext4/inode.c | 13 +++++++++---- fs/ext4/ioctl.c | 3 ++- fs/ext4/namei.c | 5 +++-- fs/ext4/super.c | 3 ++- fs/ext4/xattr.c | 5 +++-- 7 files changed, 28 insertions(+), 17 deletions(-)
fs/ext4/dir.c +5 -4
@@ -25,6 +25,7 @@#include <linux/fs.h> #include <linux/buffer_head.h> #include <linux/slab.h> +#include <linux/iversion.h> #include "ext4.h" #include "xattr.h"
@@ -208,7 +209,7 @@ static int ext4_readdir(struct file *file, struct dir_context *ctx)* readdir(2), then we might be pointing to an invalid * dirent right now. Scan from the start of the block * to make sure. */ -if (file->f_version != inode->i_version) { +if (inode_cmp_iversion (inode, file->f_version)) {for (i =0 ; i < sb->s_blocksize && i < offset; ) { de = (struct ext4_dir_entry_2 *) (bh->b_data + i);
@@ -227,7 +228,7 @@ static int ext4_readdir(struct file *file, struct dir_context *ctx) offset = i; ctx->pos = (ctx->pos & ~(sb->s_blocksize -1 )) | offset; - file->f_version = inode->i_version; + file->f_version =inode_query_iversion (inode); } while (ctx->pos < inode->i_size
@@ -568,10 +569,10 @@ static int ext4_dx_readdir(struct file *file, struct dir_context *ctx)* cached entries. */ if ((!info->curr_node) || - (file->f_version != inode->i_version)) { +inode_cmp_iversion (inode, file->f_version)) { info->curr_node =NULL ;free_rb_tree_fname (&info->root); - file->f_version = inode->i_version; + file->f_version =inode_query_iversion (inode); ret =ext4_htree_fill_tree (file, info->curr_hash, info->curr_minor_hash, &info->next_hash);
fs/ext4/inline.c +4 -3
@@ -14,6 +14,7 @@#include <linux/iomap.h> #include <linux/fiemap.h> +#include <linux/iversion.h> #include "ext4_jbd2.h" #include "ext4.h"
@@ -1042,7 +1043,7 @@ static int ext4_add_dirent_to_inline(handle_t *handle,*/ dir->i_mtime = dir->i_ctime =current_time (dir);ext4_update_dx_flag (dir); - dir->i_version++; +inode_inc_iversion (dir);return 1 ; }
@@ -1494,7 +1495,7 @@ int ext4_read_inline_dir(struct file *file,* dirent right now. Scan from the start of the inline * dir to make sure. */ -if (file->f_version != inode->i_version) { +if (inode_cmp_iversion (inode, file->f_version)) {for (i =0 ; i < extra_size && i < offset;) {/* * "." is with offset 0 and
@@ -1526,7 +1527,7 @@ int ext4_read_inline_dir(struct file *file, } offset = i; ctx->pos = offset; - file->f_version = inode->i_version; + file->f_version =inode_query_iversion (inode); }while (ctx->pos < extra_size) {
fs/ext4/inode.c +9 -4
@@ -39,6 +39,7 @@#include <linux/slab.h> #include <linux/bitops.h> #include <linux/iomap.h> +#include <linux/iversion.h> #include "ext4_jbd2.h" #include "xattr.h"
@@ -4873,12 +4874,14 @@ struct inode *ext4_iget(struct super_block *sb, unsigned long ino)EXT4_EINODE_GET_XTIME (i_crtime, ei, raw_inode);if (likely (!test_opt2 (inode->i_sb,HURD_COMPAT ))) { - inode->i_version =le32_to_cpu (raw_inode->i_disk_version); +u64 ivers =le32_to_cpu (raw_inode->i_disk_version); +if (EXT4_INODE_SIZE (inode->i_sb) >EXT4_GOOD_OLD_INODE_SIZE ) {if (EXT4_FITS_IN_INODE (raw_inode, ei, i_version_hi)) - inode->i_version |= + ivers |= (__u64)(le32_to_cpu (raw_inode->i_version_hi)) <<32 ; } +inode_set_iversion_queried (inode, ivers); } ret =0 ;
@@ -5164,11 +5167,13 @@ static int ext4_do_update_inode(handle_t *handle, }if (likely (!test_opt2 (inode->i_sb,HURD_COMPAT ))) { - raw_inode->i_disk_version =cpu_to_le32 (inode->i_version); +u64 ivers =inode_peek_iversion (inode); + + raw_inode->i_disk_version =cpu_to_le32 (ivers);if (ei->i_extra_isize) {if (EXT4_FITS_IN_INODE (raw_inode, ei, i_version_hi)) raw_inode->i_version_hi = -cpu_to_le32 (inode->i_version >>32 ); +cpu_to_le32 (ivers >>32 ); raw_inode->i_extra_isize =cpu_to_le16 (ei->i_extra_isize); }
fs/ext4/ioctl.c +2 -1
@@ -19,6 +19,7 @@#include <linux/uuid.h> #include <linux/uaccess.h> #include <linux/delay.h> +#include <linux/iversion.h> #include "ext4_jbd2.h" #include "ext4.h" #include <linux/fsmap.h>
@@ -144,7 +145,7 @@ static long swap_inode_boot_loader(struct super_block *sb,i_gid_write (inode_bl,0 ); inode_bl->i_flags =0 ; ei_bl->i_flags =0 ; - inode_bl->i_version =1 ; +inode_set_iversion (inode_bl,1 );i_size_write (inode_bl,0 ); inode_bl->i_mode =S_IFREG ;if (ext4_has_feature_extents (sb)) {
fs/ext4/namei.c +3 -2
@@ -34,6 +34,7 @@#include <linux/quotaops.h> #include <linux/buffer_head.h> #include <linux/bio.h> +#include <linux/iversion.h> #include "ext4.h" #include "ext4_jbd2.h"
@@ -2955,7 +2956,7 @@ static int ext4_rmdir(struct inode *dir, struct dentry *dentry)"empty directory '%.*s' has too many links (%u)" , dentry->d_name.len, dentry->d_name.name, inode->i_nlink); - inode->i_version++; +inode_inc_iversion (inode);clear_nlink (inode);/* There's no need to set i_disksize: the fact that i_nlink is * zero will ensure that the right thing happens during any
@@ -3361,7 +3362,7 @@ static int ext4_setent(handle_t *handle, struct ext4_renament *ent, ent->de->inode =cpu_to_le32 (ino);if (ext4_has_feature_filetype (ent->dir->i_sb)) ent->de->file_type = file_type; - ent->dir->i_version++; +inode_inc_iversion (ent->dir); ent->dir->i_ctime = ent->dir->i_mtime =current_time (ent->dir);ext4_mark_inode_dirty (handle, ent->dir);
fs/ext4/super.c +2 -1
@@ -40,6 +40,7 @@#include <linux/dax.h> #include <linux/cleancache.h> #include <linux/uaccess.h> +#include <linux/iversion.h> #include <linux/kthread.h> #include <linux/freezer.h>
@@ -967,7 +968,7 @@ static struct inode *ext4_alloc_inode(struct super_block *sb)if (!ei)return NULL ; - ei->vfs_inode.i_version =1 ; +inode_set_iversion (&ei->vfs_inode,1 );spin_lock_init (&ei->i_raw_lock);INIT_LIST_HEAD (&ei->i_prealloc_list);spin_lock_init (&ei->i_prealloc_lock);
fs/ext4/xattr.c +3 -2
@@ -56,6 +56,7 @@#include <linux/slab.h> #include <linux/mbcache.h> #include <linux/quotaops.h> +#include <linux/iversion.h> #include "ext4_jbd2.h" #include "ext4.h" #include "xattr.h"
@@ -294,13 +295,13 @@ ext4_xattr_inode_hash(struct ext4_sb_info *sbi, const void *buffer, size_t size)static u64 ext4_xattr_inode_get_ref (struct inode *ea_inode) {return ((u64 )ea_inode->i_ctime.tv_sec <<32 ) | - ((u32 )ea_inode->i_version); + (u32 )inode_peek_iversion (ea_inode); }static void ext4_xattr_inode_set_ref (struct inode *ea_inode,u64 ref_count) { ea_inode->i_ctime.tv_sec = (u32)(ref_count >>32 ); - ea_inode->i_version = (u32 )ref_count; +inode_set_iversion (ea_inode, ref_count &0xffffffff ); }static u32 ext4_xattr_inode_get_hash (struct inode *ea_inode)
Signature · 1 line
2.14.3
Jeff Layton
maintainerPATCH v3 10/19 nfs: convert to new i_version API
In reply to Jeff LaytonFrom: Jeff Layton <[email protected]>
For NFS, we just use the "raw" API since the i_version is mostly managed by the server. The exception there is when the client holds a write delegation, but we only need to bump it once there anyway to handle CB_GETATTR.
--- fs/nfs/delegation.c | 3 ++- fs/nfs/fscache-index.c | 5 +++-- fs/nfs/inode.c | 18 +++++++++--------- fs/nfs/nfs4proc.c | 10 ++++++---- fs/nfs/nfstrace.h | 5 +++-- fs/nfs/write.c | 8 +++----- 6 files changed, 26 insertions(+), 23 deletions(-)
fs/nfs/delegation.c +2 -1
@@ -12,6 +12,7 @@#include <linux/sched.h> #include <linux/slab.h> #include <linux/spinlock.h> +#include <linux/iversion.h> #include <linux/nfs4.h> #include <linux/nfs_fs.h>
@@ -347,7 +348,7 @@ int nfs_inode_set_delegation(struct inode *inode, struct rpc_cred *cred, structnfs4_stateid_copy (&delegation->stateid, &res->delegation); delegation->type = res->delegation_type; delegation->pagemod_limit = res->pagemod_limit; - delegation->change_attr = inode->i_version; + delegation->change_attr =inode_peek_iversion_raw (inode); delegation->cred =get_rpccred (cred); delegation->inode = inode; delegation->flags =1 <<NFS_DELEGATION_REFERENCED ;
fs/nfs/fscache-index.c +3 -2
@@ -16,6 +16,7 @@#include <linux/nfs_fs.h> #include <linux/nfs_fs_sb.h> #include <linux/in6.h> +#include <linux/iversion.h> #include "internal.h" #include "fscache.h"
@@ -211,7 +212,7 @@ static uint16_t nfs_fscache_inode_get_aux(const void *cookie_netfs_data, auxdata.ctime = nfsi->vfs_inode.i_ctime;if (NFS_SERVER (&nfsi->vfs_inode)->nfs_client->rpc_ops->version ==4 ) - auxdata.change_attr = nfsi->vfs_inode.i_version; + auxdata.change_attr =inode_peek_iversion_raw (&nfsi->vfs_inode);if (bufmax >sizeof (auxdata)) bufmax =sizeof (auxdata);
@@ -243,7 +244,7 @@ enum fscache_checkaux nfs_fscache_inode_check_aux(void *cookie_netfs_data, auxdata.ctime = nfsi->vfs_inode.i_ctime;if (NFS_SERVER (&nfsi->vfs_inode)->nfs_client->rpc_ops->version ==4 ) - auxdata.change_attr = nfsi->vfs_inode.i_version; + auxdata.change_attr =inode_peek_iversion_raw (&nfsi->vfs_inode);if (memcmp (data, &auxdata, datalen) !=0 )return FSCACHE_CHECKAUX_OBSOLETE ;
fs/nfs/inode.c +9 -9
@@ -38,8 +38,8 @@#include <linux/slab.h> #include <linux/compat.h> #include <linux/freezer.h> -#include <linux/uaccess.h> +#include <linux/iversion.h> #include "nfs4_fs.h" #include "callback.h"
@@ -483,7 +483,7 @@ nfs_fhget(struct super_block *sb, struct nfs_fh *fh, struct nfs_fattr *fattr, stmemset (&inode->i_atime,0 ,sizeof (inode->i_atime));memset (&inode->i_mtime,0 ,sizeof (inode->i_mtime));memset (&inode->i_ctime,0 ,sizeof (inode->i_ctime)); - inode->i_version =0 ; +inode_set_iversion_raw (inode,0 ); inode->i_size =0 ;clear_nlink (inode); inode->i_uid =make_kuid (&init_user_ns,-2 );
@@ -508,7 +508,7 @@ nfs_fhget(struct super_block *sb, struct nfs_fh *fh, struct nfs_fattr *fattr, stelse if (nfs_server_capable (inode,NFS_CAP_CTIME ))nfs_set_cache_invalid (inode,NFS_INO_INVALID_ATTR );if (fattr->valid &NFS_ATTR_FATTR_CHANGE ) - inode->i_version = fattr->change_attr; +inode_set_iversion_raw (inode, fattr->change_attr);else nfs_set_cache_invalid (inode,NFS_INO_INVALID_ATTR |NFS_INO_REVAL_PAGECACHE );
@@ -1289,8 +1289,8 @@ static unsigned long nfs_wcc_update_inode(struct inode *inode, struct nfs_fattrif ((fattr->valid &NFS_ATTR_FATTR_PRECHANGE ) && (fattr->valid &NFS_ATTR_FATTR_CHANGE ) - && inode->i_version == fattr->pre_change_attr) { - inode->i_version = fattr->change_attr; + && !inode_cmp_iversion (inode, fattr->pre_change_attr)) { +inode_set_iversion_raw (inode, fattr->change_attr);if (S_ISDIR (inode->i_mode))nfs_set_cache_invalid (inode,NFS_INO_INVALID_DATA ); ret |=NFS_INO_INVALID_ATTR ;
@@ -1348,7 +1348,7 @@ static int nfs_check_inode_attributes(struct inode *inode, struct nfs_fattr *fatif (!nfs_file_has_buffered_writers (nfsi)) {/* Verify a few of the more important attributes */ -if ((fattr->valid &NFS_ATTR_FATTR_CHANGE ) !=0 && inode->i_version != fattr->change_attr) +if ((fattr->valid &NFS_ATTR_FATTR_CHANGE ) !=0 &&inode_cmp_iversion (inode, fattr->change_attr)) invalid |=NFS_INO_INVALID_ATTR |NFS_INO_REVAL_PAGECACHE ;if ((fattr->valid &NFS_ATTR_FATTR_MTIME ) && !timespec_equal (&inode->i_mtime, &fattr->mtime))
@@ -1642,7 +1642,7 @@ int nfs_post_op_update_inode_force_wcc_locked(struct inode *inode, struct nfs_fa }if ((fattr->valid &NFS_ATTR_FATTR_CHANGE ) !=0 && (fattr->valid &NFS_ATTR_FATTR_PRECHANGE ) ==0 ) { - fattr->pre_change_attr = inode->i_version; + fattr->pre_change_attr =inode_peek_iversion_raw (inode); fattr->valid |=NFS_ATTR_FATTR_PRECHANGE ; } if ((fattr->valid & NFS_ATTR_FATTR_CTIME) != 0 &&
@@ -1778,7 +1778,7 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)/* More cache consistency checks */ if (fattr->valid &NFS_ATTR_FATTR_CHANGE ) { -if (inode->i_version != fattr->change_attr) { +if (inode_cmp_iversion (inode, fattr->change_attr)) {dprintk ("NFS: change_attr change on server for file %s/%ld\n" , inode->i_sb->s_id, inode->i_ino);/* Could it be a race with writeback? */
@@ -1790,7 +1790,7 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)if (S_ISDIR (inode->i_mode))nfs_force_lookup_revalidate (inode); } - inode->i_version = fattr->change_attr; +inode_set_iversion_raw (inode, fattr->change_attr); } }else { nfsi->cache_validity |= save_cache_validity;
fs/nfs/nfs4proc.c +6 -4
@@ -54,6 +54,7 @@#include <linux/xattr.h> #include <linux/utsname.h> #include <linux/freezer.h> +#include <linux/iversion.h> #include "nfs4_fs.h" #include "delegation.h"
@@ -1045,16 +1046,16 @@ static void update_changeattr(struct inode *dir, struct nfs4_change_info *cinfo,spin_lock (&dir->i_lock); nfsi->cache_validity |=NFS_INO_INVALID_ATTR |NFS_INO_INVALID_DATA ; -if (cinfo->atomic && cinfo->before == dir->i_version) { +if (cinfo->atomic && cinfo->before ==inode_peek_iversion_raw (dir)) { nfsi->cache_validity &= ~NFS_INO_REVAL_PAGECACHE ; nfsi->attrtimeo_timestamp = jiffies; }else {nfs_force_lookup_revalidate (dir); -if (cinfo->before != dir->i_version) +if (cinfo->before !=inode_peek_iversion_raw (dir)) nfsi->cache_validity |=NFS_INO_INVALID_ACCESS |NFS_INO_INVALID_ACL ; } - dir->i_version = cinfo->after; +inode_set_iversion_raw (dir, cinfo->after); nfsi->read_cache_jiffies = timestamp; nfsi->attr_gencount =nfs_inc_attr_generation_counter ();nfs_fscache_invalidate (dir);
@@ -2454,7 +2455,8 @@ static int _nfs4_proc_open(struct nfs4_opendata *data) data->file_created = true; else if (o_res->cinfo.before != o_res->cinfo.after) data->file_created = true; -if (data->file_created || dir->i_version != o_res->cinfo.after) +if (data->file_created || +inode_peek_iversion_raw (dir) != o_res->cinfo.after)update_changeattr (dir, &o_res->cinfo, o_res->f_attr->time_start); }
fs/nfs/nfstrace.h +3 -2
@@ -9,6 +9,7 @@#define _TRACE_NFS_H#include <linux/tracepoint.h> +#include <linux/iversion.h> #define nfs_show_file_type (ftype) \ __print_symbolic(ftype, \
@@ -61,7 +62,7 @@ DECLARE_EVENT_CLASS(nfs_inode_event, __entry->dev = inode->i_sb->s_dev; __entry->fileid = nfsi->fileid; __entry->fhandle =nfs_fhandle_hash (&nfsi->fh); - __entry->version = inode->i_version; + __entry->version =inode_peek_iversion_raw (inode); ), TP_printk(
@@ -100,7 +101,7 @@ DECLARE_EVENT_CLASS(nfs_inode_event_done, __entry->fileid = nfsi->fileid; __entry->fhandle =nfs_fhandle_hash (&nfsi->fh); __entry->type =nfs_umode_to_dtype (inode->i_mode); - __entry->version = inode->i_version; + __entry->version =inode_peek_iversion_raw (inode); __entry->size =i_size_read (inode); __entry->nfsi_flags = nfsi->flags; __entry->cache_validity = nfsi->cache_validity;
fs/nfs/write.c +3 -5
@@ -23,6 +23,7 @@#include <linux/export.h> #include <linux/freezer.h> #include <linux/wait.h> +#include <linux/iversion.h> #include <linux/uaccess.h>
@@ -753,11 +754,8 @@ static void nfs_inode_add_request(struct inode *inode, struct nfs_page *req)*/ spin_lock (&mapping->private_lock);if (!nfs_have_writebacks (inode) && -NFS_PROTO (inode)->have_delegation (inode,FMODE_WRITE )) { -spin_lock (&inode->i_lock); - inode->i_version++; -spin_unlock (&inode->i_lock); - } +NFS_PROTO (inode)->have_delegation (inode,FMODE_WRITE )) +inode_inc_iversion (inode);if (likely (!PageSwapCache (req->wb_page))) {set_bit (PG_MAPPED , &req->wb_flags);SetPagePrivate (req->wb_page);
Signature · 1 line
2.14.3
Jeff Layton
maintainerPATCH v3 11/19 nfsd: convert to new i_version API
In reply to Jeff LaytonFrom: Jeff Layton <[email protected]>
Mostly just making sure we use the "get" wrappers so we know when it is being fetched for later use.
--- fs/nfsd/nfsfh.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
fs/nfsd/nfsfh.h +2 -1
@@ -11,6 +11,7 @@#include <linux/crc32.h> #include <linux/sunrpc/svc.h> #include <uapi/linux/nfsd/nfsfh.h> +#include <linux/iversion.h> static inline __u32 ino_t_to_u32 (ino_t ino) {
@@ -259,7 +260,7 @@ static inline u64 nfsd4_change_attribute(struct inode *inode) chattr = inode->i_ctime.tv_sec; chattr <<=30 ; chattr += inode->i_ctime.tv_nsec; - chattr += inode->i_version; + chattr +=inode_query_iversion (inode);return chattr; }
Signature · 1 line
2.14.3
Jeff Layton
maintainerPATCH v3 12/19 ocfs2: convert to new i_version API
In reply to Jeff LaytonFrom: Jeff Layton <[email protected]>
--- fs/ocfs2/dir.c | 15 ++++++++------- fs/ocfs2/inode.c | 3 ++- fs/ocfs2/namei.c | 3 ++- fs/ocfs2/quota_global.c | 3 ++- 4 files changed, 14 insertions(+), 10 deletions(-)
fs/ocfs2/dir.c +8 -7
@@ -42,6 +42,7 @@#include <linux/highmem.h> #include <linux/quotaops.h> #include <linux/sort.h> +#include <linux/iversion.h> #include <cluster/masklog.h>
@@ -1174,7 +1175,7 @@ static int __ocfs2_delete_entry(handle_t *handle, struct inode *dir,le16_add_cpu (&pde->rec_len,le16_to_cpu (de->rec_len)); de->inode =0 ; - dir->i_version++; +inode_inc_iversion (dir);ocfs2_journal_dirty (handle, bh); goto bail; }
@@ -1729,7 +1730,7 @@ int __ocfs2_add_entry(handle_t *handle,if (ocfs2_dir_indexed (dir))ocfs2_recalc_free_list (dir, handle, lookup); - dir->i_version++; +inode_inc_iversion (dir);ocfs2_journal_dirty (handle, insert_bh); retval =0 ; goto bail;
@@ -1775,7 +1776,7 @@ static int ocfs2_dir_foreach_blk_id(struct inode *inode,* readdir(2), then we might be pointing to an invalid * dirent right now. Scan from the start of the block * to make sure. */ -if (*f_version != inode->i_version) { +if (inode_cmp_iversion (inode, *f_version)) {for (i =0 ; i <i_size_read (inode) && i < offset; ) { de = (struct ocfs2_dir_entry *) (data->id_data + i);
@@ -1791,7 +1792,7 @@ static int ocfs2_dir_foreach_blk_id(struct inode *inode, i +=le16_to_cpu (de->rec_len); } ctx->pos = offset = i; - *f_version = inode->i_version; + *f_version =inode_query_iversion (inode); } de = (struct ocfs2_dir_entry *) (data->id_data + ctx->pos);
@@ -1869,7 +1870,7 @@ static int ocfs2_dir_foreach_blk_el(struct inode *inode,* readdir(2), then we might be pointing to an invalid * dirent right now. Scan from the start of the block * to make sure. */ -if (*f_version != inode->i_version) { +if (inode_cmp_iversion (inode, *f_version)) {for (i =0 ; i < sb->s_blocksize && i < offset; ) { de = (struct ocfs2_dir_entry *) (bh->b_data + i);/* It's too expensive to do a full
@@ -1886,7 +1887,7 @@ static int ocfs2_dir_foreach_blk_el(struct inode *inode, offset = i; ctx->pos = (ctx->pos & ~(sb->s_blocksize -1 )) | offset; - *f_version = inode->i_version; + *f_version =inode_query_iversion (inode); } while (ctx->pos < i_size_read(inode)
@@ -1940,7 +1941,7 @@ static int ocfs2_dir_foreach_blk(struct inode *inode, u64 *f_version,*/ int ocfs2_dir_foreach (struct inode *inode,struct dir_context *ctx) { -u64 version = inode->i_version; +u64 version =inode_query_iversion (inode);ocfs2_dir_foreach_blk (inode, &version, ctx, true);return 0 ; }
fs/ocfs2/inode.c +2 -1
@@ -28,6 +28,7 @@#include <linux/highmem.h> #include <linux/pagemap.h> #include <linux/quotaops.h> +#include <linux/iversion.h> #include <asm/byteorder.h>
@@ -302,7 +303,7 @@ void ocfs2_populate_inode(struct inode *inode, struct ocfs2_dinode *fe,OCFS2_I (inode)->ip_attr =le32_to_cpu (fe->i_attr);OCFS2_I (inode)->ip_dyn_features =le16_to_cpu (fe->i_dyn_features); - inode->i_version =1 ; +inode_set_iversion (inode,1 ); inode->i_generation =le32_to_cpu (fe->i_generation); inode->i_rdev =huge_decode_dev (le64_to_cpu (fe->id1.dev1.i_rdev)); inode->i_mode =le16_to_cpu (fe->i_mode);
fs/ocfs2/namei.c +2 -1
@@ -41,6 +41,7 @@#include <linux/slab.h> #include <linux/highmem.h> #include <linux/quotaops.h> +#include <linux/iversion.h> #include <cluster/masklog.h>
@@ -1520,7 +1521,7 @@ static int ocfs2_rename(struct inode *old_dir,mlog_errno (status); goto bail; } - new_dir->i_version++; +inode_inc_iversion (new_dir);if (S_ISDIR (new_inode->i_mode))ocfs2_set_links_count (newfe,0 );
fs/ocfs2/quota_global.c +2 -1
@@ -12,6 +12,7 @@#include <linux/writeback.h> #include <linux/workqueue.h> #include <linux/llist.h> +#include <linux/iversion.h> #include <cluster/masklog.h>
@@ -289,7 +290,7 @@ ssize_t ocfs2_quota_write(struct super_block *sb, int type,mlog_errno (err);return err; } - gqinode->i_version++; +inode_query_iversion (gqinode);ocfs2_mark_inode_dirty (handle, gqinode, oinfo->dqi_gqi_bh);return len; }
Signature · 6 lines
2.14.3 -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Jeff Layton
maintainerPATCH v3 13/19 ufs: use new i_version API
In reply to Jeff LaytonFrom: Jeff Layton <[email protected]>
--- fs/ufs/dir.c | 9 +++++---- fs/ufs/inode.c | 3 ++- fs/ufs/super.c | 3 ++- 3 files changed, 9 insertions(+), 6 deletions(-)
fs/ufs/dir.c +5 -4
@@ -20,6 +20,7 @@#include <linux/time.h> #include <linux/fs.h> #include <linux/swap.h> +#include <linux/iversion.h> #include "ufs_fs.h" #include "ufs.h"
@@ -47,7 +48,7 @@ static int ufs_commit_chunk(struct page *page, loff_t pos, unsigned len)struct inode *dir = mapping->host;int err =0 ; - dir->i_version++; +inode_inc_iversion (dir);block_write_end (NULL , mapping, pos, len, len, page,NULL );if (pos+len > dir->i_size) {i_size_write (dir, pos+len);
@@ -428,7 +429,7 @@ ufs_readdir(struct file *file, struct dir_context *ctx)unsigned long n = pos >>PAGE_SHIFT ;unsigned long npages =dir_pages (inode);unsigned chunk_mask = ~(UFS_SB (sb)->s_uspi->s_dirblksize -1 ); -int need_revalidate = file->f_version != inode->i_version; +bool need_revalidate =inode_cmp_iversion (inode, file->f_version);unsigned flags =UFS_SB (sb)->s_flags;UFSD ("BEGIN\n" );
@@ -455,8 +456,8 @@ ufs_readdir(struct file *file, struct dir_context *ctx) offset =ufs_validate_entry (sb, kaddr, offset, chunk_mask); ctx->pos = (n<<PAGE_SHIFT ) + offset; } - file->f_version = inode->i_version; - need_revalidate =0 ; + file->f_version =inode_query_iversion (inode); + need_revalidate = false; } de = (struct ufs_dir_entry *)(kaddr+offset); limit = kaddr +ufs_last_byte (inode, n) -UFS_DIR_REC_LEN (1 );
fs/ufs/inode.c +2 -1
@@ -36,6 +36,7 @@#include <linux/mm.h> #include <linux/buffer_head.h> #include <linux/writeback.h> +#include <linux/iversion.h> #include "ufs_fs.h" #include "ufs.h"
@@ -693,7 +694,7 @@ struct inode *ufs_iget(struct super_block *sb, unsigned long ino)if (err) goto bad_inode; - inode->i_version++; +inode_inc_iversion (inode); ufsi->i_lastfrag = (inode->i_size + uspi->s_fsize -1 ) >> uspi->s_fshift; ufsi->i_dir_start_lookup =0 ;
fs/ufs/super.c +2 -1
@@ -88,6 +88,7 @@#include <linux/log2.h> #include <linux/mount.h> #include <linux/seq_file.h> +#include <linux/iversion.h> #include "ufs_fs.h" #include "ufs.h"
@@ -1440,7 +1441,7 @@ static struct inode *ufs_alloc_inode(struct super_block *sb)if (!ei)return NULL ; - ei->vfs_inode.i_version =1 ; +inode_set_iversion (&ei->vfs_inode,1 );seqlock_init (&ei->meta_lock);mutex_init (&ei->truncate_mutex);return &ei->vfs_inode;
Signature · 1 line
2.14.3
Jeff Layton
maintainerPATCH v3 14/19 xfs: convert to new i_version API
In reply to Jeff LaytonFrom: Jeff Layton <[email protected]>
--- fs/xfs/libxfs/xfs_inode_buf.c | 7 +++++-- fs/xfs/xfs_icache.c | 5 +++-- fs/xfs/xfs_inode.c | 3 ++- fs/xfs/xfs_inode_item.c | 3 ++- fs/xfs/xfs_trans_inode.c | 4 +++- 5 files changed, 15 insertions(+), 7 deletions(-)
fs/xfs/libxfs/xfs_inode_buf.c +5 -2
@@ -32,6 +32,8 @@#include "xfs_ialloc.h" #include "xfs_dir2.h" +#include <linux/iversion.h> +/* * Check that none of the inode's in the buffer have a next * unlinked field of 0.
@@ -264,7 +266,8 @@ xfs_inode_from_disk( to->di_flags =be16_to_cpu (from->di_flags);if (to->di_version ==3 ) { - inode->i_version =be64_to_cpu (from->di_changecount); +inode_set_iversion_queried (inode, +be64_to_cpu (from->di_changecount)); to->di_crtime.t_sec =be32_to_cpu (from->di_crtime.t_sec); to->di_crtime.t_nsec =be32_to_cpu (from->di_crtime.t_nsec); to->di_flags2 =be64_to_cpu (from->di_flags2);
@@ -314,7 +317,7 @@ xfs_inode_to_disk( to->di_flags =cpu_to_be16 (from->di_flags);if (from->di_version ==3 ) { - to->di_changecount =cpu_to_be64 (inode->i_version); + to->di_changecount =cpu_to_be64 (inode_peek_iversion (inode)); to->di_crtime.t_sec =cpu_to_be32 (from->di_crtime.t_sec); to->di_crtime.t_nsec =cpu_to_be32 (from->di_crtime.t_nsec); to->di_flags2 =cpu_to_be64 (from->di_flags2);
fs/xfs/xfs_icache.c +3 -2
@@ -37,6 +37,7 @@#include <linux/kthread.h> #include <linux/freezer.h> +#include <linux/iversion.h> /* * Allocate and initialise an xfs_inode.
@@ -293,14 +294,14 @@ xfs_reinit_inode(int error;uint32_t nlink = inode->i_nlink;uint32_t generation = inode->i_generation; -uint64_t version = inode->i_version; +uint64_t version =inode_peek_iversion (inode);umode_t mode = inode->i_mode; error =inode_init_always (mp->m_super, inode);set_nlink (inode, nlink); inode->i_generation = generation; - inode->i_version = version; +inode_set_iversion_queried (inode, version); inode->i_mode = mode;return error; }
fs/xfs/xfs_inode.c +2 -1
@@ -16,6 +16,7 @@* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA */ #include <linux/log2.h> +#include <linux/iversion.h> #include "xfs.h" #include "xfs_fs.h"
@@ -833,7 +834,7 @@ xfs_ialloc( ip->i_d.di_flags =0 ;if (ip->i_d.di_version ==3 ) { - inode->i_version =1 ; +inode_set_iversion (inode,1 ); ip->i_d.di_flags2 =0 ; ip->i_d.di_cowextsize =0 ; ip->i_d.di_crtime.t_sec = (int32_t )tv.tv_sec;
fs/xfs/xfs_inode_item.c +2 -1
@@ -30,6 +30,7 @@#include "xfs_buf_item.h" #include "xfs_log.h" +#include <linux/iversion.h> kmem_zone_t *xfs_ili_zone;/* inode log item zone */
@@ -354,7 +355,7 @@ xfs_inode_to_log_dinode( to->di_next_unlinked =NULLAGINO ;if (from->di_version ==3 ) { - to->di_changecount = inode->i_version; + to->di_changecount =inode_peek_iversion (inode); to->di_crtime.t_sec = from->di_crtime.t_sec; to->di_crtime.t_nsec = from->di_crtime.t_nsec; to->di_flags2 = from->di_flags2;
fs/xfs/xfs_trans_inode.c +3 -1
@@ -28,6 +28,8 @@#include "xfs_inode_item.h" #include "xfs_trace.h" +#include <linux/iversion.h> +/* * Add a locked inode to the transaction. *
@@ -117,7 +119,7 @@ xfs_trans_log_inode(*/ if (!(ip->i_itemp->ili_item.li_desc->lid_flags &XFS_LID_DIRTY ) &&IS_I_VERSION (VFS_I (ip))) { -VFS_I (ip)->i_version++; +inode_inc_iversion (VFS_I (ip)); flags |=XFS_ILOG_CORE ; }
Signature · 1 line
2.14.3
Jeff Layton
maintainerPATCH v3 15/19 IMA: switch IMA over to new i_version API
In reply to Jeff LaytonFrom: Jeff Layton <[email protected]>
--- security/integrity/ima/ima_api.c | 3 ++- security/integrity/ima/ima_main.c | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-)
security/integrity/ima/ima_api.c +2 -1
@@ -18,6 +18,7 @@#include <linux/fs.h> #include <linux/xattr.h> #include <linux/evm.h> +#include <linux/iversion.h> #include "ima.h"
@@ -215,7 +216,7 @@ int ima_collect_measurement(struct integrity_iint_cache *iint,* which do not support i_version, support is limited to an initial * measurement/appraisal/audit. */ - i_version =file_inode (file)->i_version; + i_version =inode_query_iversion (inode); hash.hdr.algo = algo;/* Initialize hash digest to 0's in case of failure */
security/integrity/ima/ima_main.c +2 -1
@@ -24,6 +24,7 @@#include <linux/slab.h> #include <linux/xattr.h> #include <linux/ima.h> +#include <linux/iversion.h> #include "ima.h"
@@ -128,7 +129,7 @@ static void ima_check_last_writer(struct integrity_iint_cache *iint,inode_lock (inode);if (atomic_read (&inode->i_writecount) ==1 ) {if (!IS_I_VERSION (inode) || - (iint->version != inode->i_version) || +inode_cmp_iversion (inode, iint->version) || (iint->flags &IMA_NEW_FILE )) { iint->flags &= ~(IMA_DONE_MASK |IMA_NEW_FILE ); iint->measured_pcrs =0 ;
Signature · 6 lines
2.14.3 -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Jeff Layton
maintainerPATCH v3 16/19 fs: only set S_VERSION when updating times if necessary
In reply to Jeff LaytonFrom: Jeff Layton <[email protected]>
We only really need to update i_version if someone has queried for it since we last incremented it. By doing that, we can avoid having to update the inode if the times haven't changed.
If the times have changed, then we go ahead and forcibly increment the counter, under the assumption that we'll be going to the storage anyway, and the increment itself is relatively cheap.
--- fs/inode.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-)
fs/inode.c +14 -6
@@ -19,6 +19,7 @@#include <linux/ratelimit.h> #include <linux/list_lru.h> #include <trace/events/writeback.h> +#include <linux/iversion.h> #include "internal.h" /*
@@ -1634,17 +1635,24 @@ static int relatime_need_update(const struct path *path, struct inode *inode,int generic_update_time (struct inode *inode,struct timespec *time,int flags) {int iflags =I_DIRTY_TIME ; +bool dirty = false; -if (flags &S_ATIME ) +if (flags &S_ATIME ) { inode->i_atime = *time; + dirty |= !(inode->i_sb->s_flags &SB_LAZYTIME ); + }if (flags &S_VERSION ) -inode_inc_iversion (inode); -if (flags &S_CTIME ) + dirty |=inode_maybe_inc_iversion (inode, dirty); +if (flags &S_CTIME ) { inode->i_ctime = *time; -if (flags &S_MTIME ) + dirty = true; + } +if (flags &S_MTIME ) { inode->i_mtime = *time; + dirty = true; + } -if (!(inode->i_sb->s_flags &SB_LAZYTIME ) || (flags &S_VERSION )) +if (dirty) iflags |=I_DIRTY_SYNC ;__mark_inode_dirty (inode, iflags);return 0 ;
@@ -1863,7 +1871,7 @@ int file_update_time(struct file *file)if (!timespec_equal (&inode->i_ctime, &now)) sync_it |=S_CTIME ; -if (IS_I_VERSION (inode)) +if (IS_I_VERSION (inode) &&inode_iversion_need_inc (inode)) sync_it |=S_VERSION ;if (!sync_it)
Signature · 1 line
2.14.3
Jan Kara
reviewerRe: [PATCH v3 16/19] fs: only set S_VERSION when updating times if necessary
Quoted 41 lines · Jeff Layton
From: Jeff Layton <[email protected]>
We only really need to update
i_versionif someone has queried for it since we last incremented it. By doing that, we can avoid having to update the inode if the times haven't changed.If the times have changed, then we go ahead and forcibly increment the counter, under the assumption that we'll be going to the storage anyway, and the increment itself is relatively cheap.
Signed-off-by Jeff Layton <[email protected]>--- fs/inode.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-)fs/inode.c +5
-3@@ -19,6 +19,7 @@#include <linux/ratelimit.h> #include <linux/list_lru.h> #include <trace/events/writeback.h> +#include <linux/iversion.h> #include "internal.h" /* @@ -1634,17 +1635,24 @@ static int relatime_need_update(const struct path *path, struct inode *inode,int generic_update_time (struct inode *inode,struct timespec *time,int flags) {int iflags =I_DIRTY_TIME ; +bool dirty = false; -if (flags &S_ATIME ) +if (flags &S_ATIME ) { inode->i_atime = *time; + dirty |= !(inode->i_sb->s_flags &SB_LAZYTIME ); + }if (flags &S_VERSION ) -inode_inc_iversion (inode); -if (flags &S_CTIME )
fs/inode.c static int relatime_need_update(const struct path *path, struct inode *inode, · line 1653
Quoted 10 lines
fs/inode.c +7
-1+ dirty |=inode_maybe_inc_iversion (inode, dirty); +if (flags &S_CTIME ) { inode->i_ctime = *time; -if (flags &S_MTIME ) + dirty = true; + } +if (flags &S_MTIME ) { inode->i_mtime = *time; + dirty = true; + }
The SB_LAZYTIME handling is wrong here. That option is not only about atime handling but rather about all inode time stamps. So you rather need something like:
if (flags & (S_ATIME | S_CTIME | S_MTIME) && !(inode->i_sb->s_flags & SB_LAZYTIME)) dirty = true; Honza
Quoted 18 lines
fs/inode.c +2
-2-if (!(inode->i_sb->s_flags &SB_LAZYTIME ) || (flags &S_VERSION )) +if (dirty) iflags |=I_DIRTY_SYNC ;__mark_inode_dirty (inode, iflags);return 0 ;@@ -1863,7 +1871,7 @@ int file_update_time(struct file *file)if (!timespec_equal (&inode->i_ctime, &now)) sync_it |=S_CTIME ; -if (IS_I_VERSION (inode)) +if (IS_I_VERSION (inode) &&inode_iversion_need_inc (inode)) sync_it |=S_VERSION ;if (!sync_it)Signature · 1 line
2.14.3
Signature · 6 lines
Jan Kara <jack-IBi9RG/[email protected]> SUSE Labs, CR -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Jeff Layton
maintainerRe: [PATCH v3 16/19] fs: only set S_VERSION when updating times if necessary
Quoted 57 lines · Jan Kara
Quoted 41 lines · Jeff Layton
From: Jeff Layton <[email protected]>
We only really need to update
i_versionif someone has queried for it since we last incremented it. By doing that, we can avoid having to update the inode if the times haven't changed.If the times have changed, then we go ahead and forcibly increment the counter, under the assumption that we'll be going to the storage anyway, and the increment itself is relatively cheap.
Signed-off-by Jeff Layton <[email protected]>--- fs/inode.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-)fs/inode.c +5
-3@@ -19,6 +19,7 @@#include <linux/ratelimit.h> #include <linux/list_lru.h> #include <trace/events/writeback.h> +#include <linux/iversion.h> #include "internal.h" /* @@ -1634,17 +1635,24 @@ static int relatime_need_update(const struct path *path, struct inode *inode,int generic_update_time (struct inode *inode,struct timespec *time,int flags) {int iflags =I_DIRTY_TIME ; +bool dirty = false; -if (flags &S_ATIME ) +if (flags &S_ATIME ) { inode->i_atime = *time; + dirty |= !(inode->i_sb->s_flags &SB_LAZYTIME ); + }if (flags &S_VERSION ) -inode_inc_iversion (inode); -if (flags &S_CTIME )fs/inode.c static int relatime_need_update(const struct path *path, struct inode *inode, · line 1653
Quoted 10 lines
fs/inode.c +7
-1+ dirty |=inode_maybe_inc_iversion (inode, dirty); +if (flags &S_CTIME ) { inode->i_ctime = *time; -if (flags &S_MTIME ) + dirty = true; + } +if (flags &S_MTIME ) { inode->i_mtime = *time; + dirty = true; + }The
SB_LAZYTIMEhandling is wrong here. That option is not only about atime handling but rather about all inode time stamps. So you rather need something like:
Quoted 6 lines
if (flags & (S_ATIME | S_CTIME | S_MTIME) && !(inode->i_sb->s_flags & SB_LAZYTIME)) dirty = true; Honza
Thanks. I misunderstood that. Will fix.
Quoted 18 lines
Quoted 18 lines
fs/inode.c +2
-2-if (!(inode->i_sb->s_flags &SB_LAZYTIME ) || (flags &S_VERSION )) +if (dirty) iflags |=I_DIRTY_SYNC ;__mark_inode_dirty (inode, iflags);return 0 ;@@ -1863,7 +1871,7 @@ int file_update_time(struct file *file)if (!timespec_equal (&inode->i_ctime, &now)) sync_it |=S_CTIME ; -if (IS_I_VERSION (inode)) +if (IS_I_VERSION (inode) &&inode_iversion_need_inc (inode)) sync_it |=S_VERSION ;if (!sync_it)Signature · 1 line
2.14.3
Signature · 1 line
Jeff Layton <[email protected]>
Jeff Layton
maintainerPATCH v3 17/19 xfs: avoid setting XFS_ILOG_CORE if i_version doesn't need incrementing
In reply to Jeff LaytonFrom: Jeff Layton <[email protected]>
If XFS_ILOG_CORE is already set then go ahead and increment it.
--- fs/xfs/xfs_trans_inode.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)
fs/xfs/xfs_trans_inode.c +8 -6
@@ -112,15 +112,17 @@ xfs_trans_log_inode(/* * First time we log the inode in a transaction, bump the inode change -* counter if it is configured for this to occur. We don't use -* inode_inc_version() because there is no need for extra locking around -* i_version as we already hold the inode locked exclusively for -* metadata modification. +* counter if it is configured for this to occur. While we have the +* inode locked exclusively for metadata modification, we can usually +* avoid setting XFS_ILOG_CORE if no one has queried the value since +* the last time it was incremented. If we have XFS_ILOG_CORE already +* set however, then go ahead and bump the i_version counter +* unconditionally. */ if (!(ip->i_itemp->ili_item.li_desc->lid_flags &XFS_LID_DIRTY ) &&IS_I_VERSION (VFS_I (ip))) { -inode_inc_iversion (VFS_I (ip)); - flags |=XFS_ILOG_CORE ; +if (inode_maybe_inc_iversion (VFS_I (ip), flags &XFS_ILOG_CORE )) + flags |=XFS_ILOG_CORE ; } tp->t_flags |=XFS_TRANS_DIRTY ;
Signature · 1 line
2.14.3
Jeff Layton
maintainerPATCH v3 18/19 btrfs: only dirty the inode in btrfs_update_time if something was changed
In reply to Jeff LaytonFrom: Jeff Layton <[email protected]>
At this point, we know that "now" and the file times may differ, and we suspect that the i_version has been flagged to be bumped. Attempt to bump the i_version, and only mark the inode dirty if that actually occurred or if one of the times was updated.
--- fs/btrfs/inode.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
fs/btrfs/inode.c +3 -2
@@ -6107,19 +6107,20 @@ static int btrfs_update_time(struct inode *inode, struct timespec *now, int flags) {struct btrfs_root *root =BTRFS_I (inode)->root; +bool dirty = flags & ~S_VERSION ;if (btrfs_root_readonly (root))return -EROFS ;if (flags &S_VERSION ) -inode_inc_iversion (inode); + dirty |=inode_maybe_inc_iversion (inode, dirty);if (flags &S_CTIME ) inode->i_ctime = *now;if (flags &S_MTIME ) inode->i_mtime = *now;if (flags &S_ATIME ) inode->i_atime = *now; -return btrfs_dirty_inode (inode); +return dirty ?btrfs_dirty_inode (inode) :0 ; }/*
Signature · 1 line
2.14.3
Jeff Layton
maintainerPATCH v3 19/19 fs: handle inode->i_version more efficiently
In reply to Jeff LaytonFrom: Jeff Layton <[email protected]>
Since i_version is mostly treated as an opaque value, we can exploit that fact to avoid incrementing it when no one is watching. With that change, we can avoid incrementing the counter on writes, unless someone has queried for it since it was last incremented. If the a/c/mtime don't change, and the i_version hasn't changed, then there's no need to dirty the inode metadata on a write.
Convert the i_version counter to an atomic64_t, and use the lowest order bit to hold a flag that will tell whether anyone has queried the value since it was last incremented.
When we go to maybe increment it, we fetch the value and check the flag bit. If it's clear then we don't need to do anything if the update isn't being forced.
If we do need to update, then we increment the counter by 2, and clear the flag bit, and then use a CAS op to swap it into place. If that works, we return true. If it doesn't then do it again with the value that we fetch from the CAS operation.
On the query side, if the flag is already set, then we just shift the value down by 1 bit and return it. Otherwise, we set the flag in our on-stack value and again use cmpxchg to swap it into place if it hasn't changed. If it has, then we use the value from the cmpxchg as the new "old" value and try again.
This method allows us to avoid incrementing the counter on writes (and dirtying the metadata) under typical workloads. We only need to increment if it has been queried since it was last changed.
--- include/linux/fs.h | 2 +- include/linux/iversion.h | 180 ++++++++++++++++++++++++++++++++++------------- 2 files changed, 131 insertions(+), 51 deletions(-)
include/linux/fs.h +1 -1
@@ -639,7 +639,7 @@ struct inode {struct hlist_head i_dentry;struct rcu_head i_rcu; }; -u64 i_version; +atomic64_t i_version;atomic_t i_count;atomic_t i_dio_count;atomic_t i_writecount;
include/linux/iversion.h +130 -50
280 lines not on this page · raw message
Signature · 1 line
2.14.3
Jan Kara
reviewerRe: [PATCH v3 19/19] fs: handle inode->i_version more efficiently
include/linux/iversion.h line 159
Quoted 12 lines · Jeff Layton
static inline bool
inode_maybe_inc_iversion(struct inode *inode, bool force){include/linux/iversion.h +6
-2-atomic64_t *ivp = (atomic64_t *)&inode->i_version; +u64 cur, old, new; -atomic64_inc (ivp); + cur = (u64 )atomic64_read (&inode->i_version); +for (;;) { +/* If flag is clear then we needn't do anything */ +if (!force && !(cur &I_VERSION_QUERIED )) +return false;
The fast path here misses any memory barrier. Thus it seems this query could be in theory reordered before any store that happened to modify the inode? Or maybe we could race and miss the fact that in fact this i_version has already been queried? But maybe there's some higher level locking that makes sure this is all a non-issue... But in that case it would deserve some comment I guess.
Quoted 12 lines
include/linux/iversion.h +9
-0+ +/* Since lowest bit is flag, add 2 to avoid it */ + new = (cur & ~I_VERSION_QUERIED ) +I_VERSION_INCREMENT ; + + old =atomic64_cmpxchg (&inode->i_version, cur, new); +if (likely (old == cur)) +break ; + cur = old; + }return true; }
...
Quoted 8 lines
static inline u64
inode_query_iversion(struct inode *inode)
{include/linux/iversion.h +4
-1-return inode_peek_iversion (inode); +u64 cur, old, new; + + cur =atomic64_read (&inode->i_version); +for (;;) {
include/linux/iversion.h inode_peek_iversion(const struct inode *inode) · line 262
Quoted 10 lines
include/linux/iversion.h +10
-0+/* If flag is already set, then no need to swap */ +if (cur &I_VERSION_QUERIED ) +break ; + + new = cur |I_VERSION_QUERIED ; + old =atomic64_cmpxchg (&inode->i_version, cur, new); +if (old == cur) +break ; + cur = old; + }
Why not just use atomic64_or() here?
Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Jeff Layton
maintainerRe: [PATCH v3 19/19] fs: handle inode->i_version more efficiently
Quoted 7 lines · Jan Kara
Quoted 6 lines · Jeff Layton
static inline bool
inode_maybe_inc_iversion(struct inode *inode, bool force){include/linux/iversion.h +1
-1-atomic64_t *ivp = (atomic64_t *)&inode->i_version; +u64 cur, old, new;
Quoted 14 lines
include/linux/iversion.h line 159
Quoted 6 lines
include/linux/iversion.h +5
-1-atomic64_inc (ivp); + cur = (u64 )atomic64_read (&inode->i_version); +for (;;) { +/* If flag is clear then we needn't do anything */ +if (!force && !(cur &I_VERSION_QUERIED )) +return false;The fast path here misses any memory barrier. Thus it seems this query could be in theory reordered before any store that happened to modify the inode? Or maybe we could race and miss the fact that in fact this
i_versionhas already been queried? But maybe there's some higher level locking that makes sure this is all a non-issue... But in that case it would deserve some comment I guess.
There's no higher-level locking. Getting locking out of this codepath is a good thing IMO. The larger question here is whether we really care about ordering this with anything else.
The i_version, as implemented today, is not ordered with actual changes to the inode. We only take the i_lock today when modifying it, not when querying it. It's possible today that you could see the results of a change and then do a fetch of the i_version that doesn't show an increment vs. a previous change.
It'd be nice if this were atomic with the actual changes that it represents, but I think that would be prohibitively expensive. That may be something we need to address. I'm not sure we really want to do it as part of this patchset though.
Quoted 27 lines
Quoted 12 lines
include/linux/iversion.h +9
-0+ +/* Since lowest bit is flag, add 2 to avoid it */ + new = (cur & ~I_VERSION_QUERIED ) +I_VERSION_INCREMENT ; + + old =atomic64_cmpxchg (&inode->i_version, cur, new); +if (likely (old == cur)) +break ; + cur = old; + }return true; }...
Quoted 12 lines
static inline u64
inode_query_iversion(struct inode *inode)
{include/linux/iversion.h +8
-1-return inode_peek_iversion (inode); +u64 cur, old, new; + + cur =atomic64_read (&inode->i_version); +for (;;) { +/* If flag is already set, then no need to swap */ +if (cur &I_VERSION_QUERIED ) +break ; +
Quoted 9 lines
include/linux/iversion.h inode_peek_iversion(const struct inode *inode) · line 262
Quoted 6 lines
include/linux/iversion.h +6
-0+ new = cur |I_VERSION_QUERIED ; + old =atomic64_cmpxchg (&inode->i_version, cur, new); +if (old == cur) +break ; + cur = old; + }Why not just use
atomic64_or()here?
If the cmpxchg fails, then either:
1) it was incremented
2) someone flagged it QUERIED
If an increment happened then we don't need to flag it as QUERIED if we're returning an older value. If we use atomic64_or, then we can't tell if an increment happened so we'd end up potentially flagging it more than necessary.
In principle, either outcome is technically OK and we don't have to loop if the cmpxchg doesn't work. That said, if we think there might be a later i_version available, then I think we probably want to try to query it again so we can return as late a one as possible.
Signature · 1 line
Jeff Layton <[email protected]>
Re: [PATCH v3 19/19] fs: handle inode->i_version more efficiently
Quoted 33 lines · Jeff Layton
Quoted 7 lines · Jan Kara
Quoted 6 lines · Jeff Layton
static inline bool
inode_maybe_inc_iversion(struct inode *inode, bool force){include/linux/iversion.h +1
-1-atomic64_t *ivp = (atomic64_t *)&inode->i_version; +u64 cur, old, new;Quoted 14 lines
include/linux/iversion.h line 159
Quoted 6 lines
include/linux/iversion.h +5
-1-atomic64_inc (ivp); + cur = (u64 )atomic64_read (&inode->i_version); +for (;;) { +/* If flag is clear then we needn't do anything */ +if (!force && !(cur &I_VERSION_QUERIED )) +return false;The fast path here misses any memory barrier. Thus it seems this query could be in theory reordered before any store that happened to modify the inode? Or maybe we could race and miss the fact that in fact this
i_versionhas already been queried? But maybe there's some higher level locking that makes sure this is all a non-issue... But in that case it would deserve some comment I guess.There's no higher-level locking. Getting locking out of this codepath is a good thing IMO. The larger question here is whether we really care about ordering this with anything else.
The
i_version, as implemented today, is not ordered with actual changes to the inode. We only take thei_locktoday when modifying it, not when querying it. It's possible today that you could see the results of a change and then do a fetch of thei_versionthat doesn't show an increment vs. a previous change.
Quoted 4 lines
It'd be nice if this were atomic with the actual changes that it represents, but I think that would be prohibitively expensive. That may be something we need to address. I'm not sure we really want to do it as part of this patchset though.
I don't think we need full atomicity, but we *do* need the i_version increment to be seen as ordered after the inode change. That should be relatively inexpensive, yes?
Otherwise an inode change could be overlooked indefinitely, because a client could cache the old contents with the new i_version value and never see the i_version change again as long as the inode isn't touched again.
(If the client instead caches new data with the old inode version, there may be an unnecessary cache invalidation next time the client queries the change attribute. That's a performance bug, but not really a correctness problem, at least for clients depending only on close-to-open semantics: they'll only depend on seeing the new change attribute after the change has been flushed to disk. The performance problem should be mitigated by the race being very rare.)
--b.
Jeff Layton
maintainerRe: [PATCH v3 19/19] fs: handle inode->i_version more efficiently
Quoted 48 lines · J. Bruce Fields
Quoted 33 lines · Jeff Layton
Quoted 7 lines · Jan Kara
Quoted 6 lines · Jeff Layton
static inline bool
inode_maybe_inc_iversion(struct inode *inode, bool force){(quoted diff) +1
-1- atomic64_t *ivp = (atomic64_t *)&inode->i_version; + u64 cur, old, new;Quoted 14 lines
Review
Quoted 6 lines
(quoted diff) +5
-1- atomic64_inc(ivp); + cur = (u64)atomic64_read(&inode->i_version); + for (;;) { + /* If flag is clear then we needn't do anything */ + if (!force && !(cur & I_VERSION_QUERIED)) + return false;The fast path here misses any memory barrier. Thus it seems this query could be in theory reordered before any store that happened to modify the inode? Or maybe we could race and miss the fact that in fact this
i_versionhas already been queried? But maybe there's some higher level locking that makes sure this is all a non-issue... But in that case it would deserve some comment I guess.There's no higher-level locking. Getting locking out of this codepath is a good thing IMO. The larger question here is whether we really care about ordering this with anything else.
The
i_version, as implemented today, is not ordered with actual changes to the inode. We only take thei_locktoday when modifying it, not when querying it. It's possible today that you could see the results of a change and then do a fetch of thei_versionthat doesn't show an increment vs. a previous change.Quoted 4 lines
It'd be nice if this were atomic with the actual changes that it represents, but I think that would be prohibitively expensive. That may be something we need to address. I'm not sure we really want to do it as part of this patchset though.
I don't think we need full atomicity, but we *do* need the
i_versionincrement to be seen as ordered after the inode change. That should be relatively inexpensive, yes?Otherwise an inode change could be overlooked indefinitely, because a client could cache the old contents with the new
i_versionvalue and never see thei_versionchange again as long as the inode isn't touched again.
Quoted 7 lines
(If the client instead caches new data with the old inode version, there may be an unnecessary cache invalidation next time the client queries the change attribute. That's a performance bug, but not really a correctness problem, at least for clients depending only on close-to-open semantics: they'll only depend on seeing the new change attribute after the change has been flushed to disk. The performance problem should be mitigated by the race being very rare.)
Perhaps something like this patch squashed into #19?
This should tighten up the initial loads and stores, like Jan was suggesting (I think). The increments are done via cmpxchg so they already have a full implied barrier (IIUC), and we don't exit the loop until it succeeds or it looks like the flag is set.
To be clear though:
This series does not try to order the i_version update with any other inode metadata or data (though we generally do update it after a write is done, rather than before or during).
Inode field updates are not at all synchronized either and this series does not change that. A stat() is also generally done locklessly so you can easily get a half-baked attributes there if you hit the timing wrong.
-------------------------8<--------------------------
[PATCH] SQUASH: add memory barriers around i_version accesses
--- include/linux/iversion.h | 40 +++++++++++++++++++++------------------- 1 file changed, 21 insertions(+), 19 deletions(-)
include/linux/iversion.h +21 -19
@@ -87,6 +87,25 @@ static inline void inode_set_iversion_raw(struct inode *inode, const u64 val) {atomic64_set (&inode->i_version, val); +smp_wmb (); +} + +/** +* inode_peek_iversion_raw - grab a "raw" iversion value +* @inode: inode from which i_version should be read +* +* Grab a "raw" inode->i_version value and return it. The i_version is not +* flagged or converted in any way. This is mostly used to access a self-managed +* i_version. +* +* With those filesystems, we want to treat the i_version as an entirely +* opaque value. +*/ +static inline u64 +inode_peek_iversion_raw (const struct inode *inode) +{ +smp_rmb (); +return atomic64_read (&inode->i_version); }/**
@@ -152,7 +171,7 @@ inode_maybe_inc_iversion(struct inode *inode, bool force) {u64 cur, old, new; - cur = (u64 )atomic64_read (&inode->i_version); + cur =inode_peek_iversion_raw (inode);for (;;) {/* If flag is clear then we needn't do anything */ if (!force && !(cur &I_VERSION_QUERIED ))
@@ -183,23 +202,6 @@ inode_inc_iversion(struct inode *inode)inode_maybe_inc_iversion (inode, true); } -/** -* inode_peek_iversion_raw - grab a "raw" iversion value -* @inode: inode from which i_version should be read -* -* Grab a "raw" inode->i_version value and return it. The i_version is not -* flagged or converted in any way. This is mostly used to access a self-managed -* i_version. -* -* With those filesystems, we want to treat the i_version as an entirely -* opaque value. -*/ -static inline u64 -inode_peek_iversion_raw (const struct inode *inode) -{ -return atomic64_read (&inode->i_version); -} -/** * inode_iversion_need_inc - is the i_version in need of being incremented? * @inode: inode to check
@@ -248,7 +250,7 @@ inode_query_iversion(struct inode *inode) {u64 cur, old, new; - cur =atomic64_read (&inode->i_version); + cur =inode_peek_iversion_raw (inode);for (;;) {/* If flag is already set, then no need to swap */ if (cur &I_VERSION_QUERIED )
Signature · 1 line
2.14.3
Re: [PATCH v3 19/19] fs: handle inode->i_version more efficiently
Quoted 1 line · Jeff Layton
[PATCH] SQUASH: add memory barriers around
i_versionaccesses
Why explicit memory barriers rather than annotating the operations with the required semantics and getting the barriers the arch requires automatically? I suspect this should be using atomic_read_acquire() and atomic_cmpxchg_release(), because AFAICT the atomic_cmpxchg needs to have release semantics to match the acquire semantics needed for the load of the current value.
Quoted 1 line
From
include/linux/atomics.h:
* For compound atomics performing both a load and a store, ACQUIRE
* semantics apply only to the load and RELEASE semantics only to the
* store portion of the operation. Note that a failed cmpxchg_acquire
* does -not- imply any memory ordering constraints.
Memory barriers hurt my brain. :/
At minimum, shouldn't the atomic op specific barriers be used rather than full memory barriers? i.e:
include/linux/iversion.h static inline void · line 90
Quoted 9 lines
include/linux/iversion.h +1
-0@@ -87,6 +87,25 @@ static inline void inode_set_iversion_raw(struct inode *inode, const u64 val) {atomic64_set (&inode->i_version, val); +smp_wmb ();
smp_mb__after_atomic();
.....
include/linux/iversion.h static inline void · line 107
Quoted 4 lines
include/linux/iversion.h +4
-0+static inline u64 +inode_peek_iversion_raw (const struct inode *inode) +{ +smp_rmb ();
smp_mb__before_atomic();
include/linux/iversion.h static inline void · line 109
Quoted 2 lines
include/linux/iversion.h +1
-0+return atomic64_read (&inode->i_version); }
And, of course, these will require comments explaining what they match with and what they are ordering.
include/linux/iversion.h inode_maybe_inc_iversion(struct inode *inode, bool force) · line 177
Quoted 9 lines
include/linux/iversion.h +1
-1@@ -152,7 +171,7 @@ inode_maybe_inc_iversion(struct inode *inode, bool force) {u64 cur, old, new; - cur = (u64 )atomic64_read (&inode->i_version); + cur =inode_peek_iversion_raw (inode);for (;;) {/* If flag is clear then we needn't do anything */ if (!force && !(cur &I_VERSION_QUERIED ))
cmpxchg in this loop needs a release barrier so everyone will see the change?
Quoted 25 lines
include/linux/iversion.h +0
-17@@ -183,23 +202,6 @@ inode_inc_iversion(struct inode *inode)inode_maybe_inc_iversion (inode, true); } -/** -* inode_peek_iversion_raw - grab a "raw" iversion value -* @inode: inode from which i_version should be read -* -* Grab a "raw" inode->i_version value and return it. The i_version is not -* flagged or converted in any way. This is mostly used to access a self-managed -* i_version. -* -* With those filesystems, we want to treat the i_version as an entirely -* opaque value. -*/ -static inline u64 -inode_peek_iversion_raw (const struct inode *inode) -{ -return atomic64_read (&inode->i_version); -} -/** * inode_iversion_need_inc - is the i_version in need of being incremented? * @inode: inode to check
include/linux/iversion.h inode_query_iversion(struct inode *inode) · line 256
Quoted 8 lines
include/linux/iversion.h +1
-1@@ -248,7 +250,7 @@ inode_query_iversion(struct inode *inode) {u64 cur, old, new; - cur =atomic64_read (&inode->i_version); + cur =inode_peek_iversion_raw (inode);for (;;) {/* If flag is already set, then no need to swap */ if (cur &I_VERSION_QUERIED )
cmpxchg in this loop needs a release barrier so everyone will see the change on load?
Cheers,
Dave.
Signature · 2 lines
Dave Chinner [email protected]
Jeff Layton
maintainerRe: [PATCH v3 19/19] fs: handle inode->i_version more efficiently
Quoted 17 lines · Dave Chinner
Quoted 1 line · Jeff Layton
[PATCH] SQUASH: add memory barriers around
i_versionaccessesWhy explicit memory barriers rather than annotating the operations with the required semantics and getting the barriers the arch requires automatically? I suspect this should be using
atomic_read_acquire()andatomic_cmpxchg_release(), because AFAICT theatomic_cmpxchgneeds to have release semantics to match the acquire semantics needed for the load of the current value.From
include/linux/atomics.h:* For compound atomics performing both a load and a store, ACQUIRE
* semantics apply only to the load and RELEASE semantics only to the
* store portion of the operation. Note that a failedcmpxchg_acquire
* does -not- imply any memory ordering constraints.
Quoted 5 lines
Memory barriers hurt my brain. :/
At minimum, shouldn't the atomic op specific barriers be used rather than full memory barriers? i.e:
They hurt my brain too. Yes, definitely atomic-specific barriers should be used here instead, since this is an atomic64_t now.
After going over the docs again...my understanding has always been that you primarily need memory barriers to order accesses to different areas of memory.
As Jan and I were discussing in the other thread, i_version is not synchronized with anything else. In this code, we're only dealing with a single 64-bit word. I don't think there are any races there wrt the API itself.
The "legacy" inode_inc_iversion() however _does_ have implied memory barriers from the i_lock. There could be some subtle de-facto ordering there, so I think we probably do want some barriers in here if only to preserve that. It's not likely to cost much, and may save us tracking down some fiddly bugs.
What about this patch? Note that I've only added barriers to inode_maybe_inc_iversion. I don't see that we need it for the other functions, but please do tell me if I'm wrong there:
--------------------8<---------------------------
[PATCH] SQUASH: add memory barriers around i_version accesses
--- include/linux/iversion.h | 53 +++++++++++++++++++++++++++++------------------- 1 file changed, 32 insertions(+), 21 deletions(-)
include/linux/iversion.h +32 -21
@@ -89,6 +89,23 @@ inode_set_iversion_raw(struct inode *inode, const u64 val)atomic64_set (&inode->i_version, val); } +/** +* inode_peek_iversion_raw - grab a "raw" iversion value +* @inode: inode from which i_version should be read +* +* Grab a "raw" inode->i_version value and return it. The i_version is not +* flagged or converted in any way. This is mostly used to access a self-managed +* i_version. +* +* With those filesystems, we want to treat the i_version as an entirely +* opaque value. +*/ +static inline u64 +inode_peek_iversion_raw (const struct inode *inode) +{ +return atomic64_read (&inode->i_version); +} +/** * inode_set_iversion - set i_version to a particular value * @inode: inode to set
@@ -152,7 +169,16 @@ inode_maybe_inc_iversion(struct inode *inode, bool force) {u64 cur, old, new; - cur = (u64 )atomic64_read (&inode->i_version); +/* +* The i_version field is not strictly ordered with any other inode +* information, but the legacy inode_inc_iversion code used a spinlock +* to serialize increments. +* +* This code adds full memory barriers to ensure that any de-facto +* ordering with other info is preserved. +*/ +smp_mb__before_atomic (); + cur =inode_peek_iversion_raw (inode);for (;;) {/* If flag is clear then we needn't do anything */ if (!force && !(cur &I_VERSION_QUERIED ))
@@ -162,8 +188,10 @@ inode_maybe_inc_iversion(struct inode *inode, bool force) new = (cur & ~I_VERSION_QUERIED ) +I_VERSION_INCREMENT ; old =atomic64_cmpxchg (&inode->i_version, cur, new); -if (likely (old == cur)) +if (likely (old == cur)) { +smp_mb__after_atomic ();break ; + } cur = old; }return true;
@@ -183,23 +211,6 @@ inode_inc_iversion(struct inode *inode)inode_maybe_inc_iversion (inode, true); } -/** -* inode_peek_iversion_raw - grab a "raw" iversion value -* @inode: inode from which i_version should be read -* -* Grab a "raw" inode->i_version value and return it. The i_version is not -* flagged or converted in any way. This is mostly used to access a self-managed -* i_version. -* -* With those filesystems, we want to treat the i_version as an entirely -* opaque value. -*/ -static inline u64 -inode_peek_iversion_raw (const struct inode *inode) -{ -return atomic64_read (&inode->i_version); -} -/** * inode_iversion_need_inc - is the i_version in need of being incremented? * @inode: inode to check
@@ -248,7 +259,7 @@ inode_query_iversion(struct inode *inode) {u64 cur, old, new; - cur =atomic64_read (&inode->i_version); + cur =inode_peek_iversion_raw (inode);for (;;) {/* If flag is already set, then no need to swap */ if (cur &I_VERSION_QUERIED )
@@ -256,7 +267,7 @@ inode_query_iversion(struct inode *inode) new = cur |I_VERSION_QUERIED ; old =atomic64_cmpxchg (&inode->i_version, cur, new); -if (old == cur) +if (likely (old == cur))break ; cur = old; }
Signature · 8 lines
2.14.3 -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Jan Kara
reviewerRe: [PATCH v3 19/19] fs: handle inode->i_version more efficiently
Quoted 24 lines · Jeff Layton
Quoted 17 lines · Dave Chinner
Quoted 1 line · Jeff Layton
[PATCH] SQUASH: add memory barriers around
i_versionaccessesWhy explicit memory barriers rather than annotating the operations with the required semantics and getting the barriers the arch requires automatically? I suspect this should be using
atomic_read_acquire()andatomic_cmpxchg_release(), because AFAICT theatomic_cmpxchgneeds to have release semantics to match the acquire semantics needed for the load of the current value.From
include/linux/atomics.h:* For compound atomics performing both a load and a store, ACQUIRE
* semantics apply only to the load and RELEASE semantics only to the
* store portion of the operation. Note that a failedcmpxchg_acquire
* does -not- imply any memory ordering constraints.Quoted 5 lines
Memory barriers hurt my brain. :/
At minimum, shouldn't the atomic op specific barriers be used rather than full memory barriers? i.e:
Quoted 6 lines
They hurt my brain too. Yes, definitely atomic-specific barriers should be used here instead, since this is an
atomic64_tnow.After going over the docs again...my understanding has always been that you primarily need memory barriers to order accesses to different areas of memory.
That is correct.
Quoted 4 lines
As Jan and I were discussing in the other thread,
i_versionis not synchronized with anything else. In this code, we're only dealing with a single 64-bit word. I don't think there are any races there wrt the API itself.
There are not but it is like saying that lock implementation is correct because the lock state does not get corrupted ;). Who cares about protected data...
Quoted 51 lines
The "legacy"
inode_inc_iversion()however _does_ have implied memory barriers from thei_lock. There could be some subtle de-facto ordering there, so I think we probably do want some barriers in here if only to preserve that. It's not likely to cost much, and may save us tracking down some fiddly bugs.What about this patch? Note that I've only added barriers to
inode_maybe_inc_iversion. I don't see that we need it for the other functions, but please do tell me if I'm wrong there:--------------------8<---------------------------
[PATCH] SQUASH: add memory barriers around
i_versionaccessesSigned-off-by Jeff Layton <[email protected]>--- include/linux/iversion.h | 53 +++++++++++++++++++++++++++++------------------- 1 file changed, 32 insertions(+), 21 deletions(-)include/linux/iversion.h +17
-0@@ -89,6 +89,23 @@ inode_set_iversion_raw(struct inode *inode, const u64 val)atomic64_set (&inode->i_version, val); } +/** +* inode_peek_iversion_raw - grab a "raw" iversion value +* @inode: inode from which i_version should be read +* +* Grab a "raw" inode->i_version value and return it. The i_version is not +* flagged or converted in any way. This is mostly used to access a self-managed +* i_version. +* +* With those filesystems, we want to treat the i_version as an entirely +* opaque value. +*/ +static inline u64 +inode_peek_iversion_raw (const struct inode *inode) +{ +return atomic64_read (&inode->i_version); +} +/** * inode_set_iversion - set i_version to a particular value * @inode: inode to set @@ -152,7 +169,16 @@ inode_maybe_inc_iversion(struct inode *inode, bool force) {u64 cur, old, new;
include/linux/iversion.h inode_maybe_inc_iversion(struct inode *inode, bool force) · line 180
Quoted 10 lines
include/linux/iversion.h +9
-1- cur = (u64 )atomic64_read (&inode->i_version); +/* +* The i_version field is not strictly ordered with any other inode +* information, but the legacy inode_inc_iversion code used a spinlock +* to serialize increments. +* +* This code adds full memory barriers to ensure that any de-facto +* ordering with other info is preserved. +*/ +smp_mb__before_atomic ();
This should be just smp_mb(). __before_atomic() pairs with atomic operations like atomic_inc(). atomic_read() is completely unordered operation (happens to be plain memory read on x86) and so __before_atomic() is not enough.
include/linux/iversion.h inode_maybe_inc_iversion(struct inode *inode, bool force) · line 192
Quoted 11 lines
include/linux/iversion.h +3
-1+ cur =inode_peek_iversion_raw (inode);for (;;) {/* If flag is clear then we needn't do anything */ if (!force && !(cur &I_VERSION_QUERIED ))@@ -162,8 +188,10 @@ inode_maybe_inc_iversion(struct inode *inode, bool force) new = (cur & ~I_VERSION_QUERIED ) +I_VERSION_INCREMENT ; old =atomic64_cmpxchg (&inode->i_version, cur, new); -if (likely (old == cur)) +if (likely (old == cur)) { +smp_mb__after_atomic ();
I don't think you need this. Cmpxchg is guaranteed to be full memory barrier - from Documentation/atomic_t.txt:
- RMW operations that have a return value are fully ordered;
Quoted 5 lines
include/linux/iversion.h +1
-0break ; + } cur = old; }return true;
...
include/linux/iversion.h inode_query_iversion(struct inode *inode) · line 265
Quoted 9 lines
include/linux/iversion.h +1
-1@@ -248,7 +259,7 @@ inode_query_iversion(struct inode *inode) {u64 cur, old, new; - cur =atomic64_read (&inode->i_version); + cur =inode_peek_iversion_raw (inode);for (;;) {/* If flag is already set, then no need to swap */ if (cur &I_VERSION_QUERIED )
And here I'd expect smp_mb() after inode_peek_iversion_raw() (actually be needed only if you are not going to do cmpxchg as that implies barrier as well). "Safe" use of i_version would be:
Update:
modify inodeinode_maybe_inc_iversion(inode)
Read:
my_version = inode_query_iversion(inode)
get inode data
And you need to make sure 'get inode data' does not get speculatively evaluated before you actually sample i_version so that you are guaranteed that if data changes, you will observe larger i_version in the future.
Also please add a comment smp_mb() in inode_maybe_inc_iversion() like:
/* This barrier pairs with the barrier in inode_query_iversion() */
and a similar comment to inode_query_iversion(). Because memory barriers make sense only in pairs (see SMP BARRIER PAIRING in Documentation/memory-barriers.txt).
Honza
Signature · 6 lines
Jan Kara <jack-IBi9RG/[email protected]> SUSE Labs, CR -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Jeff Layton
maintainerRe: [PATCH v3 19/19] fs: handle inode->i_version more efficiently
Quoted 162 lines · Jan Kara
Quoted 24 lines · Jeff Layton
Quoted 17 lines · Dave Chinner
Quoted 1 line · Jeff Layton
[PATCH] SQUASH: add memory barriers around
i_versionaccessesWhy explicit memory barriers rather than annotating the operations with the required semantics and getting the barriers the arch requires automatically? I suspect this should be using
atomic_read_acquire()andatomic_cmpxchg_release(), because AFAICT theatomic_cmpxchgneeds to have release semantics to match the acquire semantics needed for the load of the current value.From
include/linux/atomics.h:* For compound atomics performing both a load and a store, ACQUIRE
* semantics apply only to the load and RELEASE semantics only to the
* store portion of the operation. Note that a failedcmpxchg_acquire
* does -not- imply any memory ordering constraints.Quoted 5 lines
Memory barriers hurt my brain. :/
At minimum, shouldn't the atomic op specific barriers be used rather than full memory barriers? i.e:
Quoted 6 lines
They hurt my brain too. Yes, definitely atomic-specific barriers should be used here instead, since this is an
atomic64_tnow.After going over the docs again...my understanding has always been that you primarily need memory barriers to order accesses to different areas of memory.
That is correct.
Quoted 4 lines
As Jan and I were discussing in the other thread,
i_versionis not synchronized with anything else. In this code, we're only dealing with a single 64-bit word. I don't think there are any races there wrt the API itself.There are not but it is like saying that lock implementation is correct because the lock state does not get corrupted ;). Who cares about protected data...
Quoted 51 lines
The "legacy"
inode_inc_iversion()however _does_ have implied memory barriers from thei_lock. There could be some subtle de-facto ordering there, so I think we probably do want some barriers in here if only to preserve that. It's not likely to cost much, and may save us tracking down some fiddly bugs.What about this patch? Note that I've only added barriers to
inode_maybe_inc_iversion. I don't see that we need it for the other functions, but please do tell me if I'm wrong there:--------------------8<---------------------------
[PATCH] SQUASH: add memory barriers around
i_versionaccessesSigned-off-by Jeff Layton <[email protected]>--- include/linux/iversion.h | 53 +++++++++++++++++++++++++++++------------------- 1 file changed, 32 insertions(+), 21 deletions(-)include/linux/iversion.h +17
-0@@ -89,6 +89,23 @@ inode_set_iversion_raw(struct inode *inode, const u64 val)atomic64_set (&inode->i_version, val); } +/** +* inode_peek_iversion_raw - grab a "raw" iversion value +* @inode: inode from which i_version should be read +* +* Grab a "raw" inode->i_version value and return it. The i_version is not +* flagged or converted in any way. This is mostly used to access a self-managed +* i_version. +* +* With those filesystems, we want to treat the i_version as an entirely +* opaque value. +*/ +static inline u64 +inode_peek_iversion_raw (const struct inode *inode) +{ +return atomic64_read (&inode->i_version); +} +/** * inode_set_iversion - set i_version to a particular value * @inode: inode to set @@ -152,7 +169,16 @@ inode_maybe_inc_iversion(struct inode *inode, bool force) {u64 cur, old, new;include/linux/iversion.h inode_maybe_inc_iversion(struct inode *inode, bool force) · line 180
Quoted 10 lines
include/linux/iversion.h +9
-1- cur = (u64 )atomic64_read (&inode->i_version); +/* +* The i_version field is not strictly ordered with any other inode +* information, but the legacy inode_inc_iversion code used a spinlock +* to serialize increments. +* +* This code adds full memory barriers to ensure that any de-facto +* ordering with other info is preserved. +*/ +smp_mb__before_atomic ();This should be just
smp_mb().__before_atomic()pairs with atomic operations likeatomic_inc().atomic_read()is completely unordered operation (happens to be plain memory read on x86) and so__before_atomic()is not enough.include/linux/iversion.h inode_maybe_inc_iversion(struct inode *inode, bool force) · line 192
Quoted 11 lines
include/linux/iversion.h +3
-1+ cur =inode_peek_iversion_raw (inode);for (;;) {/* If flag is clear then we needn't do anything */ if (!force && !(cur &I_VERSION_QUERIED ))@@ -162,8 +188,10 @@ inode_maybe_inc_iversion(struct inode *inode, bool force) new = (cur & ~I_VERSION_QUERIED ) +I_VERSION_INCREMENT ; old =atomic64_cmpxchg (&inode->i_version, cur, new); -if (likely (old == cur)) +if (likely (old == cur)) { +smp_mb__after_atomic ();I don't think you need this. Cmpxchg is guaranteed to be full memory barrier - from
Documentation/atomic_t.txt:
- RMW operations that have a return value are fully ordered;Quoted 5 lines
include/linux/iversion.h +1
-0break ; + } cur = old; }return true;...
include/linux/iversion.h inode_query_iversion(struct inode *inode) · line 265
Quoted 9 lines
include/linux/iversion.h +1
-1@@ -248,7 +259,7 @@ inode_query_iversion(struct inode *inode) {u64 cur, old, new; - cur =atomic64_read (&inode->i_version); + cur =inode_peek_iversion_raw (inode);for (;;) {/* If flag is already set, then no need to swap */ if (cur &I_VERSION_QUERIED )And here I'd expect
smp_mb()afterinode_peek_iversion_raw()(actually be needed only if you are not going to do cmpxchg as that implies barrier as well). "Safe" use ofi_versionwould be:Update:
modify inode
inode_maybe_inc_iversion(inode)Read:
my_version=inode_query_iversion(inode)
get inode dataAnd you need to make sure 'get inode data' does not get speculatively evaluated before you actually sample
i_versionso that you are guaranteed that if data changes, you will observe largeri_versionin the future.
Quoted 8 lines
Also please add a comment
smp_mb()ininode_maybe_inc_iversion()like:/* This barrier pairs with the barrier in
inode_query_iversion()*/and a similar comment to
inode_query_iversion(). Because memory barriers make sense only in pairs (see SMP BARRIER PAIRING inDocumentation/memory-barriers.txt).
Got it, I think. How about this (sorry for the unrelated deltas here):
[PATCH] SQUASH: add memory barriers around i_version accesses
--- include/linux/iversion.h | 60 +++++++++++++++++++++++++++++++----------------- 1 file changed, 39 insertions(+), 21 deletions(-)
include/linux/iversion.h +39 -21
@@ -89,6 +89,23 @@ inode_set_iversion_raw(struct inode *inode, const u64 val)atomic64_set (&inode->i_version, val); } +/** +* inode_peek_iversion_raw - grab a "raw" iversion value +* @inode: inode from which i_version should be read +* +* Grab a "raw" inode->i_version value and return it. The i_version is not +* flagged or converted in any way. This is mostly used to access a self-managed +* i_version. +* +* With those filesystems, we want to treat the i_version as an entirely +* opaque value. +*/ +static inline u64 +inode_peek_iversion_raw (const struct inode *inode) +{ +return atomic64_read (&inode->i_version); +} +/** * inode_set_iversion - set i_version to a particular value * @inode: inode to set
@@ -152,7 +169,18 @@ inode_maybe_inc_iversion(struct inode *inode, bool force) {u64 cur, old, new; - cur = (u64 )atomic64_read (&inode->i_version); +/* +* The i_version field is not strictly ordered with any other inode +* information, but the legacy inode_inc_iversion code used a spinlock +* to serialize increments. +* +* Here, we add full memory barriers to ensure that any de-facto +* ordering with other info is preserved. +* +* This barrier pairs with the barrier in inode_query_iversion() +*/ +smp_mb (); + cur =inode_peek_iversion_raw (inode);for (;;) {/* If flag is clear then we needn't do anything */ if (!force && !(cur &I_VERSION_QUERIED ))
@@ -183,23 +211,6 @@ inode_inc_iversion(struct inode *inode)inode_maybe_inc_iversion (inode, true); } -/** -* inode_peek_iversion_raw - grab a "raw" iversion value -* @inode: inode from which i_version should be read -* -* Grab a "raw" inode->i_version value and return it. The i_version is not -* flagged or converted in any way. This is mostly used to access a self-managed -* i_version. -* -* With those filesystems, we want to treat the i_version as an entirely -* opaque value. -*/ -static inline u64 -inode_peek_iversion_raw (const struct inode *inode) -{ -return atomic64_read (&inode->i_version); -} -/** * inode_iversion_need_inc - is the i_version in need of being incremented? * @inode: inode to check
@@ -248,15 +259,22 @@ inode_query_iversion(struct inode *inode) {u64 cur, old, new; - cur =atomic64_read (&inode->i_version); + cur =inode_peek_iversion_raw (inode);for (;;) {/* If flag is already set, then no need to swap */ -if (cur &I_VERSION_QUERIED ) +if (cur &I_VERSION_QUERIED ) { +/* +* This barrier (and the implicit barrier in the +* cmpxchg below) pairs with the barrier in +* inode_maybe_inc_iversion(). +*/ +smp_mb ();break ; + } new = cur |I_VERSION_QUERIED ; old =atomic64_cmpxchg (&inode->i_version, cur, new); -if (old == cur) +if (likely (old == cur))break ; cur = old; }
Signature · 6 lines
2.14.3 -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Jan Kara
reviewerRe: [PATCH v3 19/19] fs: handle inode->i_version more efficiently
Quoted 3 lines · Jeff Layton
Got it, I think. How about this (sorry for the unrelated deltas here):
[PATCH] SQUASH: add memory barriers around
i_versionaccesses
Yep, this looks good to me.
Honza
Quoted 107 lines
Signed-off-by Jeff Layton <[email protected]>--- include/linux/iversion.h | 60 +++++++++++++++++++++++++++++++----------------- 1 file changed, 39 insertions(+), 21 deletions(-)include/linux/iversion.h +39
-21@@ -89,6 +89,23 @@ inode_set_iversion_raw(struct inode *inode, const u64 val)atomic64_set (&inode->i_version, val); } +/** +* inode_peek_iversion_raw - grab a "raw" iversion value +* @inode: inode from which i_version should be read +* +* Grab a "raw" inode->i_version value and return it. The i_version is not +* flagged or converted in any way. This is mostly used to access a self-managed +* i_version. +* +* With those filesystems, we want to treat the i_version as an entirely +* opaque value. +*/ +static inline u64 +inode_peek_iversion_raw (const struct inode *inode) +{ +return atomic64_read (&inode->i_version); +} +/** * inode_set_iversion - set i_version to a particular value * @inode: inode to set @@ -152,7 +169,18 @@ inode_maybe_inc_iversion(struct inode *inode, bool force) {u64 cur, old, new; - cur = (u64 )atomic64_read (&inode->i_version); +/* +* The i_version field is not strictly ordered with any other inode +* information, but the legacy inode_inc_iversion code used a spinlock +* to serialize increments. +* +* Here, we add full memory barriers to ensure that any de-facto +* ordering with other info is preserved. +* +* This barrier pairs with the barrier in inode_query_iversion() +*/ +smp_mb (); + cur =inode_peek_iversion_raw (inode);for (;;) {/* If flag is clear then we needn't do anything */ if (!force && !(cur &I_VERSION_QUERIED ))@@ -183,23 +211,6 @@ inode_inc_iversion(struct inode *inode)inode_maybe_inc_iversion (inode, true); } -/** -* inode_peek_iversion_raw - grab a "raw" iversion value -* @inode: inode from which i_version should be read -* -* Grab a "raw" inode->i_version value and return it. The i_version is not -* flagged or converted in any way. This is mostly used to access a self-managed -* i_version. -* -* With those filesystems, we want to treat the i_version as an entirely -* opaque value. -*/ -static inline u64 -inode_peek_iversion_raw (const struct inode *inode) -{ -return atomic64_read (&inode->i_version); -} -/** * inode_iversion_need_inc - is the i_version in need of being incremented? * @inode: inode to check @@ -248,15 +259,22 @@ inode_query_iversion(struct inode *inode) {u64 cur, old, new; - cur =atomic64_read (&inode->i_version); + cur =inode_peek_iversion_raw (inode);for (;;) {/* If flag is already set, then no need to swap */ -if (cur &I_VERSION_QUERIED ) +if (cur &I_VERSION_QUERIED ) { +/* +* This barrier (and the implicit barrier in the +* cmpxchg below) pairs with the barrier in +* inode_maybe_inc_iversion(). +*/ +smp_mb ();break ; + } new = cur |I_VERSION_QUERIED ; old =atomic64_cmpxchg (&inode->i_version, cur, new); -if (old == cur) +if (likely (old == cur))break ; cur = old; }Signature · 1 line
2.14.3
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Jan Kara
reviewerRe: [PATCH v3 19/19] fs: handle inode->i_version more efficiently
In reply to Jeff LaytonQuoted 27 lines · Jeff Layton
Quoted 7 lines · Jan Kara
Quoted 6 lines · Jeff Layton
static inline bool
inode_maybe_inc_iversion(struct inode *inode, bool force){include/linux/iversion.h +1
-1-atomic64_t *ivp = (atomic64_t *)&inode->i_version; +u64 cur, old, new;Quoted 14 lines
include/linux/iversion.h line 159
Quoted 6 lines
include/linux/iversion.h +5
-1-atomic64_inc (ivp); + cur = (u64 )atomic64_read (&inode->i_version); +for (;;) { +/* If flag is clear then we needn't do anything */ +if (!force && !(cur &I_VERSION_QUERIED )) +return false;The fast path here misses any memory barrier. Thus it seems this query could be in theory reordered before any store that happened to modify the inode? Or maybe we could race and miss the fact that in fact this
i_versionhas already been queried? But maybe there's some higher level locking that makes sure this is all a non-issue... But in that case it would deserve some comment I guess.There's no higher-level locking. Getting locking out of this codepath is a good thing IMO. The larger question here is whether we really care about ordering this with anything else.
Quoted 5 lines
The
i_version, as implemented today, is not ordered with actual changes to the inode. We only take thei_locktoday when modifying it, not when querying it. It's possible today that you could see the results of a change and then do a fetch of thei_versionthat doesn't show an increment vs. a previous change.
Yeah, so I don't suggest that you should fix unrelated issues but original i_lock protection did actually provide memory barriers (although semi-permeable, but in practice they are very often enough) and your patch removing those could have changed a theoretical issue to a practical problem. So at least preserving that original acquire-release semantics of i_version handling would be IMHO good.
Quoted 52 lines
It'd be nice if this were atomic with the actual changes that it represents, but I think that would be prohibitively expensive. That may be something we need to address. I'm not sure we really want to do it as part of this patchset though.
Quoted 27 lines
Quoted 12 lines
include/linux/iversion.h +9
-0+ +/* Since lowest bit is flag, add 2 to avoid it */ + new = (cur & ~I_VERSION_QUERIED ) +I_VERSION_INCREMENT ; + + old =atomic64_cmpxchg (&inode->i_version, cur, new); +if (likely (old == cur)) +break ; + cur = old; + }return true; }...
Quoted 12 lines
static inline u64
inode_query_iversion(struct inode *inode)
{include/linux/iversion.h +8
-1-return inode_peek_iversion (inode); +u64 cur, old, new; + + cur =atomic64_read (&inode->i_version); +for (;;) { +/* If flag is already set, then no need to swap */ +if (cur &I_VERSION_QUERIED ) +break ; +Quoted 9 lines
include/linux/iversion.h inode_peek_iversion(const struct inode *inode) · line 262
Quoted 6 lines
include/linux/iversion.h +6
-0+ new = cur |I_VERSION_QUERIED ; + old =atomic64_cmpxchg (&inode->i_version, cur, new); +if (old == cur) +break ; + cur = old; + }Why not just use
atomic64_or()here?If the cmpxchg fails, then either:
1) it was incremented
2) someone flagged it QUERIEDIf an increment happened then we don't need to flag it as QUERIED if we're returning an older value. If we use
atomic64_or, then we can't tell if an increment happened so we'd end up potentially flagging it more than necessary.
Quoted 4 lines
In principle, either outcome is technically OK and we don't have to loop if the cmpxchg doesn't work. That said, if we think there might be a later
i_versionavailable, then I think we probably want to try to query it again so we can return as late a one as possible.
OK, makes sense. I'm just a bit vary of cmpxchg loops as they tend to behave pretty badly in contended cases but I guess i_version won't be hammered *that* hard.
Honza
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Jeff Layton
maintainerRe: [PATCH v3 19/19] fs: handle inode->i_version more efficiently
Quoted 34 lines · Jan Kara
Quoted 27 lines · Jeff Layton
Quoted 7 lines · Jan Kara
Quoted 6 lines · Jeff Layton
static inline bool
inode_maybe_inc_iversion(struct inode *inode, bool force){include/linux/iversion.h +1
-1-atomic64_t *ivp = (atomic64_t *)&inode->i_version; +u64 cur, old, new;Quoted 14 lines
include/linux/iversion.h line 159
Quoted 6 lines
include/linux/iversion.h +5
-1-atomic64_inc (ivp); + cur = (u64 )atomic64_read (&inode->i_version); +for (;;) { +/* If flag is clear then we needn't do anything */ +if (!force && !(cur &I_VERSION_QUERIED )) +return false;The fast path here misses any memory barrier. Thus it seems this query could be in theory reordered before any store that happened to modify the inode? Or maybe we could race and miss the fact that in fact this
i_versionhas already been queried? But maybe there's some higher level locking that makes sure this is all a non-issue... But in that case it would deserve some comment I guess.There's no higher-level locking. Getting locking out of this codepath is a good thing IMO. The larger question here is whether we really care about ordering this with anything else.
Quoted 5 lines
The
i_version, as implemented today, is not ordered with actual changes to the inode. We only take thei_locktoday when modifying it, not when querying it. It's possible today that you could see the results of a change and then do a fetch of thei_versionthat doesn't show an increment vs. a previous change.
Quoted 7 lines
Yeah, so I don't suggest that you should fix unrelated issues but original
i_lockprotection did actually provide memory barriers (although semi-permeable, but in practice they are very often enough) and your patch removing those could have changed a theoretical issue to a practical problem. So at least preserving that original acquire-release semantics ofi_versionhandling would be IMHO good.
Agreed. I've no objection to memory barriers here and I'm looking at that, I just need to go over Dave's comments and memory-barriers.txt (again!) to make sure I get them right.
Quoted 52 lines
Quoted 52 lines
It'd be nice if this were atomic with the actual changes that it represents, but I think that would be prohibitively expensive. That may be something we need to address. I'm not sure we really want to do it as part of this patchset though.
Quoted 27 lines
Quoted 12 lines
include/linux/iversion.h +9
-0+ +/* Since lowest bit is flag, add 2 to avoid it */ + new = (cur & ~I_VERSION_QUERIED ) +I_VERSION_INCREMENT ; + + old =atomic64_cmpxchg (&inode->i_version, cur, new); +if (likely (old == cur)) +break ; + cur = old; + }return true; }...
Quoted 12 lines
static inline u64
inode_query_iversion(struct inode *inode)
{include/linux/iversion.h +8
-1-return inode_peek_iversion (inode); +u64 cur, old, new; + + cur =atomic64_read (&inode->i_version); +for (;;) { +/* If flag is already set, then no need to swap */ +if (cur &I_VERSION_QUERIED ) +break ; +Quoted 9 lines
include/linux/iversion.h inode_peek_iversion(const struct inode *inode) · line 262
Quoted 6 lines
include/linux/iversion.h +6
-0+ new = cur |I_VERSION_QUERIED ; + old =atomic64_cmpxchg (&inode->i_version, cur, new); +if (old == cur) +break ; + cur = old; + }Why not just use
atomic64_or()here?If the cmpxchg fails, then either:
1) it was incremented
2) someone flagged it QUERIEDIf an increment happened then we don't need to flag it as QUERIED if we're returning an older value. If we use
atomic64_or, then we can't tell if an increment happened so we'd end up potentially flagging it more than necessary.
Quoted 9 lines
Quoted 4 lines
In principle, either outcome is technically OK and we don't have to loop if the cmpxchg doesn't work. That said, if we think there might be a later
i_versionavailable, then I think we probably want to try to query it again so we can return as late a one as possible.OK, makes sense. I'm just a bit vary of cmpxchg loops as they tend to behave pretty badly in contended cases but I guess
i_versionwon't be hammered *that* hard.
That's the principle I'm operating under here, and I think it's valid for almost all workloads. Incrementing the i_version on parallel writes should be mostly uncontended now, whereas they at had to serialize on the i_lock before.
The pessimal case here, I think is parallel increments and queries. We may see that sort of workload under knfsd, but I'm fine with giving knfsd a small performance hit to help performance on other workloads.
While we're on the subject of looping here, should I add a cpu_relax() into these loops?
Thanks for the review so far!
Signature · 5 lines
Jeff Layton <[email protected]> -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html