Re: [PATCH v2] block/laptop_mode: Convert timers to use timer_setup()

[Jan Kara]

On Fri 06-10-17 01:20:20, Christoph Hellwig wrote:
> From 77881bd72b5fb1219fc74625b0380930f9c580df Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <hch@xxxxxx>
> 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
> 
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> ---
>  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(-)
> 
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 14f7674fa0b1..f5f916b28c40 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -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);
> diff --git a/include/linux/writeback.h b/include/linux/writeback.h
> index 9c0091678af4..e6ba35a5e1f7 100644
> --- a/include/linux/writeback.h
> +++ b/include/linux/writeback.h
> @@ -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
> diff --git a/mm/backing-dev.c b/mm/backing-dev.c
> index e19606bb41a0..cb36f07f2af2 100644
> --- a/mm/backing-dev.c
> +++ b/mm/backing-dev.c
> @@ -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);
> diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> index 8d1fc593bce8..f8fe90dc529d 100644
> --- a/mm/page-writeback.c
> +++ b/mm/page-writeback.c
> @@ -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.
> -- 
> 2.14.1
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR