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

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

[Lukas Czerner]

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>
---
[v2]: prevent sleepeng for a loong time when jiffies wraps
      (Thanks to Eric Sandeen)
 fs/ext4/ext4.h  |    4 ----
 fs/ext4/super.c |   31 ++++++-------------------------
 2 files changed, 6 insertions(+), 29 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..f0e4c3a 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)
 {
@@ -2756,14 +2750,10 @@ static int ext4_lazyinit_thread(void *arg)
 	struct ext4_lazy_init *eli = (struct ext4_lazy_init *)arg;
 	struct list_head *pos, *n;
 	struct ext4_li_request *elr;
-	unsigned long next_wakeup;
-	DEFINE_WAIT(wait);
+	unsigned long next_wakeup, cur;
 
 	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);
 
@@ -2797,19 +2787,15 @@ cont_thread:
 		if (freezing(current))
 			refrigerator();
 
-		if ((time_after_eq(jiffies, next_wakeup)) ||
+		cur = jiffies;
+		if ((time_after_eq(cur, next_wakeup)) ||
 		    (MAX_JIFFY_OFFSET == next_wakeup)) {
 			cond_resched();
 			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 - cur);
+
 		if (kthread_should_stop()) {
 			ext4_clear_request_list();
 			goto exit_thread;
@@ -2833,12 +2819,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 +2850,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 +2898,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;
-- 
1.7.4.4

[PATCH 2/4 v2] ext4: Remove unnecessary wait_event ext4_run_lazyinit_thread()

[Lukas Czerner]

For some reason we have been waiting for lazyinit thread to start in the
ext4_run_lazyinit_thread() but it is not needed anymore so get rid of
it. We can also remove li_task and li_wait_task since it is not used
anymore.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 fs/ext4/ext4.h  |    2 --
 fs/ext4/super.c |   10 ----------
 2 files changed, 0 insertions(+), 12 deletions(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 1e37c09..8689f97 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -1590,8 +1590,6 @@ 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_task;
-	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 f0e4c3a..6ccf0e2 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -2754,9 +2754,6 @@ static int ext4_lazyinit_thread(void *arg)
 
 	BUG_ON(NULL == eli);
 
-	eli->li_task = current;
-	wake_up(&eli->li_wait_task);
-
 cont_thread:
 	while (true) {
 		next_wakeup = MAX_JIFFY_OFFSET;
@@ -2819,9 +2816,6 @@ exit_thread:
 		goto cont_thread;
 	}
 	mutex_unlock(&eli->li_list_mtx);
-	eli->li_task = NULL;
-	wake_up(&eli->li_wait_task);
-
 	kfree(ext4_li_info);
 	ext4_li_info = NULL;
 	mutex_unlock(&ext4_li_mtx);
@@ -2858,8 +2852,6 @@ static int ext4_run_lazyinit_thread(void)
 		return err;
 	}
 	ext4_li_info->li_state |= EXT4_LAZYINIT_RUNNING;

Re: [PATCH 2/4 v2] ext4: Remove unnecessary wait_event ext4_run_lazyinit_thread()

[Eric Sandeen]

On 5/9/11 10:57 AM, Lukas Czerner wrote:
> For some reason we have been waiting for lazyinit thread to start in the
> ext4_run_lazyinit_thread() but it is not needed anymore so get rid of
> it. We can also remove li_task and li_wait_task since it is not used
> anymore.
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>

Can you add to the changelog why this is "not needed anymore?"
What changed?  Was there any reason that it was waiting before,
or was that just unnecessary complexity?  I don't think
there is a need to wait for the thread to ping us back
after calling kthread_run() but I just wonder why it was
there in the first place...

Other than that, seems fine, so:

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
>  fs/ext4/ext4.h  |    2 --
>  fs/ext4/super.c |   10 ----------
>  2 files changed, 0 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 1e37c09..8689f97 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -1590,8 +1590,6 @@ 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_task;
> -	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 f0e4c3a..6ccf0e2 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -2754,9 +2754,6 @@ static int ext4_lazyinit_thread(void *arg)
>  
>  	BUG_ON(NULL == eli);
>  
> -	eli->li_task = current;
> -	wake_up(&eli->li_wait_task);
> -
>  cont_thread:
>  	while (true) {
>  		next_wakeup = MAX_JIFFY_OFFSET;
> @@ -2819,9 +2816,6 @@ exit_thread:
>  		goto cont_thread;
>  	}
>  	mutex_unlock(&eli->li_list_mtx);
> -	eli->li_task = NULL;
> -	wake_up(&eli->li_wait_task);
> -
>  	kfree(ext4_li_info);
>  	ext4_li_info = NULL;
>  	mutex_unlock(&ext4_li_mtx);
> @@ -2858,8 +2852,6 @@ static int ext4_run_lazyinit_thread(void)
>  		return err;
>  	}
>  	ext4_li_info->li_state |= EXT4_LAZYINIT_RUNNING;
> -
> -	wait_event(ext4_li_info->li_wait_task, ext4_li_info->li_task != NULL);
>  	return 0;
>  }
>  
> @@ -2894,11 +2886,9 @@ static int ext4_li_info_new(void)
>  	if (!eli)
>  		return -ENOMEM;
>  
> -	eli->li_task = NULL;
>  	INIT_LIST_HEAD(&eli->li_request_list);
>  	mutex_init(&eli->li_list_mtx);
>  
> -	init_waitqueue_head(&eli->li_wait_task);
>  	eli->li_state |= EXT4_LAZYINIT_QUIT;
>  
>  	ext4_li_info = eli;

Re: [PATCH 2/4 v2] ext4: Remove unnecessary wait_event ext4_run_lazyinit_thread()

[Lukas Czerner]

On Thu, 19 May 2011, Eric Sandeen wrote:

> On 5/9/11 10:57 AM, Lukas Czerner wrote:
> > For some reason we have been waiting for lazyinit thread to start in the
> > ext4_run_lazyinit_thread() but it is not needed anymore so get rid of
> > it. We can also remove li_task and li_wait_task since it is not used
> > anymore.
> > 
> > Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> 
> Can you add to the changelog why this is "not needed anymore?"
> What changed?  Was there any reason that it was waiting before,
> or was that just unnecessary complexity?  I don't think
> there is a need to wait for the thread to ping us back
> after calling kthread_run() but I just wonder why it was
> there in the first place...

Ok, I'll try to figure that out :) But probably just unnecessary
complexity :)

Thanks for the review!
-Lukas

> 
> Other than that, seems fine, so:
> 
> Reviewed-by: Eric Sandeen <sandeen@redhat.com>
> 
> > ---
> >  fs/ext4/ext4.h  |    2 --
> >  fs/ext4/super.c |   10 ----------
> >  2 files changed, 0 insertions(+), 12 deletions(-)
> > 
> > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> > index 1e37c09..8689f97 100644
> > --- a/fs/ext4/ext4.h
> > +++ b/fs/ext4/ext4.h
> > @@ -1590,8 +1590,6 @@ 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_task;
> > -	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 f0e4c3a..6ccf0e2 100644
> > --- a/fs/ext4/super.c
> > +++ b/fs/ext4/super.c
> > @@ -2754,9 +2754,6 @@ static int ext4_lazyinit_thread(void *arg)
> >  
> >  	BUG_ON(NULL == eli);
> >  
> > -	eli->li_task = current;
> > -	wake_up(&eli->li_wait_task);
> > -
> >  cont_thread:
> >  	while (true) {
> >  		next_wakeup = MAX_JIFFY_OFFSET;
> > @@ -2819,9 +2816,6 @@ exit_thread:
> >  		goto cont_thread;
> >  	}
> >  	mutex_unlock(&eli->li_list_mtx);
> > -	eli->li_task = NULL;
> > -	wake_up(&eli->li_wait_task);
> > -
> >  	kfree(ext4_li_info);
> >  	ext4_li_info = NULL;
> >  	mutex_unlock(&ext4_li_mtx);
> > @@ -2858,8 +2852,6 @@ static int ext4_run_lazyinit_thread(void)
> >  		return err;
> >  	}
> >  	ext4_li_info->li_state |= EXT4_LAZYINIT_RUNNING;
> > -
> > -	wait_event(ext4_li_info->li_wait_task, ext4_li_info->li_task != NULL);
> >  	return 0;
> >  }
> >  
> > @@ -2894,11 +2886,9 @@ static int ext4_li_info_new(void)
> >  	if (!eli)
> >  		return -ENOMEM;
> >  
> > -	eli->li_task = NULL;
> >  	INIT_LIST_HEAD(&eli->li_request_list);
> >  	mutex_init(&eli->li_list_mtx);
> >  
> > -	init_waitqueue_head(&eli->li_wait_task);
> >  	eli->li_state |= EXT4_LAZYINIT_QUIT;
> >  
> >  	ext4_li_info = eli;
> 
> 

-- 

[PATCH 3/4] ext4: fix init_itable=n to work as expected for n=0

[Lukas Czerner]

For some reason, when we set mount option init_itable=0 it behaves as
we would set init_itable=20 which is not right at all. Basically when we
set it to zero we are saying to lazyinit thread not to wait between
zeroing the inode table (except of cond_resched()) so this commit fixes
that and removes the unnecessary condition. The 'n' should be also
properly used on remount.

When the n is not set at all, it means that the default miltiplier
EXT4_DEF_LI_WAIT_MULT is set instead.

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
Reported-by: Eric Sandeen <sandeen@redhat.com>
---
 fs/ext4/super.c |   21 +++++++++++++++------
 1 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 6ccf0e2..c379af6 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -2690,11 +2690,8 @@ static int ext4_run_li_request(struct ext4_li_request *elr)
 		ret = ext4_init_inode_table(sb, group,
 					    elr->lr_timeout ? 0 : 1);
 		if (elr->lr_timeout == 0) {
-			timeout = jiffies - timeout;
-			if (elr->lr_sbi->s_li_wait_mult)
-				timeout *= elr->lr_sbi->s_li_wait_mult;
-			else
-				timeout *= 20;
+			timeout = (jiffies - timeout) *
+				  elr->lr_sbi->s_li_wait_mult;
 			elr->lr_timeout = timeout;
 		}
 		elr->lr_next_sched = jiffies + elr->lr_timeout;
@@ -2931,8 +2928,14 @@ static int ext4_register_li_request(struct super_block *sb,
 	ext4_group_t ngroups = EXT4_SB(sb)->s_groups_count;
 	int ret = 0;
 
-	if (sbi->s_li_request != NULL)
+	if (sbi->s_li_request != NULL) {
+		/*
+		 * Reset timeout so it can be computed again, because
+		 * s_li_wait_mult might have changed.
+		 */
+		sbi->s_li_request->lr_timeout = 0;
 		return 0;
+	}
 
 	if (first_not_zeroed == ngroups ||
 	    (sb->s_flags & MS_RDONLY) ||
@@ -3137,6 +3140,12 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 	    ((def_mount_opts & EXT4_DEFM_NODELALLOC) == 0))
 		set_opt(sb, DELALLOC);
 
+	/*
+	 * set default s_li_wait_mult for lazyinit, for the case there is
+	 * no mount option specified.
+	 */
+	sbi->s_li_wait_mult = EXT4_DEF_LI_WAIT_MULT;
+
 	if (!parse_options((char *) sbi->s_es->s_mount_opts, sb,
 			   &journal_devnum, &journal_ioprio, NULL, 0)) {
 		ext4_msg(sb, KERN_WARNING,
-- 
1.7.4.4

Re: [PATCH 3/4] ext4: fix init_itable=n to work as expected for n=0

[Eric Sandeen]

On 5/9/11 10:57 AM, Lukas Czerner wrote:
> For some reason, when we set mount option init_itable=0 it behaves as
> we would set init_itable=20 which is not right at all. Basically when we
> set it to zero we are saying to lazyinit thread not to wait between
> zeroing the inode table (except of cond_resched()) so this commit fixes
> that and removes the unnecessary condition. The 'n' should be also
> properly used on remount.
> 
> When the n is not set at all, it means that the default miltiplier
> EXT4_DEF_LI_WAIT_MULT is set instead.
> 
> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> Reported-by: Eric Sandeen <sandeen@redhat.com>

This is why last-minute maintainer changes that bypass the list can be dangerous ;)
(This code wasn't in Lukas' last version on the list...)

nitpick: Documentation/fs/ext4.txt should probably be updated to clarify
that the "=n" value is optional and defaults to 20...

nitpick2: Is there any reason to set the default in parse_options() since it's
already set in ext4_fill_super just prior?

i.e. would something like:

                case Opt_init_inode_table:
                        set_opt(sb, INIT_INODE_TABLE);
                        if (args[0].from) {
                                if (match_int(&args[0], &option))
                                        return 0;
	                        if (option < 0)
					return 0;
				sbi->s_li_wait_mult = option;
                        }
                        break;

suffice?

-Eric

> ---
>  fs/ext4/super.c |   21 +++++++++++++++------
>  1 files changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 6ccf0e2..c379af6 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -2690,11 +2690,8 @@ static int ext4_run_li_request(struct ext4_li_request *elr)
>  		ret = ext4_init_inode_table(sb, group,
>  					    elr->lr_timeout ? 0 : 1);
>  		if (elr->lr_timeout == 0) {
> -			timeout = jiffies - timeout;
> -			if (elr->lr_sbi->s_li_wait_mult)
> -				timeout *= elr->lr_sbi->s_li_wait_mult;
> -			else
> -				timeout *= 20;
> +			timeout = (jiffies - timeout) *
> +				  elr->lr_sbi->s_li_wait_mult;
>  			elr->lr_timeout = timeout;
>  		}
>  		elr->lr_next_sched = jiffies + elr->lr_timeout;
> @@ -2931,8 +2928,14 @@ static int ext4_register_li_request(struct super_block *sb,
>  	ext4_group_t ngroups = EXT4_SB(sb)->s_groups_count;
>  	int ret = 0;
>  
> -	if (sbi->s_li_request != NULL)
> +	if (sbi->s_li_request != NULL) {
> +		/*
> +		 * Reset timeout so it can be computed again, because
> +		 * s_li_wait_mult might have changed.
> +		 */
> +		sbi->s_li_request->lr_timeout = 0;
>  		return 0;
> +	}
>  
>  	if (first_not_zeroed == ngroups ||
>  	    (sb->s_flags & MS_RDONLY) ||
> @@ -3137,6 +3140,12 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
>  	    ((def_mount_opts & EXT4_DEFM_NODELALLOC) == 0))
>  		set_opt(sb, DELALLOC);
>  
> +	/*
> +	 * set default s_li_wait_mult for lazyinit, for the case there is
> +	 * no mount option specified.
> +	 */
> +	sbi->s_li_wait_mult = EXT4_DEF_LI_WAIT_MULT;

>  	if (!parse_options((char *) sbi->s_es->s_mount_opts, sb,
>  			   &journal_devnum, &journal_ioprio, NULL, 0)) {
>  		ext4_msg(sb, KERN_WARNING,

Re: [PATCH 3/4] ext4: fix init_itable=n to work as expected for n=0

[Lukas Czerner]

On Thu, 19 May 2011, Eric Sandeen wrote:

> On 5/9/11 10:57 AM, Lukas Czerner wrote:
> > For some reason, when we set mount option init_itable=0 it behaves as
> > we would set init_itable=20 which is not right at all. Basically when we
> > set it to zero we are saying to lazyinit thread not to wait between
> > zeroing the inode table (except of cond_resched()) so this commit fixes
> > that and removes the unnecessary condition. The 'n' should be also
> > properly used on remount.
> > 
> > When the n is not set at all, it means that the default miltiplier
> > EXT4_DEF_LI_WAIT_MULT is set instead.
> > 
> > Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> > Reported-by: Eric Sandeen <sandeen@redhat.com>
> 
> This is why last-minute maintainer changes that bypass the list can be dangerous ;)
> (This code wasn't in Lukas' last version on the list...)
> 
> nitpick: Documentation/fs/ext4.txt should probably be updated to clarify
> that the "=n" value is optional and defaults to 20...

Will do.

> 
> nitpick2: Is there any reason to set the default in parse_options() since it's
> already set in ext4_fill_super just prior?
> 
> i.e. would something like:
> 
>                 case Opt_init_inode_table:
>                         set_opt(sb, INIT_INODE_TABLE);
>                         if (args[0].from) {
>                                 if (match_int(&args[0], &option))
>                                         return 0;
> 	                        if (option < 0)
> 					return 0;
> 				sbi->s_li_wait_mult = option;
>                         }
>                         break;
> 
> suffice?
Yes, it did not notice that.

Thanks!
-Lukas

> 
> -Eric
> 
> > ---
> >  fs/ext4/super.c |   21 +++++++++++++++------
> >  1 files changed, 15 insertions(+), 6 deletions(-)
> > 
> > diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> > index 6ccf0e2..c379af6 100644
> > --- a/fs/ext4/super.c
> > +++ b/fs/ext4/super.c
> > @@ -2690,11 +2690,8 @@ static int ext4_run_li_request(struct ext4_li_request *elr)
> >  		ret = ext4_init_inode_table(sb, group,
> >  					    elr->lr_timeout ? 0 : 1);
> >  		if (elr->lr_timeout == 0) {
> > -			timeout = jiffies - timeout;
> > -			if (elr->lr_sbi->s_li_wait_mult)
> > -				timeout *= elr->lr_sbi->s_li_wait_mult;
> > -			else
> > -				timeout *= 20;
> > +			timeout = (jiffies - timeout) *
> > +				  elr->lr_sbi->s_li_wait_mult;
> >  			elr->lr_timeout = timeout;
> >  		}
> >  		elr->lr_next_sched = jiffies + elr->lr_timeout;
> > @@ -2931,8 +2928,14 @@ static int ext4_register_li_request(struct super_block *sb,
> >  	ext4_group_t ngroups = EXT4_SB(sb)->s_groups_count;
> >  	int ret = 0;
> >  
> > -	if (sbi->s_li_request != NULL)
> > +	if (sbi->s_li_request != NULL) {
> > +		/*
> > +		 * Reset timeout so it can be computed again, because
> > +		 * s_li_wait_mult might have changed.
> > +		 */
> > +		sbi->s_li_request->lr_timeout = 0;
> >  		return 0;
> > +	}
> >  
> >  	if (first_not_zeroed == ngroups ||
> >  	    (sb->s_flags & MS_RDONLY) ||
> > @@ -3137,6 +3140,12 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> >  	    ((def_mount_opts & EXT4_DEFM_NODELALLOC) == 0))
> >  		set_opt(sb, DELALLOC);
> >  
> > +	/*
> > +	 * set default s_li_wait_mult for lazyinit, for the case there is
> > +	 * no mount option specified.
> > +	 */
> > +	sbi->s_li_wait_mult = EXT4_DEF_LI_WAIT_MULT;
> 
> >  	if (!parse_options((char *) sbi->s_es->s_mount_opts, sb,
> >  			   &journal_devnum, &journal_ioprio, NULL, 0)) {
> >  		ext4_msg(sb, KERN_WARNING,
> 
> 

-- 

[PATCH 4/4] ext4: fix possible use-after-free ext4_remove_li_request()

[Lukas Czerner]

We need to take reference to the s_li_request after we take a mutex,
because it might be freed since then, hence result in accessing old
already freed memory. Also we should protect the whole
ext4_remove_li_request() because ext4_li_info might be in the process of
being freed in ext4_lazyinit_thread().

Signed-off-by: Lukas Czerner <lczerner@redhat.com>
---
 fs/ext4/super.c |   10 ++++++----
 1 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index c379af6..6a8e48f 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -2721,14 +2721,16 @@ static void ext4_remove_li_request(struct ext4_li_request *elr)
 
 static void ext4_unregister_li_request(struct super_block *sb)
 {
-	struct ext4_li_request *elr = EXT4_SB(sb)->s_li_request;

Re: [PATCH 4/4] ext4: fix possible use-after-free ext4_remove_li_request()

[Eric Sandeen]

On 5/9/11 10:57 AM, Lukas Czerner wrote:
> We need to take reference to the s_li_request after we take a mutex,
> because it might be freed since then, hence result in accessing old
> already freed memory. Also we should protect the whole
> ext4_remove_li_request() because ext4_li_info might be in the process of
> being freed in ext4_lazyinit_thread().

It'd be really great to have some comments which explain just what
ext4_li_mtx protects, but I'm working on an add-comments patch for
the lazyinit stuff (I commented things a bit as I reviewed your 
changes) so I'll send that along later.

in any case, the change looks ok, thanks.

Reviewed-by: Eric Sandeen <sandeen@redhat.com>


> Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> ---
>  fs/ext4/super.c |   10 ++++++----
>  1 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index c379af6..6a8e48f 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -2721,14 +2721,16 @@ static void ext4_remove_li_request(struct ext4_li_request *elr)
>  
>  static void ext4_unregister_li_request(struct super_block *sb)
>  {
> -	struct ext4_li_request *elr = EXT4_SB(sb)->s_li_request;
> -
> -	if (!ext4_li_info)
> +	mutex_lock(&ext4_li_mtx);
> +	if (!ext4_li_info) {
> +		mutex_unlock(&ext4_li_mtx);
>  		return;
> +	}
>  
>  	mutex_lock(&ext4_li_info->li_list_mtx);
> -	ext4_remove_li_request(elr);
> +	ext4_remove_li_request(EXT4_SB(sb)->s_li_request);
>  	mutex_unlock(&ext4_li_info->li_list_mtx);
> +	mutex_unlock(&ext4_li_mtx);
>  }
>  
>  static struct task_struct *ext4_lazyinit_task;

Re: [PATCH 4/4] ext4: fix possible use-after-free ext4_remove_li_request()

[Lukas Czerner]

On Thu, 19 May 2011, Eric Sandeen wrote:

> On 5/9/11 10:57 AM, Lukas Czerner wrote:
> > We need to take reference to the s_li_request after we take a mutex,
> > because it might be freed since then, hence result in accessing old
> > already freed memory. Also we should protect the whole
> > ext4_remove_li_request() because ext4_li_info might be in the process of
> > being freed in ext4_lazyinit_thread().
> 
> It'd be really great to have some comments which explain just what
> ext4_li_mtx protects, but I'm working on an add-comments patch for
> the lazyinit stuff (I commented things a bit as I reviewed your 
> changes) so I'll send that along later.

The ext4_li_mtx is protecting the whole ext4_li_info structure so it can
be atomically created and freed.

> 
> in any case, the change looks ok, thanks.
> 
> Reviewed-by: Eric Sandeen <sandeen@redhat.com>

Thanks!
-Lukas
> 
> 
> > Signed-off-by: Lukas Czerner <lczerner@redhat.com>
> > ---
> >  fs/ext4/super.c |   10 ++++++----
> >  1 files changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> > index c379af6..6a8e48f 100644
> > --- a/fs/ext4/super.c
> > +++ b/fs/ext4/super.c
> > @@ -2721,14 +2721,16 @@ static void ext4_remove_li_request(struct ext4_li_request *elr)
> >  
> >  static void ext4_unregister_li_request(struct super_block *sb)
> >  {
> > -	struct ext4_li_request *elr = EXT4_SB(sb)->s_li_request;
> > -
> > -	if (!ext4_li_info)
> > +	mutex_lock(&ext4_li_mtx);
> > +	if (!ext4_li_info) {
> > +		mutex_unlock(&ext4_li_mtx);
> >  		return;
> > +	}
> >  
> >  	mutex_lock(&ext4_li_info->li_list_mtx);
> > -	ext4_remove_li_request(elr);
> > +	ext4_remove_li_request(EXT4_SB(sb)->s_li_request);
> >  	mutex_unlock(&ext4_li_info->li_list_mtx);
> > +	mutex_unlock(&ext4_li_mtx);
> >  }
> >  
> >  static struct task_struct *ext4_lazyinit_task;
> 
> 

Re: [PATCH 4/4] ext4: fix possible use-after-free ext4_remove_li_request()

[Ted Ts'o]

Lukas, are you going to be providing a new version of these patch
series or not?  

If you are, could you do it as a separate patch series, instead of
only updating one patch as a reply to the mail thread.  When people do
this, I find it painful since I need to figure out, "ok, I need v2 of
the 1/4 patch, v3 of the 2/4 patch, v4 of the 3/4 patch, and v3 of of
the 4/4 patch.  To provide context, please add version descriptors
after the --- of the patch.  (i.e, "v3 --> v4; fixed commit message")

Also, if we're going to be doing extended review of patches like this,
instead of my just fixing things up when I pull stuff in, people need
to start authoring patches ***much*** sooner.  Doing multiple publish
and review cycles now that the merge window is open really doesn't
work.  One way of solving this in the future is to simply not take any
patch that is first submitted after say, -rc5 or -rc6 until the next
merge window.  But given that some patches didn't *start* getting much
review until 2-3 weeks ago, that wouldn't be entirely fair.

But for the next merge window, if this is going to work, we need
people submitting patches earlier, and people reviewing patches
earlier.

Thanks,

						- Ted

Re: [PATCH 4/4] ext4: fix possible use-after-free ext4_remove_li_request()

[Eric Sandeen]

On 5/20/11 11:03 AM, Ted Ts'o wrote:
> Lukas, are you going to be providing a new version of these patch
> series or not?  
> 
> If you are, could you do it as a separate patch series, instead of
> only updating one patch as a reply to the mail thread.  When people do
> this, I find it painful since I need to figure out, "ok, I need v2 of
> the 1/4 patch, v3 of the 2/4 patch, v4 of the 3/4 patch, and v3 of of
> the 4/4 patch.  To provide context, please add version descriptors
> after the --- of the patch.  (i.e, "v3 --> v4; fixed commit message")
> 
> Also, if we're going to be doing extended review of patches like this,
> instead of my just fixing things up when I pull stuff in, people need
> to start authoring patches ***much*** sooner.  Doing multiple publish
> and review cycles now that the merge window is open really doesn't
> work.  One way of solving this in the future is to simply not take any
> patch that is first submitted after say, -rc5 or -rc6 until the next
> merge window.  But given that some patches didn't *start* getting much
> review until 2-3 weeks ago, that wouldn't be entirely fair.
> 
> But for the next merge window, if this is going to work, we need
> people submitting patches earlier, and people reviewing patches
> earlier.

How about a reasonable sounding convention like: if it's non-critical
it's too late, but if it's critical you'll try to get it in.

Windows are windows, reviews are reviews, and if it's too late,
it's too late ... You ultimately get to decide what goes in
and when.

But skipping thorough review simply because the window is open now
doesn't make sense to me.

-Eric

Re: [PATCH 4/4] ext4: fix possible use-after-free ext4_remove_li_request()

[Ted Ts'o]

On Fri, May 20, 2011 at 11:12:05AM -0500, Eric Sandeen wrote:
> 
> But skipping thorough review simply because the window is open now
> doesn't make sense to me.

Sure, but adding a 24-hour turnaround for at least simple fix ups
(commit descriptions, comments, white space, etc) doesn't make a lot
of sense either.

						- Ted

Re: [PATCH 4/4] ext4: fix possible use-after-free ext4_remove_li_request()

[Eric Sandeen]

On 5/20/11 12:47 PM, Ted Ts'o wrote:
> On Fri, May 20, 2011 at 11:12:05AM -0500, Eric Sandeen wrote:
>>
>> But skipping thorough review simply because the window is open now
>> doesn't make sense to me.
> 
> Sure, but adding a 24-hour turnaround for at least simple fix ups
> (commit descriptions, comments, white space, etc) doesn't make a lot
> of sense either.
> 
> 						- Ted

I think you are free to use your discretion for those things :)

I mean if the last review comment is "you have a typo here" and
you really want to make the window, it seems quite reasonable to
fix it up, with a note in the commit log.

-Eric

Re: [PATCH 4/4] ext4: fix possible use-after-free ext4_remove_li_request()

[Lukas Czerner]

Hi Ted,

On Fri, 20 May 2011, Ted Ts'o wrote:

> Lukas, are you going to be providing a new version of these patch
> series or not?  

I already did send it as a separate patch series with the version
description.

> 
> If you are, could you do it as a separate patch series, instead of
> only updating one patch as a reply to the mail thread.  When people do
> this, I find it painful since I need to figure out, "ok, I need v2 of
> the 1/4 patch, v3 of the 2/4 patch, v4 of the 3/4 patch, and v3 of of
> the 4/4 patch.  To provide context, please add version descriptors
> after the --- of the patch.  (i.e, "v3 --> v4; fixed commit message")
> 
> Also, if we're going to be doing extended review of patches like this,
> instead of my just fixing things up when I pull stuff in, people need
> to start authoring patches ***much*** sooner.  Doing multiple publish
> and review cycles now that the merge window is open really doesn't
> work.  One way of solving this in the future is to simply not take any
> patch that is first submitted after say, -rc5 or -rc6 until the next
> merge window.  But given that some patches didn't *start* getting much
> review until 2-3 weeks ago, that wouldn't be entirely fair.

We all should do better, but I am not sure that limits like that are very
useful. Some level of flexibility is always needed.

> 
> But for the next merge window, if this is going to work, we need
> people submitting patches earlier, and people reviewing patches
> earlier.

So let's to that people ;).

> 
> Thanks,
> 
> 						- Ted

Thanks Ted!
-Lukas

> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 

Re: [PATCH 4/4] ext4: fix possible use-after-free ext4_remove_li_request()

[Ted Ts'o]

On Fri, May 20, 2011 at 06:16:34PM +0200, Lukas Czerner wrote:
> 
> I already did send it as a separate patch series with the version
> description.

What, you mean the -v2 patch series?

But Eric has made further suggestions where you said you might try to
do more work.  Or did I misread the mail thread?  This is why I hate
it when you reply to the mail thread with an updated patch.  It makes
it very hard to figure out what's going on....

So let me rephrase the question:  Is -v2 your final answer?

       	  	       		     	 - Ted

Re: [PATCH 4/4] ext4: fix possible use-after-free ext4_remove_li_request()

[Ted Ts'o]

On Fri, May 20, 2011 at 01:39:46PM -0400, Ted Ts'o wrote:
> On Fri, May 20, 2011 at 06:16:34PM +0200, Lukas Czerner wrote:
> > 
> > I already did send it as a separate patch series with the version
> > description.
> 
> What, you mean the -v2 patch series?
> 
> 
> So let me rephrase the question:  Is -v2 your final answer?

Ah, never mind.  I got confused when with the other patch series where
half the patches where -v3, and the other half were -v2.  It makes
what shows up in patchwork hard to follow.  In the future, please bump
the version number on the entire series, not just on individual
patches, as it makes it a lot easier for me to track what is going
on....

						- Ted

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

[Eric Sandeen]

On 5/9/11 10:57 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>

Thanks Lukas - this looks good to me and takes care of the concern
about jiffies wrapping.

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
> [v2]: prevent sleepeng for a loong time when jiffies wraps
>       (Thanks to Eric Sandeen)
>  fs/ext4/ext4.h  |    4 ----
>  fs/ext4/super.c |   31 ++++++-------------------------
>  2 files changed, 6 insertions(+), 29 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..f0e4c3a 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)
>  {
> @@ -2756,14 +2750,10 @@ static int ext4_lazyinit_thread(void *arg)
>  	struct ext4_lazy_init *eli = (struct ext4_lazy_init *)arg;
>  	struct list_head *pos, *n;
>  	struct ext4_li_request *elr;
> -	unsigned long next_wakeup;
> -	DEFINE_WAIT(wait);
> +	unsigned long next_wakeup, cur;
>  
>  	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);
>  
> @@ -2797,19 +2787,15 @@ cont_thread:
>  		if (freezing(current))
>  			refrigerator();
>  
> -		if ((time_after_eq(jiffies, next_wakeup)) ||
> +		cur = jiffies;
> +		if ((time_after_eq(cur, next_wakeup)) ||
>  		    (MAX_JIFFY_OFFSET == next_wakeup)) {
>  			cond_resched();
>  			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 - cur);
> +
>  		if (kthread_should_stop()) {
>  			ext4_clear_request_list();
>  			goto exit_thread;
> @@ -2833,12 +2819,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 +2850,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 +2898,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;