Re: [PATCH v1 1/9] ext2: fix ext2_xattr_delete_inode() context analysis warning

[Timothy Day]

On Mon, 13 Jul 2026 10:32:56 -0400, Theodore Tso wrote:
> On Sun, Jul 12, 2026 at 12:56:02PM -0500, Timothy Day wrote:
> > Clang reports the following warning when context analysis is enabled:
> >
> > fs/ext2/xattr.c:829:6: error: rw_semaphore 'EXT2_I().xattr_sem' is not \
> >   held on every path through here [-Werror,-Wthread-safety-analysis]
> >   829 |         if (WARN_ON_ONCE(!down_write_trylock(&EXT2_I(inode)->xattr_sem)))
> >       |             ^
> >
> > When WARN_ON_ONCE() wraps down_write_trylock(), it adds an unlikely()
> > annotation which hides the conditional acquire annotation from Clang's
> > context analysis. This triggers a false positive. Pull the trylock out
> > of the macro to silence this warning.
>
> Does this mean that using Clang's context analysis means that we can't
> use unlikely()?  That seems.... unfortunate.  Is there any other way
> we can fix the false positive?

We can't wrap a function that acquires a context inside unlikely(). We
can use unlikely everywhere else. AFAICS, I think the trylock functions
are only problem right now. Grepping for ".*WARN.*trylock.*" and
".*unlikely.*trylock.*" shows a few other examples of code like this.
Not a huge amount, but not zero. I looked at a few other
series [1][2][3][4], but I haven't seen anyone do a similar conversion.

[1] (block) https://lore.kernel.org/all/91cb8c790fc8b26b8aa742569fbf8c2c1d099dac.1780682325.git.bvanassche@xxxxxxx/
[2] (drdb) https://lore.kernel.org/all/77d0fd75811d7f604fa80b5c93172b5653b52880.1781042470.git.bvanassche@xxxxxxx/
[3] (raid) https://lore.kernel.org/all/20260708090614.1431014-2-hch@xxxxxx/
[4] (nvme) https://lore.kernel.org/all/20260713115444.465704-19-nilay@xxxxxxxxxxxxx/

> Even disabling unlikely() if and only
> if Clang context analysis is enabled might be a better choice, since
> we don't necessarily need to build with context analysis enabled when
> building a production kernel.

I don't think we'd want to disable unlikely() globally when context
analysis is enabled. Only for cases where it'd conflict with the
context analysis annotation. Long term, I think we'd want context
analysis enabled by default for Clang builds.

I think we have a couple options:

a) Current fix - it's okay, but a bit ugly
b) Teach LLVM/Clang to handle this better - no idea how practical this is
c) Rework the code to not take the lock at all. The comment "We could
   as well just not acquire xattr_sem at all" seems to imply this is fine.
   Although I haven't tested it very much:

diff --git a/fs/ext2/xattr.c b/fs/ext2/xattr.c
index e55d16abf422f..d2aebf4b2ced9 100644
--- a/fs/ext2/xattr.c
+++ b/fs/ext2/xattr.c
@@ -815,6 +815,7 @@ ext2_xattr_set2(struct inode *inode, struct buffer_head *old_bh,
  */
 void
 ext2_xattr_delete_inode(struct inode *inode)
+       __must_not_hold(&EXT2_I(inode)->xattr_sem)
 {
        struct buffer_head *bh = NULL;
        struct ext2_sb_info *sbi = EXT2_SB(inode->i_sb);
@@ -826,8 +827,7 @@ ext2_xattr_delete_inode(struct inode *inode)
         * here to avoid false-positive warning from lockdep about reclaim
         * circular dependency.
         */
-       if (WARN_ON_ONCE(!down_write_trylock(&EXT2_I(inode)->xattr_sem)))
-               return;
+       lockdep_assert_not_held(&EXT2_I(inode)->xattr_sem);
        if (!EXT2_I(inode)->i_file_acl)
                goto cleanup;
 
@@ -857,7 +857,6 @@ ext2_xattr_delete_inode(struct inode *inode)
 
 cleanup:
        brelse(bh);
-       up_write(&EXT2_I(inode)->xattr_sem);
 }

d) Use some macro magic in unlikely() to skip the annotation if
   wrapping a context-acquiring function.

I think a) or c) is most practical. Open to other ideas.

- Tim Day