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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.