improve log context annotations

improve log context annotations

[Christoph Hellwig]

Hi Carlos,

this series improves the lock annotations in the XFS code.

Note that right now this is all mostly unused - the less stringent sparse
lock context checks got disabled when the clang version was merged, and
we're not quite ready to enable the latter yet due to our conditional
locking in the ilock helper and the locking contexts transferred to the
transaction for the inode and dquot locks, but I'm working on that.

In the meantime having these corrected at least serves as useful
documentation.

Diffstat:
 xfs_extent_busy.c |    4 ++--
 xfs_icache.c      |    3 ++-
 xfs_log.c         |   33 +++++++++++++++++++++------------
 xfs_log_cil.c     |    3 ++-
 xfs_log_priv.h    |    4 ++--
 xfs_mru_cache.c   |    2 +-
 xfs_trans_ail.c   |    4 +++-
 xfs_zone_alloc.c  |    2 ++
 8 files changed, 35 insertions(+), 20 deletions(-)

[PATCH 1/8] xfs: fix the lock annotation on xfs_iget_cache_hit

[Christoph Hellwig]

The newer clang context analysis requires __releases_shared for the RCU
pseudo-lock.

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

diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
index 9d8dd30bd927..340fd95dfcdf 100644
--- a/fs/xfs/xfs_icache.c
+++ b/fs/xfs/xfs_icache.c
@@ -501,7 +501,8 @@ xfs_iget_cache_hit(
 	struct xfs_inode	*ip,
 	xfs_ino_t		ino,
 	int			flags,
-	int			lock_flags) __releases(RCU)
+	int			lock_flags)
+		__releases_shared(RCU)
 {
 	struct inode		*inode = VFS_I(ip);
 	struct xfs_mount	*mp = ip->i_mount;
-- 
2.53.0

[PATCH 2/8] xfs: fix the lock annotation in xfs_extent_busy_update_extent

[Christoph Hellwig]

Name the correct lock.

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

diff --git a/fs/xfs/xfs_extent_busy.c b/fs/xfs/xfs_extent_busy.c
index 41cf0605ec22..6da8c1f938aa 100644
--- a/fs/xfs/xfs_extent_busy.c
+++ b/fs/xfs/xfs_extent_busy.c
@@ -161,8 +161,8 @@ xfs_extent_busy_update_extent(
 	xfs_agblock_t		fbno,
 	xfs_extlen_t		flen,
 	bool			userdata)
-		__releases(&eb->eb_lock)
-		__acquires(&eb->eb_lock)
+		__releases(&xg->xg_busy_extents->eb_lock)
+		__acquires(&xg->xg_busy_extents->eb_lock)
 {
 	struct xfs_extent_busy_tree *eb = xg->xg_busy_extents;
 	xfs_agblock_t		fend = fbno + flen;
-- 
2.53.0

[PATCH 3/8] xfs: fix the lock annotation in xfs_mru_cache_lookup

[Christoph Hellwig]

Name the actual lock.  Unlike sparse, clang wants the annotation to
be correct.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_mru_cache.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_mru_cache.c b/fs/xfs/xfs_mru_cache.c
index d61ec8cb126d..3f3af2e2e31c 100644
--- a/fs/xfs/xfs_mru_cache.c
+++ b/fs/xfs/xfs_mru_cache.c
@@ -520,7 +520,7 @@ xfs_mru_cache_lookup(
 	if (elem) {
 		list_del(&elem->list_node);
 		_xfs_mru_cache_list_insert(mru, elem);
-		__release(mru_lock); /* help sparse not be stupid */
+		__release(&mru->lock);
 	} else
 		spin_unlock(&mru->lock);
 
-- 
2.53.0

[PATCH 4/8] xfs: improve lock annotations in the log code

[Christoph Hellwig]

Improve the __acquires and __releases annotations so that the new
clang code that is a bit more picky than sparse is happy.  This involves
passing an explicit struct xlog argument in a few places because
alias analysis can't figure out it is the same lock when dereferencing
changing iclogs.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_log.c      | 31 +++++++++++++++++++------------
 fs/xfs/xfs_log_cil.c  |  2 +-
 fs/xfs/xfs_log_priv.h |  4 ++--
 3 files changed, 22 insertions(+), 15 deletions(-)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index f807f8f4f705..0294ac277f35 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -470,6 +470,8 @@ xlog_state_release_iclog(
 	struct xlog		*log,
 	struct xlog_in_core	*iclog,
 	struct xlog_ticket	*ticket)
+		__releases(&log->l_icloglock)
+		__acquires(&log->l_icloglock)
 {
 	bool			last_ref;
 
@@ -744,13 +746,16 @@ xfs_log_mount_cancel(
  */
 static inline int
 xlog_force_iclog(
+	struct xlog		*log,
 	struct xlog_in_core	*iclog)
+		__releases(&log->l_icloglock)
+		__acquires(&log->l_icloglock)
 {
 	atomic_inc(&iclog->ic_refcnt);
 	iclog->ic_flags |= XLOG_ICL_NEED_FLUSH | XLOG_ICL_NEED_FUA;
 	if (iclog->ic_state == XLOG_STATE_ACTIVE)
-		xlog_state_switch_iclogs(iclog->ic_log, iclog, 0);
-	return xlog_state_release_iclog(iclog->ic_log, iclog, NULL);
+		xlog_state_switch_iclogs(log, iclog, 0);
+	return xlog_state_release_iclog(log, iclog, NULL);
 }
 
 /*
@@ -778,11 +783,10 @@ xlog_wait_iclog_completion(struct xlog *log)
  */
 int
 xlog_wait_on_iclog(
+	struct xlog		*log,
 	struct xlog_in_core	*iclog)
-		__releases(iclog->ic_log->l_icloglock)
+		__releases(log->l_icloglock)
 {
-	struct xlog		*log = iclog->ic_log;
-
 	trace_xlog_iclog_wait_on(iclog, _RET_IP_);
 	if (!xlog_is_shutdown(log) &&
 	    iclog->ic_state != XLOG_STATE_ACTIVE &&
@@ -879,8 +883,8 @@ xlog_unmount_write(
 
 	spin_lock(&log->l_icloglock);
 	iclog = log->l_iclog;
-	error = xlog_force_iclog(iclog);
-	xlog_wait_on_iclog(iclog);
+	error = xlog_force_iclog(log, iclog);
+	xlog_wait_on_iclog(log, iclog);
 
 	if (tic) {
 		trace_xfs_log_umount_write(log, tic);
@@ -2741,14 +2745,17 @@ xlog_state_switch_iclogs(
  */
 static int
 xlog_force_and_check_iclog(
+	struct xlog		*log,
 	struct xlog_in_core	*iclog,
 	bool			*completed)
+		__releases(&log->l_icloglock)
+		__acquires(&log->l_icloglock)
 {
 	xfs_lsn_t		lsn = be64_to_cpu(iclog->ic_header->h_lsn);
 	int			error;
 
 	*completed = false;
-	error = xlog_force_iclog(iclog);
+	error = xlog_force_iclog(log, iclog);
 	if (error)
 		return error;
 
@@ -2825,7 +2832,7 @@ xfs_log_force(
 			/* We have exclusive access to this iclog. */
 			bool	completed;
 
-			if (xlog_force_and_check_iclog(iclog, &completed))
+			if (xlog_force_and_check_iclog(log, iclog, &completed))
 				goto out_error;
 
 			if (completed)
@@ -2850,7 +2857,7 @@ xfs_log_force(
 		iclog->ic_flags |= XLOG_ICL_NEED_FLUSH | XLOG_ICL_NEED_FUA;
 
 	if (flags & XFS_LOG_SYNC)
-		return xlog_wait_on_iclog(iclog);
+		return xlog_wait_on_iclog(log, iclog);
 out_unlock:
 	spin_unlock(&log->l_icloglock);
 	return 0;
@@ -2920,7 +2927,7 @@ xlog_force_lsn(
 					&log->l_icloglock);
 			return -EAGAIN;
 		}
-		if (xlog_force_and_check_iclog(iclog, &completed))
+		if (xlog_force_and_check_iclog(log, iclog, &completed))
 			goto out_error;
 		if (log_flushed)
 			*log_flushed = 1;
@@ -2948,7 +2955,7 @@ xlog_force_lsn(
 	}
 
 	if (flags & XFS_LOG_SYNC)
-		return xlog_wait_on_iclog(iclog);
+		return xlog_wait_on_iclog(log, iclog);
 out_unlock:
 	spin_unlock(&log->l_icloglock);
 	return 0;
diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
index 639f875a8fb2..ae1ed16aeb2f 100644
--- a/fs/xfs/xfs_log_cil.c
+++ b/fs/xfs/xfs_log_cil.c
@@ -1556,7 +1556,7 @@ xlog_cil_push_work(
 			 * iclogs older than ic_prev. Hence we only need to wait
 			 * on the most recent older iclog here.
 			 */
-			xlog_wait_on_iclog(ctx->commit_iclog->ic_prev);
+			xlog_wait_on_iclog(log, ctx->commit_iclog->ic_prev);
 			spin_lock(&log->l_icloglock);
 		}
 
diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
index cf1e4ce61a8c..6d9673c41cdf 100644
--- a/fs/xfs/xfs_log_priv.h
+++ b/fs/xfs/xfs_log_priv.h
@@ -605,8 +605,8 @@ xlog_wait(
 	remove_wait_queue(wq, &wait);
 }
 
-int xlog_wait_on_iclog(struct xlog_in_core *iclog)
-		__releases(iclog->ic_log->l_icloglock);
+int xlog_wait_on_iclog(struct xlog *log, struct xlog_in_core *iclog)
+		__releases(log->l_icloglock);
 
 /* Calculate the distance between two LSNs in bytes */
 static inline uint64_t
-- 
2.53.0

[PATCH 5/8] xfs: add lock annotations to xfs_try_open_zone

[Christoph Hellwig]

Improve the __acquires and __releases annotations so that the new
clang code that is a bit more picky than sparse is happy.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_zone_alloc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/xfs/xfs_zone_alloc.c b/fs/xfs/xfs_zone_alloc.c
index 7d13fa7ab30a..3f42202c3f61 100644
--- a/fs/xfs/xfs_zone_alloc.c
+++ b/fs/xfs/xfs_zone_alloc.c
@@ -475,6 +475,8 @@ static struct xfs_open_zone *
 xfs_try_open_zone(
 	struct xfs_mount	*mp,
 	enum rw_hint		write_hint)
+		__releases(&mp->m_zone_info->zi_open_zones_lock)
+		__acquires(&mp->m_zone_info->zi_open_zones_lock)
 {
 	struct xfs_zone_info	*zi = mp->m_zone_info;
 	struct xfs_open_zone	*oz;
-- 
2.53.0

[PATCH 6/8] xfs: add lock annotations to xlog_state_shutdown_callbacks

[Christoph Hellwig]

Sparse used to get away without these despite dropping and reacquiring
l_icloglock

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_log.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 0294ac277f35..2a34611d81f6 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -422,6 +422,8 @@ xfs_log_reserve(
 static void
 xlog_state_shutdown_callbacks(
 	struct xlog		*log)
+		__releases(&log->l_icloglock)
+		__acquires(&log->l_icloglock)
 {
 	struct xlog_in_core	*iclog;
 	LIST_HEAD(cb_list);
-- 
2.53.0

[PATCH 7/8] xfs: add a lock annotation to xlog_cil_push_background

[Christoph Hellwig]

This is required to make the clang context analysis happy, which is
more strict than the old sparse lock context tracking.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_log_cil.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
index ae1ed16aeb2f..166531018ce4 100644
--- a/fs/xfs/xfs_log_cil.c
+++ b/fs/xfs/xfs_log_cil.c
@@ -1627,6 +1627,7 @@ xlog_cil_push_work(
 static void
 xlog_cil_push_background(
 	struct xlog	*log)
+		__releases_shared(&log->l_cilp->xc_ctx_lock)
 {
 	struct xfs_cil	*cil = log->l_cilp;
 	int		space_used = atomic_read(&cil->xc_ctx->space_used);
-- 
2.53.0

[PATCH 8/8] xfs: add lock annotations to xfs_ail_delete*

[Christoph Hellwig]

Pass up the __must_hold as clang requires it, and also fix the formatting
of the __must_hold on xfs_ail_check to match how we do it elsewhere.

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

diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
index 99a9bf3762b7..f955479a08fd 100644
--- a/fs/xfs/xfs_trans_ail.c
+++ b/fs/xfs/xfs_trans_ail.c
@@ -33,7 +33,7 @@ STATIC void
 xfs_ail_check(
 	struct xfs_ail		*ailp,
 	struct xfs_log_item	*lip)
-	__must_hold(&ailp->ail_lock)
+		__must_hold(&ailp->ail_lock)
 {
 	struct xfs_log_item	*prev_lip;
 	struct xfs_log_item	*next_lip;
@@ -321,6 +321,7 @@ static void
 xfs_ail_delete(
 	struct xfs_ail		*ailp,
 	struct xfs_log_item	*lip)
+		__must_hold(&ailp->ail_lock)
 {
 	xfs_ail_check(ailp, lip);
 	list_del(&lip->li_ail);
@@ -899,6 +900,7 @@ xfs_lsn_t
 xfs_ail_delete_one(
 	struct xfs_ail		*ailp,
 	struct xfs_log_item	*lip)
+		__must_hold(&ailp->ail_lock)
 {
 	struct xfs_log_item	*mlip = xfs_ail_min(ailp);
 	xfs_lsn_t		lsn = lip->li_lsn;
-- 
2.53.0

Re: improve log context annotations

[Carlos Maiolino]

On Mon, Jul 20, 2026 at 11:45:37AM +0200, Christoph Hellwig wrote:
> Hi Carlos,
> 
> this series improves the lock annotations in the XFS code.
> 
> Note that right now this is all mostly unused - the less stringent sparse
> lock context checks got disabled when the clang version was merged, and
> we're not quite ready to enable the latter yet due to our conditional
> locking in the ilock helper and the locking contexts transferred to the
> transaction for the inode and dquot locks, but I'm working on that.
> 
> In the meantime having these corrected at least serves as useful
> documentation.

Thanks. Those all look good for me. But I'm adding Dave to the thread as
he has much more expertise with lockdep than me and I'd appreciate his
input.


> 
> Diffstat:
>  xfs_extent_busy.c |    4 ++--
>  xfs_icache.c      |    3 ++-
>  xfs_log.c         |   33 +++++++++++++++++++++------------
>  xfs_log_cil.c     |    3 ++-
>  xfs_log_priv.h    |    4 ++--
>  xfs_mru_cache.c   |    2 +-
>  xfs_trans_ail.c   |    4 +++-
>  xfs_zone_alloc.c  |    2 ++
>  8 files changed, 35 insertions(+), 20 deletions(-)
> 

Re: improve log context annotations

[Christoph Hellwig]

On Tue, Jul 21, 2026 at 11:16:40AM +0200, Carlos Maiolino wrote:
> Thanks. Those all look good for me. But I'm adding Dave to the thread as
> he has much more expertise with lockdep than me and I'd appreciate his
> input.

More review is always good.  But as a reminder for the audience:
lockdep and the lock context annotations are two totally seaprate
things.  The former verifies the locking state at runtime, while
the latter is about compile-time enforcement for the cases where
that is possible.

Re: improve log context annotations

[Darrick J. Wong]

On Tue, Jul 21, 2026 at 05:01:42PM +0200, Christoph Hellwig wrote:
> On Tue, Jul 21, 2026 at 11:16:40AM +0200, Carlos Maiolino wrote:
> > Thanks. Those all look good for me. But I'm adding Dave to the thread as
> > he has much more expertise with lockdep than me and I'd appreciate his
> > input.
> 
> More review is always good.  But as a reminder for the audience:
> lockdep and the lock context annotations are two totally seaprate
> things.  The former verifies the locking state at runtime, while
> the latter is about compile-time enforcement for the cases where
> that is possible.

<shrug> The new annotations are consistent with my understanding of the
annotated functions, but as I don't build with clang I can't say I've
actually dug into its auto-verifier features, let alone run it.

--D

Re: improve log context annotations

[Christoph Hellwig]

On Tue, Jul 21, 2026 at 08:23:48AM -0700, Darrick J. Wong wrote:
> > More review is always good.  But as a reminder for the audience:
> > lockdep and the lock context annotations are two totally seaprate
> > things.  The former verifies the locking state at runtime, while
> > the latter is about compile-time enforcement for the cases where
> > that is possible.
> 
> <shrug> The new annotations are consistent with my understanding of the
> annotated functions, but as I don't build with clang I can't say I've
> actually dug into its auto-verifier features, let alone run it.

Even with clang you need to opt into the in the Makefile for XFS.
As mentioned in this cover letter we're not quite ready for
that yet, but I want to get there.

Re: improve log context annotations

[Dave Chinner]

On Tue, Jul 21, 2026 at 11:16:40AM +0200, Carlos Maiolino wrote:
> On Mon, Jul 20, 2026 at 11:45:37AM +0200, Christoph Hellwig wrote:
> > Hi Carlos,
> > 
> > this series improves the lock annotations in the XFS code.
> > 
> > Note that right now this is all mostly unused - the less stringent sparse
> > lock context checks got disabled when the clang version was merged, and
> > we're not quite ready to enable the latter yet due to our conditional
> > locking in the ilock helper and the locking contexts transferred to the
> > transaction for the inode and dquot locks, but I'm working on that.
> > 
> > In the meantime having these corrected at least serves as useful
> > documentation.
> 
> Thanks. Those all look good for me. But I'm adding Dave to the thread as
> he has much more expertise with lockdep than me and I'd appreciate his
> input.

Honestly, I'd prefer to kill the annotations and replace them with
asserts that the lock is held so there's actual runtime validation
that catches errors when they occur. These lock annotations don't
tell you that the locking is correct, just that the function
manipulates a lock.

The sparse annotations have always been more of an annoyance and
reactive, because we only use them sporadically to suppress errors.
We do not use them to prove locking is actually correct, nor can
they function as such.  i.e. They exist purely to shut up a set of
"not smart enough to understand lock context" tooling failures from
a tool that almost nobody ever ran.

Maybe the clang checker will be better, but I still don't see how it
will ever be able to provide any sort of "locking is correct" proof,
nor that it will be something everyone always runs.

Hence this just seems like trying to maintain a technical debt that
doesn't really serve any useful purpose anymore.

-Dave.
-- 
Dave Chinner
dgc@kernel.org

Re: improve log context annotations

[Christoph Hellwig]

On Wed, Jul 22, 2026 at 10:08:34AM +1000, Dave Chinner wrote:
> Honestly, I'd prefer to kill the annotations and replace them with
> asserts that the lock is held so there's actual runtime validation
> that catches errors when they occur. These lock annotations don't
> tell you that the locking is correct, just that the function
> manipulates a lock.

Well, very strong disagreement here, as compile time annotations
are much more poweful as they are statically verified.

> The sparse annotations have always been more of an annoyance and
> reactive, because we only use them sporadically to suppress errors.

And the difference is that with the clang context annotations we
now have a reliably architected way to enforce them, which get
picked up all over the kernel.

The spare checks are gone, so there is no point in discussing them
any more.