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

From: Yufen Yu <yuyufen@huawei.com>
To: Keith Busch <kbusch@kernel.org>
Cc: <axboe@kernel.dk>, <linux-block@vger.kernel.org>, <hch@lst.de>,
	<ming.lei@redhat.com>, <bvanassche@acm.org>
Subject: Re: [RFC PATCH] block: protect bi_status with spinlock
Date: Mon, 29 Mar 2021 16:08:21 +0800	[thread overview]
Message-ID: <e6ec05a8-1db5-6cb2-aa76-987ce6f1852c@huawei.com> (raw)
In-Reply-To: <20210329030246.GA15392@redsun51.ssa.fujisawa.hgst.com>

On 2021/3/29 11:02, 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;
> .
>

Yeah, this is more smart. Thanks a lot for your suggestion.
I will send v2.

Thanks,
Yufen