On 11/14/18 5:52 PM, shenghui wrote:
>
>
> On 11/14/2018 03:04 PM, Coly Li wrote:
>> 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 :-)
>
> Got it. Will figure out a new patch.
Indeed I expect two patches, one for setting dc->writeback_running =
true properly, one for rejecting setting it to 1 via sysfs if the
kthread is not created yet.
Thanks.
Coly Li
