On Fri, Nov 28, 2025 at 9:32 AM zhangshida <[email protected]> wrote:> > From: Shida Zhang <[email protected]> > > Andreas point out that multiple completions can race setting > bi_status. > > The check (parent->bi_status) and the subsequent write are not an > atomic operation. The value of parent->bi_status could have changed > between the time you read it for the if check and the time you write > to it. So we use cmpxchg to fix the race, as suggested by Christoph. > > Suggested-by: Andreas Gruenbacher <[email protected]> > Suggested-by: Christoph Hellwig <[email protected]> > Signed-off-by: Shida Zhang <[email protected]> > --- > block/bio.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/block/bio.c b/block/bio.c > index 55c2c1a0020..aa43435c15f 100644 > --- a/block/bio.c > +++ b/block/bio.c > @@ -313,9 +313,12 @@ EXPORT_SYMBOL(bio_reset); > static struct bio *__bio_chain_endio(struct bio *bio) > { > struct bio *parent = bio->bi_private; > + blk_status_t *status = &parent->bi_status; > + blk_status_t new_status = bio->bi_status; > + > + if (new_status != BLK_STS_OK) > + cmpxchg(status, BLK_STS_OK, new_status);
This isn't wrong, but bi_status is explicitly set to 0 and compared with 0 all over the place, so putting in BLK_STS_OK here doesn't really help IMHO. > > - if (bio->bi_status && !parent->bi_status) > - parent->bi_status = bio->bi_status; > bio_put(bio); > return parent; > } > -- > 2.34.1 > Thanks, Andreas
