| Message ID | 20210809141744.1203023-2-hch@lst.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/5] mm: hide laptop_mode_wb_timer entirely behind the BDI API | expand |
On 09/08/2021 16:19, Christoph Hellwig wrote: > Don't leak the detaіls of the timer into the block layer, instead > initialize the timer in bdi_alloc and delete it in bdi_unregister. > Note that this means the timer is initialized (but not armed) for > non-block queues as well now. And laptop_mode_timer_fn() is always present now. Other than that, Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
On Mon 09-08-21 16:17:40, Christoph Hellwig wrote: > Don't leak the detaіls of the timer into the block layer, instead > initialize the timer in bdi_alloc and delete it in bdi_unregister. > Note that this means the timer is initialized (but not armed) for > non-block queues as well now. > > Signed-off-by: Christoph Hellwig <hch@lst.de> Looks good. Feel free to add: Reviewed-by: Jan Kara <jack@suse.cz> Honza > --- > block/blk-core.c | 5 ----- > mm/backing-dev.c | 3 +++ > mm/page-writeback.c | 2 -- > 3 files changed, 3 insertions(+), 7 deletions(-) > > diff --git a/block/blk-core.c b/block/blk-core.c > index 04477697ee4b..5897bc37467d 100644 > --- a/block/blk-core.c > +++ b/block/blk-core.c > @@ -394,10 +394,7 @@ void blk_cleanup_queue(struct request_queue *q) > /* for synchronous bio-based driver finish in-flight integrity i/o */ > 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 (queue_is_mq(q)) > blk_mq_exit_queue(q); > > @@ -546,8 +543,6 @@ struct request_queue *blk_alloc_queue(int node_id) > > atomic_set(&q->nr_active_requests_shared_sbitmap, 0); > > - 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_WORK(&q->timeout_work, blk_timeout_work); > INIT_LIST_HEAD(&q->icq_list); > diff --git a/mm/backing-dev.c b/mm/backing-dev.c > index f5561ea7d90a..cd06dca232c3 100644 > --- a/mm/backing-dev.c > +++ b/mm/backing-dev.c > @@ -807,6 +807,7 @@ struct backing_dev_info *bdi_alloc(int node_id) > bdi->capabilities = BDI_CAP_WRITEBACK | BDI_CAP_WRITEBACK_ACCT; > bdi->ra_pages = VM_READAHEAD_PAGES; > bdi->io_pages = VM_READAHEAD_PAGES; > + timer_setup(&bdi->laptop_mode_wb_timer, laptop_mode_timer_fn, 0); > return bdi; > } > EXPORT_SYMBOL(bdi_alloc); > @@ -928,6 +929,8 @@ static void bdi_remove_from_list(struct backing_dev_info *bdi) > > void bdi_unregister(struct backing_dev_info *bdi) > { > + del_timer_sync(&bdi->laptop_mode_wb_timer); > + > /* make sure nobody finds us on the bdi_list anymore */ > bdi_remove_from_list(bdi); > wb_shutdown(&bdi->wb); > diff --git a/mm/page-writeback.c b/mm/page-writeback.c > index 9f63548f247c..c12f67cbfa19 100644 > --- a/mm/page-writeback.c > +++ b/mm/page-writeback.c > @@ -2010,7 +2010,6 @@ int dirty_writeback_centisecs_handler(struct ctl_table *table, int write, > return ret; > } > > -#ifdef CONFIG_BLOCK > void laptop_mode_timer_fn(struct timer_list *t) > { > struct backing_dev_info *backing_dev_info = > @@ -2045,7 +2044,6 @@ void laptop_sync_completion(void) > > rcu_read_unlock(); > } > -#endif > > /* > * If ratelimit_pages is too high then we can get into dirty-data overload > -- > 2.30.2 >
On Mon, Aug 09, 2021 at 04:17:40PM +0200, Christoph Hellwig wrote: > Don't leak the detaіls of the timer into the block layer, instead > initialize the timer in bdi_alloc and delete it in bdi_unregister. > Note that this means the timer is initialized (but not armed) for > non-block queues as well now. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- Just in case this hasn't been reported yet. This patch results in a widespread build failure. Example: Building x86_64:tinyconfig ... failed -------------- Error log: mm/page-writeback.c:2044:6: error: redefinition of 'laptop_sync_completion' 2044 | void laptop_sync_completion(void) | ^~~~~~~~~~~~~~~~~~~~~~ In file included from include/linux/memcontrol.h:22, from include/linux/swap.h:9, from mm/page-writeback.c:20: include/linux/writeback.h:345:20: note: previous definition of 'laptop_sync_completion' with type 'void(void)' 345 | static inline void laptop_sync_completion(void) { } | ^~~~~~~~~~~~~~~~~~~~~~ make[2]: *** [scripts/Makefile.build:272: mm/page-writeback.o] Error 1 Guenter --- bisect log: # bad: [92d00774360dfd4151f15ab9905c643347b9f242] Add linux-next specific files for 20210810 # good: [36a21d51725af2ce0700c6ebcb6b9594aac658a6] Linux 5.14-rc5 git bisect start 'HEAD' 'v5.14-rc5' # good: [01dda625c9b7cfd3bf5ac05f73da8c512792f94c] Merge remote-tracking branch 'crypto/master' git bisect good 01dda625c9b7cfd3bf5ac05f73da8c512792f94c # bad: [75cadd49361c6650764d35bcbb6c9cb9f0a9d9a3] Merge remote-tracking branch 'irqchip/irq/irqchip-next' git bisect bad 75cadd49361c6650764d35bcbb6c9cb9f0a9d9a3 # good: [511b0c991c9d49fd6d8188f799b10aa0465cecf3] Merge remote-tracking branch 'drm-intel/for-linux-next' git bisect good 511b0c991c9d49fd6d8188f799b10aa0465cecf3 # good: [f3b48aa06fb8b4384b90e41220da8be5a4013a6d] Merge remote-tracking branch 'input/next' git bisect good f3b48aa06fb8b4384b90e41220da8be5a4013a6d # bad: [87470038c43f9577a300a29ba6c2c95d28039464] Merge remote-tracking branch 'regulator/for-next' git bisect bad 87470038c43f9577a300a29ba6c2c95d28039464 # bad: [e1796683109e4ba27c73f099486555a36820b175] Merge remote-tracking branch 'device-mapper/for-next' git bisect bad e1796683109e4ba27c73f099486555a36820b175 # bad: [a11d7fc2d05fb509cd9e33d4093507d6eda3ad53] block: remove the bd_bdi in struct block_device git bisect bad a11d7fc2d05fb509cd9e33d4093507d6eda3ad53 # good: [a291bb43e5c9fdedc4be3dfd496e64e7c5a78b1f] block: use the %pg format specifier in show_partition git bisect good a291bb43e5c9fdedc4be3dfd496e64e7c5a78b1f # good: [2112f5c1330a671fa852051d85cb9eadc05d7eb7] loop: Select I/O scheduler 'none' from inside add_disk() git bisect good 2112f5c1330a671fa852051d85cb9eadc05d7eb7 # good: [ba30585936b0b88f0fb2b19be279b346a6cc87eb] dm: move setting md->type into dm_setup_md_queue git bisect good ba30585936b0b88f0fb2b19be279b346a6cc87eb # bad: [5ed964f8e54eb3191b8b7b45aeb52672a0c995dc] mm: hide laptop_mode_wb_timer entirely behind the BDI API git bisect bad 5ed964f8e54eb3191b8b7b45aeb52672a0c995dc # good: [d1254a8749711e0d7441036a74ce592341f89697] block: remove support for delayed queue registrations git bisect good d1254a8749711e0d7441036a74ce592341f89697 # first bad commit: [5ed964f8e54eb3191b8b7b45aeb52672a0c995dc] mm: hide laptop_mode_wb_timer entirely behind the BDI API
On Tue, Aug 10, 2021 at 02:56:22PM -0700, Guenter Roeck wrote: > On Mon, Aug 09, 2021 at 04:17:40PM +0200, Christoph Hellwig wrote: > > Don't leak the detaіls of the timer into the block layer, instead > > initialize the timer in bdi_alloc and delete it in bdi_unregister. > > Note that this means the timer is initialized (but not armed) for > > non-block queues as well now. > > > > Signed-off-by: Christoph Hellwig <hch@lst.de> > > --- > > Just in case this hasn't been reported yet. > This patch results in a widespread build failure. Example: Sorry. This was reported before and a fix is already queued up here: https://git.kernel.dk/cgit/linux-block/commit/?h=for-5.15/block&id=99d26de2f6d79badc80f55b54bd90d4cb9d1ad90
diff --git a/block/blk-core.c b/block/blk-core.c index 04477697ee4b..5897bc37467d 100644 --- a/block/blk-core.c +++ b/block/blk-core.c @@ -394,10 +394,7 @@ void blk_cleanup_queue(struct request_queue *q) /* for synchronous bio-based driver finish in-flight integrity i/o */ 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 (queue_is_mq(q)) blk_mq_exit_queue(q); @@ -546,8 +543,6 @@ struct request_queue *blk_alloc_queue(int node_id) atomic_set(&q->nr_active_requests_shared_sbitmap, 0); - 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_WORK(&q->timeout_work, blk_timeout_work); INIT_LIST_HEAD(&q->icq_list); diff --git a/mm/backing-dev.c b/mm/backing-dev.c index f5561ea7d90a..cd06dca232c3 100644 --- a/mm/backing-dev.c +++ b/mm/backing-dev.c @@ -807,6 +807,7 @@ struct backing_dev_info *bdi_alloc(int node_id) bdi->capabilities = BDI_CAP_WRITEBACK | BDI_CAP_WRITEBACK_ACCT; bdi->ra_pages = VM_READAHEAD_PAGES; bdi->io_pages = VM_READAHEAD_PAGES; + timer_setup(&bdi->laptop_mode_wb_timer, laptop_mode_timer_fn, 0); return bdi; } EXPORT_SYMBOL(bdi_alloc); @@ -928,6 +929,8 @@ static void bdi_remove_from_list(struct backing_dev_info *bdi) void bdi_unregister(struct backing_dev_info *bdi) { + del_timer_sync(&bdi->laptop_mode_wb_timer); + /* make sure nobody finds us on the bdi_list anymore */ bdi_remove_from_list(bdi); wb_shutdown(&bdi->wb); diff --git a/mm/page-writeback.c b/mm/page-writeback.c index 9f63548f247c..c12f67cbfa19 100644 --- a/mm/page-writeback.c +++ b/mm/page-writeback.c @@ -2010,7 +2010,6 @@ int dirty_writeback_centisecs_handler(struct ctl_table *table, int write, return ret; } -#ifdef CONFIG_BLOCK void laptop_mode_timer_fn(struct timer_list *t) { struct backing_dev_info *backing_dev_info = @@ -2045,7 +2044,6 @@ void laptop_sync_completion(void) rcu_read_unlock(); } -#endif /* * If ratelimit_pages is too high then we can get into dirty-data overload
Don't leak the detaіls of the timer into the block layer, instead initialize the timer in bdi_alloc and delete it in bdi_unregister. Note that this means the timer is initialized (but not armed) for non-block queues as well now. Signed-off-by: Christoph Hellwig <hch@lst.de> --- block/blk-core.c | 5 ----- mm/backing-dev.c | 3 +++ mm/page-writeback.c | 2 -- 3 files changed, 3 insertions(+), 7 deletions(-)