Re: [PATCH] bcache: PI controller for writeback rate V2
From: Coly Li <hidden>
Date: 2017-09-26 04:46:36
On 2017/9/8 上午11:17, Michael Lyle wrote:
bcache uses a control system to attempt to keep the amount of dirty data in cache at a user-configured level, while not responding excessively to transients and variations in write rate. Previously, the system was a PD controller; but the output from it was integrated, turning the Proportional term into an Integral term, and turning the Derivative term into a crude Proportional term. Performance of the controller has been uneven in production, and it has tended to respond slowly, oscillate, and overshoot. This patch set replaces the current control system with an explicit PI controller and tuning that should be correct for most hardware. By default, it attempts to write at a rate that would retire 1/40th of the current excess blocks per second. An integral term in turn works to remove steady state errors. IMO, this yields benefits in simplicity (removing weighted average filtering, etc) and system performance. Another small change is a tunable parameter is introduced to allow the user to specify a minimum rate at which dirty blocks are retired. Ideally one would set this writeback_rate_minimum to a small percentage of disk bandwidth, allowing the dirty data to be slowly cleaned out when the system is inactive. The old behavior would try and retire 1 sector/second, and the new default is 5 sectors/second. Signed-off-by: Michael Lyle <redacted> --- drivers/md/bcache/bcache.h | 9 +++-- drivers/md/bcache/sysfs.c | 20 ++++++----- drivers/md/bcache/writeback.c | 84 +++++++++++++++++++++++-------------------- 3 files changed, 61 insertions(+), 52 deletions(-)
[snip]
quoted hunk ↗ jump to hunk
@@ -492,8 +499,6 @@ void bch_sectors_dirty_init(struct cached_dev *dc) bch_btree_map_keys(&op.op, dc->disk.c, &KEY(op.inode, 0, 0), sectors_dirty_init_fn, 0); - - dc->disk.sectors_dirty_last = bcache_dev_sectors_dirty(&dc->disk); }
Hi Mike, I have no question for most part of this patch, it is clear to me. I start to run this PI controller in my test kernel, so far it works fine.
quoted hunk ↗ jump to hunk
void bch_cached_dev_writeback_init(struct cached_dev *dc)@@ -507,10 +512,11 @@ void bch_cached_dev_writeback_init(struct cached_dev *dc) dc->writeback_percent = 10; dc->writeback_delay = 30; dc->writeback_rate.rate = 1024; + dc->writeback_rate_minimum = 5;
Here you set dc->writeback_rate_minimum to 5, and in your next patch it is set to 8. I know 1 is an inaccurate value, could you please set it to 8 here ?
dc->writeback_rate_update_seconds = 5; - dc->writeback_rate_d_term = 30; - dc->writeback_rate_p_term_inverse = 6000; + dc->writeback_rate_p_term_inverse = 40; + dc->writeback_rate_i_term_inverse = 10000; INIT_DELAYED_WORK(&dc->writeback_rate_update, update_writeback_rate); }
We discussed why you set dc->writeback_rate_p_term_inverse to 40, could you please to copy (maybe modify a bit) the discussion into patch commit log, to explain why the parameters are decided. I have no more comments. Looking for ward to your next version patch. Thanks for your effort. -- Coly Li