Re: [RFC PATCH] block: protect bi_status with spinlock

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:

quoted

 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.