iversion: make inode_cmp_iversion{+raw} return bool instead of s64

PATCH Superseded Jeff Layton 3 participants 8 messages Jan 30, 2018 – Jan 31, 2018

Subsystems (5)

VFS Btrfs file system NFS client NFS server +1 more subsystems

lore

Share & tools

lore

Jeff Layton

author maintainer

2018-01-30 17:31 UTC #0 lore raw

PATCH 1/1 iversion: make inode_cmp_iversion{+raw} return bool instead of s64

From: Jeff Layton <[email protected]>

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 <[email protected]>

---
 include/linux/iversion.h | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

include/linux/iversion.h +9 ~~-11~~

index 858463fca249..ace32775c5f0 100644
@@ -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);
 }
 #endif

Signature · 1 line

2.14.3

Trond Myklebust

2018-01-30 17:50 UTC #1 lore raw

Quoted 92 lines · Jeff Layton

From: Jeff Layton <[email protected]>
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 <[email protected]>
---
 include/linux/iversion.h | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)
include/linux/iversion.h +0 -1
index 858463fca249..ace32775c5f0 100644
@@ -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. */
include/linux/iversion.h +2 -3
-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.
*/
include/linux/iversion.h +1 -2
-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) -
include/linux/iversion.h +1 -1
-	       (s64)(old << I_VERSION_QUERIED_SHIFT);
+	return (inode_peek_iversion_raw(inode) & ~I_VERSION_QUERIED)
!=

include/linux/iversion.h inode_cmp_iversion_raw(const struct inode *inode, u64 old) · line 338

Quoted 2 lines

include/linux/iversion.h +1 -0
+			(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'?

Quoted 1 line

#endif

Signature · 3 lines

Trond Myklebust
Linux NFS client maintainer, PrimaryData
[email protected]

Jeff Layton

author maintainer

2018-01-30 19:42 UTC #2 lore raw

Quoted 84 lines · Trond Myklebust

Quoted 83 lines · Jeff Layton
From: Jeff Layton <[email protected]>
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 <[email protected]>
---
 include/linux/iversion.h | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)
include/linux/iversion.h +0 -1
index 858463fca249..ace32775c5f0 100644
@@ -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. */
include/linux/iversion.h +2 -3
-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.
*/

Quoted 15 lines

include/linux/iversion.h inode_cmp_iversion_raw(const struct inode *inode, u64 old) · line 338
Quoted 11 lines
include/linux/iversion.h +1 -2
-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) -
include/linux/iversion.h +1 -1
-	       (s64)(old << I_VERSION_QUERIED_SHIFT);
+	return (inode_peek_iversion_raw(inode) & ~I_VERSION_QUERIED)
!=
include/linux/iversion.h +1 -0
+			(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'?

None at all. I'll send a v2.

Signature · 1 line

Jeff Layton <[email protected]>

Jeff Layton

author maintainer

2018-01-30 20:32 UTC #3 lore raw

PATCH v2 1/1 iversion: make inode_cmp_iversion{+raw} return bool instead of s64

In reply to Jeff Layton

From: Jeff Layton <[email protected]>

As Linus points out:

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

Why?

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.

With this change we can also reimplement inode_cmp_iversion in a simpler way using inode_peek_iversion.

Signed-off-by Jeff Layton <[email protected]>

---
 include/linux/iversion.h | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

include/linux/iversion.h +8 ~~-12~~

index 858463fca249..3d2fd06495ec 100644
@@ -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,15 @@ 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(inode) != old;
 }
 #endif

Signature · 1 line

2.14.3

Linus Torvalds

2018-01-30 20:53 UTC #4 lore raw

Ack. Should I expect this in a future pull request, or take it directly?

There's no hurry about this, since none of the existing users of that function actually do anything but test the return value against zero, and nobody saves it into anything but a "bool" (which has magical casting properties and does not lose upper bits).

Linus
--
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

author maintainer

2018-01-31 12:29 UTC #5 lore raw

Quoted 8 lines · Linus Torvalds

Ack. Should I expect this in a future pull request, or take it directly?
There's no hurry about this, since none of the existing users of that function actually do anything but test the return value against zero, and nobody saves it into anything but a "bool" (which has magical casting properties and does not lose upper bits).
Linus

Do you mind just taking it directly? I don't have anything else queued up for this cycle.

Thanks,

Signature · 1 line

Jeff Layton <[email protected]>

Linus Torvalds

2018-01-31 16:46 UTC #6 lore raw

Quoted 3 lines · Jeff Layton

Do you mind just taking it directly? I don't have anything else queued up for this cycle.

Done.

I wonder if "false for same, true for different" calling convention makes much sense, but it matches the old "0 for same" so obviously makes for a smaller diff.

If it ever ends up confusing people, maybe the sense of that function should be reversed, and the name changed to something like "same_inode_version()" or something.

But at least for now the situation seems ok to me,

Linus

Jeff Layton

author maintainer

2018-01-31 17:55 UTC #7 lore raw

Quoted 7 lines · Linus Torvalds

Quoted 3 lines · Jeff Layton
Do you mind just taking it directly? I don't have anything else queued up for this cycle.
Done.

Thanks...and also many thanks for spotting the original issue. I agree that this makes it much harder for the callers to get things wrong (and is probably much more efficient on some arches, as Ted pointed out).

Quoted 10 lines

I wonder if "false for same, true for different" calling convention makes much sense, but it matches the old "0 for same" so obviously makes for a smaller diff.
If it ever ends up confusing people, maybe the sense of that function should be reversed, and the name changed to something like "same_inode_version()" or something.
But at least for now the situation seems ok to me,

G. Baroncelli suggested changing the name too, so maybe we should just go ahead and do it. Let me think on what the best approach is and I may try to send another patch or PR before the end of the merge window.

Cheers,

Signature · 1 line

Jeff Layton <[email protected]>