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