linux-ext4.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jeff Layton <jlayton@kernel.org>
To: torvalds@linux-foundation.org
Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
	viro@zeniv.linux.org.uk, linux-nfs@vger.kernel.org,
	linux-ext4@vger.kernel.org, tytso@mit.edu,
	linux-xfs@vger.kernel.org, linux-btrfs@vger.kernel.org,
	linux-integrity@vger.kernel.org, trondmy@primarydata.com
Subject: [PATCH v2] iversion: make inode_cmp_iversion{+raw} return bool instead of s64
Date: Tue, 30 Jan 2018 15:32:21 -0500	[thread overview]
Message-ID: <20180130203221.29310-1-jlayton@kernel.org> (raw)
In-Reply-To: <20180130173126.2806-1-jlayton@kernel.org>

From: Jeff Layton <jlayton@redhat.com>

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.

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

Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 include/linux/iversion.h | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/include/linux/iversion.h b/include/linux/iversion.h
index 858463fca249..3d2fd06495ec 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,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
-- 
2.14.3

  parent reply	other threads:[~2018-01-30 20:32 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-01-30 17:31 [PATCH] iversion: make inode_cmp_iversion{+raw} return bool instead of s64 Jeff Layton
2018-01-30 17:50 ` Trond Myklebust
2018-01-30 19:42   ` Jeff Layton
2018-01-30 20:32 ` Jeff Layton [this message]
     [not found]   ` <20180130203221.29310-1-jlayton-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
2018-01-30 20:53     ` [PATCH v2] " Linus Torvalds
2018-01-31 12:29       ` Jeff Layton
2018-01-31 16:46         ` Linus Torvalds
2018-01-31 17:55           ` Jeff Layton

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180130203221.29310-1-jlayton@kernel.org \
    --to=jlayton@kernel.org \
    --cc=linux-btrfs@vger.kernel.org \
    --cc=linux-ext4@vger.kernel.org \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-integrity@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-nfs@vger.kernel.org \
    --cc=linux-xfs@vger.kernel.org \
    --cc=torvalds@linux-foundation.org \
    --cc=trondmy@primarydata.com \
    --cc=tytso@mit.edu \
    --cc=viro@zeniv.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).