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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> > 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.
--
Jeff Layton <jlayton@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
