[7/8] wbt: add general throttling mechanism

Commit Message

Jens Axboe April 27, 2016, 3:21 p.m. UTC

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.

Comments

xiakaixu April 28, 2016, 3:29 a.m. UTC | #1

? 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.

Patch

diff --git a/lib/wbt.c b/lib/wbt.c
index 650da911f24f..a6b80c135510 100644
--- a/lib/wbt.c
+++ b/lib/wbt.c
@@ -98,18 +98,23 @@  void __wbt_done(struct rq_wb *rwb)
 	else
 		limit = rwb->wb_normal;
 
+	inflight = atomic_dec_return(&rwb->inflight);
+
 	/*
-	 * 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.
+	 * wbt got disabled with IO in flight. Wake up any potential
+	 * waiters, we don't have to do more than that.
 	 */
-	inflight = atomic_dec_return(&rwb->inflight);
-	if (limit && inflight >= limit) {
-		if (!rwb->wb_max)
-			wake_up_all(&rwb->wait);
+	if (!rwb_enabled(rwb)) {
+		wake_up_all(&rwb->wait);
 		return;
 	}
 
+	/*
+	 * Don't wake anyone up if we are above the normal limit.
+	 */
+	if (inflight >= limit)
+		return;
+
 	if (waitqueue_active(&rwb->wait)) {
 		int diff = limit - inflight;