From: "Darrick J. Wong" <djwong@kernel.org>
To: Fedor Pchelkin <pchelkin@ispras.ru>
Cc: linux-xfs@vger.kernel.org, aalbersh@kernel.org, cem@kernel.org,
cmaiolino@redhat.com, hch@lst.de, pranav.tyagi03@gmail.com,
sandeen@redhat.com
Subject: Re: [PATCH 3/11] [PATCH] xfs: refactor cmp_two_keys routines to take advantage of cmp_int()
Date: Wed, 8 Oct 2025 13:36:07 -0700 [thread overview]
Message-ID: <20251008203607.GA6215@frogsfrogsfrogs> (raw)
In-Reply-To: <iklfrlwctavbl6yf2jdfs2q4me2gqtzl3rqxttxecicdt4fcyi@5nwin5x7u2tb>
On Wed, Oct 08, 2025 at 06:55:10PM +0200, Fedor Pchelkin wrote:
> Source kernel commit: 3b583adf55c649d5ba37bcd1ca87644b0bc10b86
>
> The net value of these functions is to determine the result of a
> three-way-comparison between operands of the same type.
>
> Simplify the code using cmp_int() to eliminate potential errors with
> opencoded casts and subtractions. This also means we can change the return
> value type of cmp_two_keys routines from int64_t to int and make the
> interface a bit clearer.
>
> Found by Linux Verification Center (linuxtesting.org).
>
> Suggested-by: Darrick J. Wong <djwong@kernel.org>
> Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> Signed-off-by: Carlos Maiolino <cem@kernel.org>
> Signed-off-by: Andrey Albershteyn <aalbersh@kernel.org>
> ---
> include/platform_defs.h | 2 ++
> libxfs/xfs_alloc_btree.c | 21 ++++++++-------------
> libxfs/xfs_bmap_btree.c | 18 +++---------------
> libxfs/xfs_btree.h | 2 +-
> libxfs/xfs_ialloc_btree.c | 6 +++---
> libxfs/xfs_refcount_btree.c | 6 +++---
> libxfs/xfs_rmap_btree.c | 29 ++++++++++++-----------------
> libxfs/xfs_rtrefcount_btree.c | 6 +++---
> libxfs/xfs_rtrmap_btree.c | 29 ++++++++++++-----------------
> repair/rcbag_btree.c | 2 +-
> scrub/inodes.c | 2 --
> 11 files changed, 48 insertions(+), 75 deletions(-)
>
> diff --git a/include/platform_defs.h b/include/platform_defs.h
> index 74a00583eb..fa66551d99 100644
> --- a/include/platform_defs.h
> +++ b/include/platform_defs.h
> @@ -294,4 +294,6 @@
> __a > __b ? (__a - __b) : (__b - __a); \
> })
>
> +#define cmp_int(l, r) ((l > r) - (l < r))
> +
> #endif /* __XFS_PLATFORM_DEFS_H__ */
> diff --git a/libxfs/xfs_alloc_btree.c b/libxfs/xfs_alloc_btree.c
> index 6e7af0020b..c3c69a9de3 100644
> --- a/libxfs/xfs_alloc_btree.c
> +++ b/libxfs/xfs_alloc_btree.c
> @@ -211,7 +211,7 @@
> return (int64_t)be32_to_cpu(kp->ar_startblock) - rec->ar_startblock;
> }
>
> -STATIC int64_t
> +STATIC int
> xfs_bnobt_cmp_two_keys(
> struct xfs_btree_cur *cur,
> const union xfs_btree_key *k1,
> @@ -220,29 +220,24 @@
> {
> ASSERT(!mask || mask->alloc.ar_startblock);
>
> - return (int64_t)be32_to_cpu(k1->alloc.ar_startblock) -
> - be32_to_cpu(k2->alloc.ar_startblock);
> + return cmp_int(be32_to_cpu(k1->alloc.ar_startblock),
> + be32_to_cpu(k2->alloc.ar_startblock));
> }
>
> -STATIC int64_t
> +STATIC int
> xfs_cntbt_cmp_two_keys(
> struct xfs_btree_cur *cur,
> const union xfs_btree_key *k1,
> const union xfs_btree_key *k2,
> const union xfs_btree_key *mask)
> {
> - int64_t diff;
> -
> ASSERT(!mask || (mask->alloc.ar_blockcount &&
> mask->alloc.ar_startblock));
>
> - diff = be32_to_cpu(k1->alloc.ar_blockcount) -
> - be32_to_cpu(k2->alloc.ar_blockcount);
> - if (diff)
> - return diff;
> -
> - return be32_to_cpu(k1->alloc.ar_startblock) -
> - be32_to_cpu(k2->alloc.ar_startblock);
> + return cmp_int(be32_to_cpu(k1->alloc.ar_blockcount),
> + be32_to_cpu(k2->alloc.ar_blockcount)) ?:
> + cmp_int(be32_to_cpu(k1->alloc.ar_startblock),
> + be32_to_cpu(k2->alloc.ar_startblock));
> }
>
> static xfs_failaddr_t
> diff --git a/libxfs/xfs_bmap_btree.c b/libxfs/xfs_bmap_btree.c
> index 3fc23444f3..19eab66fad 100644
> --- a/libxfs/xfs_bmap_btree.c
> +++ b/libxfs/xfs_bmap_btree.c
> @@ -377,29 +377,17 @@
> cur->bc_rec.b.br_startoff;
> }
>
> -STATIC int64_t
> +STATIC int
> xfs_bmbt_cmp_two_keys(
> struct xfs_btree_cur *cur,
> const union xfs_btree_key *k1,
> const union xfs_btree_key *k2,
> const union xfs_btree_key *mask)
> {
> - uint64_t a = be64_to_cpu(k1->bmbt.br_startoff);
> - uint64_t b = be64_to_cpu(k2->bmbt.br_startoff);
> -
> ASSERT(!mask || mask->bmbt.br_startoff);
>
> - /*
> - * Note: This routine previously casted a and b to int64 and subtracted
> - * them to generate a result. This lead to problems if b was the
> - * "maximum" key value (all ones) being signed incorrectly, hence this
> - * somewhat less efficient version.
> - */
> - if (a > b)
> - return 1;
> - if (b > a)
> - return -1;
> - return 0;
> + return cmp_int(be64_to_cpu(k1->bmbt.br_startoff),
> + be64_to_cpu(k2->bmbt.br_startoff));
> }
>
> static xfs_failaddr_t
> diff --git a/libxfs/xfs_btree.h b/libxfs/xfs_btree.h
> index e72a10ba7e..fecd9f0b93 100644
> --- a/libxfs/xfs_btree.h
> +++ b/libxfs/xfs_btree.h
> @@ -184,7 +184,7 @@
> * each key field to be used in the comparison must contain a nonzero
> * value.
> */
> - int64_t (*cmp_two_keys)(struct xfs_btree_cur *cur,
> + int (*cmp_two_keys)(struct xfs_btree_cur *cur,
> const union xfs_btree_key *key1,
> const union xfs_btree_key *key2,
> const union xfs_btree_key *mask);
> diff --git a/libxfs/xfs_ialloc_btree.c b/libxfs/xfs_ialloc_btree.c
> index d56876c5be..973ae62d39 100644
> --- a/libxfs/xfs_ialloc_btree.c
> +++ b/libxfs/xfs_ialloc_btree.c
> @@ -273,7 +273,7 @@
> cur->bc_rec.i.ir_startino;
> }
>
> -STATIC int64_t
> +STATIC int
> xfs_inobt_cmp_two_keys(
> struct xfs_btree_cur *cur,
> const union xfs_btree_key *k1,
> @@ -282,8 +282,8 @@
> {
> ASSERT(!mask || mask->inobt.ir_startino);
>
> - return (int64_t)be32_to_cpu(k1->inobt.ir_startino) -
> - be32_to_cpu(k2->inobt.ir_startino);
> + return cmp_int(be32_to_cpu(k1->inobt.ir_startino),
> + be32_to_cpu(k2->inobt.ir_startino));
> }
>
> static xfs_failaddr_t
> diff --git a/libxfs/xfs_refcount_btree.c b/libxfs/xfs_refcount_btree.c
> index 0924ab7eb7..668c788dca 100644
> --- a/libxfs/xfs_refcount_btree.c
> +++ b/libxfs/xfs_refcount_btree.c
> @@ -187,7 +187,7 @@
> return (int64_t)be32_to_cpu(kp->rc_startblock) - start;
> }
>
> -STATIC int64_t
> +STATIC int
> xfs_refcountbt_cmp_two_keys(
> struct xfs_btree_cur *cur,
> const union xfs_btree_key *k1,
> @@ -196,8 +196,8 @@
> {
> ASSERT(!mask || mask->refc.rc_startblock);
>
> - return (int64_t)be32_to_cpu(k1->refc.rc_startblock) -
> - be32_to_cpu(k2->refc.rc_startblock);
> + return cmp_int(be32_to_cpu(k1->refc.rc_startblock),
> + be32_to_cpu(k2->refc.rc_startblock));
> }
>
> STATIC xfs_failaddr_t
> diff --git a/libxfs/xfs_rmap_btree.c b/libxfs/xfs_rmap_btree.c
> index ea946616bf..ab207b9cc2 100644
> --- a/libxfs/xfs_rmap_btree.c
> +++ b/libxfs/xfs_rmap_btree.c
> @@ -272,7 +272,7 @@
> return 0;
> }
>
> -STATIC int64_t
> +STATIC int
> xfs_rmapbt_cmp_two_keys(
> struct xfs_btree_cur *cur,
> const union xfs_btree_key *k1,
> @@ -281,36 +281,31 @@
> {
> const struct xfs_rmap_key *kp1 = &k1->rmap;
> const struct xfs_rmap_key *kp2 = &k2->rmap;
> - int64_t d;
> - __u64 x, y;
> + int d;
>
> /* Doesn't make sense to mask off the physical space part */
> ASSERT(!mask || mask->rmap.rm_startblock);
>
> - d = (int64_t)be32_to_cpu(kp1->rm_startblock) -
> - be32_to_cpu(kp2->rm_startblock);
> + d = cmp_int(be32_to_cpu(kp1->rm_startblock),
> + be32_to_cpu(kp2->rm_startblock));
> if (d)
> return d;
>
> if (!mask || mask->rmap.rm_owner) {
> - x = be64_to_cpu(kp1->rm_owner);
> - y = be64_to_cpu(kp2->rm_owner);
> - if (x > y)
> - return 1;
> - else if (y > x)
> - return -1;
> + d = cmp_int(be64_to_cpu(kp1->rm_owner),
> + be64_to_cpu(kp2->rm_owner));
> + if (d)
> + return d;
> }
>
> if (!mask || mask->rmap.rm_offset) {
> /* Doesn't make sense to allow offset but not owner */
> ASSERT(!mask || mask->rmap.rm_owner);
>
> - x = offset_keymask(be64_to_cpu(kp1->rm_offset));
> - y = offset_keymask(be64_to_cpu(kp2->rm_offset));
> - if (x > y)
> - return 1;
> - else if (y > x)
> - return -1;
> + d = cmp_int(offset_keymask(be64_to_cpu(kp1->rm_offset)),
> + offset_keymask(be64_to_cpu(kp2->rm_offset)));
> + if (d)
> + return d;
> }
>
> return 0;
> diff --git a/libxfs/xfs_rtrefcount_btree.c b/libxfs/xfs_rtrefcount_btree.c
> index 7a4eec49ca..7fbbc6387c 100644
> --- a/libxfs/xfs_rtrefcount_btree.c
> +++ b/libxfs/xfs_rtrefcount_btree.c
> @@ -168,7 +168,7 @@
> return (int64_t)be32_to_cpu(kp->rc_startblock) - start;
> }
>
> -STATIC int64_t
> +STATIC int
> xfs_rtrefcountbt_cmp_two_keys(
> struct xfs_btree_cur *cur,
> const union xfs_btree_key *k1,
> @@ -177,8 +177,8 @@
> {
> ASSERT(!mask || mask->refc.rc_startblock);
>
> - return (int64_t)be32_to_cpu(k1->refc.rc_startblock) -
> - be32_to_cpu(k2->refc.rc_startblock);
> + return cmp_int(be32_to_cpu(k1->refc.rc_startblock),
> + be32_to_cpu(k2->refc.rc_startblock));
> }
>
> static xfs_failaddr_t
> diff --git a/libxfs/xfs_rtrmap_btree.c b/libxfs/xfs_rtrmap_btree.c
> index 59a99bb42c..0492cd55d5 100644
> --- a/libxfs/xfs_rtrmap_btree.c
> +++ b/libxfs/xfs_rtrmap_btree.c
> @@ -214,7 +214,7 @@
> return 0;
> }
>
> -STATIC int64_t
> +STATIC int
> xfs_rtrmapbt_cmp_two_keys(
> struct xfs_btree_cur *cur,
> const union xfs_btree_key *k1,
> @@ -223,36 +223,31 @@
> {
> const struct xfs_rmap_key *kp1 = &k1->rmap;
> const struct xfs_rmap_key *kp2 = &k2->rmap;
> - int64_t d;
> - __u64 x, y;
> + int d;
>
> /* Doesn't make sense to mask off the physical space part */
> ASSERT(!mask || mask->rmap.rm_startblock);
>
> - d = (int64_t)be32_to_cpu(kp1->rm_startblock) -
> - be32_to_cpu(kp2->rm_startblock);
> + d = cmp_int(be32_to_cpu(kp1->rm_startblock),
> + be32_to_cpu(kp2->rm_startblock));
> if (d)
> return d;
>
> if (!mask || mask->rmap.rm_owner) {
> - x = be64_to_cpu(kp1->rm_owner);
> - y = be64_to_cpu(kp2->rm_owner);
> - if (x > y)
> - return 1;
> - else if (y > x)
> - return -1;
> + d = cmp_int(be64_to_cpu(kp1->rm_owner),
> + be64_to_cpu(kp2->rm_owner));
> + if (d)
> + return d;
> }
>
> if (!mask || mask->rmap.rm_offset) {
> /* Doesn't make sense to allow offset but not owner */
> ASSERT(!mask || mask->rmap.rm_owner);
>
> - x = offset_keymask(be64_to_cpu(kp1->rm_offset));
> - y = offset_keymask(be64_to_cpu(kp2->rm_offset));
> - if (x > y)
> - return 1;
> - else if (y > x)
> - return -1;
> + d = cmp_int(offset_keymask(be64_to_cpu(kp1->rm_offset)),
> + offset_keymask(be64_to_cpu(kp2->rm_offset)));
> + if (d)
> + return d;
> }
>
> return 0;
> diff --git a/repair/rcbag_btree.c b/repair/rcbag_btree.c
> index 704e66e9fb..c42a2ca0d1 100644
> --- a/repair/rcbag_btree.c
> +++ b/repair/rcbag_btree.c
> @@ -72,7 +72,7 @@
> return 0;
> }
>
> -STATIC int64_t
> +STATIC int
> rcbagbt_cmp_two_keys(
> struct xfs_btree_cur *cur,
> const union xfs_btree_key *k1,
Dumb nit: You could port this one too:
@@ -81,25 +81,19 @@ rcbagbt_cmp_two_keys(
{
const struct rcbag_key *kp1 = (const struct rcbag_key *)k1;
const struct rcbag_key *kp2 = (const struct rcbag_key *)k2;
+ int d;
ASSERT(mask == NULL);
- if (kp1->rbg_startblock > kp2->rbg_startblock)
- return 1;
- if (kp1->rbg_startblock < kp2->rbg_startblock)
- return -1;
-
- if (kp1->rbg_blockcount > kp2->rbg_blockcount)
- return 1;
- if (kp1->rbg_blockcount < kp2->rbg_blockcount)
- return -1;
+ d = cmp_int(kp1->rbg_startblock, kp2->rbg_startblock);
+ if (d)
+ return d;
- if (kp1->rbg_ino > kp2->rbg_ino)
- return 1;
- if (kp1->rbg_ino < kp2->rbg_ino)
- return -1;
+ d = cmp_int(kp1->rbg_blockcount, kp2->rbg_blockcount);
+ if (d)
+ return d;
- return 0;
+ return cmp_int(kp1->rbg_ino, kp2->rbg_ino);
}
STATIC int
--D
> diff --git a/scrub/inodes.c b/scrub/inodes.c
> index 2f3c87be79..4ed7cd9963 100644
> --- a/scrub/inodes.c
> +++ b/scrub/inodes.c
> @@ -197,8 +197,6 @@
> return seen_mask;
> }
>
> -#define cmp_int(l, r) ((l > r) - (l < r))
> -
> /* Compare two bulkstat records by inumber. */
> static int
> compare_bstat(
next prev parent reply other threads:[~2025-10-08 20:36 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-10-08 16:41 [PATCH 0/12] xfsprogs: libxfs sync v6.17 Andrey Albershteyn
2025-10-08 16:53 ` [PATCH 1/11] [PATCH] xfs: rename diff_two_keys routines Fedor Pchelkin
2025-10-08 16:54 ` [PATCH 2/11] [PATCH] xfs: rename key_diff routines Fedor Pchelkin
2025-10-08 16:55 ` [PATCH 3/11] [PATCH] xfs: refactor cmp_two_keys routines to take advantage of cmp_int() Fedor Pchelkin
2025-10-08 20:36 ` Darrick J. Wong [this message]
2025-10-08 16:55 ` [PATCH 4/11] [PATCH] xfs: refactor cmp_key_with_cur " Fedor Pchelkin
2025-10-08 20:36 ` Darrick J. Wong
2025-10-08 16:55 ` [PATCH 5/11] [PATCH] xfs: use a proper variable name and type for storing a comparison result Fedor Pchelkin
2025-10-08 16:55 ` [PATCH 6/11] [PATCH] xfs: refactor xfs_btree_diff_two_ptrs() to take advantage of cmp_int() Fedor Pchelkin
2025-10-08 16:56 ` [PATCH 7/11] [PATCH] xfs: return the allocated transaction from xfs_trans_alloc_empty Christoph Hellwig
2025-10-08 16:56 ` [PATCH 8/11] [PATCH] xfs: improve the xg_active_ref check in xfs_group_free Christoph Hellwig
2025-10-08 16:56 ` [PATCH 9/11] [PATCH] fs/xfs: replace strncpy with memtostr_pad() Pranav Tyagi
2025-10-08 16:56 ` [PATCH 10/11] [PATCH] xfs: don't use a xfs_log_iovec for ri_buf in log recovery Christoph Hellwig
2025-10-08 16:57 ` [PATCH 11/11] [PATCH] xfs: do not propagate ENODATA disk errors into xattr code Eric Sandeen
2025-10-08 20:39 ` [PATCH 0/12] xfsprogs: libxfs sync v6.17 Darrick J. Wong
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=20251008203607.GA6215@frogsfrogsfrogs \
--to=djwong@kernel.org \
--cc=aalbersh@kernel.org \
--cc=cem@kernel.org \
--cc=cmaiolino@redhat.com \
--cc=hch@lst.de \
--cc=linux-xfs@vger.kernel.org \
--cc=pchelkin@ispras.ru \
--cc=pranav.tyagi03@gmail.com \
--cc=sandeen@redhat.com \
/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).