Re: [PATCH v4 3/3] block: prevent race condition on bi_status in __bio_chain_endio

Andreas Gruenbacher <[email protected]> 于2025年12月3日周三 05:15写道：
>
> On Tue, Dec 2, 2025 at 6:48 AM Christoph Hellwig <[email protected]> wrote:
> > On Mon, Dec 01, 2025 at 02:07:07PM +0100, Andreas Gruenbacher wrote:
> > > On Mon, Dec 1, 2025 at 12:25 PM Christoph Hellwig <[email protected]> wrote:
> > > > On Mon, Dec 01, 2025 at 11:22:32AM +0100, Andreas Gruenbacher wrote:
> > > > > > -       if (bio->bi_status && !parent->bi_status)
> > > > > > -               parent->bi_status = bio->bi_status;
> > > > > > +       if (bio->bi_status)
> > > > > > +               cmpxchg(&parent->bi_status, 0, bio->bi_status);
> > > > >
> > > > > Hmm. I don't think cmpxchg() actually is of any value here: for all
> > > > > the chained bios, bi_status is initialized to 0, and it is only set
> > > > > again (to a non-0 value) when a failure occurs. When there are
> > > > > multiple failures, we only need to make sure that one of those
> > > > > failures is eventually reported, but for that, a simple assignment is
> > > > > enough here.
> > > >
> > > > A simple assignment doesn't guarantee atomicy.
> > >
> > > Well, we've already discussed that bi_status is a single byte and so
> > > tearing won't be an issue. Otherwise, WRITE_ONCE() would still be
> > > enough here.
> >
> > No.  At least older alpha can tear byte updates as they need a
> > read-modify-write cycle.
>
> I know this used to be a thing in the past, but to see that none of
> that is relevant anymore today, have a look at where [*] quotes the
> C11 standard:
>
>         memory location
>                 either an object of scalar type, or a maximal sequence
>                 of adjacent bit-fields all having nonzero width
>
>                 NOTE 1: Two threads of execution can update and access
>                 separate memory locations without interfering with
>                 each other.
>
>                 NOTE 2: A bit-field and an adjacent non-bit-field member
>                 are in separate memory locations. The same applies
>                 to two bit-fields, if one is declared inside a nested
>                 structure declaration and the other is not, or if the two
>                 are separated by a zero-length bit-field declaration,
>                 or if they are separated by a non-bit-field member
>                 declaration. It is not safe to concurrently update two
>                 bit-fields in the same structure if all members declared
>                 between them are also bit-fields, no matter what the
>                 sizes of those intervening bit-fields happen to be.
>
> [*] Documentation/memory-barriers.txt
>
> > But even on normal x86 the check and the update would be racy.
>
> There is no check and update (RMW), though. Quoting what I wrote
> earlier in this thread:
>
> On Mon, Dec 1, 2025 at 11:22 AM Andreas Gruenbacher <[email protected]> wrote:
> > Hmm. I don't think cmpxchg() actually is of any value here: for all
> > the chained bios, bi_status is initialized to 0, and it is only set
> > again (to a non-0 value) when a failure occurs. When there are
> > multiple failures, we only need to make sure that one of those
> > failures is eventually reported, but for that, a simple assignment is
> > enough here. The cmpxchg() won't guarantee that a specific error value
> > will survive; it all still depends on the timing. The cmpxchg() only
> > makes it look like something special is happening here with respect to
> > ordering.
>
> So with or without the cmpxchg(), if there are multiple errors, we
> won't know which bi_status code will survive, but we do know that we
> will end up with one of those error codes.
>

Thank you for sharing your insights—I found the discussion very enlightening.

While I agree with Andreas’s perspective, I also very much appreciate
the clarity
and precision offered by the cmpxchg() approach. That’s why when Christoph
suggested it, I was happy to incorporate it into the code.

But a cmpxchg is a little bit redundant here.
so we will change it to the simple assignment:

-       if (bio->bi_status && !parent->bi_status)
                 parent->bi_status = bio->bi_status;
+       if (bio->bi_status)
                 parent->bi_status = bio->bi_status;

I will integrate this discussion into the commit message, it is very insightful.

Thanks,
Shida

> Andreas
>