| Message ID | 1509437651-24052-1-git-send-email-tang.junhui@zte.com.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2017/10/31 下午4:14, tang.junhui@zte.com.cn wrote: > From: Tang Junhui <tang.junhui@zte.com.cn> > > Currently, when a cached device detaching from cache, writeback thread is not stopped, > and writeback_rate_update work is not canceled. For example, after bellow command: > echo 1 >/sys/block/sdb/bcache/detach > you can still see the writeback thread. Then you attach the device to the cache again, > bcache will create another writeback thread, for example, after bellow command: > echo ba0fb5cd-658a-4533-9806-6ce166d883b9 > /sys/block/sdb/bcache/attach > then you will see 2 writeback threads. > This patch stops writeback thread and cancels writeback_rate_update work when cached > device detaching from cache. > > Signed-off-by: Tang Junhui <tang.junhui@zte.com.cn> If the change can be inside bch_register_lock, it would (just) be more comfortable. The code is correct, because attach/detach sysfs is created after writeback_thread created and writeback_rate_update worker initialized, even these resources are initialized within bch_register_lock and released out of bch)register_lock in your patch, there won't be any race. It's OK to me. Reviewed-by: Coly Li <colyli@suse.de> Thanks. Coly Li > --- > drivers/md/bcache/super.c | 6 ++++++ > 1 file changed, 6 insertions(+) > mode change 100644 => 100755 drivers/md/bcache/super.c > > diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c > old mode 100644 > new mode 100755 > index 8352fad..bf79892 > --- a/drivers/md/bcache/super.c > +++ b/drivers/md/bcache/super.c > @@ -891,6 +891,12 @@ static void cached_dev_detach_finish(struct work_struct *w) > BUG_ON(!test_bit(BCACHE_DEV_DETACHING, &dc->disk.flags)); > BUG_ON(atomic_read(&dc->count)); > > + cancel_delayed_work_sync(&dc->writeback_rate_update); > + if (!IS_ERR_OR_NULL(dc->writeback_thread)) { > + kthread_stop(dc->writeback_thread); > + dc->writeback_thread = NULL; > + } > + > mutex_lock(&bch_register_lock); > > memset(&dc->sb.set_uuid, 0, 16); >
Tang Junhui-- Thanks for noticing this issue. On Wed, Nov 1, 2017 at 4:55 AM, Coly Li <i@coly.li> wrote: > On 2017/10/31 下午4:14, tang.junhui@zte.com.cn wrote: >> From: Tang Junhui <tang.junhui@zte.com.cn> >> >> Currently, when a cached device detaching from cache, writeback thread is not stopped, >> and writeback_rate_update work is not canceled. For example, after bellow command: >> echo 1 >/sys/block/sdb/bcache/detach >> you can still see the writeback thread. Then you attach the device to the cache again, >> bcache will create another writeback thread, for example, after bellow command: >> echo ba0fb5cd-658a-4533-9806-6ce166d883b9 > /sys/block/sdb/bcache/attach >> then you will see 2 writeback threads. >> This patch stops writeback thread and cancels writeback_rate_update work when cached >> device detaching from cache. >> >> Signed-off-by: Tang Junhui <tang.junhui@zte.com.cn> > > If the change can be inside bch_register_lock, it would (just) be more > comfortable. The code is correct, because attach/detach sysfs is created > after writeback_thread created and writeback_rate_update worker > initialized, even these resources are initialized within > bch_register_lock and released out of bch)register_lock in your patch, > there won't be any race. It's OK to me. > I think I agree with Coly that I'd prefer it to be moved down into the register lock, as I think that will be safer with any future changes. Are you willing to adjust it this way? Thanks, Mike
diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c old mode 100644 new mode 100755 index 8352fad..bf79892 --- a/drivers/md/bcache/super.c +++ b/drivers/md/bcache/super.c @@ -891,6 +891,12 @@ static void cached_dev_detach_finish(struct work_struct *w) BUG_ON(!test_bit(BCACHE_DEV_DETACHING, &dc->disk.flags)); BUG_ON(atomic_read(&dc->count)); + cancel_delayed_work_sync(&dc->writeback_rate_update); + if (!IS_ERR_OR_NULL(dc->writeback_thread)) { + kthread_stop(dc->writeback_thread); + dc->writeback_thread = NULL; + } + mutex_lock(&bch_register_lock); memset(&dc->sb.set_uuid, 0, 16);