[PATCH 1/1] block: prevent calls to should_fail_bio() optimized by gcc

[PATCH 1/1] block: prevent calls to should_fail_bio() optimized by gcc

[Prasad Singamsetty]

When CONFIG_FAIL_MAKE_REQUEST is not enabled, gcc may optimize out
calls to should_fail_bio() because the content of should_fail_bio()
is empty returning always 'false'. The gcc compiler then detects
the function call to should_fail_bio() being empty and optimizes
out the call to it. This prevents block I/O error injection programs
attached to it from working. The compiler is not aware of the side
effect of calling this probe function.

This issue is seen with gcc compiler version 14. Previous versions
of gcc compiler (checked 9, 11, 12, 13) don't have this optimization.

Clang compiler (seen with version 18.1.18) has the same issue of
optimizing out calls to should_fail_bio().

Adding the compiler attribute __attribute__((noipa)) to should_fail_bio()
function avoids this optimization. This attribute is available starting
from gcc compiler version 8.1. Adding this attribute avoids the issue
and only side effect is the slight increase in the code size of the
binary blk-core.o (e.g. 16 bytes with gcc version 11 and 48 bytes
with gcc version 14) as expected.

For Clang case, 'noipa' attribute is not available but it has a similar
attribute, 'optnone', with the same effect and fixes the issue. So, the
patch adds either 'noipa' attribute for gcc case or 'optnone' for
Clang case.

Cc: stable@vger.kernel.org
Signed-off-by: Prasad Singamsetty <prasad.singamsetty@oracle.com>
---
 block/blk-core.c                    |  2 +-
 include/linux/compiler_attributes.h | 15 +++++++++++++++
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index e8cc270a453f..fb1da9ea92bb 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -539,7 +539,7 @@ static inline void bio_check_ro(struct bio *bio)
 	}
 }
 
-static noinline int should_fail_bio(struct bio *bio)
+static noipa noinline int should_fail_bio(struct bio *bio)
 {
 	if (should_fail_request(bdev_whole(bio->bi_bdev), bio->bi_iter.bi_size))
 		return -EIO;
diff --git a/include/linux/compiler_attributes.h b/include/linux/compiler_attributes.h
index c16d4199bf92..9d4726c3426e 100644
--- a/include/linux/compiler_attributes.h
+++ b/include/linux/compiler_attributes.h
@@ -230,6 +230,21 @@
  */
 #define   noinline                      __attribute__((__noinline__))
 
+/*
+ * Optional: only supported since gcc >= 8
+ * Optional: Not supported by clang. "optnone" is used to
+ *	     disable all otipmizations
+ *
+ * gcc: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#index-noipa-function-attribute
+ */
+#if __has_attribute(__noipa__)
+#define   noipa                      __attribute__((__noipa__))
+#elif __has_attribute(__optnone__)
+#define   noipa                      __attribute__((__optnone__))
+#else
+#define   noipa
+#endif
+
 /*
  * Optional: only supported since gcc >= 8
  * Optional: not supported by clang

base-commit: 1a1d569a75f3ab2923cb62daf356d102e4df2b86
-- 
2.43.5

Re: [PATCH 1/1] block: prevent calls to should_fail_bio() optimized by gcc

[Jens Axboe]

On 4/17/25 10:34 AM, Prasad Singamsetty wrote:
> When CONFIG_FAIL_MAKE_REQUEST is not enabled, gcc may optimize out
> calls to should_fail_bio() because the content of should_fail_bio()
> is empty returning always 'false'. The gcc compiler then detects
> the function call to should_fail_bio() being empty and optimizes
> out the call to it. This prevents block I/O error injection programs
> attached to it from working. The compiler is not aware of the side
> effect of calling this probe function.

That's working as designed and is what we would want. Rather than patch
around that, why not just enable CONFIG_FAIL_MAKE_REQUEST for whatever
you're trying to do?

-- 
Jens Axboe

Re: [PATCH 1/1] block: prevent calls to should_fail_bio() optimized by gcc

[Miguel Ojeda]

On Thu, Apr 17, 2025 at 6:31 PM Prasad Singamsetty
<prasad.singamsetty@oracle.com> wrote:
>
> When CONFIG_FAIL_MAKE_REQUEST is not enabled, gcc may optimize out
> calls to should_fail_bio() because the content of should_fail_bio()
> is empty returning always 'false'. The gcc compiler then detects

`should_fail_request` is the one that returns `false`, no? i.e.
`should_fail_bio` is the one that gets optimized due to that to return
`0`.

> This issue is seen with gcc compiler version 14. Previous versions
> of gcc compiler (checked 9, 11, 12, 13) don't have this optimization.

I wonder if whatever changed could be making other things fail.

Anyway, given what Jens said, this may not be needed to begin with.

But if it is, then as far as I recall, we try to avoid that kind of
"don't optimize" attribute (the one you used for Clang). So it would
be nice to find other ways to do it.

For instance, if what you need is to keep the actual calls to
`should_fail_bio` (not `should_fail_request`), then adding a side
effect like the GCC manual suggests in the `noinline` attribute seems
also to work from a quick test:

    Even if a function is declared with the noinline attribute, there
are optimizations other than inlining that can cause calls to be
optimized away if it does not have side effects, although the function
call is live. To keep such calls from being optimized away, put asm
(""); in the called function, to serve as a special side effect.

And if you also need to keep the function name emitted as-is, then
`__used` is also needed in GCC, but that would make sense to add
anyway if these functions were expected to be there. Given what Jens
said, I imagine that is why they aren't annotated like that already.

I hope that helps.

Cheers,
Miguel

Re: [PATCH 1/1] block: prevent calls to should_fail_bio() optimized by gcc

[Prasad Singamsetty]

> On Apr 17, 2025, at 10:50 AM, Jens Axboe <axboe@kernel.dk> wrote:
> 
> On 4/17/25 10:34 AM, Prasad Singamsetty wrote:
>> When CONFIG_FAIL_MAKE_REQUEST is not enabled, gcc may optimize out
>> calls to should_fail_bio() because the content of should_fail_bio()
>> is empty returning always 'false'. The gcc compiler then detects
>> the function call to should_fail_bio() being empty and optimizes
>> out the call to it. This prevents block I/O error injection programs
>> attached to it from working. The compiler is not aware of the side
>> effect of calling this probe function.
> 
> That's working as designed and is what we would want. Rather than patch
> around that, why not just enable CONFIG_FAIL_MAKE_REQUEST for whatever
> you're trying to do?

We do not have CONFIG_FAIL_MAKE_REQUEST enabled on the production (UEK)
Kernels. We have a bpf program that was working before we switched to gcc
version 14 compiler. We tried with asm(“”) option to establish side effect
and that also works. We can use that option as Miguel suggested.

Thanks,
—Prasad

Re: [PATCH 1/1] block: prevent calls to should_fail_bio() optimized by gcc

[Prasad Singamsetty]

> On Apr 17, 2025, at 10:55 AM, Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote:
> 
> On Thu, Apr 17, 2025 at 6:31 PM Prasad Singamsetty
> <prasad.singamsetty@oracle.com> wrote:
>> 
>> When CONFIG_FAIL_MAKE_REQUEST is not enabled, gcc may optimize out
>> calls to should_fail_bio() because the content of should_fail_bio()
>> is empty returning always 'false'. The gcc compiler then detects
> 
> `should_fail_request` is the one that returns `false`, no? i.e.
> `should_fail_bio` is the one that gets optimized due to that to return
> `0`.
> 

That is correct.  I will update the commit message text.

>> This issue is seen with gcc compiler version 14. Previous versions
>> of gcc compiler (checked 9, 11, 12, 13) don't have this optimization.
> 
> I wonder if whatever changed could be making other things fail.
> 
> Anyway, given what Jens said, this may not be needed to begin with.
> 
> But if it is, then as far as I recall, we try to avoid that kind of
> "don't optimize" attribute (the one you used for Clang). So it would
> be nice to find other ways to do it.

Thanks for the clarification.

> 
> For instance, if what you need is to keep the actual calls to
> `should_fail_bio` (not `should_fail_request`), then adding a side
> effect like the GCC manual suggests in the `noinline` attribute seems
> also to work from a quick test:
> 
>    Even if a function is declared with the noinline attribute, there
> are optimizations other than inlining that can cause calls to be
> optimized away if it does not have side effects, although the function
> call is live. To keep such calls from being optimized away, put asm
> (""); in the called function, to serve as a special side effect.

Thanks for the clarifications. The asm(“”) option also works and we will
use that option to fix the issue as that is a preferred method.

> 
> And if you also need to keep the function name emitted as-is, then
> `__used` is also needed in GCC, but that would make sense to add
> anyway if these functions were expected to be there. Given what Jens
> said, I imagine that is why they aren't annotated like that already.

Currently GCC is keeping the function even though the call is optimized out.
I will update the patch to use asm(“”) option and resend it for review.

Thanks,
—Prasad

> 
> I hope that helps.
> 
> Cheers,
> Miguel

Re: [PATCH 1/1] block: prevent calls to should_fail_bio() optimized by gcc

[Christoph Hellwig]

On Thu, Apr 17, 2025 at 09:34:32AM -0700, Prasad Singamsetty wrote:
> When CONFIG_FAIL_MAKE_REQUEST is not enabled, gcc may optimize out
> calls to should_fail_bio() because the content of should_fail_bio()
> is empty returning always 'false'. The gcc compiler then detects
> the function call to should_fail_bio() being empty and optimizes
> out the call to it.

Yes, that's intentional and a good thing becaue we don't want to pay
the overhead for the fault injetion helper.

> This prevents block I/O error injection programs
> attached to it from working. The compiler is not aware of the side
> effect of calling this probe function.

I can't see any attachment.  But if this is a bpf program or kernel
module using kprobes then there is absolutely zero expectation that
a static inline function actually exists in the binary kernel, so
you should not rely on that.

Re: [PATCH 1/1] block: prevent calls to should_fail_bio() optimized by gcc

[Prasad Singamsetty]

> On Apr 21, 2025, at 4:51 AM, Christoph Hellwig <hch@infradead.org> wrote:
> 
> On Thu, Apr 17, 2025 at 09:34:32AM -0700, Prasad Singamsetty wrote:
>> When CONFIG_FAIL_MAKE_REQUEST is not enabled, gcc may optimize out
>> calls to should_fail_bio() because the content of should_fail_bio()
>> is empty returning always 'false'. The gcc compiler then detects
>> the function call to should_fail_bio() being empty and optimizes
>> out the call to it.
> 
> Yes, that's intentional and a good thing becaue we don't want to pay
> the overhead for the fault injetion helper.
> 
>> This prevents block I/O error injection programs
>> attached to it from working. The compiler is not aware of the side
>> effect of calling this probe function.
> 
> I can't see any attachment.  But if this is a bpf program or kernel
> module using kprobes then there is absolutely zero expectation that
> a static inline function actually exists in the binary kernel, so
> you should not rely on that.
> 

Thank you Christoph, for the clarifications and comments. Yes, it is a bpf program that
gets loaded/attached to the kprobe function, should_fail_bio(). We used this program
for a use case for block i/o filtering and it was working fine until we moved to gcc 14
compiler. Kernel has this kprobe function defined so the bpf program gets attached
fine but not getting invoked as the call is optimized out.

Agree that any additional overhead to this code path is not acceptable and
in this case it is an existing overhead prior to the latest compiler version usage.

We are also aware of the block filter project that was active and not sure about
the current status of it. We will look into that. 

Thanks,
—Prasad

Re: [PATCH 1/1] block: prevent calls to should_fail_bio() optimized by gcc

[Christoph Hellwig]

On Tue, Apr 22, 2025 at 09:58:34PM +0000, Prasad Singamsetty wrote:
> We are also aware of the block filter project that was active and not sure about
> the current status of it. We will look into that. 

Please help with upstreaming it and your code depending on it.