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

[Marco Elver]

On Tue, 14 Jul 2026 at 00:27, Marco Elver <elver@xxxxxxxxxx> wrote:
>
> 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).

Relatively certain statement expressions were once again the problem -
this should fix it: https://github.com/llvm/llvm-project/pull/209330

I haven't had time to test the Clang fix on  fs/ext2/xattr.c with
context analysis enabled  (please do so if you already build Clang
yourself, otherwise I might get to it in ~week).