[PATCH] bcache: fixup init dirty data errors

[PATCH] bcache: fixup init dirty data errors

[Mingzhe Zou]

We found that after long run, the dirty_data of the bcache device
will have errors. This error cannot be eliminated unless re-register.

We also found that reattach after detach, this error can accumulate.

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@easystack.cn>
---
 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);
+
 	/* 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);
+		}
 
 		rw_unlock(0, c->root);
 		return;
-- 
2.17.1.windows.2

Re: [PATCH] bcache: fixup init dirty data errors

[Coly Li]

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?

> 
> We also found that reattach after detach, this error can accumulate.
>

Could you elaborate how the error can accumulate?

 
> 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@easystack.cn>
> ---
>  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.



>  	/* 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?

Thanks for the fixup.


>  		rw_unlock(0, c->root);
>  		return;
> -- 
> 2.17.1.windows.2
> 

-- 
Coly Li

Re: [PATCH] bcache: fixup init dirty data errors

[kernel test robot]

Hi Mingzhe,

kernel test robot noticed the following build errors:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.5-rc7 next-20230822]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Mingzhe-Zou/bcache-fixup-init-dirty-data-errors/20230822-182044
base:   linus/master
patch link:    https://lore.kernel.org/r/20230822101958.2577-1-mingzhe.zou%40easystack.cn
patch subject: [PATCH] bcache: fixup init dirty data errors
config: openrisc-randconfig-r011-20230822 (https://download.01.org/0day-ci/archive/20230823/202308230238.6Oed7zAt-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20230823/202308230238.6Oed7zAt-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308230238.6Oed7zAt-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/md/bcache/writeback.c: In function 'bch_sectors_dirty_init':
>> drivers/md/bcache/writeback.c:986:27: error: 'struct bcache_device' has no member named 'dirty_sectors'
     986 |         atomic_long_set(&d->dirty_sectors, 0);
         |                           ^~


vim +986 drivers/md/bcache/writeback.c

   976	
   977	void bch_sectors_dirty_init(struct bcache_device *d)
   978	{
   979		int i;
   980		struct bkey *k = NULL;
   981		struct btree_iter iter;
   982		struct sectors_dirty_init op;
   983		struct cache_set *c = d->c;
   984		struct bch_dirty_init_state state;
   985	
 > 986		atomic_long_set(&d->dirty_sectors, 0);
   987	
   988		/* Just count root keys if no leaf node */
   989		rw_lock(0, c->root, c->root->level);
   990		if (c->root->level == 0) {
   991			bch_btree_op_init(&op.op, -1);
   992			op.inode = d->id;
   993			op.count = 0;
   994	
   995			for_each_key_filter(&c->root->keys,
   996					    k, &iter, bch_ptr_invalid) {
   997				if (KEY_INODE(k) != op.inode)
   998					continue;
   999				sectors_dirty_init_fn(&op.op, c->root, k);
  1000			}
  1001	
  1002			rw_unlock(0, c->root);
  1003			return;
  1004		}
  1005	
  1006		memset(&state, 0, sizeof(struct bch_dirty_init_state));
  1007		state.c = c;
  1008		state.d = d;
  1009		state.total_threads = bch_btre_dirty_init_thread_nr();
  1010		state.key_idx = 0;
  1011		spin_lock_init(&state.idx_lock);
  1012		atomic_set(&state.started, 0);
  1013		atomic_set(&state.enough, 0);
  1014		init_waitqueue_head(&state.wait);
  1015	
  1016		for (i = 0; i < state.total_threads; i++) {
  1017			/* Fetch latest state.enough earlier */
  1018			smp_mb__before_atomic();
  1019			if (atomic_read(&state.enough))
  1020				break;
  1021	
  1022			state.infos[i].state = &state;
  1023			state.infos[i].thread =
  1024				kthread_run(bch_dirty_init_thread, &state.infos[i],
  1025					    "bch_dirtcnt[%d]", i);
  1026			if (IS_ERR(state.infos[i].thread)) {
  1027				pr_err("fails to run thread bch_dirty_init[%d]\n", i);
  1028				for (--i; i >= 0; i--)
  1029					kthread_stop(state.infos[i].thread);
  1030				goto out;
  1031			}
  1032			atomic_inc(&state.started);
  1033		}
  1034	
  1035	out:
  1036		/* Must wait for all threads to stop. */
  1037		wait_event(state.wait, atomic_read(&state.started) == 0);
  1038		rw_unlock(0, c->root);
  1039	}
  1040	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH] bcache: fixup init dirty data errors

[kernel test robot]

Hi Mingzhe,

kernel test robot noticed the following build errors:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.5-rc7 next-20230822]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Mingzhe-Zou/bcache-fixup-init-dirty-data-errors/20230822-182044
base:   linus/master
patch link:    https://lore.kernel.org/r/20230822101958.2577-1-mingzhe.zou%40easystack.cn
patch subject: [PATCH] bcache: fixup init dirty data errors
config: i386-randconfig-011-20230822 (https://download.01.org/0day-ci/archive/20230823/202308231002.XWYzjVgk-lkp@intel.com/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce: (https://download.01.org/0day-ci/archive/20230823/202308231002.XWYzjVgk-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308231002.XWYzjVgk-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/md/bcache/writeback.c:986:22: error: no member named 'dirty_sectors' in 'struct bcache_device'
           atomic_long_set(&d->dirty_sectors, 0);
                            ~  ^
   1 error generated.


vim +986 drivers/md/bcache/writeback.c

   976	
   977	void bch_sectors_dirty_init(struct bcache_device *d)
   978	{
   979		int i;
   980		struct bkey *k = NULL;
   981		struct btree_iter iter;
   982		struct sectors_dirty_init op;
   983		struct cache_set *c = d->c;
   984		struct bch_dirty_init_state state;
   985	
 > 986		atomic_long_set(&d->dirty_sectors, 0);
   987	
   988		/* Just count root keys if no leaf node */
   989		rw_lock(0, c->root, c->root->level);
   990		if (c->root->level == 0) {
   991			bch_btree_op_init(&op.op, -1);
   992			op.inode = d->id;
   993			op.count = 0;
   994	
   995			for_each_key_filter(&c->root->keys,
   996					    k, &iter, bch_ptr_invalid) {
   997				if (KEY_INODE(k) != op.inode)
   998					continue;
   999				sectors_dirty_init_fn(&op.op, c->root, k);
  1000			}
  1001	
  1002			rw_unlock(0, c->root);
  1003			return;
  1004		}
  1005	
  1006		memset(&state, 0, sizeof(struct bch_dirty_init_state));
  1007		state.c = c;
  1008		state.d = d;
  1009		state.total_threads = bch_btre_dirty_init_thread_nr();
  1010		state.key_idx = 0;
  1011		spin_lock_init(&state.idx_lock);
  1012		atomic_set(&state.started, 0);
  1013		atomic_set(&state.enough, 0);
  1014		init_waitqueue_head(&state.wait);
  1015	
  1016		for (i = 0; i < state.total_threads; i++) {
  1017			/* Fetch latest state.enough earlier */
  1018			smp_mb__before_atomic();
  1019			if (atomic_read(&state.enough))
  1020				break;
  1021	
  1022			state.infos[i].state = &state;
  1023			state.infos[i].thread =
  1024				kthread_run(bch_dirty_init_thread, &state.infos[i],
  1025					    "bch_dirtcnt[%d]", i);
  1026			if (IS_ERR(state.infos[i].thread)) {
  1027				pr_err("fails to run thread bch_dirty_init[%d]\n", i);
  1028				for (--i; i >= 0; i--)
  1029					kthread_stop(state.infos[i].thread);
  1030				goto out;
  1031			}
  1032			atomic_inc(&state.started);
  1033		}
  1034	
  1035	out:
  1036		/* Must wait for all threads to stop. */
  1037		wait_event(state.wait, atomic_read(&state.started) == 0);
  1038		rw_unlock(0, c->root);
  1039	}
  1040	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re:Re: [PATCH] bcache: fixup init dirty data errors

[邹明哲]

From: Coly Li <colyli@suse.de>
Date: 2023-08-23 01:49:32
To:  Mingzhe Zou <mingzhe.zou@easystack.cn>
Cc:  bcache@lists.ewheeler.net,linux-bcache@vger.kernel.org,zoumingzhe@qq.com
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@easystack.cn>
>> ---
>>  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

Re: [PATCH] bcache: fixup init dirty data errors

[Coly Li]

> 2023年8月24日 20:49，邹明哲 <mingzhe.zou@easystack.cn> 写道：
> 
> From: Coly Li <colyli@suse.de>
> Date: 2023-08-23 01:49:32
> To:  Mingzhe Zou <mingzhe.zou@easystack.cn>
> Cc:  bcache@lists.ewheeler.net,linux-bcache@vger.kernel.org,zoumingzhe@qq.com
> 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@easystack.cn>
>>> ---
>>> 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.

Copied. Thanks for the information, I will take the v2 patch.

Coly Li

Re: [PATCH] bcache: fixup init dirty data errors

[Eddie Chapman]

Coly Li wrote:
>
>> 2023年8月24日 20:49，邹明哲 <mingzhe.zou@easystack.cn> 写道：
>>
>> From: Coly Li <colyli@suse.de>
>> Date: 2023-08-23 01:49:32
>> To:  Mingzhe Zou <mingzhe.zou@easystack.cn>
>> Cc:
>> bcache@lists.ewheeler.net,linux-bcache@vger.kernel.org,zoumingzhe@qq.co
>> m 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@easystack.cn>
>>>> ---
>>>> 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.
>
> Copied. Thanks for the information, I will take the v2 patch.
>
> Coly Li
>

Hi Coly, Mingzhe,

Can I ask, how far back would this fix be needed, in terms of stable
versions?

Thanks,
Eddie

Re:Re: [PATCH] bcache: fixup init dirty data errors

[邹明哲]

件人：Eddie Chapman <eddie@ehuk.net>
发送日期：2023-09-07 19:14:10
收件人：Coly Li <colyli@suse.de>,Mingzhe Zou <mingzhe.zou@easystack.cn>
抄送人：Eric Wheeler <bcache@lists.ewheeler.net>,linux-bcache@vger.kernel.org,zoumingzhe@qq.com
主题：Re: [PATCH] bcache: fixup init dirty data errors>Coly Li wrote:
>>
>>> 2023年8月24日 20:49，邹明哲 <mingzhe.zou@easystack.cn> 写道：
>>>
>>> From: Coly Li <colyli@suse.de>
>>> Date: 2023-08-23 01:49:32
>>> To:  Mingzhe Zou <mingzhe.zou@easystack.cn>
>>> Cc:
>>> bcache@lists.ewheeler.net,linux-bcache@vger.kernel.org,zoumingzhe@qq.co
>>> m 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@easystack.cn>

Hi, Eddie

The patch fix up commit b144e45fc57649e15cbc79ff2d32a942af1d91d5.

The following are all tags containing this commit-id:

$ git tag --contains b144e45fc576 
v5.10
v5.10-rc1
v5.10-rc2
v5.10-rc3
v5.10-rc4
v5.10-rc5
v5.10-rc6
v5.10-rc7
v5.11
v5.11-rc1
v5.11-rc2
v5.11-rc3
v5.11-rc4
v5.11-rc5
v5.11-rc6
v5.11-rc7
v5.12
v5.12-rc1-dontuse
v5.12-rc2
v5.12-rc3
v5.12-rc4
v5.12-rc5
v5.12-rc6
v5.12-rc7
v5.12-rc8
v5.13
v5.13-rc1
v5.13-rc2
v5.13-rc3
v5.13-rc4
v5.13-rc5
v5.13-rc6
v5.13-rc7
v5.14
v5.14-rc1
v5.14-rc2
v5.14-rc3
v5.14-rc4
v5.14-rc5
v5.14-rc6
v5.14-rc7
v5.15
v5.15-rc1
v5.15-rc2
v5.15-rc3
v5.15-rc4
v5.15-rc5
v5.15-rc6
v5.15-rc7
v5.16
v5.16-rc1
v5.16-rc2
v5.16-rc3
v5.16-rc4
v5.16-rc5
v5.16-rc6
v5.16-rc7
v5.16-rc8
v5.17
v5.17-rc1
v5.17-rc2
v5.17-rc3
v5.17-rc4
v5.17-rc5
v5.17-rc6
v5.17-rc7
v5.17-rc8
v5.18
v5.18-rc1
v5.18-rc2
v5.18-rc3
v5.18-rc4
v5.18-rc5
v5.18-rc6
v5.18-rc7
v5.19
v5.19-rc1
v5.19-rc2
v5.19-rc3
v5.19-rc4
v5.19-rc5
v5.19-rc6
v5.19-rc7
v5.19-rc8
v5.7
v5.7-rc1
v5.7-rc2
v5.7-rc3
v5.7-rc4
v5.7-rc5
v5.7-rc6
v5.7-rc7
v5.8
v5.8-rc1
v5.8-rc2
v5.8-rc3
v5.8-rc4
v5.8-rc5
v5.8-rc6
v5.8-rc7
v5.9
v5.9-rc1
v5.9-rc2
v5.9-rc3
v5.9-rc4
v5.9-rc5
v5.9-rc6
v5.9-rc7
v5.9-rc8
v6.0
v6.0-rc1
v6.0-rc2
v6.0-rc3
v6.0-rc4
v6.0-rc5
v6.0-rc6
v6.0-rc7
v6.1
v6.1-rc1
v6.1-rc2
v6.1-rc3
v6.1-rc4
v6.1-rc5
v6.1-rc6
v6.1-rc7
v6.1-rc8
v6.2
v6.2-rc1
v6.2-rc2
v6.2-rc3
v6.2-rc4
v6.2-rc5
v6.2-rc6
v6.2-rc7
v6.2-rc8
v6.3
v6.3-rc1
v6.3-rc2
v6.3-rc3
v6.3-rc4
v6.3-rc5
v6.3-rc6
v6.3-rc7
v6.4
v6.4-rc1
v6.4-rc2
v6.4-rc3
v6.4-rc4
v6.4-rc5
v6.4-rc6
v6.4-rc7
v6.5
v6.5-rc1
v6.5-rc2
v6.5-rc3
v6.5-rc4
v6.5-rc5
v6.5-rc6
v6.5-rc7

>>>>> ---
>>>>> 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.
>>
>> Copied. Thanks for the information, I will take the v2 patch.
>>
>> Coly Li
>>
>
>Hi Coly, Mingzhe,
>
>Can I ask, how far back would this fix be needed, in terms of stable
>versions?
>
>Thanks,
>Eddie
>