[PATCH 0/5] xfs: sparse warning fixes

[PATCH 0/5] xfs: sparse warning fixes

[Dave Chinner]

In chasing down a recent regression, I realised I wasn't running
sparse regularly on my "fs/xfs-only" kernel builds, and so I missed
a warning that would have prevented a bug from slipping through.

I've modified my build scripts to always run sparse when I do these
delta builds as I develop code, and I found a few other warnings
that needed to be addressed. No bugs were uncovered - they are
simply modifications and/or annotations to the code to address the
warnings and make them go away.

There remains a couple of sparse warnings in the transaction
reservation calculations - sparse throws "too many tokens" warnings
from the max(t4, max3(t1, t2, t3)) calculations. These are sparse
code parser issues, not a code problem, so I have ignored them for
now.

[PATCH 1/5] xfs: fix CIL sparse lock context warnings

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

Sparse reports:

fs/xfs/xfs_log_cil.c:1127:1: warning: context imbalance in 'xlog_cil_push_work' - different lock contexts for basic block
fs/xfs/xfs_log_cil.c:1380:1: warning: context imbalance in 'xlog_cil_push_background' - wrong count at exit
fs/xfs/xfs_log_cil.c:1623:9: warning: context imbalance in 'xlog_cil_commit' - unexpected unlock

xlog_cil_push_background() has a locking annotations for an rw_sem.
Sparse does not track lock contexts for rw_sems, so the
annotation generates false warnings. Remove the annotation.

xlog_wait_on_iclog() drops the log->l_ic_loglock. The function has a
sparse annotation, but the prototype in xfs_log_priv.h does not.
Hence the warning from xlog_cil_push_work() which calls
xlog_wait_on_iclog(). Add the missing annotation.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_log_cil.c  | 2 +-
 fs/xfs/xfs_log_priv.h | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
index 73f5b7f628f4..f51cbc6405c1 100644
--- a/fs/xfs/xfs_log_cil.c
+++ b/fs/xfs/xfs_log_cil.c
@@ -1378,7 +1378,7 @@ xlog_cil_push_work(
  */
 static void
 xlog_cil_push_background(
-	struct xlog	*log) __releases(cil->xc_ctx_lock)
+	struct xlog	*log)
 {
 	struct xfs_cil	*cil = log->l_cilp;
 	int		space_used = atomic_read(&cil->xc_ctx->space_used);
diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
index e30c06ec20e3..c39956c41374 100644
--- a/fs/xfs/xfs_log_priv.h
+++ b/fs/xfs/xfs_log_priv.h
@@ -623,7 +623,8 @@ xlog_wait(
 	remove_wait_queue(wq, &wait);
 }
 
-int xlog_wait_on_iclog(struct xlog_in_core *iclog);
+int xlog_wait_on_iclog(struct xlog_in_core *iclog)
+		__releases(iclog->ic_log->l_icloglock);
 
 /*
  * The LSN is valid so long as it is behind the current LSN. If it isn't, this
-- 
2.43.0

[PATCH 2/5] xfs: fix sparse warning in xfs_extent_busy_clear

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

Sparse reports:

fs/xfs/xfs_extent_busy.c:588:17: warning: context imbalance in 'xfs_extent_busy_clear' - unexpected unlock

But there is no locking bug here. Sparse simply doesn't understand
the logic and locking in the busy extent processing loop.
xfs_extent_busy_put_pag() has an annotation to suppresses an
unexpected unlock warning, but that isn't sufficient.

If we move the pag existence check into xfs_extent_busy_put_pag() and
annotate that with a __release() so that this function always
appears to release the pag->pagb_lock, sparse now thinks the loop
locking is balanced (one unlock, one lock per loop) but still throws
an unexpected unlock warning after loop cleanup.

i.e. it does not understand that we enter the loop without any locks
held and exit it with the last lock still held. Whilst the locking
within the loop is inow balanced, we need to add an __acquire() to
xfs_extent_busy_clear() to set the initial lock context needed to
avoid false warnings.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_extent_busy.c | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/fs/xfs/xfs_extent_busy.c b/fs/xfs/xfs_extent_busy.c
index 56cfa1498571..686b67372030 100644
--- a/fs/xfs/xfs_extent_busy.c
+++ b/fs/xfs/xfs_extent_busy.c
@@ -534,12 +534,24 @@ xfs_extent_busy_clear_one(
 	kfree(busyp);
 }
 
+/*
+ * Sparse has real trouble with the structure of xfs_extent_busy_clear() and it
+ * is impossible to annotate it correctly if we leave the 'if (pag)' conditional
+ * in xfs_extent_busy_clear(). Hence we always "release" the lock in
+ * xfs_extent_busy_put_pag() so sparse only ever sees one possible path to
+ * drop the lock.
+ */
 static void
 xfs_extent_busy_put_pag(
 	struct xfs_perag	*pag,
 	bool			wakeup)
 		__releases(pag->pagb_lock)
 {
+	if (!pag) {
+		__release(pag->pagb_lock);
+		return;
+	}
+
 	if (wakeup) {
 		pag->pagb_gen++;
 		wake_up_all(&pag->pagb_wait);
@@ -565,10 +577,18 @@ xfs_extent_busy_clear(
 	xfs_agnumber_t		agno = NULLAGNUMBER;
 	bool			wakeup = false;
 
+	/*
+	 * Sparse thinks the locking in the loop below is balanced (one unlock,
+	 * one lock per loop iteration) and doesn't understand that we enter
+	 * with no lock held and exit with a lock held. Hence we need to
+	 * "acquire" the lock to create the correct initial condition for the
+	 * cleanup after loop termination to avoid an unexpected unlock warning.
+	 */
+	__acquire(pag->pagb_lock);
+
 	list_for_each_entry_safe(busyp, n, list, list) {
 		if (busyp->agno != agno) {
-			if (pag)
-				xfs_extent_busy_put_pag(pag, wakeup);
+			xfs_extent_busy_put_pag(pag, wakeup);
 			agno = busyp->agno;
 			pag = xfs_perag_get(mp, agno);
 			spin_lock(&pag->pagb_lock);
@@ -584,8 +604,7 @@ xfs_extent_busy_clear(
 		}
 	}
 
-	if (pag)
-		xfs_extent_busy_put_pag(pag, wakeup);
+	xfs_extent_busy_put_pag(pag, wakeup);
 }
 
 /*
-- 
2.43.0

[PATCH 3/5] xfs: silence sparse warning when checking version number

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

Scrub checks the superblock version number against the known good
feature bits that can be set in the version mask. It calculates
the version mask to compare like so:

	vernum_mask = cpu_to_be16(~XFS_SB_VERSION_OKBITS |
                                  XFS_SB_VERSION_NUMBITS |
                                  XFS_SB_VERSION_ALIGNBIT |
                                  XFS_SB_VERSION_DALIGNBIT |
                                  XFS_SB_VERSION_SHAREDBIT |
                                  XFS_SB_VERSION_LOGV2BIT |
                                  XFS_SB_VERSION_SECTORBIT |
                                  XFS_SB_VERSION_EXTFLGBIT |
                                  XFS_SB_VERSION_DIRV2BIT);

This generates a sparse warning:

fs/xfs/scrub/agheader.c:168:23: warning: cast truncates bits from constant value (ffff3f8f becomes 3f8f)

This is because '~XFS_SB_VERSION_OKBITS' is considered a 32 bit
constant, even though it's value is always under 16 bits.

This is a kinda silly thing to do, because:

/*
 * Supported feature bit list is just all bits in the versionnum field because
 * we've used them all up and understand them all. Except, of course, for the
 * shared superblock bit, which nobody knows what it does and so is unsupported.
 */
#define XFS_SB_VERSION_OKBITS           \
        ((XFS_SB_VERSION_NUMBITS | XFS_SB_VERSION_ALLFBITS) & \
                ~XFS_SB_VERSION_SHAREDBIT)

#define XFS_SB_VERSION_NUMBITS          0x000f
#define XFS_SB_VERSION_ALLFBITS         0xfff0
#define XFS_SB_VERSION_SHAREDBIT        0x0200


XFS_SB_VERSION_OKBITS has a value of 0xfdff, and so
~XFS_SB_VERSION_OKBITS == XFS_SB_VERSION_SHAREDBIT.  The calculated
mask already sets XFS_SB_VERSION_SHAREDBIT, so starting with
~XFS_SB_VERSION_OKBITS is completely redundant....

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/scrub/agheader.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/xfs/scrub/agheader.c b/fs/xfs/scrub/agheader.c
index e954f07679dd..d6a1a9fc63c9 100644
--- a/fs/xfs/scrub/agheader.c
+++ b/fs/xfs/scrub/agheader.c
@@ -165,8 +165,7 @@ xchk_superblock(
 		xchk_block_set_corrupt(sc, bp);
 
 	/* Check sb_versionnum bits that are set at mkfs time. */
-	vernum_mask = cpu_to_be16(~XFS_SB_VERSION_OKBITS |
-				  XFS_SB_VERSION_NUMBITS |
+	vernum_mask = cpu_to_be16(XFS_SB_VERSION_NUMBITS |
 				  XFS_SB_VERSION_ALIGNBIT |
 				  XFS_SB_VERSION_DALIGNBIT |
 				  XFS_SB_VERSION_SHAREDBIT |
-- 
2.43.0

[PATCH 4/5] xfs: remove unused is_rt_data_fork() function

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

Sparse warns that is_rt_data_fork() is unused. Indeed, it is a
static inline function that isn't used in the file it is defined in.
It looks like xfs_ifork_is_realtime() has superceded this function,
so remove it and get rid of the sparse warning.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/scrub/rmap_repair.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/fs/xfs/scrub/rmap_repair.c b/fs/xfs/scrub/rmap_repair.c
index e8e07b683eab..7e73ddfb3d44 100644
--- a/fs/xfs/scrub/rmap_repair.c
+++ b/fs/xfs/scrub/rmap_repair.c
@@ -432,14 +432,6 @@ xrep_rmap_scan_iroot_btree(
 	return error;
 }
 
-static inline bool
-is_rt_data_fork(
-	struct xfs_inode	*ip,
-	int			whichfork)
-{
-	return XFS_IS_REALTIME_INODE(ip) && whichfork == XFS_DATA_FORK;
-}
-
 /*
  * Iterate the block mapping btree to collect rmap records for anything in this
  * fork that matches the AG.  Sets @mappings_done to true if we've scanned the
-- 
2.43.0

[PATCH 5/5] xfs: fix sparse warnings about unused interval tree functions

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

Sparse throws warnings about the interval tree functions that are
defined and then not used in the scrub bitmap code:

fs/xfs/scrub/bitmap.c:57:1: warning: unused function 'xbitmap64_tree_iter_next' [-Wunused-function]
INTERVAL_TREE_DEFINE(struct xbitmap64_node, bn_rbnode, uint64_t,
^
./include/linux/interval_tree_generic.h:151:33: note: expanded from macro 'INTERVAL_TREE_DEFINE'
ITSTATIC ITSTRUCT *                                                           \
                                                                              ^
<scratch space>:3:1: note: expanded from here
xbitmap64_tree_iter_next
^
fs/xfs/scrub/bitmap.c:331:1: warning: unused function 'xbitmap32_tree_iter_next' [-Wunused-function]
INTERVAL_TREE_DEFINE(struct xbitmap32_node, bn_rbnode, uint32_t,
^
./include/linux/interval_tree_generic.h:151:33: note: expanded from macro 'INTERVAL_TREE_DEFINE'
ITSTATIC ITSTRUCT *                                                           \
                                                                              ^
<scratch space>:59:1: note: expanded from here
xbitmap32_tree_iter_next

Fix these by marking the functions created by the interval tree
creation macro as __maybe_unused to suppress this warning.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/scrub/bitmap.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/fs/xfs/scrub/bitmap.c b/fs/xfs/scrub/bitmap.c
index 0cb8d43912a8..7ba35a7a7920 100644
--- a/fs/xfs/scrub/bitmap.c
+++ b/fs/xfs/scrub/bitmap.c
@@ -40,22 +40,23 @@ struct xbitmap64_node {
  * These functions are defined by the INTERVAL_TREE_DEFINE macro, but we'll
  * forward-declare them anyway for clarity.
  */
-static inline void
+static inline __maybe_unused void
 xbitmap64_tree_insert(struct xbitmap64_node *node, struct rb_root_cached *root);
 
-static inline void
+static inline __maybe_unused void
 xbitmap64_tree_remove(struct xbitmap64_node *node, struct rb_root_cached *root);
 
-static inline struct xbitmap64_node *
+static inline __maybe_unused struct xbitmap64_node *
 xbitmap64_tree_iter_first(struct rb_root_cached *root, uint64_t start,
 			uint64_t last);
 
-static inline struct xbitmap64_node *
+static inline __maybe_unused struct xbitmap64_node *
 xbitmap64_tree_iter_next(struct xbitmap64_node *node, uint64_t start,
 		       uint64_t last);
 
 INTERVAL_TREE_DEFINE(struct xbitmap64_node, bn_rbnode, uint64_t,
-		__bn_subtree_last, START, LAST, static inline, xbitmap64_tree)
+		__bn_subtree_last, START, LAST, static inline __maybe_unused,
+		xbitmap64_tree)
 
 /* Iterate each interval of a bitmap.  Do not change the bitmap. */
 #define for_each_xbitmap64_extent(bn, bitmap) \
@@ -314,22 +315,23 @@ struct xbitmap32_node {
  * These functions are defined by the INTERVAL_TREE_DEFINE macro, but we'll
  * forward-declare them anyway for clarity.
  */
-static inline void
+static inline __maybe_unused void
 xbitmap32_tree_insert(struct xbitmap32_node *node, struct rb_root_cached *root);
 
-static inline void
+static inline __maybe_unused void
 xbitmap32_tree_remove(struct xbitmap32_node *node, struct rb_root_cached *root);
 
-static inline struct xbitmap32_node *
+static inline __maybe_unused struct xbitmap32_node *
 xbitmap32_tree_iter_first(struct rb_root_cached *root, uint32_t start,
 			  uint32_t last);
 
-static inline struct xbitmap32_node *
+static inline __maybe_unused struct xbitmap32_node *
 xbitmap32_tree_iter_next(struct xbitmap32_node *node, uint32_t start,
 			 uint32_t last);
 
 INTERVAL_TREE_DEFINE(struct xbitmap32_node, bn_rbnode, uint32_t,
-		__bn_subtree_last, START, LAST, static inline, xbitmap32_tree)
+		__bn_subtree_last, START, LAST, static inline __maybe_unused,
+		xbitmap32_tree)
 
 /* Iterate each interval of a bitmap.  Do not change the bitmap. */
 #define for_each_xbitmap32_extent(bn, bitmap) \
-- 
2.43.0

Re: [PATCH 1/5] xfs: fix CIL sparse lock context warnings

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 5/5] xfs: fix sparse warnings about unused interval tree functions

[Darrick J. Wong]

On Wed, Apr 03, 2024 at 08:28:32AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Sparse throws warnings about the interval tree functions that are
> defined and then not used in the scrub bitmap code:
> 
> fs/xfs/scrub/bitmap.c:57:1: warning: unused function 'xbitmap64_tree_iter_next' [-Wunused-function]
> INTERVAL_TREE_DEFINE(struct xbitmap64_node, bn_rbnode, uint64_t,
> ^
> ./include/linux/interval_tree_generic.h:151:33: note: expanded from macro 'INTERVAL_TREE_DEFINE'
> ITSTATIC ITSTRUCT *                                                           \
>                                                                               ^
> <scratch space>:3:1: note: expanded from here
> xbitmap64_tree_iter_next
> ^
> fs/xfs/scrub/bitmap.c:331:1: warning: unused function 'xbitmap32_tree_iter_next' [-Wunused-function]
> INTERVAL_TREE_DEFINE(struct xbitmap32_node, bn_rbnode, uint32_t,
> ^
> ./include/linux/interval_tree_generic.h:151:33: note: expanded from macro 'INTERVAL_TREE_DEFINE'
> ITSTATIC ITSTRUCT *                                                           \
>                                                                               ^
> <scratch space>:59:1: note: expanded from here
> xbitmap32_tree_iter_next
> 
> Fix these by marking the functions created by the interval tree
> creation macro as __maybe_unused to suppress this warning.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Much better than the hatchet job sent in by the last person...
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/scrub/bitmap.c | 22 ++++++++++++----------
>  1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/xfs/scrub/bitmap.c b/fs/xfs/scrub/bitmap.c
> index 0cb8d43912a8..7ba35a7a7920 100644
> --- a/fs/xfs/scrub/bitmap.c
> +++ b/fs/xfs/scrub/bitmap.c
> @@ -40,22 +40,23 @@ struct xbitmap64_node {
>   * These functions are defined by the INTERVAL_TREE_DEFINE macro, but we'll
>   * forward-declare them anyway for clarity.
>   */
> -static inline void
> +static inline __maybe_unused void
>  xbitmap64_tree_insert(struct xbitmap64_node *node, struct rb_root_cached *root);
>  
> -static inline void
> +static inline __maybe_unused void
>  xbitmap64_tree_remove(struct xbitmap64_node *node, struct rb_root_cached *root);
>  
> -static inline struct xbitmap64_node *
> +static inline __maybe_unused struct xbitmap64_node *
>  xbitmap64_tree_iter_first(struct rb_root_cached *root, uint64_t start,
>  			uint64_t last);
>  
> -static inline struct xbitmap64_node *
> +static inline __maybe_unused struct xbitmap64_node *
>  xbitmap64_tree_iter_next(struct xbitmap64_node *node, uint64_t start,
>  		       uint64_t last);
>  
>  INTERVAL_TREE_DEFINE(struct xbitmap64_node, bn_rbnode, uint64_t,
> -		__bn_subtree_last, START, LAST, static inline, xbitmap64_tree)
> +		__bn_subtree_last, START, LAST, static inline __maybe_unused,
> +		xbitmap64_tree)
>  
>  /* Iterate each interval of a bitmap.  Do not change the bitmap. */
>  #define for_each_xbitmap64_extent(bn, bitmap) \
> @@ -314,22 +315,23 @@ struct xbitmap32_node {
>   * These functions are defined by the INTERVAL_TREE_DEFINE macro, but we'll
>   * forward-declare them anyway for clarity.
>   */
> -static inline void
> +static inline __maybe_unused void
>  xbitmap32_tree_insert(struct xbitmap32_node *node, struct rb_root_cached *root);
>  
> -static inline void
> +static inline __maybe_unused void
>  xbitmap32_tree_remove(struct xbitmap32_node *node, struct rb_root_cached *root);
>  
> -static inline struct xbitmap32_node *
> +static inline __maybe_unused struct xbitmap32_node *
>  xbitmap32_tree_iter_first(struct rb_root_cached *root, uint32_t start,
>  			  uint32_t last);
>  
> -static inline struct xbitmap32_node *
> +static inline __maybe_unused struct xbitmap32_node *
>  xbitmap32_tree_iter_next(struct xbitmap32_node *node, uint32_t start,
>  			 uint32_t last);
>  
>  INTERVAL_TREE_DEFINE(struct xbitmap32_node, bn_rbnode, uint32_t,
> -		__bn_subtree_last, START, LAST, static inline, xbitmap32_tree)
> +		__bn_subtree_last, START, LAST, static inline __maybe_unused,
> +		xbitmap32_tree)
>  
>  /* Iterate each interval of a bitmap.  Do not change the bitmap. */
>  #define for_each_xbitmap32_extent(bn, bitmap) \
> -- 
> 2.43.0
> 
> 

Re: [PATCH 4/5] xfs: remove unused is_rt_data_fork() function

[Darrick J. Wong]

On Wed, Apr 03, 2024 at 08:28:31AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Sparse warns that is_rt_data_fork() is unused. Indeed, it is a
> static inline function that isn't used in the file it is defined in.
> It looks like xfs_ifork_is_realtime() has superceded this function,
> so remove it and get rid of the sparse warning.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/scrub/rmap_repair.c | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/fs/xfs/scrub/rmap_repair.c b/fs/xfs/scrub/rmap_repair.c
> index e8e07b683eab..7e73ddfb3d44 100644
> --- a/fs/xfs/scrub/rmap_repair.c
> +++ b/fs/xfs/scrub/rmap_repair.c
> @@ -432,14 +432,6 @@ xrep_rmap_scan_iroot_btree(
>  	return error;
>  }
>  
> -static inline bool
> -is_rt_data_fork(
> -	struct xfs_inode	*ip,
> -	int			whichfork)
> -{
> -	return XFS_IS_REALTIME_INODE(ip) && whichfork == XFS_DATA_FORK;
> -}
> -
>  /*
>   * Iterate the block mapping btree to collect rmap records for anything in this
>   * fork that matches the AG.  Sets @mappings_done to true if we've scanned the
> -- 
> 2.43.0
> 
> 

Re: [PATCH 3/5] xfs: silence sparse warning when checking version number

[Darrick J. Wong]

On Wed, Apr 03, 2024 at 08:28:30AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Scrub checks the superblock version number against the known good
> feature bits that can be set in the version mask. It calculates
> the version mask to compare like so:
> 
> 	vernum_mask = cpu_to_be16(~XFS_SB_VERSION_OKBITS |
>                                   XFS_SB_VERSION_NUMBITS |
>                                   XFS_SB_VERSION_ALIGNBIT |
>                                   XFS_SB_VERSION_DALIGNBIT |
>                                   XFS_SB_VERSION_SHAREDBIT |
>                                   XFS_SB_VERSION_LOGV2BIT |
>                                   XFS_SB_VERSION_SECTORBIT |
>                                   XFS_SB_VERSION_EXTFLGBIT |
>                                   XFS_SB_VERSION_DIRV2BIT);
> 
> This generates a sparse warning:
> 
> fs/xfs/scrub/agheader.c:168:23: warning: cast truncates bits from constant value (ffff3f8f becomes 3f8f)
> 
> This is because '~XFS_SB_VERSION_OKBITS' is considered a 32 bit
> constant, even though it's value is always under 16 bits.
> 
> This is a kinda silly thing to do, because:
> 
> /*
>  * Supported feature bit list is just all bits in the versionnum field because
>  * we've used them all up and understand them all. Except, of course, for the
>  * shared superblock bit, which nobody knows what it does and so is unsupported.
>  */
> #define XFS_SB_VERSION_OKBITS           \
>         ((XFS_SB_VERSION_NUMBITS | XFS_SB_VERSION_ALLFBITS) & \
>                 ~XFS_SB_VERSION_SHAREDBIT)
> 
> #define XFS_SB_VERSION_NUMBITS          0x000f
> #define XFS_SB_VERSION_ALLFBITS         0xfff0
> #define XFS_SB_VERSION_SHAREDBIT        0x0200
> 
> 
> XFS_SB_VERSION_OKBITS has a value of 0xfdff, and so
> ~XFS_SB_VERSION_OKBITS == XFS_SB_VERSION_SHAREDBIT.  The calculated
> mask already sets XFS_SB_VERSION_SHAREDBIT, so starting with
> ~XFS_SB_VERSION_OKBITS is completely redundant....
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

What a tongue twister!
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/scrub/agheader.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/scrub/agheader.c b/fs/xfs/scrub/agheader.c
> index e954f07679dd..d6a1a9fc63c9 100644
> --- a/fs/xfs/scrub/agheader.c
> +++ b/fs/xfs/scrub/agheader.c
> @@ -165,8 +165,7 @@ xchk_superblock(
>  		xchk_block_set_corrupt(sc, bp);
>  
>  	/* Check sb_versionnum bits that are set at mkfs time. */
> -	vernum_mask = cpu_to_be16(~XFS_SB_VERSION_OKBITS |
> -				  XFS_SB_VERSION_NUMBITS |
> +	vernum_mask = cpu_to_be16(XFS_SB_VERSION_NUMBITS |
>  				  XFS_SB_VERSION_ALIGNBIT |
>  				  XFS_SB_VERSION_DALIGNBIT |
>  				  XFS_SB_VERSION_SHAREDBIT |
> -- 
> 2.43.0
> 
> 

Re: [PATCH 2/5] xfs: fix sparse warning in xfs_extent_busy_clear

[Darrick J. Wong]

On Wed, Apr 03, 2024 at 08:28:29AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Sparse reports:
> 
> fs/xfs/xfs_extent_busy.c:588:17: warning: context imbalance in 'xfs_extent_busy_clear' - unexpected unlock
> 
> But there is no locking bug here. Sparse simply doesn't understand
> the logic and locking in the busy extent processing loop.
> xfs_extent_busy_put_pag() has an annotation to suppresses an
> unexpected unlock warning, but that isn't sufficient.
> 
> If we move the pag existence check into xfs_extent_busy_put_pag() and
> annotate that with a __release() so that this function always
> appears to release the pag->pagb_lock, sparse now thinks the loop
> locking is balanced (one unlock, one lock per loop) but still throws
> an unexpected unlock warning after loop cleanup.
> 
> i.e. it does not understand that we enter the loop without any locks
> held and exit it with the last lock still held. Whilst the locking
> within the loop is inow balanced, we need to add an __acquire() to
> xfs_extent_busy_clear() to set the initial lock context needed to
> avoid false warnings.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_extent_busy.c | 27 +++++++++++++++++++++++----
>  1 file changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/xfs_extent_busy.c b/fs/xfs/xfs_extent_busy.c
> index 56cfa1498571..686b67372030 100644
> --- a/fs/xfs/xfs_extent_busy.c
> +++ b/fs/xfs/xfs_extent_busy.c
> @@ -534,12 +534,24 @@ xfs_extent_busy_clear_one(
>  	kfree(busyp);
>  }
>  
> +/*
> + * Sparse has real trouble with the structure of xfs_extent_busy_clear() and it
> + * is impossible to annotate it correctly if we leave the 'if (pag)' conditional
> + * in xfs_extent_busy_clear(). Hence we always "release" the lock in
> + * xfs_extent_busy_put_pag() so sparse only ever sees one possible path to
> + * drop the lock.
> + */
>  static void
>  xfs_extent_busy_put_pag(
>  	struct xfs_perag	*pag,
>  	bool			wakeup)
>  		__releases(pag->pagb_lock)
>  {
> +	if (!pag) {
> +		__release(pag->pagb_lock);
> +		return;
> +	}

Passing in a null pointer so we can fake out a compliance tool with a
nonsense annotation really feels like the height of software bureaucracy
compliance culture now...

I don't want to RVB this but I'm so tired of fighting pointless battles
with people over their clearly inadequate tooling, so GIGO:

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> +
>  	if (wakeup) {
>  		pag->pagb_gen++;
>  		wake_up_all(&pag->pagb_wait);
> @@ -565,10 +577,18 @@ xfs_extent_busy_clear(
>  	xfs_agnumber_t		agno = NULLAGNUMBER;
>  	bool			wakeup = false;
>  
> +	/*
> +	 * Sparse thinks the locking in the loop below is balanced (one unlock,
> +	 * one lock per loop iteration) and doesn't understand that we enter
> +	 * with no lock held and exit with a lock held. Hence we need to
> +	 * "acquire" the lock to create the correct initial condition for the
> +	 * cleanup after loop termination to avoid an unexpected unlock warning.
> +	 */
> +	__acquire(pag->pagb_lock);
> +
>  	list_for_each_entry_safe(busyp, n, list, list) {
>  		if (busyp->agno != agno) {
> -			if (pag)
> -				xfs_extent_busy_put_pag(pag, wakeup);
> +			xfs_extent_busy_put_pag(pag, wakeup);
>  			agno = busyp->agno;
>  			pag = xfs_perag_get(mp, agno);
>  			spin_lock(&pag->pagb_lock);
> @@ -584,8 +604,7 @@ xfs_extent_busy_clear(
>  		}
>  	}
>  
> -	if (pag)
> -		xfs_extent_busy_put_pag(pag, wakeup);
> +	xfs_extent_busy_put_pag(pag, wakeup);
>  }
>  
>  /*
> -- 
> 2.43.0
> 
> 

Re: [PATCH 2/5] xfs: fix sparse warning in xfs_extent_busy_clear

[Christoph Hellwig]

So while this works it looks pretty ugly.  I'd rewrite the nested loop
as what it is: a nested loop.  Something like the patch below.  Only
compile tested so far, but I'll kick off a real test later.

diff --git a/fs/xfs/xfs_extent_busy.c b/fs/xfs/xfs_extent_busy.c
index 2ccde32c9a9e97..1334360128662a 100644
--- a/fs/xfs/xfs_extent_busy.c
+++ b/fs/xfs/xfs_extent_busy.c
@@ -533,21 +533,6 @@ xfs_extent_busy_clear_one(
 	kmem_free(busyp);
 }
 
-static void
-xfs_extent_busy_put_pag(
-	struct xfs_perag	*pag,
-	bool			wakeup)
-		__releases(pag->pagb_lock)
-{
-	if (wakeup) {
-		pag->pagb_gen++;
-		wake_up_all(&pag->pagb_wait);
-	}
-
-	spin_unlock(&pag->pagb_lock);
-	xfs_perag_put(pag);
-}
-
 /*
  * Remove all extents on the passed in list from the busy extents tree.
  * If do_discard is set skip extents that need to be discarded, and mark
@@ -559,32 +544,43 @@ xfs_extent_busy_clear(
 	struct list_head	*list,
 	bool			do_discard)
 {
-	struct xfs_extent_busy	*busyp, *n;
-	struct xfs_perag	*pag = NULL;
-	xfs_agnumber_t		agno = NULLAGNUMBER;
-	bool			wakeup = false;
-
-	list_for_each_entry_safe(busyp, n, list, list) {
-		if (busyp->agno != agno) {
-			if (pag)
-				xfs_extent_busy_put_pag(pag, wakeup);
-			agno = busyp->agno;
-			pag = xfs_perag_get(mp, agno);
-			spin_lock(&pag->pagb_lock);
-			wakeup = false;
-		}
+	struct xfs_extent_busy	*busyp;
 
-		if (do_discard && busyp->length &&
-		    !(busyp->flags & XFS_EXTENT_BUSY_SKIP_DISCARD)) {
-			busyp->flags = XFS_EXTENT_BUSY_DISCARDED;
-		} else {
-			xfs_extent_busy_clear_one(mp, pag, busyp);
-			wakeup = true;
-		}
-	}
+	busyp = list_first_entry_or_null(list, typeof(*busyp), list);
+	if (!busyp)
+		return;
+
+	do {
+		struct xfs_perag	*pag = xfs_perag_get(mp, busyp->agno);
+		bool			wakeup = false;
+		struct xfs_extent_busy	*next;
 
-	if (pag)
-		xfs_extent_busy_put_pag(pag, wakeup);
+		spin_lock(&pag->pagb_lock);
+		for (;;) {
+			next = list_next_entry(busyp, list);
+
+			if (do_discard && busyp->length &&
+			    !(busyp->flags & XFS_EXTENT_BUSY_SKIP_DISCARD)) {
+				busyp->flags = XFS_EXTENT_BUSY_DISCARDED;
+			} else {
+				xfs_extent_busy_clear_one(mp, pag, busyp);
+				wakeup = true;
+			}
+	
+			if (list_entry_is_head(next, list, list) ||
+			    next->agno != pag->pag_agno)
+				break;
+			busyp = next;
+		}
+		if (wakeup) {
+			pag->pagb_gen++;
+			wake_up_all(&pag->pagb_wait);
+		}
+		spin_unlock(&pag->pagb_lock);
+		xfs_perag_put(pag);
+	
+		busyp = next;
+	} while (!list_entry_is_head(busyp, list, list));
 }
 
 /*

Re: [PATCH 3/5] xfs: silence sparse warning when checking version number

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 4/5] xfs: remove unused is_rt_data_fork() function

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 5/5] xfs: fix sparse warnings about unused interval tree functions

[Christoph Hellwig]

On Wed, Apr 03, 2024 at 08:28:32AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Sparse throws warnings about the interval tree functions that are
> defined and then not used in the scrub bitmap code:

Hmm, what sparse version and kernel config is this?  I don't see
warnings in my builds, and I also can't see where they'd come from.