From: Jeff Layton <jlayton@kernel.org> Date: 2018-01-30 17:31:29
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(-)
On Tue, 2018-01-30 at 12:31 -0500, Jeff Layton wrote:
quoted 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(-)
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;
}
/**
*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
From: Jeff Layton <jlayton@kernel.org> Date: 2018-01-30 19:42:59
On Tue, 2018-01-30 at 17:50 +0000, Trond Myklebust wrote:
On Tue, 2018-01-30 at 12:31 -0500, Jeff Layton wrote:
quoted
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(-)
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;
}
/**
*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 [off-list ref]
From: Jeff Layton <jlayton@kernel.org> Date: 2018-01-30 20:32:25
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.
With this change we can also reimplement inode_cmp_iversion in a simpler
way using inode_peek_iversion.
Signed-off-by: Jeff Layton <redacted>
---
include/linux/iversion.h | 20 ++++++++------------
1 file changed, 8 insertions(+), 12 deletions(-)
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
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Jeff Layton <jlayton@kernel.org> Date: 2018-01-31 12:29:27
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 [off-list ref]
On Wed, Jan 31, 2018 at 4:29 AM, Jeff Layton [off-list ref] 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
From: Jeff Layton <jlayton@kernel.org> Date: 2018-01-31 17:55:30
On Wed, 2018-01-31 at 08:46 -0800, Linus Torvalds wrote:
On Wed, Jan 31, 2018 at 4:29 AM, Jeff Layton [off-list ref] wrote:
quoted
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 [off-list ref]