From: Coly Li <colyli@xxxxxxx>
Date: 2023-08-23 01:49:32
To: Mingzhe Zou <mingzhe.zou@xxxxxxxxxxxx>
Cc: bcache@xxxxxxxxxxxxxxxxxx,linux-bcache@xxxxxxxxxxxxxxx,zoumingzhe@xxxxxx
Subject: Re: [PATCH] bcache: fixup init dirty data errors>Hi Mingzhe,
>
>On Tue, Aug 22, 2023 at 06:19:58PM +0800, Mingzhe Zou wrote:
>> We found that after long run, the dirty_data of the bcache device
>> will have errors. This error cannot be eliminated unless re-register.
>
>Could you explain what the error was?
>
Hi, Coly
We discovered dirty_data was inaccurate a long time ago.
When writeback thread flushes all dirty data, dirty_data via sysfs shows that
there are still several K to tens of M of dirty data.
At that time, I thought it might be a calculation error at runtime, but after
reviewing the relevant code, no error was found.
Last month, our online environment found that a certain device had more than
200G of dirty_data. This brings us back to the question.
>>
>> We also found that reattach after detach, this error can accumulate.
>>
>
>Could you elaborate how the error can accumulate?
>
I found that when dirty_data, error, detach and then re-attach can not
eliminate the error, the error will continue.
In bch_cached_dev_attach(), after bch_sectors_dirty_init(), attach may still fail,
but dirty_data is not cleared when it fails
```
bch_sectors_dirty_init(&dc->disk);
ret = bch_cached_dev_run(dc);
if (ret && (ret != -EBUSY)) {
up_write(&dc->writeback_lock);
/*
* bch_register_lock is held, bcache_device_stop() is not
* able to be directly called. The kthread and kworker
* created previously in bch_cached_dev_writeback_start()
* have to be stopped manually here.
*/
kthread_stop(dc->writeback_thread);
dc->writeback_thread = NULL;
cancel_writeback_rate_update_dwork(dc);
pr_err("Couldn't run cached device %s",
dc->backing_dev_name);
return ret;
}
```
>
>> In bch_sectors_dirty_init(), all inode <= d->id keys will be recounted
>> again. This is wrong, we only need to count the keys of the current
>> device.
>>
>> Fixes: b144e45fc576 ("bcache: make bch_sectors_dirty_init() to be multithreaded")
>> Signed-off-by: Mingzhe Zou <mingzhe.zou@xxxxxxxxxxxx>
>> ---
>> drivers/md/bcache/writeback.c | 7 ++++++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/md/bcache/writeback.c b/drivers/md/bcache/writeback.c
>> index 24c049067f61..71d0dabcbf9d 100644
>> --- a/drivers/md/bcache/writeback.c
>> +++ b/drivers/md/bcache/writeback.c
>> @@ -983,6 +983,8 @@ void bch_sectors_dirty_init(struct bcache_device *d)
>> struct cache_set *c = d->c;
>> struct bch_dirty_init_state state;
>>
>> + atomic_long_set(&d->dirty_sectors, 0);
>> +
>
>The above change is not upstreamed yet, if you are fixing upstream code please
>avoid to use d->dirty_sectors here.
>
Yes, dirty_sectors is a set of resize patches submitted to the community before,
these patches have not been merged into upstream, I will remove this line in v2.
In fact, the change about dirty_sectors is only a prerequisite for resize, and it can
be promoted first. It will greatly reduce the memory requirements of high-capacity
devices.
>
>
>> /* Just count root keys if no leaf node */
>> rw_lock(0, c->root, c->root->level);
>> if (c->root->level == 0) {
>> @@ -991,8 +993,11 @@ void bch_sectors_dirty_init(struct bcache_device *d)
>> op.count = 0;
>>
>> for_each_key_filter(&c->root->keys,
>> - k, &iter, bch_ptr_invalid)
>> + k, &iter, bch_ptr_invalid) {
>> + if (KEY_INODE(k) != op.inode)
>> + continue;
>> sectors_dirty_init_fn(&op.op, c->root, k);
>> + }
>>
>
>Nice catch! IMHO if I take the above change, setting d->dirty_sectors by 0
>might be unncessary in ideal condition, am I right?
>
In bch_cached_dev_attach () may still fail and exit, I think it is necessary to clear 0.
mingzhe
>Thanks for the fixup.
>
>
>> rw_unlock(0, c->root);
>> return;
>> --
>> 2.17.1.windows.2
>>
>
>--
>Coly Li
