Thread (8 messages) 8 messages, 3 authors, 2018-01-31

Re: [PATCH] iversion: make inode_cmp_iversion{+raw} return bool instead of s64

From: Trond Myklebust <hidden>
Date: 2018-01-30 17:50:13
Also in: linux-btrfs, linux-ext4, linux-nfs, linux-xfs

On Tue, 2018-01-30 at 12:31 -0500, Jeff Layton wrote:
quoted hunk ↗ jump to hunk
From: Jeff Layton <redacted>

As Linus points out:

    The inode_cmp_iversion{+raw}() functions are pure and utter crap.

    Why?

    You say that they return 0/negative/positive, but they do so in a
    completely broken manner. They return that ternary value as the
    sequence number difference in a 's64', which means that if you
    actually care about that ternary value, and do the *sane* thing
that
    the kernel-doc of the function implies is the right thing, you
would
    do

        int cmp = inode_cmp_iversion(inode, old);
        if (cmp < 0 ...

    and as a result you get code that looks sane, but that doesn't
    actually *WORK* right.

Since none of the callers actually care about the ternary value here,
convert the inode_cmp_iversion{+raw} functions to just return a
boolean
value (false for matching, true for non-matching).

This matches the existing use of these functions just fine, and makes
it
simple to convert them to return a ternary value in the future if we
grow callers that need it.

Signed-off-by: Jeff Layton <redacted>
---
 include/linux/iversion.h | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)
diff --git a/include/linux/iversion.h b/include/linux/iversion.h
index 858463fca249..ace32775c5f0 100644
--- a/include/linux/iversion.h
+++ b/include/linux/iversion.h
@@ -309,13 +309,13 @@ inode_query_iversion(struct inode *inode)
  * @inode: inode to check
  * @old: old value to check against its i_version
  *
- * Compare the current raw i_version counter with a previous one.
Returns 0 if
- * they are the same or non-zero if they are different.
+ * Compare the current raw i_version counter with a previous one.
Returns false
+ * if they are the same or true if they are different.
  */
-static inline s64
+static inline bool
 inode_cmp_iversion_raw(const struct inode *inode, u64 old)
 {
-	return (s64)inode_peek_iversion_raw(inode) - (s64)old;
+	return inode_peek_iversion_raw(inode) != old;
 }
 
 /**
@@ -323,19 +323,17 @@ inode_cmp_iversion_raw(const struct inode
*inode, u64 old)
  * @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, a positive value if the one in the inode appears newer
than @old,
- * and a negative value if @old appears to be newer than the one in
the
- * inode.
+ * Compare an i_version counter with a previous one. Returns false
if they are
+ * the same, and true if they are different.
  *
  * Note that we don't need to set the QUERIED flag in this case, as
the value
  * in the inode is not being recorded for later use.
  */
 
-static inline s64
+static inline bool
 inode_cmp_iversion(const struct inode *inode, u64 old)
 {
-	return (s64)(inode_peek_iversion_raw(inode) &
~I_VERSION_QUERIED) -
-	       (s64)(old << I_VERSION_QUERIED_SHIFT);
+	return (inode_peek_iversion_raw(inode) & ~I_VERSION_QUERIED)
!=
+			(old << I_VERSION_QUERIED_SHIFT);
 }
Is there any reason why this couldn't just use inode_peek_iversion()
instead of having to both mask the output from
inode_peek_iversion_raw() and shift 'old'?
 #endif
-- 
Trond Myklebust
Linux NFS client maintainer, PrimaryData
trond.myklebust@primarydata.com
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help