make xfs sparse-warning free

make xfs sparse-warning free

[Christoph Hellwig]

Hi all,

this series isn't really a series, but a collection of two very different
patches toward the result of having no sparse warnings for fs/xfs/.

Patch 1 adds a cond_lock annotation to the lockref code.  This also fixes
warnings (but resurfaces new ones) in erofs and gfs2.

Patch 2 moves some XFS code around to help the lock context tracking. 
I actually think this improves the code, so I think this should go into
the XFS tree.

Patch 3 duplicates some XFS code to work around the lock context tracking,
but I think it is pretty silly.  Maybe it's a good example to help improve
this code in sparse?  It would not be horrible to apply given how little
code it duplicates, but a fix in sparse would be much nicer.

The kernel MAINTAINERS still list Luc as sparse maintainer, but sparse
itself lists Chris again.  Do we need to update the kernel MAINTAINERS
file, or are those separate roles?

[PATCH 1/3] lockref: add a __cond_lock annotation for lockref_put_or_lock

[Christoph Hellwig]

Add a cond_lock annotation for lockref_put_or_lock to make sparse
happy with using it.  Note that for this the return value has to be
double-inverted as the return value convention of lockref_put_or_lock
is inverted compared to _trylock conventions expected by __cond_lock,
as lockref_put_or_lock returns true when it did not need to take the
lock.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 include/linux/lockref.h | 4 +++-
 lib/lockref.c           | 4 ++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/include/linux/lockref.h b/include/linux/lockref.h
index 676721ee878d..7b4bb67db216 100644
--- a/include/linux/lockref.h
+++ b/include/linux/lockref.h
@@ -49,7 +49,9 @@ static inline void lockref_init(struct lockref *lockref)
 void lockref_get(struct lockref *lockref);
 int lockref_put_return(struct lockref *lockref);
 bool lockref_get_not_zero(struct lockref *lockref);
-bool lockref_put_or_lock(struct lockref *lockref);
+bool _lockref_put_or_lock(struct lockref *lockref);
+#define lockref_put_or_lock(_lockref) \
+	(!__cond_lock((_lockref)->lock, !_lockref_put_or_lock(_lockref)))
 
 void lockref_mark_dead(struct lockref *lockref);
 bool lockref_get_not_dead(struct lockref *lockref);
diff --git a/lib/lockref.c b/lib/lockref.c
index 5d8e3ef3860e..667f0c30c867 100644
--- a/lib/lockref.c
+++ b/lib/lockref.c
@@ -105,7 +105,7 @@ EXPORT_SYMBOL(lockref_put_return);
  * @lockref: pointer to lockref structure
  * Return: 1 if count updated successfully or 0 if count <= 1 and lock taken
  */
-bool lockref_put_or_lock(struct lockref *lockref)
+bool _lockref_put_or_lock(struct lockref *lockref)
 {
 	CMPXCHG_LOOP(
 		new.count--;
@@ -122,7 +122,7 @@ bool lockref_put_or_lock(struct lockref *lockref)
 	spin_unlock(&lockref->lock);
 	return true;
 }
-EXPORT_SYMBOL(lockref_put_or_lock);
+EXPORT_SYMBOL(_lockref_put_or_lock);
 
 /**
  * lockref_mark_dead - mark lockref dead
-- 
2.47.3

Re: [PATCH 1/3] lockref: add a __cond_lock annotation for lockref_put_or_lock

[Linus Torvalds]

[ Christoph, while I have some minor comments on the patch, the
primary reason for this reply is actually that this was in my spam
box.

  Your email sending is violating DKIM rules: your "From:" is
"lst.de", but you use "infradead.org" as your SMTP server, and the
DKIM signature then has d=infradead.org as a result.

  That's a so-called "alignment" failure, and is pretty fundamental
and a sign of spam. You can't verify that an email is from a valid
address by then using a different domain entirely as the "look, trust
me" thing.

  So your emails all end up with dmarc failures, and then are much
more likely to be marked as spam. ]

On Thu, Nov 13, 2025 at 9:54 PM Christoph Hellwig <hch@lst.de> wrote:
>
> -bool lockref_put_or_lock(struct lockref *lockref);
> +bool _lockref_put_or_lock(struct lockref *lockref);
> +#define lockref_put_or_lock(_lockref) \
> +       (!__cond_lock((_lockref)->lock, !_lockref_put_or_lock(_lockref)))

I don't think this is wrong per se, but rather than rename the
function, I wonder if it wouldn't be simpler to just have a

  #undef lockref_put_or_lock

in lib/lockref.c and just keep the same name for the function and the macro.

Macro expansion isn't recursive, and having

    #define a(x) something-something a(x)

is actually perfectly fine, and something we do intentionally for
other reasons (typically because it also allows us to then use "#ifdef
a" to check whether there is some architecture-specific implementation
of 'a()')

And yes, you do need that "#undef" to then not get crazy parse errors
in the actual definition and export of the function, but it would
allow us to avoid yet another "underscore version of the function".

I dunno. Not a big deal, but it seems annoying to make up a new name for this.

            Linus

Re: [PATCH 1/3] lockref: add a __cond_lock annotation for lockref_put_or_lock

[Christoph Hellwig]

On Fri, Nov 14, 2025 at 10:18:24AM -0800, Linus Torvalds wrote:
> Macro expansion isn't recursive, and having
> 
>     #define a(x) something-something a(x)
> 
> is actually perfectly fine, and something we do intentionally for
> other reasons (typically because it also allows us to then use "#ifdef
> a" to check whether there is some architecture-specific implementation
> of 'a()')
> 
> And yes, you do need that "#undef" to then not get crazy parse errors
> in the actual definition and export of the function, but it would
> allow us to avoid yet another "underscore version of the function".
> 
> I dunno. Not a big deal, but it seems annoying to make up a new name
> for this.

I know you can redefine names using macros, and now that you remind
me I remember that you like that.  I personally hate it as it means
there are two things with the same name, which makes understanding
the code much harder and confuses tools like cscope.  But I can
switch to that approach if you prefer.

[PATCH 2/3] xfs: move some code out of xfs_iget_recycle

[Christoph Hellwig]

Having a function drop locks, reacquire them and release them again
seems to confuse the clang lock analysis even more than it confuses
humans.  Keep the humans and machines sanity by moving a chunk of
code into the caller to simplify the lock tracking.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_icache.c | 31 +++++++++++++------------------
 1 file changed, 13 insertions(+), 18 deletions(-)

diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index e44040206851..546efa6cec72 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -358,7 +358,7 @@ xfs_reinit_inode(
 static int
 xfs_iget_recycle(
 	struct xfs_perag	*pag,
-	struct xfs_inode	*ip) __releases(&ip->i_flags_lock)
+	struct xfs_inode	*ip)
 {
 	struct xfs_mount	*mp = ip->i_mount;
 	struct inode		*inode = VFS_I(ip);
@@ -366,20 +366,6 @@ xfs_iget_recycle(
 
 	trace_xfs_iget_recycle(ip);
 
-	if (!xfs_ilock_nowait(ip, XFS_ILOCK_EXCL))
-		return -EAGAIN;
-
-	/*
-	 * We need to make it look like the inode is being reclaimed to prevent
-	 * the actual reclaim workers from stomping over us while we recycle
-	 * the inode.  We can't clear the radix tree tag yet as it requires
-	 * pag_ici_lock to be held exclusive.
-	 */
-	ip->i_flags |= XFS_IRECLAIM;
-
-	spin_unlock(&ip->i_flags_lock);
-	rcu_read_unlock();
-
 	ASSERT(!rwsem_is_locked(&inode->i_rwsem));
 	error = xfs_reinit_inode(mp, inode);
 	xfs_iunlock(ip, XFS_ILOCK_EXCL);
@@ -576,10 +562,19 @@ xfs_iget_cache_hit(
 
 	/* The inode fits the selection criteria; process it. */
 	if (ip->i_flags & XFS_IRECLAIMABLE) {
-		/* Drops i_flags_lock and RCU read lock. */
-		error = xfs_iget_recycle(pag, ip);
-		if (error == -EAGAIN)
+		/*
+		 * We need to make it look like the inode is being reclaimed to
+		 * prevent the actual reclaim workers from stomping over us
+		 * while we recycle the inode.  We can't clear the radix tree
+		 * tag yet as it requires pag_ici_lock to be held exclusive.
+		 */
+		if (!xfs_ilock_nowait(ip, XFS_ILOCK_EXCL))
 			goto out_skip;
+		ip->i_flags |= XFS_IRECLAIM;
+		spin_unlock(&ip->i_flags_lock);
+		rcu_read_unlock();
+
+		error = xfs_iget_recycle(pag, ip);
 		if (error)
 			return error;
 	} else {
-- 
2.47.3

Re: [PATCH 2/3] xfs: move some code out of xfs_iget_recycle

[Darrick J. Wong]

On Fri, Nov 14, 2025 at 06:52:24AM +0100, Christoph Hellwig wrote:
> Having a function drop locks, reacquire them and release them again
> seems to confuse the clang lock analysis even more than it confuses
> humans.  Keep the humans and machines sanity by moving a chunk of
> code into the caller to simplify the lock tracking.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_icache.c | 31 +++++++++++++------------------
>  1 file changed, 13 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> index e44040206851..546efa6cec72 100644
> --- a/fs/xfs/xfs_icache.c
> +++ b/fs/xfs/xfs_icache.c
> @@ -358,7 +358,7 @@ xfs_reinit_inode(
>  static int
>  xfs_iget_recycle(
>  	struct xfs_perag	*pag,
> -	struct xfs_inode	*ip) __releases(&ip->i_flags_lock)
> +	struct xfs_inode	*ip)
>  {
>  	struct xfs_mount	*mp = ip->i_mount;
>  	struct inode		*inode = VFS_I(ip);
> @@ -366,20 +366,6 @@ xfs_iget_recycle(
>  
>  	trace_xfs_iget_recycle(ip);
>  
> -	if (!xfs_ilock_nowait(ip, XFS_ILOCK_EXCL))
> -		return -EAGAIN;
> -
> -	/*
> -	 * We need to make it look like the inode is being reclaimed to prevent
> -	 * the actual reclaim workers from stomping over us while we recycle
> -	 * the inode.  We can't clear the radix tree tag yet as it requires
> -	 * pag_ici_lock to be held exclusive.
> -	 */
> -	ip->i_flags |= XFS_IRECLAIM;
> -
> -	spin_unlock(&ip->i_flags_lock);
> -	rcu_read_unlock();
> -
>  	ASSERT(!rwsem_is_locked(&inode->i_rwsem));
>  	error = xfs_reinit_inode(mp, inode);
>  	xfs_iunlock(ip, XFS_ILOCK_EXCL);
> @@ -576,10 +562,19 @@ xfs_iget_cache_hit(
>  
>  	/* The inode fits the selection criteria; process it. */
>  	if (ip->i_flags & XFS_IRECLAIMABLE) {
> -		/* Drops i_flags_lock and RCU read lock. */
> -		error = xfs_iget_recycle(pag, ip);
> -		if (error == -EAGAIN)
> +		/*
> +		 * We need to make it look like the inode is being reclaimed to
> +		 * prevent the actual reclaim workers from stomping over us
> +		 * while we recycle the inode.  We can't clear the radix tree
> +		 * tag yet as it requires pag_ici_lock to be held exclusive.
> +		 */
> +		if (!xfs_ilock_nowait(ip, XFS_ILOCK_EXCL))
>  			goto out_skip;
> +		ip->i_flags |= XFS_IRECLAIM;
> +		spin_unlock(&ip->i_flags_lock);
> +		rcu_read_unlock();

I wonder, does sparse get confused by rcu_read_lock having been taken by
the caller but unlocked here?

The code move looks correct though.
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D


> +
> +		error = xfs_iget_recycle(pag, ip);
>  		if (error)
>  			return error;
>  	} else {
> -- 
> 2.47.3
> 
> 

Re: [PATCH 2/3] xfs: move some code out of xfs_iget_recycle

[Linus Torvalds]

On Fri, 14 Nov 2025 at 09:06, Darrick J. Wong <djwong@kernel.org> wrote:
>
> I wonder, does sparse get confused by rcu_read_lock having been taken by
> the caller but unlocked here?

I think we'll sunset all the sparse lock context checks - they were
never very good, but they were "all we had".  It was useful in limited
and simpler places (because the sparse logic itself was limited and
simple), but it never worked well for anything more complex.

Now that clang is about to get context checking, and doing it much
more properly, the half-arsed sparse complaints should be ignored in
favor of just trying to make clang understand things well enough.

Put another way: don't worry about sparse.

                Linus

Re: [PATCH 2/3] xfs: move some code out of xfs_iget_recycle

[Christoph Hellwig]

On Fri, Nov 14, 2025 at 09:04:02AM -0800, Darrick J. Wong wrote:
> I wonder, does sparse get confused by rcu_read_lock having been taken by
> the caller but unlocked here?

Probably.  Plus the conditional trylock.  Note that the more complex
clang context tracking also didn't like it.

[PATCH 3/3] xfs: work around sparse context tracking in xfs_qm_dquot_isolate

[Christoph Hellwig]

sparse gets confused by the goto after spin_trylock:

fs/xfs/xfs_qm.c:486:33: warning: context imbalance in 'xfs_qm_dquot_isolate' - different lock contexts for basic block

work around this by duplicating the trivial amount of code after the
label.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_qm.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
index 95be67ac6eb4..66d25ac9600b 100644
--- a/fs/xfs/xfs_qm.c
+++ b/fs/xfs/xfs_qm.c
@@ -422,8 +422,11 @@ xfs_qm_dquot_isolate(
 	struct xfs_qm_isolate	*isol = arg;
 	enum lru_status		ret = LRU_SKIP;
 
-	if (!spin_trylock(&dqp->q_lockref.lock))
-		goto out_miss_busy;
+	if (!spin_trylock(&dqp->q_lockref.lock)) {
+		trace_xfs_dqreclaim_busy(dqp);
+		XFS_STATS_INC(dqp->q_mount, xs_qm_dqreclaim_misses);
+		return LRU_SKIP;
+	}
 
 	/*
 	 * If something else is freeing this dquot and hasn't yet removed it
@@ -482,7 +485,6 @@ xfs_qm_dquot_isolate(
 
 out_miss_unlock:
 	spin_unlock(&dqp->q_lockref.lock);
-out_miss_busy:
 	trace_xfs_dqreclaim_busy(dqp);
 	XFS_STATS_INC(dqp->q_mount, xs_qm_dqreclaim_misses);
 	return ret;
-- 
2.47.3

Re: [PATCH 3/3] xfs: work around sparse context tracking in xfs_qm_dquot_isolate

[Darrick J. Wong]

On Fri, Nov 14, 2025 at 06:52:25AM +0100, Christoph Hellwig wrote:
> sparse gets confused by the goto after spin_trylock:
> 
> fs/xfs/xfs_qm.c:486:33: warning: context imbalance in 'xfs_qm_dquot_isolate' - different lock contexts for basic block
> 
> work around this by duplicating the trivial amount of code after the
> label.

Might want to leave a code comment about shutting up sparse so that
someone doesn't revert this change to optimize LOC.  That said ...
what is the differing lock context?  Does sparse not understand the
spin_trylock?

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

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

--D

> ---
>  fs/xfs/xfs_qm.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/xfs_qm.c b/fs/xfs/xfs_qm.c
> index 95be67ac6eb4..66d25ac9600b 100644
> --- a/fs/xfs/xfs_qm.c
> +++ b/fs/xfs/xfs_qm.c
> @@ -422,8 +422,11 @@ xfs_qm_dquot_isolate(
>  	struct xfs_qm_isolate	*isol = arg;
>  	enum lru_status		ret = LRU_SKIP;
>  
> -	if (!spin_trylock(&dqp->q_lockref.lock))
> -		goto out_miss_busy;
> +	if (!spin_trylock(&dqp->q_lockref.lock)) {
> +		trace_xfs_dqreclaim_busy(dqp);
> +		XFS_STATS_INC(dqp->q_mount, xs_qm_dqreclaim_misses);
> +		return LRU_SKIP;
> +	}
>  
>  	/*
>  	 * If something else is freeing this dquot and hasn't yet removed it
> @@ -482,7 +485,6 @@ xfs_qm_dquot_isolate(
>  
>  out_miss_unlock:
>  	spin_unlock(&dqp->q_lockref.lock);
> -out_miss_busy:
>  	trace_xfs_dqreclaim_busy(dqp);
>  	XFS_STATS_INC(dqp->q_mount, xs_qm_dqreclaim_misses);
>  	return ret;
> -- 
> 2.47.3
> 
> 

Re: [PATCH 3/3] xfs: work around sparse context tracking in xfs_qm_dquot_isolate

[Christoph Hellwig]

On Fri, Nov 14, 2025 at 09:06:23AM -0800, Darrick J. Wong wrote:
> On Fri, Nov 14, 2025 at 06:52:25AM +0100, Christoph Hellwig wrote:
> > sparse gets confused by the goto after spin_trylock:
> > 
> > fs/xfs/xfs_qm.c:486:33: warning: context imbalance in 'xfs_qm_dquot_isolate' - different lock contexts for basic block
> > 
> > work around this by duplicating the trivial amount of code after the
> > label.
> 
> Might want to leave a code comment about shutting up sparse so that
> someone doesn't revert this change to optimize LOC.  That said ...
> what is the differing lock context?  Does sparse not understand the
> spin_trylock?

So in case that my cover letter wasn't clear enough (or not widely read
:)), I'm somewhat doubtful about wanting to actually merge this upstream.
It just feels wrong to me.  But it was the list thing to need a clean
compile, so I wanted to demonstrate it.

Re: make xfs sparse-warning free

[Linus Torvalds]

On Thu, 13 Nov 2025 at 21:54, Christoph Hellwig <hch@lst.de> wrote:
>
> this series isn't really a series, but a collection of two very different
> patches toward the result of having no sparse warnings for fs/xfs/.

So I answered the wrong email (because I saw the other email first).

I think making it sparse-clean is obviously good, but as mentioned in
the other email, I think the clang context tracking is the (near)
future when it comes to static help in lock context tracking.

I know you looked at that clang context thing earlier, and assumed
that that is what triggered this work in the first place?

Anyway, iirc Chris Li already at some point indicated that he'd rather
remove the sparse context checking entirely than try to make it
smarter.

I do think that being sparse-clean for the current sparse context
tracking is a "good thing", but not really because it makes sparse
happy: it's a good thing mainly because *if* you can do it, it tends
to mean that the lock context rules are really simple and
straightforward, because sparse just doesn't do anything non-simple
very well in this area.

So when you say about patch 2:

> I actually think this improves the code, so I think this should go into
> the XFS tree.

I heartily agree. But then

> Patch 3 duplicates some XFS code to work around the lock context tracking,
> but I think it is pretty silly.

makes me go "if you have to make the code worse to make sparse happy,
maybe just look at the clang context tracking instead?"

Because I *assume* that the more complete clang context tracking
series doesn't need that?

            Linus

Re: make xfs sparse-warning free

[Christoph Hellwig]

On Fri, Nov 14, 2025 at 09:56:51AM -0800, Linus Torvalds wrote:
> I know you looked at that clang context thing earlier, and assumed
> that that is what triggered this work in the first place?

In this case not directly, although patch 2 was originally done for
that.  Patch 1 is newer than those experiments, but only because we
just started using lockref in this merge window, and it should apply
for that as well.

> > Patch 3 duplicates some XFS code to work around the lock context tracking,
> > but I think it is pretty silly.
> 
> makes me go "if you have to make the code worse to make sparse happy,
> maybe just look at the clang context tracking instead?"

Well, that's why I said I didn't like it it and included it more as
an example for the sparse developers to see what goes wrong.

> Because I *assume* that the more complete clang context tracking
> series doesn't need that?

I assume the same, but the quota changes the cause this are new, and I
haven't combined them yet with the context tracking experiments I did a
while ago.