lkml.org 
[lkml]   [2016]   [Apr]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCHSET v5] Make background writeback great again for the first time
From
Date
On 04/27/2016 10:06 PM, xiakaixu wrote:
>> diff --git a/lib/wbt.c b/lib/wbt.c
>> index 650da911f24f..322f5e04e994 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;
> Hi Jens,
>
> This statement 'limit = rwb->wb_normal' is executed twice, maybe once is
> enough. It is not a big deal anyway :)

I'll clean that up, thanks for noticing. No functional difference.

> Another question about this if branch:
>
> if (rwb->wc && !atomic_read(&rwb->bdi->wb.dirty_sleeping))
> limit = 0;
>
> I can't follow the logic of this if branch. why set limit equal to 0
> when the device supports write back caches and there are tasks being
> limited in balance_dirty_pages(). Could you pelase give more info
> about this ? Thanks!

Sure. So for write back caching, we have to try a bit harder to ensure
that the device doesn't build up long internal queues with a lot of
dirty data in the cache. So for the case where we have write back
caching AND we don't have anyone waiting for the IO, allow the queue
depth to drain to zero before building it back up again.

Does that make sense?

>>
>> + 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;
>> }
>
> Maybe it is better that executing this if branch earlier. So we can wake up
> potential waiters in time when wbt got disabled.

The !rwb_enabled() case will only happen if someone disabled wbt while
we had tracked IO in flight. We have to it below the
atomic_dec_return(), so we could reorder that to be at the front.
Ideally we just want it out-of-line instead, as it's the unexpected
slower path.

--
Jens Axboe

\
 
 \ /
  Last update: 2016-04-28 20:41    [W:0.162 / U:0.380 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site