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

[Marco Elver]

On Mon, 13 Jul 2026 at 18:44, Timothy Day
<timday@xxxxxxxxxxxxxxxxxxxxxxx> wrote:
>
> 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

I'm not quite sure what's going on here yet, but
__builtin_expect-wrapped trylock function calls are supported by
Context Analysis (Clang's Thread Safety Analysis). Some of the
additional layers that WARN_ON_ONCE adds is likely throwing it off.
I'll take a look, but might be a week or so. We are already bumping
min Clang version for Context Analysis to Clang 23, so I'm hoping this
can be fixed in Clang 23 (which is not yet released).