Re: [PATCH v3 19/19] fs: handle inode->i_version more efficiently
From: Dave Chinner <david@fromorbit.com>
Date: 2017-12-18 22:08:05
Also in:
linux-btrfs, linux-fsdevel, linux-integrity, linux-nfs, linux-xfs, lkml
On Mon, Dec 18, 2017 at 02:35:20PM -0500, Jeff Layton wrote:
[PATCH] SQUASH: add memory barriers around i_version accesses
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.
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:
quoted hunk ↗ jump to hunk
diff --git a/include/linux/iversion.h b/include/linux/iversion.h index a9fbf99709df..39ec9aa9e08e 100644 --- a/include/linux/iversion.h +++ b/include/linux/iversion.h@@ -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(); .....
+static inline u64
+inode_peek_iversion_raw(const struct inode *inode)
+{
+ smp_rmb();smp_mb__before_atomic();
+ return atomic64_read(&inode->i_version); }
And, of course, these will require comments explaining what they match with and what they are ordering.
quoted hunk ↗ jump to hunk
@@ -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 hunk ↗ jump to hunk
@@ -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)
cmpxchg in this loop needs a release barrier so everyone will see the change on load? Cheers, Dave. -- Dave Chinner david@fromorbit.com