On 10/9/18 8:36 PM, Shenghui Wang wrote:
> A fresh backing device is not attached to any cache_set, and
> has no writeback kthread created until first attached to some
> cache_set.
>
> But bch_cached_dev_writeback_init run
> "
> dc->writeback_running = true;
> WARN_ON(test_and_clear_bit(BCACHE_DEV_WB_RUNNING,
> &dc->disk.flags));
> "
> for any newly formatted backing devices.
>
Hi Shenghui,
> For a fresh standalone backing device, we can get something like
> following even if no writeback kthread created:
> ------------------------
> /sys/block/bcache0/bcache$ cat writeback_rate_debug
> rate: 512.0k/sec
> dirty: 0.0k
> target: 0.0k
> proportional: 0.0k
> integral: 0.0k
> change: 0.0k/sec
> next io: -15427384ms
>
So the above output is OK for you?
> /sys/block/bcache0/bcache$ cat writeback_running
> 1
>
I see, the out put here should be 0.
> Besides that, "echo 1 > writeback_running" will mark writeback_running
> when no writeback kthread created as "d_strtoul(writeback_running)" will
> simply set dc->writeback_running without checking the existence of
> dc->writeback_thread.
>
> This patch fixes above 2 issues: for a fresh standalone backing device,
> * dc->writeback_running is set to false, until attached to cache_set.
> * "echo 1 > writeback_running" will not mark writeback_running.
>
Yes, sounds reasonable.
> Signed-off-by: Shenghui Wang <shhuiw@xxxxxxxxxxx>
> ---
> drivers/md/bcache/writeback.c | 2 +-
> drivers/md/bcache/writeback.h | 5 ++++-
> 2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/md/bcache/writeback.c b/drivers/md/bcache/writeback.c
> index 08c3a9f9676c..2443621d07d6 100644
> --- a/drivers/md/bcache/writeback.c
> +++ b/drivers/md/bcache/writeback.c
> @@ -777,7 +777,7 @@ void bch_cached_dev_writeback_init(struct cached_dev *dc)
> bch_keybuf_init(&dc->writeback_keys);
>
> dc->writeback_metadata = true;
> - dc->writeback_running = true;
> + dc->writeback_running = false;
> dc->writeback_percent = 10;
> dc->writeback_delay = 30;
> atomic_long_set(&dc->writeback_rate.rate, 1024);
> diff --git a/drivers/md/bcache/writeback.h b/drivers/md/bcache/writeback.h
It is good to me to set dc->writeback_running to false here.
> index d2b9fdbc8994..0601fe1b40f4 100644
> --- a/drivers/md/bcache/writeback.h
> +++ b/drivers/md/bcache/writeback.h
> @@ -78,8 +78,11 @@ static inline bool should_writeback(struct cached_dev *dc, struct bio *bio,
>
> static inline void bch_writeback_queue(struct cached_dev *dc)
> {
> - if (!IS_ERR_OR_NULL(dc->writeback_thread))
> + if (!IS_ERR_OR_NULL(dc->writeback_thread)) {
> wake_up_process(dc->writeback_thread);
> + dc->writeback_running = true;
> + } else
> + dc->writeback_running = false;
> }
>
I have question for the above change.
Indeed once a wrteback thread is created, dc->writeback_runing will be
always true, and won't be false again. So I suggest to set
dc->writeback_runing to true in bch_cached_dev_writeback_start() after
dc->writeback_thread is created.
""echo 1 > writeback_running" will not mark writeback_running." should
be fixed in sysfs interface, somewhere in STORE(bch_cached_dev),
386
387 if (attr == &sysfs_writeback_running)
388 bch_writeback_queue(dc);
Don't touch bch_writeback_queue() for your fix :-)
Thanks for your changes, waiting for the update patches :-)
Coly Li
