Re: [PATCH 1/2] ext4: Use schedule_timeout_interruptible() for waiting in lazyinit thread

[Eric Sandeen <sandeen@redhat.com>]

On 5/2/11 6:22 AM, Lukas Czerner wrote:
> In order to make lazyinit eat approx. 10% of io bandwidth at max, we are
> sleeping between zeroing each single inode table. For that purpose we
> are using timer which wakes up thread when it expires. It is set via
> add_timer() and this may cause troubles in the case that thread has been
> woken up earlier and in next iteration we call add_timer() on still
> running timer hence hitting BUG_ON in add_timer(). We could fix that by
> using mod_timer() instead however we can use
> schedule_timeout_interruptible() for waiting and hence simplifying
> things a lot.
> 
> This commit exchange the old "waiting mechanism" with simple
> schedule_timeout_interruptible(), setting the time to sleep. Hence we do
> not longer need li_wait_daemon waiting queue and others, so get rid of
> it.
> 
> This solves Red Hat bug 699708.
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> ---
>  fs/ext4/ext4.h  |    4 ----
>  fs/ext4/super.c |   26 +++-----------------------
>  2 files changed, 3 insertions(+), 27 deletions(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 4daaf2b..1e37c09 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -1590,12 +1590,8 @@ void ext4_get_group_no_and_offset(struct super_block *sb, ext4_fsblk_t blocknr,
>   */
>  struct ext4_lazy_init {
>  	unsigned long		li_state;
> -
> -	wait_queue_head_t	li_wait_daemon;
>  	wait_queue_head_t	li_wait_task;
> -	struct timer_list	li_timer;
>  	struct task_struct	*li_task;
> -
>  	struct list_head	li_request_list;
>  	struct mutex		li_list_mtx;
>  };
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 8553dfb..daf3300 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -2659,12 +2659,6 @@ static void print_daily_error_info(unsigned long arg)
>  	mod_timer(&sbi->s_err_report, jiffies + 24*60*60*HZ);  /* Once a day */
>  }
>  
> -static void ext4_lazyinode_timeout(unsigned long data)
> -{
> -	struct task_struct *p = (struct task_struct *)data;
> -	wake_up_process(p);
> -}
> -
>  /* Find next suitable group and run ext4_init_inode_table */
>  static int ext4_run_li_request(struct ext4_li_request *elr)
>  {
> @@ -2712,7 +2706,7 @@ static int ext4_run_li_request(struct ext4_li_request *elr)
>  
>  /*
>   * Remove lr_request from the list_request and free the
> - * request tructure. Should be called with li_list_mtx held
> + * request structure. Should be called with li_list_mtx held
>   */
>  static void ext4_remove_li_request(struct ext4_li_request *elr)
>  {
> @@ -2757,13 +2751,9 @@ static int ext4_lazyinit_thread(void *arg)
>  	struct list_head *pos, *n;
>  	struct ext4_li_request *elr;
>  	unsigned long next_wakeup;
> -	DEFINE_WAIT(wait);
>  
>  	BUG_ON(NULL == eli);
>  
> -	eli->li_timer.data = (unsigned long)current;
> -	eli->li_timer.function = ext4_lazyinode_timeout;
> -
>  	eli->li_task = current;
>  	wake_up(&eli->li_wait_task);
>  
> @@ -2803,13 +2793,8 @@ cont_thread:
>  			continue;
>  		}
>  
> -		eli->li_timer.expires = next_wakeup;
> -		add_timer(&eli->li_timer);
> -		prepare_to_wait(&eli->li_wait_daemon, &wait,
> -				TASK_INTERRUPTIBLE);
> -		if (time_before(jiffies, next_wakeup))
> -			schedule();
> -		finish_wait(&eli->li_wait_daemon, &wait);
> +		schedule_timeout_interruptible(next_wakeup - jiffies);

Jeff asked whether this can wrap.  I think it can?

Do we need to do the trick of delta = (long)next_wakeup - (long)jiffies to make sure we don't schedule out forever if jiffies has wrapped and next_wakeup has not?  Or maybe I'm behind on my jiffy-math.... :(

-Eric

> +
>  		if (kthread_should_stop()) {
>  			ext4_clear_request_list();
>  			goto exit_thread;
> @@ -2833,12 +2818,10 @@ exit_thread:
>  		goto cont_thread;
>  	}
>  	mutex_unlock(&eli->li_list_mtx);
> -	del_timer_sync(&ext4_li_info->li_timer);
>  	eli->li_task = NULL;
>  	wake_up(&eli->li_wait_task);
>  
>  	kfree(ext4_li_info);
> -	ext4_lazyinit_task = NULL;
>  	ext4_li_info = NULL;
>  	mutex_unlock(&ext4_li_mtx);
>  
> @@ -2866,7 +2849,6 @@ static int ext4_run_lazyinit_thread(void)
>  	if (IS_ERR(ext4_lazyinit_task)) {
>  		int err = PTR_ERR(ext4_lazyinit_task);
>  		ext4_clear_request_list();
> -		del_timer_sync(&ext4_li_info->li_timer);
>  		kfree(ext4_li_info);
>  		ext4_li_info = NULL;
>  		printk(KERN_CRIT "EXT4: error %d creating inode table "
> @@ -2915,9 +2897,7 @@ static int ext4_li_info_new(void)
>  	INIT_LIST_HEAD(&eli->li_request_list);
>  	mutex_init(&eli->li_list_mtx);
>  
> -	init_waitqueue_head(&eli->li_wait_daemon);
>  	init_waitqueue_head(&eli->li_wait_task);
> -	init_timer(&eli->li_timer);
>  	eli->li_state |= EXT4_LAZYINIT_QUIT;
>  
>  	ext4_li_info = eli;