On 04/01/2018 8:41 PM, tang.junhui@xxxxxxxxxx wrote: > From: Tang Junhui <tang.junhui@xxxxxxxxxx> > > Hello Coly, > >> When cache set is stopping, calculating writeback rate is wast of time. >> This is the purpose of the first checking, to avoid unnecessary delay >>from bcache_flash_devs_sectors_dirty() inside __update_writeback_rate(). > > I thought twice, and found there is still a race. > > work update_writeback_rate is runing: >> @@ -91,6 +91,11 @@ static void update_writeback_rate(struct work_struct *work) >> struct cached_dev *dc = container_of(to_delayed_work(work), >> struct cached_dev, >> writeback_rate_update); >> + struct cache_set *c = dc->disk.c; >> + >> + /* quit directly if cache set is stopping */ >> + if (test_bit(CACHE_SET_STOPPING, &c->flags)) >> + return; >> >> down_read(&dc->writeback_lock); >> >> @@ -100,6 +105,10 @@ static void update_writeback_rate(struct work_struct *work) >> >> up_read(&dc->writeback_lock); >> >> + /* do not schedule delayed work if cache set is stopping */ >> + if (test_bit(CACHE_SET_STOPPING, &c->flags)) >> + return; >> + > > and if cancel_delayed_wsrksync() runs at this moment, the code can not prevent the > work re-arming itself to workqueue. So maybe we need a locker to protect it. > Hi Junhui, Nice catch! Yes, there should be something here to avoid such race. I will fix it in v2 patch. Thanks for the hint. Coly Li >> schedule_delayed_work(&dc->writeback_rate_update, >> dc->writeback_rate_update_seconds * HZ); >> }
