On Mon, Mar 29, 2021 at 12:02:46PM +0900, Keith Busch wrote:
> On Sun, Mar 28, 2021 at 10:23:37PM -0400, Yufen Yu wrote:
> > static struct bio *__bio_chain_endio(struct bio *bio)
> > {
> > struct bio *parent = bio->bi_private;
> > + unsigned long flags;
> >
> > + spin_lock_irqsave(&parent->bi_lock, flags);
> > if (!parent->bi_status)
> > parent->bi_status = bio->bi_status;
> > + spin_unlock_irqrestore(&parent->bi_lock, flags);
>
>
> I don't see a spin_lock_init() on this new lock, though a spinlock seems
> overkill here. If you need an atomic update, you could do:
>
> cmpxchg(&parent->bi_status, 0, bio->bi_status);
>
> But I don't even think that's necessary. There really is no need to set
> parent->bi_status if bio->bi_status is 0, so something like this should
> be fine:
>
> if (bio->bi_status && !parent->bi_status)
> parent->bi_status = bio->bi_status;
At very least we'd need READ_ONCE/WRITE_ONCE annotations, but yes,
those should be enough if every place that sets bi_status is careful.
