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

Message ID 20180130173126.2806-1-jlayton@kernel.org
State New
Headers show

Commit Message

Jeff Layton Jan. 30, 2018, 5:31 p.m. UTC
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.

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

Comments

Trond Myklebust Jan. 30, 2018, 5:50 p.m. UTC | #1
On Tue, 2018-01-30 at 12:31 -0500, Jeff Layton wrote:
> 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.

> 

> Signed-off-by: Jeff Layton <jlayton@redhat.com>

> ---

>  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
Jeff Layton Jan. 30, 2018, 7:42 p.m. UTC | #2
On Tue, 2018-01-30 at 17:50 +0000, Trond Myklebust wrote:
> On Tue, 2018-01-30 at 12:31 -0500, Jeff Layton wrote:
> > 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.
> > 
> > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > ---
> >  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'?

None at all. I'll send a v2.

Patch
diff mbox

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);
 }
 #endif