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(-)
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
--
2.14.3
On Tue, 2018-01-30 at 12:31 -0500, Jeff Layton wrote:
> 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(-)
>
> 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
[email protected]
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 <[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(-)
> >
> > 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 <[email protected]>
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.
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(-)
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
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
On Tue, 2018-01-30 at 12:53 -0800, Linus Torvalds wrote:
> 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,
--
Jeff Layton <[email protected]>
On Wed, Jan 31, 2018 at 4:29 AM, Jeff Layton <[email protected]> wrote:
>
> 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
On Wed, 2018-01-31 at 08:46 -0800, Linus Torvalds wrote:
> On Wed, Jan 31, 2018 at 4:29 AM, Jeff Layton <[email protected]> wrote:
> >
> > 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).
> 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,
--
Jeff Layton <[email protected]>