block/laptop_mode: Convert timers to use timer_setup()

PATCH In review Kees Cook 4 participants 4 messages Oct 5, 2017 – Oct 9, 2017

Subsystems (4)

Memory management Block layer Page cache MM misc

lore

Share & tools

lore

Kees Cook

author

2017-10-05 23:16 UTC #0 lore raw

PATCH v2 1/1 block/laptop_mode: Convert timers to use timer_setup()

In preparation for unconditionally passing the struct timer_list pointer to all timer callbacks, switch to using the new timer_setup() and from_timer() to pass the timer pointer explicitly.

Cc Jens Axboe <[email protected]>

Cc Michal Hocko <[email protected]>

Cc Andrew Morton <[email protected]>

Cc Jan Kara <[email protected]>

Cc Johannes Weiner <[email protected]>

Cc Nicholas Piggin <[email protected]>

Cc Vladimir Davydov <[email protected]>

Cc Matthew Wilcox <[email protected]>

Cc Jeff Layton <[email protected]>

Cc [email protected]

Cc Thomas Gleixner <[email protected]>

Signed-off-by Kees Cook <[email protected]>

---
v2: Rebased to linux-block.git/for-4.15/timer

---
 block/blk-core.c          | 10 +++++-----
 include/linux/writeback.h |  2 +-
 mm/page-writeback.c       |  7 ++++---
 3 files changed, 10 insertions(+), 9 deletions(-)

block/blk-core.c +5 -5

index 14f7674fa0b1..596255822d7d 100644
@@ -803,9 +803,9 @@ static void blk_queue_usage_counter_release(struct percpu_ref *ref)
 	wake_up_all(&q->mq_freeze_wq);
 }
 
-static void blk_rq_timed_out_timer(unsigned long data)
+static void blk_rq_timed_out_timer(struct timer_list *t)
 {
-	struct request_queue *q = (struct request_queue *)data;
+	struct request_queue *q = from_timer(q, t, timeout);
 
 	kblockd_schedule_work(&q->timeout_work);
 }

@@ -841,9 +841,9 @@ struct request_queue *blk_alloc_queue_node(gfp_t gfp_mask, int node_id)
 	q->backing_dev_info->name = "block";
 	q->node = node_id;
 
-	setup_timer(&q->backing_dev_info->laptop_mode_wb_timer,
-		    laptop_mode_timer_fn, (unsigned long) q);
-	setup_timer(&q->timeout, blk_rq_timed_out_timer, (unsigned long) q);
+	timer_setup(&q->backing_dev_info->laptop_mode_wb_timer,
+		    laptop_mode_timer_fn, 0);
+	timer_setup(&q->timeout, blk_rq_timed_out_timer, 0);
 	INIT_LIST_HEAD(&q->queue_head);
 	INIT_LIST_HEAD(&q->timeout_list);
 	INIT_LIST_HEAD(&q->icq_list);

include/linux/writeback.h +1 -1

index dd1d2c23f743..80944e6bf484 100644
@@ -309,7 +309,7 @@ static inline void cgroup_writeback_umount(void)
 void laptop_io_completion(struct backing_dev_info *info);
 void laptop_sync_completion(void);
 void laptop_mode_sync(struct work_struct *work);
-void laptop_mode_timer_fn(unsigned long data);
+void laptop_mode_timer_fn(struct timer_list *t);
 #else
 static inline void laptop_sync_completion(void) { }
 #endif

mm/page-writeback.c +4 -3

index 8d1fc593bce8..138b8016f759 100644
@@ -1977,11 +1977,12 @@ int dirty_writeback_centisecs_handler(struct ctl_table *table, int write,
 }
 
 #ifdef CONFIG_BLOCK
-void laptop_mode_timer_fn(unsigned long data)
+void laptop_mode_timer_fn(struct timer_list *t)
 {
-	struct request_queue *q = (struct request_queue *)data;
+	struct backing_dev_info *backing_dev_info =
+		from_timer(backing_dev_info, t, laptop_mode_wb_timer);
 
-	wakeup_flusher_threads_bdi(q->backing_dev_info, WB_REASON_LAPTOP_TIMER);
+	wakeup_flusher_threads_bdi(backing_dev_info, WB_REASON_LAPTOP_TIMER);
 }
 
 /*

Signature · 6 lines

2.7.4


-- 
Kees Cook
Pixel Security

Christoph Hellwig

2017-10-06 08:20 UTC #1 lore raw

Review

Quoted 8 lines

(quoted diff) +2 -2

-static void blk_rq_timed_out_timer(unsigned long data)
+static void blk_rq_timed_out_timer(struct timer_list *t)
 {
-	struct request_queue *q = (struct request_queue *)data;
+	struct request_queue *q = from_timer(q, t, timeout);
 
 	kblockd_schedule_work(&q->timeout_work);
 }

This isn't the laptop_mode timer, although the change itself looks fine.

Review

Quoted 2 lines

(quoted diff) +2 -0

+	timer_setup(&q->backing_dev_info->laptop_mode_wb_timer,
+		    laptop_mode_timer_fn, 0);

And I already pointed out to Jens when he did the previous changes to this one that it has no business being in the block code, it really should move to mm/page-writeback.c with the rest of the handling of this timer. Once that is fixed up your automated script should pick it up, so we wouldn't need the manual change.

Untested patch for that below:

---

Quoted 1 line

From 77881bd72b5fb1219fc74625b0380930f9c580df Mon Sep 17 00:00:00 2001

From: Christoph Hellwig <[email protected]>
Date: Fri, 6 Oct 2017 10:18:53 +0200
Subject: mm: move all laptop_mode handling to backing-dev.c

It isn't block-device specific and oddly spread over multiple files at the moment:

TODO: audit that the unregistration changes are fine

Signed-off-by Christoph Hellwig <[email protected]>

---
 block/blk-core.c          |  3 ---
 include/linux/writeback.h |  6 ------
 mm/backing-dev.c          | 36 ++++++++++++++++++++++++++++++++++++
 mm/page-writeback.c       | 36 ------------------------------------
 4 files changed, 36 insertions(+), 45 deletions(-)

block/blk-core.c +0 -3

index 14f7674fa0b1..f5f916b28c40 100644
@@ -662,7 +662,6 @@ void blk_cleanup_queue(struct request_queue *q)
 	blk_flush_integrity();
 
 	/* @q won't process any more request, flush async actions */
-	del_timer_sync(&q->backing_dev_info->laptop_mode_wb_timer);
 	blk_sync_queue(q);
 
 	if (q->mq_ops)

@@ -841,8 +840,6 @@ struct request_queue *blk_alloc_queue_node(gfp_t gfp_mask, int node_id)
 	q->backing_dev_info->name = "block";
 	q->node = node_id;
 
-	setup_timer(&q->backing_dev_info->laptop_mode_wb_timer,
-		    laptop_mode_timer_fn, (unsigned long) q);
 	setup_timer(&q->timeout, blk_rq_timed_out_timer, (unsigned long) q);
 	INIT_LIST_HEAD(&q->queue_head);
 	INIT_LIST_HEAD(&q->timeout_list);

include/linux/writeback.h +0 -6

index 9c0091678af4..e6ba35a5e1f7 100644
@@ -327,14 +327,8 @@ static inline void cgroup_writeback_umount(void)
 /*
  * mm/page-writeback.c
  */
-#ifdef CONFIG_BLOCK
 void laptop_io_completion(struct backing_dev_info *info);
 void laptop_sync_completion(void);
-void laptop_mode_sync(struct work_struct *work);
-void laptop_mode_timer_fn(unsigned long data);
-#else
-static inline void laptop_sync_completion(void) { }
-#endif
 bool node_dirty_ok(struct pglist_data *pgdat);
 int wb_domain_init(struct wb_domain *dom, gfp_t gfp);
 #ifdef CONFIG_CGROUP_WRITEBACK

mm/backing-dev.c +36 -0

index e19606bb41a0..cb36f07f2af2 100644
@@ -822,6 +822,38 @@ static void cgwb_remove_from_bdi_list(struct bdi_writeback *wb)
 
 #endif	/* CONFIG_CGROUP_WRITEBACK */
 
+static void laptop_mode_timer_fn(unsigned long data)
+{
+	struct backing_dev_info *bdi = (struct backing_dev_info *)data;
+
+	wakeup_flusher_threads_bdi(bdi, WB_REASON_LAPTOP_TIMER);
+}
+
+/*
+ * We've spun up the disk and we're in laptop mode: schedule writeback
+ * of all dirty data a few seconds from now.  If the flush is already scheduled
+ * then push it back - the user is still using the disk.
+ */
+void laptop_io_completion(struct backing_dev_info *bdi)
+{
+	mod_timer(&bdi->laptop_mode_wb_timer, jiffies + laptop_mode);
+}
+
+/*
+ * We're in laptop mode and we've just synced. The sync's writes will have
+ * caused another writeback to be scheduled by laptop_io_completion.
+ * Nothing needs to be written back anymore, so we unschedule the writeback.
+ */
+void laptop_sync_completion(void)
+{
+	struct backing_dev_info *bdi;
+
+	rcu_read_lock();
+	list_for_each_entry_rcu(bdi, &bdi_list, bdi_list)
+		del_timer(&bdi->laptop_mode_wb_timer);
+	rcu_read_unlock();
+}
+
 static int bdi_init(struct backing_dev_info *bdi)
 {
 	int ret;

@@ -835,6 +867,8 @@ static int bdi_init(struct backing_dev_info *bdi)
 	INIT_LIST_HEAD(&bdi->bdi_list);
 	INIT_LIST_HEAD(&bdi->wb_list);
 	init_waitqueue_head(&bdi->wb_waitq);
+	setup_timer(&bdi->laptop_mode_wb_timer,
+		    laptop_mode_timer_fn, (unsigned long)bdi);
 
 	ret = cgwb_bdi_init(bdi);

@@ -916,6 +950,8 @@ EXPORT_SYMBOL(bdi_register_owner);
  */
 static void bdi_remove_from_list(struct backing_dev_info *bdi)
 {
+	del_timer_sync(&bdi->laptop_mode_wb_timer);
+
 	spin_lock_bh(&bdi_lock);
 	list_del_rcu(&bdi->bdi_list);
 	spin_unlock_bh(&bdi_lock);

mm/page-writeback.c +0 ~~-36~~

index 8d1fc593bce8..f8fe90dc529d 100644
@@ -1976,42 +1976,6 @@ int dirty_writeback_centisecs_handler(struct ctl_table *table, int write,
 	return 0;
 }
 
-#ifdef CONFIG_BLOCK
-void laptop_mode_timer_fn(unsigned long data)
-{
-	struct request_queue *q = (struct request_queue *)data;
-
-	wakeup_flusher_threads_bdi(q->backing_dev_info, WB_REASON_LAPTOP_TIMER);
-}
-
-/*
- * We've spun up the disk and we're in laptop mode: schedule writeback
- * of all dirty data a few seconds from now.  If the flush is already scheduled
- * then push it back - the user is still using the disk.
- */
-void laptop_io_completion(struct backing_dev_info *info)
-{
-	mod_timer(&info->laptop_mode_wb_timer, jiffies + laptop_mode);
-}
-
-/*
- * We're in laptop mode and we've just synced. The sync's writes will have
- * caused another writeback to be scheduled by laptop_io_completion.
- * Nothing needs to be written back anymore, so we unschedule the writeback.
- */
-void laptop_sync_completion(void)
-{
-	struct backing_dev_info *bdi;
-
-	rcu_read_lock();
-
-	list_for_each_entry_rcu(bdi, &bdi_list, bdi_list)
-		del_timer(&bdi->laptop_mode_wb_timer);
-
-	rcu_read_unlock();
-}
-#endif
-
 /*
  * If ratelimit_pages is too high then we can get into dirty-data overload
  * if a large number of processes all perform writes at the same time.

Signature · 1 line

2.14.1

Jens Axboe

maintainer

2017-10-06 14:28 UTC #2 lore raw

Quoted 11 lines · Christoph Hellwig

Review

Quoted 8 lines

(quoted diff) +2 -2

-static void blk_rq_timed_out_timer(unsigned long data)
+static void blk_rq_timed_out_timer(struct timer_list *t)
 {
-	struct request_queue *q = (struct request_queue *)data;
+	struct request_queue *q = from_timer(q, t, timeout);
 
 	kblockd_schedule_work(&q->timeout_work);
 }

This isn't the laptop_mode timer, although the change itself looks fine.

Quoted 8 lines

Review
Quoted 2 lines
(quoted diff) +2 -0
+	timer_setup(&q->backing_dev_info->laptop_mode_wb_timer,
+		    laptop_mode_timer_fn, 0);
And I already pointed out to Jens when he did the previous changes to this one that it has no business being in the block code, it really should move to mm/page-writeback.c with the rest of the handling of this timer. Once that is fixed up your automated script should pick it up, so we wouldn't need the manual change.

Looks reasonable to me, one comment:

mm/backing-dev.c EXPORT_SYMBOL(bdi_register_owner); · line 957

Quoted 9 lines

mm/backing-dev.c +2 -0

@@ -916,6 +950,8 @@ EXPORT_SYMBOL(bdi_register_owner);
  */
 static void bdi_remove_from_list(struct backing_dev_info *bdi)
 {
+	del_timer_sync(&bdi->laptop_mode_wb_timer);
+
 	spin_lock_bh(&bdi_lock);
 	list_del_rcu(&bdi->bdi_list);
 	spin_unlock_bh(&bdi_lock);

This should go into bdi_unregister() instead.

The rest is mostly mechanical and looks fine to me.

Signature · 1 line

Jens Axboe

Jan Kara

maintainer

2017-10-09 09:14 UTC #3 lore raw

In reply to Christoph Hellwig

Quoted 9 lines · Christoph Hellwig

From 77881bd72b5fb1219fc74625b0380930f9c580df Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <[email protected]>
Date: Fri, 6 Oct 2017 10:18:53 +0200
Subject: mm: move all laptop_mode handling to backing-dev.c
It isn't block-device specific and oddly spread over multiple files at the moment:
TODO: audit that the unregistration changes are fine

Yeah, I'm a bit concerned about those. You cleanup the timer in bdi_unregister() which pairs with bdi_register(). However you don't have to call bdi_register() (and thus consequently call bdi_unregister() on device shutdown) in order to do IO to a device. bdi_register() is only needed to setup flusher threads and similar stuff. Also laptop_io_completion(), which arms the timer, is called when any IO request is completed again independently of BDI registration / unregistration.

But maybe we could just make laptop_io_completion() not arm the timer for unregistered BDIs (calling wakeup_flusher_threads_bdi() won't have any effect anyway) and then cleaning up the timer in bdi_unregister() would be a safe thing to do?

Honza

Quoted 160 lines

Signed-off-by Christoph Hellwig <[email protected]>

---
 block/blk-core.c          |  3 ---
 include/linux/writeback.h |  6 ------
 mm/backing-dev.c          | 36 ++++++++++++++++++++++++++++++++++++
 mm/page-writeback.c       | 36 ------------------------------------
 4 files changed, 36 insertions(+), 45 deletions(-)

block/blk-core.c +0 -3

index 14f7674fa0b1..f5f916b28c40 100644
@@ -662,7 +662,6 @@ void blk_cleanup_queue(struct request_queue *q)
 	blk_flush_integrity();
 
 	/* @q won't process any more request, flush async actions */
-	del_timer_sync(&q->backing_dev_info->laptop_mode_wb_timer);
 	blk_sync_queue(q);
 
 	if (q->mq_ops)

@@ -841,8 +840,6 @@ struct request_queue *blk_alloc_queue_node(gfp_t gfp_mask, int node_id)
 	q->backing_dev_info->name = "block";
 	q->node = node_id;
 
-	setup_timer(&q->backing_dev_info->laptop_mode_wb_timer,
-		    laptop_mode_timer_fn, (unsigned long) q);
 	setup_timer(&q->timeout, blk_rq_timed_out_timer, (unsigned long) q);
 	INIT_LIST_HEAD(&q->queue_head);
 	INIT_LIST_HEAD(&q->timeout_list);

include/linux/writeback.h +0 -6

index 9c0091678af4..e6ba35a5e1f7 100644
@@ -327,14 +327,8 @@ static inline void cgroup_writeback_umount(void)
 /*
  * mm/page-writeback.c
  */
-#ifdef CONFIG_BLOCK
 void laptop_io_completion(struct backing_dev_info *info);
 void laptop_sync_completion(void);
-void laptop_mode_sync(struct work_struct *work);
-void laptop_mode_timer_fn(unsigned long data);
-#else
-static inline void laptop_sync_completion(void) { }
-#endif
 bool node_dirty_ok(struct pglist_data *pgdat);
 int wb_domain_init(struct wb_domain *dom, gfp_t gfp);
 #ifdef CONFIG_CGROUP_WRITEBACK

mm/backing-dev.c +36 -0

index e19606bb41a0..cb36f07f2af2 100644
@@ -822,6 +822,38 @@ static void cgwb_remove_from_bdi_list(struct bdi_writeback *wb)
 
 #endif	/* CONFIG_CGROUP_WRITEBACK */
 
+static void laptop_mode_timer_fn(unsigned long data)
+{
+	struct backing_dev_info *bdi = (struct backing_dev_info *)data;
+
+	wakeup_flusher_threads_bdi(bdi, WB_REASON_LAPTOP_TIMER);
+}
+
+/*
+ * We've spun up the disk and we're in laptop mode: schedule writeback
+ * of all dirty data a few seconds from now.  If the flush is already scheduled
+ * then push it back - the user is still using the disk.
+ */
+void laptop_io_completion(struct backing_dev_info *bdi)
+{
+	mod_timer(&bdi->laptop_mode_wb_timer, jiffies + laptop_mode);
+}
+
+/*
+ * We're in laptop mode and we've just synced. The sync's writes will have
+ * caused another writeback to be scheduled by laptop_io_completion.
+ * Nothing needs to be written back anymore, so we unschedule the writeback.
+ */
+void laptop_sync_completion(void)
+{
+	struct backing_dev_info *bdi;
+
+	rcu_read_lock();
+	list_for_each_entry_rcu(bdi, &bdi_list, bdi_list)
+		del_timer(&bdi->laptop_mode_wb_timer);
+	rcu_read_unlock();
+}
+
 static int bdi_init(struct backing_dev_info *bdi)
 {
 	int ret;

@@ -835,6 +867,8 @@ static int bdi_init(struct backing_dev_info *bdi)
 	INIT_LIST_HEAD(&bdi->bdi_list);
 	INIT_LIST_HEAD(&bdi->wb_list);
 	init_waitqueue_head(&bdi->wb_waitq);
+	setup_timer(&bdi->laptop_mode_wb_timer,
+		    laptop_mode_timer_fn, (unsigned long)bdi);
 
 	ret = cgwb_bdi_init(bdi);

@@ -916,6 +950,8 @@ EXPORT_SYMBOL(bdi_register_owner);
  */
 static void bdi_remove_from_list(struct backing_dev_info *bdi)
 {
+	del_timer_sync(&bdi->laptop_mode_wb_timer);
+
 	spin_lock_bh(&bdi_lock);
 	list_del_rcu(&bdi->bdi_list);
 	spin_unlock_bh(&bdi_lock);

mm/page-writeback.c +0 ~~-36~~

index 8d1fc593bce8..f8fe90dc529d 100644
@@ -1976,42 +1976,6 @@ int dirty_writeback_centisecs_handler(struct ctl_table *table, int write,
 	return 0;
 }
 
-#ifdef CONFIG_BLOCK
-void laptop_mode_timer_fn(unsigned long data)
-{
-	struct request_queue *q = (struct request_queue *)data;
-
-	wakeup_flusher_threads_bdi(q->backing_dev_info, WB_REASON_LAPTOP_TIMER);
-}
-
-/*
- * We've spun up the disk and we're in laptop mode: schedule writeback
- * of all dirty data a few seconds from now.  If the flush is already scheduled
- * then push it back - the user is still using the disk.
- */
-void laptop_io_completion(struct backing_dev_info *info)
-{
-	mod_timer(&info->laptop_mode_wb_timer, jiffies + laptop_mode);
-}
-
-/*
- * We're in laptop mode and we've just synced. The sync's writes will have
- * caused another writeback to be scheduled by laptop_io_completion.
- * Nothing needs to be written back anymore, so we unschedule the writeback.
- */
-void laptop_sync_completion(void)
-{
-	struct backing_dev_info *bdi;
-
-	rcu_read_lock();
-
-	list_for_each_entry_rcu(bdi, &bdi_list, bdi_list)
-		del_timer(&bdi->laptop_mode_wb_timer);
-
-	rcu_read_unlock();
-}
-#endif
-
 /*
  * If ratelimit_pages is too high then we can get into dirty-data overload
  * if a large number of processes all perform writes at the same time.

Signature · 1 line

2.14.1

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR