Re: [PATCH 7/8] wbt: add general throttling mechanism

From: xiakaixu <xiakaixu@huawei.com>
To: Jens Axboe <axboe@fb.com>
Cc: <linux-kernel@vger.kernel.org>, <linux-fsdevel@vger.kernel.org>,
	<linux-block@vger.kernel.org>, <jack@suse.cz>,
	<dchinner@redhat.com>, <sedat.dilek@gmail.com>,
	"miaoxie (A)" <miaoxie@huawei.com>,
	Huxinwei <huxinwei@huawei.com>, Bintian <bintian.wang@huawei.com>
Subject: Re: [PATCH 7/8] wbt: add general throttling mechanism
Date: Thu, 28 Apr 2016 11:29:08 +0800	[thread overview]
Message-ID: <57218384.4080609@huawei.com> (raw)
In-Reply-To: <5720D90F.6000609@fb.com>

于 2016/4/27 23:21, Jens Axboe 写道:
> On 04/27/2016 06:06 AM, xiakaixu wrote:
>>> +void __wbt_done(struct rq_wb *rwb)
>>> +{
>>> +    int inflight, limit = rwb->wb_normal;
>>> +
>>> +    /*
>>> +     * If the device does write back caching, drop further down
>>> +     * before we wake people up.
>>> +     */
>>> +    if (rwb->wc && !atomic_read(&rwb->bdi->wb.dirty_sleeping))
>>> +        limit = 0;
>>> +    else
>>> +        limit = rwb->wb_normal;
>>> +
>>> +    /*
>>> +     * Don't wake anyone up if we are above the normal limit. If
>>> +     * throttling got disabled (limit == 0) with waiters, ensure
>>> +     * that we wake them up.
>>> +     */
>>> +    inflight = atomic_dec_return(&rwb->inflight);
>>> +    if (limit && inflight >= limit) {
>>> +        if (!rwb->wb_max)
>>> +            wake_up_all(&rwb->wait);
>>> +        return;
>>> +    }
>>> +
>> Hi Jens,
>>
>> Just a little confused about this. The rwb->wb_max can't be 0 if the variable
>> 'limit' does not equal to 0. So the if (!rwb->wb_max) branch maybe does not
>> make sense.
> 
> You are right, it doesn't make a lot of sense. I think it suffers from code shuffling. How about the attached? The important part is that we wake up waiters, if wbt got disabled while we had tracked IO in flight.
>
Hi Jens,

The modified patch in another mail looks better. Maybe there are still
some places coube be improved. You can find them in that mail.

-- 
Regards
Kaixu Xia