Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [PATCH -next v2 9/9] blk-iocost: fix walk_list corruption
To:     Tejun Heo <tj@kernel.org>, Yu Kuai <yukuai1@huaweicloud.com>
Cc:     Li Nan <linan122@huawei.com>, josef@toxicpanda.com,
        axboe@kernel.dk, cgroups@vger.kernel.org,
        linux-block@vger.kernel.org, linux-kernel@vger.kernel.org,
        yi.zhang@huawei.com, "yukuai (C)" <yukuai3@huawei.com>
References: <20221130132156.2836184-1-linan122@huawei.com>
 <20221130132156.2836184-10-linan122@huawei.com>
 <Y4fEKZy4rTE5rG/5@slm.duckdns.org>
 <c028dd77-cabf-edd6-c893-8ee24762ac8c@huaweicloud.com>
 <Y4h7RxdT83g+zFN0@slm.duckdns.org>
 <de04965e-1341-3053-0f4b-395b8390d00c@huaweicloud.com>
 <Y4iB97kcdKHEQP86@slm.duckdns.org>
From:   Yu Kuai <yukuai1@huaweicloud.com>
Message-ID: <0f55cf25-d3f9-4f12-220e-9d06a601ed7a@huaweicloud.com>
Date:   Thu, 1 Dec 2022 21:43:25 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.8.0
MIME-Version: 1.0
In-Reply-To: <Y4iB97kcdKHEQP86@slm.duckdns.org>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


在 2022/12/01 18:29, Tejun Heo 写道:
> On Thu, Dec 01, 2022 at 06:14:32PM +0800, Yu Kuai wrote:
>> Hi,
>>
>> 在 2022/12/01 18:00, Tejun Heo 写道:
>>> On Thu, Dec 01, 2022 at 09:19:54AM +0800, Yu Kuai wrote:
>>>>>> diff --git a/block/blk-iocost.c b/block/blk-iocost.c
>>>>>> index 710cf63a1643..d2b873908f88 100644
>>>>>> --- a/block/blk-iocost.c
>>>>>> +++ b/block/blk-iocost.c
>>>>>> @@ -2813,13 +2813,14 @@ static void ioc_rqos_exit(struct rq_qos *rqos)
>>>>>>     {
>>>>>>     	struct ioc *ioc = rqos_to_ioc(rqos);
>>>>>> +	del_timer_sync(&ioc->timer);
>>>>>> +
>>>>>>     	blkcg_deactivate_policy(rqos->q, &blkcg_policy_iocost);
>>>>>>     	spin_lock_irq(&ioc->lock);
>>>>>>     	ioc->running = IOC_STOP;
>>>>>>     	spin_unlock_irq(&ioc->lock);
>>>>>> -	del_timer_sync(&ioc->timer);
>>>>>
>>>>> I don't about this workaround. Let's fix properly?
>>>>
>>>> Ok, and by the way, is there any reason to delete timer after
>>>> deactivate policy? This seems a litter wreid to me.
>>>
>>> ioc->running is what controls whether the timer gets rescheduled or not. If
>>> we don't shut that down, the timer may as well get rescheduled after being
>>> deleted. Here, the only extra activation point is IO issue which shouldn't
>>> trigger during rq_qos_exit, so the ordering shouldn't matter but this is the
>>> right order for anything which can get restarted.
>>
>> Thanks for the explanation.
>>
>> I'm trying to figure out how to make sure child blkg pins it's parent,
>> btw, do you think following cleanup make sense?
> 
> It's on you to explain why any change that you're suggesting is better and
> safe. I know it's not intentional but you're repeatedly suggesting operation
> reorderings in code paths which are really sensitive to ordering at least
> seemingly without putting much effort into thinking through the side
> effects. This costs disproportionate amount of review bandwidth, and
> increases the chance of new subtle bugs. Can you please slow down a bit and
> be more deliberate?

Thanks for the suggestion, I'll pay close attention to explain this "why
the change is better and safe". And sorry for the review pressure. ????

> 
> Thanks.
>