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

[Christoph Hellwig <hch@lst.de>]

On Mon, Dec 01, 2025 at 02:07:07PM +0100, Andreas Gruenbacher wrote:
> On Mon, Dec 1, 2025 at 12:25 PM Christoph Hellwig <hch@infradead.org> 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.  But even on normal x86 the check and the update
would be racy.  The cmpxchg makes the intentions very clear, works
everywhere and given it only happens in the error path does not create
any fast path overhead.