On 29/01/2018 3:35 PM, tang.junhui@xxxxxxxxxx wrote:
> From: Tang Junhui <tang.junhui@xxxxxxxxxx>
>
> Hello Coly:
>
> This patch is somewhat difficult for me,
> I think we can resolve it in a simple way.
>
> We can take the schedule_delayed_work() under the protection of
> dc->writeback_lock, and judge if we need re-arm this work to queue.
>
> 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);
>
> down_read(&dc->writeback_lock);
>
> if (atomic_read(&dc->has_dirty) &&
> dc->writeback_percent)
> __update_writeback_rate(dc);
>
> - up_read(&dc->writeback_lock);
> + if (NEED_RE-AEMING)
> schedule_delayed_work(&dc->writeback_rate_update,
> dc->writeback_rate_update_seconds * HZ);
> + up_read(&dc->writeback_lock);
> }
>
> In cached_dev_detach_finish() and cached_dev_free() we can set the no need
> flag under the protection of dc->writeback_lock, for example:
>
> static void cached_dev_detach_finish(struct work_struct *w)
> {
> ...
> + down_write(&dc->writeback_lock);
> + SET NO NEED RE-ARM FLAG
> + up_write(&dc->writeback_lock);
> cancel_delayed_work_sync(&dc->writeback_rate_update);
> }
>
> I think this way is more simple and readable.
>
Hi Junhui,
Your suggest is essentially almost same to my patch,
- clear BCACHE_DEV_DETACHING bit acts as SET NO NEED RE-ARM FLAG.
- cancel_writeback_rate_update_dwork acts as some kind of locking with a
timeout.
The difference is I don't use dc->writeback_lock, and replace it by
BCACHE_DEV_RATE_DW_RUNNING.
The reason is my following development. I plan to implement a real-time
update stripe_sectors_dirty of bcache device and cache set, then
bcache_flash_devs_sectors_dirty() can be very fast and bch_register_lock
can be removed here. And then I also plan to remove reference of
dc->writeback_lock in update_writeback_rate() because indeed it is
unnecessary here (the patch is held by Mike's locking resort work).
Since I plan to remove dc->writeback_lock from update_writeback_rate(),
I don't want to reference dc->writeback in the delayed work.
The basic idea behind your suggestion and this patch, is almost
identical. The only difference might be the timeout in
cancel_writeback_rate_update_dwork().
Thanks.
Coly Li
--
To unsubscribe from this list: send the line "unsubscribe linux-bcache" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
