[PATCH 1/3] nilfs2: remove nilfs_segctor_init() in segment.c

[PATCH 1/3] nilfs2: remove nilfs_segctor_init() in segment.c

[Li Hong]

Hi KONISHI Ryusuke,

Three new patches based on nilfs2/for-next branch. New code has been built and
loaded successfully, and also passed a light-weight reads and writes test.

Thanks,
Li Hong

---------------------------- cut here --------------------------

From 2c622d0f59782321204bf1fde7eea4a593cc6b65 Mon Sep 17 00:00:00 2001
From: Li Hong <lihong.hi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Date: Sat, 10 Apr 2010 21:57:11 +0800
Subject: [PATCH 1/3] nilfs2: remove nilfs_segctor_init() in segment.c

There are only two lines of code in nilfs_segctor_init(). From a logic design
view, the first line 'sci->sc_seq_done = sci->sc_seq_request;' should be put in
nilfs_segctor_new(). Even in nilfs_segctor_new(), this initialization is
needless because sci is kzalloc-ed. So nilfs_segctor_init() is only a wrap call
to nilfs_segctor_start_thread(). This removes an indirect call overhead.

Signed-off-by: Li Hong <lihong.hi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 fs/nilfs2/segment.c |    9 +--------
 1 files changed, 1 insertions(+), 8 deletions(-)

diff --git a/fs/nilfs2/segment.c b/fs/nilfs2/segment.c
index f235fc0..514620d 100644
--- a/fs/nilfs2/segment.c
+++ b/fs/nilfs2/segment.c
@@ -2684,13 +2684,6 @@ static void nilfs_segctor_kill_thread(struct nilfs_sc_info *sci)
 	}
 }
 
-static int nilfs_segctor_init(struct nilfs_sc_info *sci)
-{
-	sci->sc_seq_done = sci->sc_seq_request;
-
-	return nilfs_segctor_start_thread(sci);
-}
-
 /*
  * Setup & clean-up functions
  */
@@ -2814,7 +2807,7 @@ int nilfs_attach_segment_constructor(struct nilfs_sb_info *sbi)
 		return -ENOMEM;
 
 	nilfs_attach_writer(nilfs, sbi);
-	err = nilfs_segctor_init(NILFS_SC(sbi));
+	err = nilfs_segctor_start_thread(NILFS_SC(sbi));
 	if (err) {
 		nilfs_detach_writer(nilfs, sbi);
 		kfree(sbi->s_sc_info);
-- 
1.6.3.3

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

[PATCH 2/3] nilfs2: nilfs_segctor_destroy() should do a full job

[Li Hong]

From 466fbbc10dabc8a14e292ee19636fc534036a8b7 Mon Sep 17 00:00:00 2001
From: Li Hong <lihong.hi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Date: Sat, 10 Apr 2010 22:26:04 +0800
Subject: [PATCH 2/3] nilfs2: nilfs_segctor_destroy() should do a full job

To set "sbi->s_sc_info = NULL;" is also a part of nilfs_segctor_destroy()'s job.
Move it into the destroy procedure.

Signed-off-by: Li Hong <lihong.hi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 fs/nilfs2/segment.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/fs/nilfs2/segment.c b/fs/nilfs2/segment.c
index 514620d..dd3c4d5 100644
--- a/fs/nilfs2/segment.c
+++ b/fs/nilfs2/segment.c
@@ -2774,6 +2774,7 @@ static void nilfs_segctor_destroy(struct nilfs_sc_info *sci)
 	down_write(&sbi->s_nilfs->ns_segctor_sem);
 
 	kfree(sci);
+	sbi->s_sc_info = NULL;
 }
 
 /**
@@ -2829,10 +2830,8 @@ void nilfs_detach_segment_constructor(struct nilfs_sb_info *sbi)
 	LIST_HEAD(garbage_list);
 
 	down_write(&nilfs->ns_segctor_sem);
-	if (NILFS_SC(sbi)) {
+	if (NILFS_SC(sbi))
 		nilfs_segctor_destroy(NILFS_SC(sbi));
-		sbi->s_sc_info = NULL;
-	}
 
 	/* Force to free the list of dirty files */
 	spin_lock(&sbi->s_inode_lock);
-- 
1.6.3.3

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

[PATCH 3/3] nilfs2: change sc_timer from a pointer to an embedded one in struct nilfs_sc_info

[Li Hong]

In nilfs_segctor_thread(), timer is a local variable allocated on stack. Its
address can't be set to sci->sc_timer and passed in several procedures.

It works now by chance, just because other procedures are called by
nilfs_segctor_thread() directly or indirectly and the stack hasn't been
deallocated yet.

Signed-off-by: Li Hong <lihong.hi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 fs/nilfs2/segment.c |   31 +++++++++++++------------------
 fs/nilfs2/segment.h |    2 +-
 2 files changed, 14 insertions(+), 19 deletions(-)

diff --git a/fs/nilfs2/segment.c b/fs/nilfs2/segment.c
index dd3c4d5..310979a 100644
--- a/fs/nilfs2/segment.c
+++ b/fs/nilfs2/segment.c
@@ -2158,9 +2158,9 @@ static int nilfs_segctor_do_construct(struct nilfs_sc_info *sci, int mode)
 static void nilfs_segctor_start_timer(struct nilfs_sc_info *sci)
 {
 	spin_lock(&sci->sc_state_lock);
-	if (sci->sc_timer && !(sci->sc_state & NILFS_SEGCTOR_COMMIT)) {
-		sci->sc_timer->expires = jiffies + sci->sc_interval;
-		add_timer(sci->sc_timer);
+	if (!(sci->sc_state & NILFS_SEGCTOR_COMMIT)) {
+		sci->sc_timer.expires = jiffies + sci->sc_interval;
+		add_timer(&sci->sc_timer);
 		sci->sc_state |= NILFS_SEGCTOR_COMMIT;
 	}
 	spin_unlock(&sci->sc_state_lock);
@@ -2365,9 +2365,7 @@ static void nilfs_segctor_accept(struct nilfs_sc_info *sci)
 	spin_lock(&sci->sc_state_lock);
 	sci->sc_seq_accepted = sci->sc_seq_request;
 	spin_unlock(&sci->sc_state_lock);
-
-	if (sci->sc_timer)
-		del_timer_sync(sci->sc_timer);
+	del_timer_sync(&sci->sc_timer);
 }
 
 /**
@@ -2393,9 +2391,9 @@ static void nilfs_segctor_notify(struct nilfs_sc_info *sci, int mode, int err)
 			sci->sc_flush_request &= ~FLUSH_DAT_BIT;
 
 		/* re-enable timer if checkpoint creation was not done */
-		if (sci->sc_timer && (sci->sc_state & NILFS_SEGCTOR_COMMIT) &&
-		    time_before(jiffies, sci->sc_timer->expires))
-			add_timer(sci->sc_timer);
+		if ((sci->sc_state & NILFS_SEGCTOR_COMMIT) &&
+		    time_before(jiffies, sci->sc_timer.expires))
+			add_timer(&sci->sc_timer);
 	}
 	spin_unlock(&sci->sc_state_lock);
 }
@@ -2574,13 +2572,10 @@ static int nilfs_segctor_thread(void *arg)
 {
 	struct nilfs_sc_info *sci = (struct nilfs_sc_info *)arg;
 	struct the_nilfs *nilfs = sci->sc_sbi->s_nilfs;
-	struct timer_list timer;
 	int timeout = 0;
 
-	init_timer(&timer);
-	timer.data = (unsigned long)current;
-	timer.function = nilfs_construction_timeout;
-	sci->sc_timer = &timer;
+	sci->sc_timer.data = (unsigned long)current;
+	sci->sc_timer.function = nilfs_construction_timeout;
 
 	/* start sync. */
 	sci->sc_task = current;
@@ -2629,7 +2624,7 @@ static int nilfs_segctor_thread(void *arg)
 			should_sleep = 0;
 		else if (sci->sc_state & NILFS_SEGCTOR_COMMIT)
 			should_sleep = time_before(jiffies,
-						   sci->sc_timer->expires);
+					sci->sc_timer.expires);
 
 		if (should_sleep) {
 			spin_unlock(&sci->sc_state_lock);
@@ -2638,7 +2633,7 @@ static int nilfs_segctor_thread(void *arg)
 		}
 		finish_wait(&sci->sc_wait_daemon, &wait);
 		timeout = ((sci->sc_state & NILFS_SEGCTOR_COMMIT) &&
-			   time_after_eq(jiffies, sci->sc_timer->expires));
+			   time_after_eq(jiffies, sci->sc_timer.expires));
 
 		if (nilfs_sb_dirty(nilfs) && nilfs_sb_need_update(nilfs))
 			set_nilfs_discontinued(nilfs);
@@ -2647,8 +2642,7 @@ static int nilfs_segctor_thread(void *arg)
 
  end_thread:
 	spin_unlock(&sci->sc_state_lock);
-	del_timer_sync(sci->sc_timer);
-	sci->sc_timer = NULL;
+	del_timer_sync(&sci->sc_timer);
 
 	/* end sync. */
 	sci->sc_task = NULL;
@@ -2707,6 +2701,7 @@ static struct nilfs_sc_info *nilfs_segctor_new(struct nilfs_sb_info *sbi)
 	INIT_LIST_HEAD(&sci->sc_write_logs);
 	INIT_LIST_HEAD(&sci->sc_gc_inodes);
 	INIT_LIST_HEAD(&sci->sc_copied_buffers);
+	init_timer(&sci->sc_timer);
 
 	sci->sc_interval = HZ * NILFS_SC_DEFAULT_TIMEOUT;
 	sci->sc_mjcp_freq = HZ * NILFS_SC_DEFAULT_SR_FREQ;
diff --git a/fs/nilfs2/segment.h b/fs/nilfs2/segment.h
index 7aca765..dca1423 100644
--- a/fs/nilfs2/segment.h
+++ b/fs/nilfs2/segment.h
@@ -177,7 +177,7 @@ struct nilfs_sc_info {
 	unsigned long		sc_lseg_stime;	/* in 1/HZ seconds */
 	unsigned long		sc_watermark;
 
-	struct timer_list      *sc_timer;
+	struct timer_list	sc_timer;
 	struct task_struct     *sc_task;
 };
 
-- 
1.6.3.3

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

Re: [PATCH 1/3] nilfs2: remove nilfs_segctor_init() in segment.c

[Ryusuke Konishi]

Hi,
On Sun, 11 Apr 2010 00:15:43 +0800, Li Hong <lihong.hi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Hi KONISHI Ryusuke,
> 
> Three new patches based on nilfs2/for-next branch. New code has been built and
> loaded successfully, and also passed a light-weight reads and writes test.
> 
> Thanks,
> Li Hong

Ok, I'll look into each of them.

> ---------------------------- cut here --------------------------
> 
> From 2c622d0f59782321204bf1fde7eea4a593cc6b65 Mon Sep 17 00:00:00 2001
> From: Li Hong <lihong.hi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Date: Sat, 10 Apr 2010 21:57:11 +0800
> Subject: [PATCH 1/3] nilfs2: remove nilfs_segctor_init() in segment.c
> 
> There are only two lines of code in nilfs_segctor_init(). From a logic design
> view, the first line 'sci->sc_seq_done = sci->sc_seq_request;' should be put in
> nilfs_segctor_new(). Even in nilfs_segctor_new(), this initialization is
> needless because sci is kzalloc-ed. So nilfs_segctor_init() is only a wrap call
> to nilfs_segctor_start_thread(). This removes an indirect call overhead.
> 
> Signed-off-by: Li Hong <lihong.hi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Looks no problem.

The reason why nilfs_segctor_init is present in that manner is
historical (just for your information. You don't have to mention this
reason).

I think you don't have to mention the indirect call overhead because
it's only triggered in the level of mount/unmount/remount and gcc will
inline it in the caller.

Thanks,
Ryusuke Konishi

> ---
>  fs/nilfs2/segment.c |    9 +--------
>  1 files changed, 1 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/nilfs2/segment.c b/fs/nilfs2/segment.c
> index f235fc0..514620d 100644
> --- a/fs/nilfs2/segment.c
> +++ b/fs/nilfs2/segment.c
> @@ -2684,13 +2684,6 @@ static void nilfs_segctor_kill_thread(struct nilfs_sc_info *sci)
>  	}
>  }
>  
> -static int nilfs_segctor_init(struct nilfs_sc_info *sci)
> -{
> -	sci->sc_seq_done = sci->sc_seq_request;
> -
> -	return nilfs_segctor_start_thread(sci);
> -}
> -
>  /*
>   * Setup & clean-up functions
>   */
> @@ -2814,7 +2807,7 @@ int nilfs_attach_segment_constructor(struct nilfs_sb_info *sbi)
>  		return -ENOMEM;
>  
>  	nilfs_attach_writer(nilfs, sbi);
> -	err = nilfs_segctor_init(NILFS_SC(sbi));
> +	err = nilfs_segctor_start_thread(NILFS_SC(sbi));
>  	if (err) {
>  		nilfs_detach_writer(nilfs, sbi);
>  		kfree(sbi->s_sc_info);
> -- 
> 1.6.3.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] nilfs2: nilfs_segctor_destroy() should do a full job

[Ryusuke Konishi]

On Sun, 11 Apr 2010 00:17:00 +0800, Li Hong <lihong.hi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> From 466fbbc10dabc8a14e292ee19636fc534036a8b7 Mon Sep 17 00:00:00 2001
> From: Li Hong <lihong.hi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Date: Sat, 10 Apr 2010 22:26:04 +0800
> Subject: [PATCH 2/3] nilfs2: nilfs_segctor_destroy() should do a full job
> 
> To set "sbi->s_sc_info = NULL;" is also a part of nilfs_segctor_destroy()'s job.
> Move it into the destroy procedure.
> 
> Signed-off-by: Li Hong <lihong.hi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

No, don't do this.  sbi->s_sc_info is the holder of the segment
constructor object and is not a part of the object though
nilfs_segctor_destroy needs some cleanup.

Ryusuke Konishi

> ---
>  fs/nilfs2/segment.c |    5 ++---
>  1 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/nilfs2/segment.c b/fs/nilfs2/segment.c
> index 514620d..dd3c4d5 100644
> --- a/fs/nilfs2/segment.c
> +++ b/fs/nilfs2/segment.c
> @@ -2774,6 +2774,7 @@ static void nilfs_segctor_destroy(struct nilfs_sc_info *sci)
>  	down_write(&sbi->s_nilfs->ns_segctor_sem);
>  
>  	kfree(sci);
> +	sbi->s_sc_info = NULL;
>  }
>  
>  /**
> @@ -2829,10 +2830,8 @@ void nilfs_detach_segment_constructor(struct nilfs_sb_info *sbi)
>  	LIST_HEAD(garbage_list);
>  
>  	down_write(&nilfs->ns_segctor_sem);
> -	if (NILFS_SC(sbi)) {
> +	if (NILFS_SC(sbi))
>  		nilfs_segctor_destroy(NILFS_SC(sbi));
> -		sbi->s_sc_info = NULL;
> -	}
>  
>  	/* Force to free the list of dirty files */
>  	spin_lock(&sbi->s_inode_lock);
> -- 
> 1.6.3.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] nilfs2: change sc_timer from a pointer to an embedded one in struct nilfs_sc_info

[Ryusuke Konishi]

Hi,
On Sun, 11 Apr 2010 00:17:52 +0800, Li Hong <lihong.hi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> In nilfs_segctor_thread(), timer is a local variable allocated on stack. Its
> address can't be set to sci->sc_timer and passed in several procedures.
> 
> It works now by chance, just because other procedures are called by
> nilfs_segctor_thread() directly or indirectly and the stack hasn't been
> deallocated yet.
> 
> Signed-off-by: Li Hong <lihong.hi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Looks to be heading in the right direction.

Note that the del_timer_sync call in nilfs_segctor_thread() should be
moved into nilfs_segctor_destroy() for safety.  Without the check if
sci->sc_timer != NULL, sci->sc_timer can be registered to the timer
list after the segment constructor thread died.  This leads to
corruption of the timer list.

add_timer(&sci->sc_timer) is called only while nilfs->ns_segctor_sem
(i.e. nilfs_transaction_lock) is held.

So, the place where del_timer_sync should be inserted, seems between

  down_write(&sbi->s_nilfs->ns_segctor_sem)

and

  kfree()

in nilfs_segctor_destroy().

Thanks,
Ryusuke Konishi

> ---
>  fs/nilfs2/segment.c |   31 +++++++++++++------------------
>  fs/nilfs2/segment.h |    2 +-
>  2 files changed, 14 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/nilfs2/segment.c b/fs/nilfs2/segment.c
> index dd3c4d5..310979a 100644
> --- a/fs/nilfs2/segment.c
> +++ b/fs/nilfs2/segment.c
> @@ -2158,9 +2158,9 @@ static int nilfs_segctor_do_construct(struct nilfs_sc_info *sci, int mode)
>  static void nilfs_segctor_start_timer(struct nilfs_sc_info *sci)
>  {
>  	spin_lock(&sci->sc_state_lock);
> -	if (sci->sc_timer && !(sci->sc_state & NILFS_SEGCTOR_COMMIT)) {
> -		sci->sc_timer->expires = jiffies + sci->sc_interval;
> -		add_timer(sci->sc_timer);
> +	if (!(sci->sc_state & NILFS_SEGCTOR_COMMIT)) {
> +		sci->sc_timer.expires = jiffies + sci->sc_interval;
> +		add_timer(&sci->sc_timer);
>  		sci->sc_state |= NILFS_SEGCTOR_COMMIT;
>  	}
>  	spin_unlock(&sci->sc_state_lock);
> @@ -2365,9 +2365,7 @@ static void nilfs_segctor_accept(struct nilfs_sc_info *sci)
>  	spin_lock(&sci->sc_state_lock);
>  	sci->sc_seq_accepted = sci->sc_seq_request;
>  	spin_unlock(&sci->sc_state_lock);
> -
> -	if (sci->sc_timer)
> -		del_timer_sync(sci->sc_timer);
> +	del_timer_sync(&sci->sc_timer);
>  }
>  
>  /**
> @@ -2393,9 +2391,9 @@ static void nilfs_segctor_notify(struct nilfs_sc_info *sci, int mode, int err)
>  			sci->sc_flush_request &= ~FLUSH_DAT_BIT;
>  
>  		/* re-enable timer if checkpoint creation was not done */
> -		if (sci->sc_timer && (sci->sc_state & NILFS_SEGCTOR_COMMIT) &&
> -		    time_before(jiffies, sci->sc_timer->expires))
> -			add_timer(sci->sc_timer);
> +		if ((sci->sc_state & NILFS_SEGCTOR_COMMIT) &&
> +		    time_before(jiffies, sci->sc_timer.expires))
> +			add_timer(&sci->sc_timer);
>  	}
>  	spin_unlock(&sci->sc_state_lock);
>  }
> @@ -2574,13 +2572,10 @@ static int nilfs_segctor_thread(void *arg)
>  {
>  	struct nilfs_sc_info *sci = (struct nilfs_sc_info *)arg;
>  	struct the_nilfs *nilfs = sci->sc_sbi->s_nilfs;
> -	struct timer_list timer;
>  	int timeout = 0;
>  
> -	init_timer(&timer);
> -	timer.data = (unsigned long)current;
> -	timer.function = nilfs_construction_timeout;
> -	sci->sc_timer = &timer;
> +	sci->sc_timer.data = (unsigned long)current;
> +	sci->sc_timer.function = nilfs_construction_timeout;
>  
>  	/* start sync. */
>  	sci->sc_task = current;
> @@ -2629,7 +2624,7 @@ static int nilfs_segctor_thread(void *arg)
>  			should_sleep = 0;
>  		else if (sci->sc_state & NILFS_SEGCTOR_COMMIT)
>  			should_sleep = time_before(jiffies,
> -						   sci->sc_timer->expires);
> +					sci->sc_timer.expires);
>  
>  		if (should_sleep) {
>  			spin_unlock(&sci->sc_state_lock);
> @@ -2638,7 +2633,7 @@ static int nilfs_segctor_thread(void *arg)
>  		}
>  		finish_wait(&sci->sc_wait_daemon, &wait);
>  		timeout = ((sci->sc_state & NILFS_SEGCTOR_COMMIT) &&
> -			   time_after_eq(jiffies, sci->sc_timer->expires));
> +			   time_after_eq(jiffies, sci->sc_timer.expires));
>  
>  		if (nilfs_sb_dirty(nilfs) && nilfs_sb_need_update(nilfs))
>  			set_nilfs_discontinued(nilfs);
> @@ -2647,8 +2642,7 @@ static int nilfs_segctor_thread(void *arg)
>  
>   end_thread:
>  	spin_unlock(&sci->sc_state_lock);
> -	del_timer_sync(sci->sc_timer);
> -	sci->sc_timer = NULL;
> +	del_timer_sync(&sci->sc_timer);
>  
>  	/* end sync. */
>  	sci->sc_task = NULL;
> @@ -2707,6 +2701,7 @@ static struct nilfs_sc_info *nilfs_segctor_new(struct nilfs_sb_info *sbi)
>  	INIT_LIST_HEAD(&sci->sc_write_logs);
>  	INIT_LIST_HEAD(&sci->sc_gc_inodes);
>  	INIT_LIST_HEAD(&sci->sc_copied_buffers);
> +	init_timer(&sci->sc_timer);
>  
>  	sci->sc_interval = HZ * NILFS_SC_DEFAULT_TIMEOUT;
>  	sci->sc_mjcp_freq = HZ * NILFS_SC_DEFAULT_SR_FREQ;
> diff --git a/fs/nilfs2/segment.h b/fs/nilfs2/segment.h
> index 7aca765..dca1423 100644
> --- a/fs/nilfs2/segment.h
> +++ b/fs/nilfs2/segment.h
> @@ -177,7 +177,7 @@ struct nilfs_sc_info {
>  	unsigned long		sc_lseg_stime;	/* in 1/HZ seconds */
>  	unsigned long		sc_watermark;
>  
> -	struct timer_list      *sc_timer;
> +	struct timer_list	sc_timer;
>  	struct task_struct     *sc_task;
>  };
>  
> -- 
> 1.6.3.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] nilfs2: remove nilfs_segctor_init() in segment.c

[Li Hong]

On Sun, Apr 11, 2010 at 02:57:15AM +0900, Ryusuke Konishi wrote:
> Hi,
> On Sun, 11 Apr 2010 00:15:43 +0800, Li Hong <lihong.hi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > Hi KONISHI Ryusuke,
> > 
> > Three new patches based on nilfs2/for-next branch. New code has been built and
> > loaded successfully, and also passed a light-weight reads and writes test.
> > 
> > Thanks,
> > Li Hong
> 
> Ok, I'll look into each of them.
> 
> > ---------------------------- cut here --------------------------
> > 
> > From 2c622d0f59782321204bf1fde7eea4a593cc6b65 Mon Sep 17 00:00:00 2001
> > From: Li Hong <lihong.hi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > Date: Sat, 10 Apr 2010 21:57:11 +0800
> > Subject: [PATCH 1/3] nilfs2: remove nilfs_segctor_init() in segment.c
> > 
> > There are only two lines of code in nilfs_segctor_init(). From a logic design
> > view, the first line 'sci->sc_seq_done = sci->sc_seq_request;' should be put in
> > nilfs_segctor_new(). Even in nilfs_segctor_new(), this initialization is
> > needless because sci is kzalloc-ed. So nilfs_segctor_init() is only a wrap call
> > to nilfs_segctor_start_thread(). This removes an indirect call overhead.
> > 
> > Signed-off-by: Li Hong <lihong.hi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> Looks no problem.
> 
> The reason why nilfs_segctor_init is present in that manner is
> historical (just for your information. You don't have to mention this
> reason).
> 
> I think you don't have to mention the indirect call overhead because
> it's only triggered in the level of mount/unmount/remount and gcc will
> inline it in the caller.
^^^^^^^^^^^^^^^^^^^^^^^^^
No, there is no inline key word in nilfs_segctor_init(). Unless you mean gcc
will try to inline small procedures if possible.

Thanks,
Li Hong

> Thanks,
> Ryusuke Konishi
> 
> > ---
> >  fs/nilfs2/segment.c |    9 +--------
> >  1 files changed, 1 insertions(+), 8 deletions(-)
> > 
> > diff --git a/fs/nilfs2/segment.c b/fs/nilfs2/segment.c
> > index f235fc0..514620d 100644
> > --- a/fs/nilfs2/segment.c
> > +++ b/fs/nilfs2/segment.c
> > @@ -2684,13 +2684,6 @@ static void nilfs_segctor_kill_thread(struct nilfs_sc_info *sci)
> >  	}
> >  }
> >  
> > -static int nilfs_segctor_init(struct nilfs_sc_info *sci)
> > -{
> > -	sci->sc_seq_done = sci->sc_seq_request;
> > -
> > -	return nilfs_segctor_start_thread(sci);
> > -}
> > -
> >  /*
> >   * Setup & clean-up functions
> >   */
> > @@ -2814,7 +2807,7 @@ int nilfs_attach_segment_constructor(struct nilfs_sb_info *sbi)
> >  		return -ENOMEM;
> >  
> >  	nilfs_attach_writer(nilfs, sbi);
> > -	err = nilfs_segctor_init(NILFS_SC(sbi));
> > +	err = nilfs_segctor_start_thread(NILFS_SC(sbi));
> >  	if (err) {
> >  		nilfs_detach_writer(nilfs, sbi);
> >  		kfree(sbi->s_sc_info);
> > -- 
> > 1.6.3.3
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] nilfs2: nilfs_segctor_destroy() should do a full job

[Li Hong]

On Sun, Apr 11, 2010 at 03:10:05AM +0900, Ryusuke Konishi wrote:
> On Sun, 11 Apr 2010 00:17:00 +0800, Li Hong <lihong.hi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > From 466fbbc10dabc8a14e292ee19636fc534036a8b7 Mon Sep 17 00:00:00 2001
> > From: Li Hong <lihong.hi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > Date: Sat, 10 Apr 2010 22:26:04 +0800
> > Subject: [PATCH 2/3] nilfs2: nilfs_segctor_destroy() should do a full job
> > 
> > To set "sbi->s_sc_info = NULL;" is also a part of nilfs_segctor_destroy()'s job.
> > Move it into the destroy procedure.
> > 
> > Signed-off-by: Li Hong <lihong.hi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> No, don't do this.  sbi->s_sc_info is the holder of the segment
> constructor object and is not a part of the object though
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
No, my meaning is that: to set "sbi->s_sc_info = NULL;" is also a _part_ of 
nilfs_segctor_destroy()'s _job_. I should make the explanation clearer. IMO, I
just think 'kfree something' and 'set the obj to NULL' can't be seperated by a
procedure.

Thanks,
Li Hong

> nilfs_segctor_destroy needs some cleanup.
> 
> Ryusuke Konishi
> 
> > ---
> >  fs/nilfs2/segment.c |    5 ++---
> >  1 files changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/nilfs2/segment.c b/fs/nilfs2/segment.c
> > index 514620d..dd3c4d5 100644
> > --- a/fs/nilfs2/segment.c
> > +++ b/fs/nilfs2/segment.c
> > @@ -2774,6 +2774,7 @@ static void nilfs_segctor_destroy(struct nilfs_sc_info *sci)
> >  	down_write(&sbi->s_nilfs->ns_segctor_sem);
> >  
> >  	kfree(sci);
> > +	sbi->s_sc_info = NULL;
> >  }
> >  
> >  /**
> > @@ -2829,10 +2830,8 @@ void nilfs_detach_segment_constructor(struct nilfs_sb_info *sbi)
> >  	LIST_HEAD(garbage_list);
> >  
> >  	down_write(&nilfs->ns_segctor_sem);
> > -	if (NILFS_SC(sbi)) {
> > +	if (NILFS_SC(sbi))
> >  		nilfs_segctor_destroy(NILFS_SC(sbi));
> > -		sbi->s_sc_info = NULL;
> > -	}
> >  
> >  	/* Force to free the list of dirty files */
> >  	spin_lock(&sbi->s_inode_lock);
> > -- 
> > 1.6.3.3
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] nilfs2: change sc_timer from a pointer to an embedded one in struct nilfs_sc_info

[Li Hong]

OK, I will improve this patch.

Thanks very much,
Li Hong

On Sun, Apr 11, 2010 at 11:40:22AM +0900, Ryusuke Konishi wrote:
> Hi,
> On Sun, 11 Apr 2010 00:17:52 +0800, Li Hong <lihong.hi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > In nilfs_segctor_thread(), timer is a local variable allocated on stack. Its
> > address can't be set to sci->sc_timer and passed in several procedures.
> > 
> > It works now by chance, just because other procedures are called by
> > nilfs_segctor_thread() directly or indirectly and the stack hasn't been
> > deallocated yet.
> > 
> > Signed-off-by: Li Hong <lihong.hi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> 
> Looks to be heading in the right direction.
> 
> Note that the del_timer_sync call in nilfs_segctor_thread() should be
> moved into nilfs_segctor_destroy() for safety.  Without the check if
> sci->sc_timer != NULL, sci->sc_timer can be registered to the timer
> list after the segment constructor thread died.  This leads to
> corruption of the timer list.
> 
> add_timer(&sci->sc_timer) is called only while nilfs->ns_segctor_sem
> (i.e. nilfs_transaction_lock) is held.
> 
> So, the place where del_timer_sync should be inserted, seems between
> 
>   down_write(&sbi->s_nilfs->ns_segctor_sem)
> 
> and
> 
>   kfree()
> 
> in nilfs_segctor_destroy().
> 
> Thanks,
> Ryusuke Konishi
> 
> > ---
> >  fs/nilfs2/segment.c |   31 +++++++++++++------------------
> >  fs/nilfs2/segment.h |    2 +-
> >  2 files changed, 14 insertions(+), 19 deletions(-)
> > 
> > diff --git a/fs/nilfs2/segment.c b/fs/nilfs2/segment.c
> > index dd3c4d5..310979a 100644
> > --- a/fs/nilfs2/segment.c
> > +++ b/fs/nilfs2/segment.c
> > @@ -2158,9 +2158,9 @@ static int nilfs_segctor_do_construct(struct nilfs_sc_info *sci, int mode)
> >  static void nilfs_segctor_start_timer(struct nilfs_sc_info *sci)
> >  {
> >  	spin_lock(&sci->sc_state_lock);
> > -	if (sci->sc_timer && !(sci->sc_state & NILFS_SEGCTOR_COMMIT)) {
> > -		sci->sc_timer->expires = jiffies + sci->sc_interval;
> > -		add_timer(sci->sc_timer);
> > +	if (!(sci->sc_state & NILFS_SEGCTOR_COMMIT)) {
> > +		sci->sc_timer.expires = jiffies + sci->sc_interval;
> > +		add_timer(&sci->sc_timer);
> >  		sci->sc_state |= NILFS_SEGCTOR_COMMIT;
> >  	}
> >  	spin_unlock(&sci->sc_state_lock);
> > @@ -2365,9 +2365,7 @@ static void nilfs_segctor_accept(struct nilfs_sc_info *sci)
> >  	spin_lock(&sci->sc_state_lock);
> >  	sci->sc_seq_accepted = sci->sc_seq_request;
> >  	spin_unlock(&sci->sc_state_lock);
> > -
> > -	if (sci->sc_timer)
> > -		del_timer_sync(sci->sc_timer);
> > +	del_timer_sync(&sci->sc_timer);
> >  }
> >  
> >  /**
> > @@ -2393,9 +2391,9 @@ static void nilfs_segctor_notify(struct nilfs_sc_info *sci, int mode, int err)
> >  			sci->sc_flush_request &= ~FLUSH_DAT_BIT;
> >  
> >  		/* re-enable timer if checkpoint creation was not done */
> > -		if (sci->sc_timer && (sci->sc_state & NILFS_SEGCTOR_COMMIT) &&
> > -		    time_before(jiffies, sci->sc_timer->expires))
> > -			add_timer(sci->sc_timer);
> > +		if ((sci->sc_state & NILFS_SEGCTOR_COMMIT) &&
> > +		    time_before(jiffies, sci->sc_timer.expires))
> > +			add_timer(&sci->sc_timer);
> >  	}
> >  	spin_unlock(&sci->sc_state_lock);
> >  }
> > @@ -2574,13 +2572,10 @@ static int nilfs_segctor_thread(void *arg)
> >  {
> >  	struct nilfs_sc_info *sci = (struct nilfs_sc_info *)arg;
> >  	struct the_nilfs *nilfs = sci->sc_sbi->s_nilfs;
> > -	struct timer_list timer;
> >  	int timeout = 0;
> >  
> > -	init_timer(&timer);
> > -	timer.data = (unsigned long)current;
> > -	timer.function = nilfs_construction_timeout;
> > -	sci->sc_timer = &timer;
> > +	sci->sc_timer.data = (unsigned long)current;
> > +	sci->sc_timer.function = nilfs_construction_timeout;
> >  
> >  	/* start sync. */
> >  	sci->sc_task = current;
> > @@ -2629,7 +2624,7 @@ static int nilfs_segctor_thread(void *arg)
> >  			should_sleep = 0;
> >  		else if (sci->sc_state & NILFS_SEGCTOR_COMMIT)
> >  			should_sleep = time_before(jiffies,
> > -						   sci->sc_timer->expires);
> > +					sci->sc_timer.expires);
> >  
> >  		if (should_sleep) {
> >  			spin_unlock(&sci->sc_state_lock);
> > @@ -2638,7 +2633,7 @@ static int nilfs_segctor_thread(void *arg)
> >  		}
> >  		finish_wait(&sci->sc_wait_daemon, &wait);
> >  		timeout = ((sci->sc_state & NILFS_SEGCTOR_COMMIT) &&
> > -			   time_after_eq(jiffies, sci->sc_timer->expires));
> > +			   time_after_eq(jiffies, sci->sc_timer.expires));
> >  
> >  		if (nilfs_sb_dirty(nilfs) && nilfs_sb_need_update(nilfs))
> >  			set_nilfs_discontinued(nilfs);
> > @@ -2647,8 +2642,7 @@ static int nilfs_segctor_thread(void *arg)
> >  
> >   end_thread:
> >  	spin_unlock(&sci->sc_state_lock);
> > -	del_timer_sync(sci->sc_timer);
> > -	sci->sc_timer = NULL;
> > +	del_timer_sync(&sci->sc_timer);
> >  
> >  	/* end sync. */
> >  	sci->sc_task = NULL;
> > @@ -2707,6 +2701,7 @@ static struct nilfs_sc_info *nilfs_segctor_new(struct nilfs_sb_info *sbi)
> >  	INIT_LIST_HEAD(&sci->sc_write_logs);
> >  	INIT_LIST_HEAD(&sci->sc_gc_inodes);
> >  	INIT_LIST_HEAD(&sci->sc_copied_buffers);
> > +	init_timer(&sci->sc_timer);
> >  
> >  	sci->sc_interval = HZ * NILFS_SC_DEFAULT_TIMEOUT;
> >  	sci->sc_mjcp_freq = HZ * NILFS_SC_DEFAULT_SR_FREQ;
> > diff --git a/fs/nilfs2/segment.h b/fs/nilfs2/segment.h
> > index 7aca765..dca1423 100644
> > --- a/fs/nilfs2/segment.h
> > +++ b/fs/nilfs2/segment.h
> > @@ -177,7 +177,7 @@ struct nilfs_sc_info {
> >  	unsigned long		sc_lseg_stime;	/* in 1/HZ seconds */
> >  	unsigned long		sc_watermark;
> >  
> > -	struct timer_list      *sc_timer;
> > +	struct timer_list	sc_timer;
> >  	struct task_struct     *sc_task;
> >  };
> >  
> > -- 
> > 1.6.3.3
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] nilfs2: remove nilfs_segctor_init() in segment.c

[Ryusuke Konishi]

On Sun, 11 Apr 2010 12:17:26 +0800, Li Hong <lihong.hi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Sun, Apr 11, 2010 at 02:57:15AM +0900, Ryusuke Konishi wrote:
> > Hi,
> > On Sun, 11 Apr 2010 00:15:43 +0800, Li Hong <lihong.hi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > > Hi KONISHI Ryusuke,
> > > 
> > > Three new patches based on nilfs2/for-next branch. New code has been built and
> > > loaded successfully, and also passed a light-weight reads and writes test.
> > > 
> > > Thanks,
> > > Li Hong
> > 
> > Ok, I'll look into each of them.
> > 
> > > ---------------------------- cut here --------------------------
> > > 
> > > From 2c622d0f59782321204bf1fde7eea4a593cc6b65 Mon Sep 17 00:00:00 2001
> > > From: Li Hong <lihong.hi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > Date: Sat, 10 Apr 2010 21:57:11 +0800
> > > Subject: [PATCH 1/3] nilfs2: remove nilfs_segctor_init() in segment.c
> > > 
> > > There are only two lines of code in nilfs_segctor_init(). From a logic design
> > > view, the first line 'sci->sc_seq_done = sci->sc_seq_request;' should be put in
> > > nilfs_segctor_new(). Even in nilfs_segctor_new(), this initialization is
> > > needless because sci is kzalloc-ed. So nilfs_segctor_init() is only a wrap call
> > > to nilfs_segctor_start_thread(). This removes an indirect call overhead.
> > > 
> > > Signed-off-by: Li Hong <lihong.hi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > 
> > Looks no problem.
> > 
> > The reason why nilfs_segctor_init is present in that manner is
> > historical (just for your information. You don't have to mention this
> > reason).
> > 
> > I think you don't have to mention the indirect call overhead because
> > it's only triggered in the level of mount/unmount/remount and gcc will
> > inline it in the caller.
> ^^^^^^^^^^^^^^^^^^^^^^^^^
> No, there is no inline key word in nilfs_segctor_init().

> Unless you mean gcc will try to inline small procedures if possible.

I mean this ;)

By "objdump -D segment.o", you can actually see the function is
inlined in the caller.  It's a default thing nowadays unless
"noinline" keyword is specified.

Anyway, the only point I feel odd is the description.  Basically, I'd
like to apply this patch for cleanup.  May I remove the comment "this
removes an indrect call oveahead"?

Thanks,
Ryusuke Konishi

> Thanks,
> Li Hong
> 
> > Thanks,
> > Ryusuke Konishi
> > 
> > > ---
> > >  fs/nilfs2/segment.c |    9 +--------
> > >  1 files changed, 1 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/fs/nilfs2/segment.c b/fs/nilfs2/segment.c
> > > index f235fc0..514620d 100644
> > > --- a/fs/nilfs2/segment.c
> > > +++ b/fs/nilfs2/segment.c
> > > @@ -2684,13 +2684,6 @@ static void nilfs_segctor_kill_thread(struct nilfs_sc_info *sci)
> > >  	}
> > >  }
> > >  
> > > -static int nilfs_segctor_init(struct nilfs_sc_info *sci)
> > > -{
> > > -	sci->sc_seq_done = sci->sc_seq_request;
> > > -
> > > -	return nilfs_segctor_start_thread(sci);
> > > -}
> > > -
> > >  /*
> > >   * Setup & clean-up functions
> > >   */
> > > @@ -2814,7 +2807,7 @@ int nilfs_attach_segment_constructor(struct nilfs_sb_info *sbi)
> > >  		return -ENOMEM;
> > >  
> > >  	nilfs_attach_writer(nilfs, sbi);
> > > -	err = nilfs_segctor_init(NILFS_SC(sbi));
> > > +	err = nilfs_segctor_start_thread(NILFS_SC(sbi));
> > >  	if (err) {
> > >  		nilfs_detach_writer(nilfs, sbi);
> > >  		kfree(sbi->s_sc_info);
> > > -- 
> > > 1.6.3.3
> > > 
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
> > > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] nilfs2: nilfs_segctor_destroy() should do a full job

[Ryusuke Konishi]

On Sun, 11 Apr 2010 12:27:49 +0800, Li Hong <lihong.hi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Sun, Apr 11, 2010 at 03:10:05AM +0900, Ryusuke Konishi wrote:
> > On Sun, 11 Apr 2010 00:17:00 +0800, Li Hong <lihong.hi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > > From 466fbbc10dabc8a14e292ee19636fc534036a8b7 Mon Sep 17 00:00:00 2001
> > > From: Li Hong <lihong.hi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > Date: Sat, 10 Apr 2010 22:26:04 +0800
> > > Subject: [PATCH 2/3] nilfs2: nilfs_segctor_destroy() should do a full job
> > > 
> > > To set "sbi->s_sc_info = NULL;" is also a part of nilfs_segctor_destroy()'s job.
> > > Move it into the destroy procedure.
> > > 
> > > Signed-off-by: Li Hong <lihong.hi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > 
> > No, don't do this.  sbi->s_sc_info is the holder of the segment
> > constructor object and is not a part of the object though
>                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> No, my meaning is that: to set "sbi->s_sc_info = NULL;" is also a _part_ of 
> nilfs_segctor_destroy()'s _job_. 

It's your opinion, and my opinion is different.  I'd prefer to
intentionally keep the clearance in
nilfs_detach_segment_constructor().  It's not a matter of your
changelog description.

Thanks,
Ryusuke Konishi

> I should make the explanation clearer. IMO, I just think 'kfree
> something' and 'set the obj to NULL' can't be seperated by a
> procedure.

> Thanks,
> Li Hong
> 
> > nilfs_segctor_destroy needs some cleanup.
> > 
> > Ryusuke Konishi
> > 
> > > ---
> > >  fs/nilfs2/segment.c |    5 ++---
> > >  1 files changed, 2 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/fs/nilfs2/segment.c b/fs/nilfs2/segment.c
> > > index 514620d..dd3c4d5 100644
> > > --- a/fs/nilfs2/segment.c
> > > +++ b/fs/nilfs2/segment.c
> > > @@ -2774,6 +2774,7 @@ static void nilfs_segctor_destroy(struct nilfs_sc_info *sci)
> > >  	down_write(&sbi->s_nilfs->ns_segctor_sem);
> > >  
> > >  	kfree(sci);
> > > +	sbi->s_sc_info = NULL;
> > >  }
> > >  
> > >  /**
> > > @@ -2829,10 +2830,8 @@ void nilfs_detach_segment_constructor(struct nilfs_sb_info *sbi)
> > >  	LIST_HEAD(garbage_list);
> > >  
> > >  	down_write(&nilfs->ns_segctor_sem);
> > > -	if (NILFS_SC(sbi)) {
> > > +	if (NILFS_SC(sbi))
> > >  		nilfs_segctor_destroy(NILFS_SC(sbi));
> > > -		sbi->s_sc_info = NULL;
> > > -	}
> > >  
> > >  	/* Force to free the list of dirty files */
> > >  	spin_lock(&sbi->s_inode_lock);
> > > -- 
> > > 1.6.3.3
> > > 
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
> > > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] nilfs2: remove nilfs_segctor_init() in segment.c

[Li Hong]

Yes, go ahead :)

Thanks,
Li Hong

On Sun, Apr 11, 2010 at 04:58:47PM +0900, Ryusuke Konishi wrote:
> On Sun, 11 Apr 2010 12:17:26 +0800, Li Hong <lihong.hi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > On Sun, Apr 11, 2010 at 02:57:15AM +0900, Ryusuke Konishi wrote:
> > > Hi,
> > > On Sun, 11 Apr 2010 00:15:43 +0800, Li Hong <lihong.hi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > > > Hi KONISHI Ryusuke,
> > > > 
> > > > Three new patches based on nilfs2/for-next branch. New code has been built and
> > > > loaded successfully, and also passed a light-weight reads and writes test.
> > > > 
> > > > Thanks,
> > > > Li Hong
> > > 
> > > Ok, I'll look into each of them.
> > > 
> > > > ---------------------------- cut here --------------------------
> > > > 
> > > > From 2c622d0f59782321204bf1fde7eea4a593cc6b65 Mon Sep 17 00:00:00 2001
> > > > From: Li Hong <lihong.hi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > > Date: Sat, 10 Apr 2010 21:57:11 +0800
> > > > Subject: [PATCH 1/3] nilfs2: remove nilfs_segctor_init() in segment.c
> > > > 
> > > > There are only two lines of code in nilfs_segctor_init(). From a logic design
> > > > view, the first line 'sci->sc_seq_done = sci->sc_seq_request;' should be put in
> > > > nilfs_segctor_new(). Even in nilfs_segctor_new(), this initialization is
> > > > needless because sci is kzalloc-ed. So nilfs_segctor_init() is only a wrap call
> > > > to nilfs_segctor_start_thread(). This removes an indirect call overhead.
> > > > 
> > > > Signed-off-by: Li Hong <lihong.hi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > 
> > > Looks no problem.
> > > 
> > > The reason why nilfs_segctor_init is present in that manner is
> > > historical (just for your information. You don't have to mention this
> > > reason).
> > > 
> > > I think you don't have to mention the indirect call overhead because
> > > it's only triggered in the level of mount/unmount/remount and gcc will
> > > inline it in the caller.
> > ^^^^^^^^^^^^^^^^^^^^^^^^^
> > No, there is no inline key word in nilfs_segctor_init().
> 
> > Unless you mean gcc will try to inline small procedures if possible.
> 
> I mean this ;)
> 
> By "objdump -D segment.o", you can actually see the function is
> inlined in the caller.  It's a default thing nowadays unless
> "noinline" keyword is specified.
> 
> Anyway, the only point I feel odd is the description.  Basically, I'd
> like to apply this patch for cleanup.  May I remove the comment "this
> removes an indrect call oveahead"?
> 
> Thanks,
> Ryusuke Konishi
> 
> > Thanks,
> > Li Hong
> > 
> > > Thanks,
> > > Ryusuke Konishi
> > > 
> > > > ---
> > > >  fs/nilfs2/segment.c |    9 +--------
> > > >  1 files changed, 1 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/fs/nilfs2/segment.c b/fs/nilfs2/segment.c
> > > > index f235fc0..514620d 100644
> > > > --- a/fs/nilfs2/segment.c
> > > > +++ b/fs/nilfs2/segment.c
> > > > @@ -2684,13 +2684,6 @@ static void nilfs_segctor_kill_thread(struct nilfs_sc_info *sci)
> > > >  	}
> > > >  }
> > > >  
> > > > -static int nilfs_segctor_init(struct nilfs_sc_info *sci)
> > > > -{
> > > > -	sci->sc_seq_done = sci->sc_seq_request;
> > > > -
> > > > -	return nilfs_segctor_start_thread(sci);
> > > > -}
> > > > -
> > > >  /*
> > > >   * Setup & clean-up functions
> > > >   */
> > > > @@ -2814,7 +2807,7 @@ int nilfs_attach_segment_constructor(struct nilfs_sb_info *sbi)
> > > >  		return -ENOMEM;
> > > >  
> > > >  	nilfs_attach_writer(nilfs, sbi);
> > > > -	err = nilfs_segctor_init(NILFS_SC(sbi));
> > > > +	err = nilfs_segctor_start_thread(NILFS_SC(sbi));
> > > >  	if (err) {
> > > >  		nilfs_detach_writer(nilfs, sbi);
> > > >  		kfree(sbi->s_sc_info);
> > > > -- 
> > > > 1.6.3.3
> > > > 
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
> > > > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] nilfs2: nilfs_segctor_destroy() should do a full job

[Li Hong]

On Sun, Apr 11, 2010 at 05:15:51PM +0900, Ryusuke Konishi wrote:
> On Sun, 11 Apr 2010 12:27:49 +0800, Li Hong <lihong.hi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > On Sun, Apr 11, 2010 at 03:10:05AM +0900, Ryusuke Konishi wrote:
> > > On Sun, 11 Apr 2010 00:17:00 +0800, Li Hong <lihong.hi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > > > From 466fbbc10dabc8a14e292ee19636fc534036a8b7 Mon Sep 17 00:00:00 2001
> > > > From: Li Hong <lihong.hi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > > Date: Sat, 10 Apr 2010 22:26:04 +0800
> > > > Subject: [PATCH 2/3] nilfs2: nilfs_segctor_destroy() should do a full job
> > > > 
> > > > To set "sbi->s_sc_info = NULL;" is also a part of nilfs_segctor_destroy()'s job.
> > > > Move it into the destroy procedure.
> > > > 
> > > > Signed-off-by: Li Hong <lihong.hi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > 
> > > No, don't do this.  sbi->s_sc_info is the holder of the segment
> > > constructor object and is not a part of the object though
> >                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > No, my meaning is that: to set "sbi->s_sc_info = NULL;" is also a _part_ of 
> > nilfs_segctor_destroy()'s _job_. 
> 
> It's your opinion, and my opinion is different.  I'd prefer to
> intentionally keep the clearance in
> nilfs_detach_segment_constructor().  It's not a matter of your
> changelog description.
> 
> Thanks,
> Ryusuke Konishi
 
 Or we may pull "kfree(sci);" out of nilfs_segctor_destroy(). My point here
 is: never seperate "kfree(ptr)" from "ptr = NULL". They should go line by
 line. Anyway, this is just a coding style and I respect your opinion:). I
 will drop this patch. 

 Thanks,
 Li Hong
> > I should make the explanation clearer. IMO, I just think 'kfree
> > something' and 'set the obj to NULL' can't be seperated by a
> > procedure.
> 
> > Thanks,
> > Li Hong
> > 
> > > nilfs_segctor_destroy needs some cleanup.
> > > 
> > > Ryusuke Konishi
> > > 
> > > > ---
> > > >  fs/nilfs2/segment.c |    5 ++---
> > > >  1 files changed, 2 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/fs/nilfs2/segment.c b/fs/nilfs2/segment.c
> > > > index 514620d..dd3c4d5 100644
> > > > --- a/fs/nilfs2/segment.c
> > > > +++ b/fs/nilfs2/segment.c
> > > > @@ -2774,6 +2774,7 @@ static void nilfs_segctor_destroy(struct nilfs_sc_info *sci)
> > > >  	down_write(&sbi->s_nilfs->ns_segctor_sem);
> > > >  
> > > >  	kfree(sci);
> > > > +	sbi->s_sc_info = NULL;
> > > >  }
> > > >  
> > > >  /**
> > > > @@ -2829,10 +2830,8 @@ void nilfs_detach_segment_constructor(struct nilfs_sb_info *sbi)
> > > >  	LIST_HEAD(garbage_list);
> > > >  
> > > >  	down_write(&nilfs->ns_segctor_sem);
> > > > -	if (NILFS_SC(sbi)) {
> > > > +	if (NILFS_SC(sbi))
> > > >  		nilfs_segctor_destroy(NILFS_SC(sbi));
> > > > -		sbi->s_sc_info = NULL;
> > > > -	}
> > > >  
> > > >  	/* Force to free the list of dirty files */
> > > >  	spin_lock(&sbi->s_inode_lock);
> > > > -- 
> > > > 1.6.3.3
> > > > 
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
> > > > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
> > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] nilfs2: remove nilfs_segctor_init() in segment.c

[Ryusuke Konishi]

On Sun, 11 Apr 2010 16:22:50 +0800, Li Hong <lihong.hi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Yes, go ahead :)
> 
> Thanks,
> Li Hong

Done.

Thanks,
Ryusuke Konishi
 
> On Sun, Apr 11, 2010 at 04:58:47PM +0900, Ryusuke Konishi wrote:
> > On Sun, 11 Apr 2010 12:17:26 +0800, Li Hong <lihong.hi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > > On Sun, Apr 11, 2010 at 02:57:15AM +0900, Ryusuke Konishi wrote:
> > > > Hi,
> > > > On Sun, 11 Apr 2010 00:15:43 +0800, Li Hong <lihong.hi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > > > > Hi KONISHI Ryusuke,
> > > > > 
> > > > > Three new patches based on nilfs2/for-next branch. New code has been built and
> > > > > loaded successfully, and also passed a light-weight reads and writes test.
> > > > > 
> > > > > Thanks,
> > > > > Li Hong
> > > > 
> > > > Ok, I'll look into each of them.
> > > > 
> > > > > ---------------------------- cut here --------------------------
> > > > > 
> > > > > From 2c622d0f59782321204bf1fde7eea4a593cc6b65 Mon Sep 17 00:00:00 2001
> > > > > From: Li Hong <lihong.hi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > > > Date: Sat, 10 Apr 2010 21:57:11 +0800
> > > > > Subject: [PATCH 1/3] nilfs2: remove nilfs_segctor_init() in segment.c
> > > > > 
> > > > > There are only two lines of code in nilfs_segctor_init(). From a logic design
> > > > > view, the first line 'sci->sc_seq_done = sci->sc_seq_request;' should be put in
> > > > > nilfs_segctor_new(). Even in nilfs_segctor_new(), this initialization is
> > > > > needless because sci is kzalloc-ed. So nilfs_segctor_init() is only a wrap call
> > > > > to nilfs_segctor_start_thread(). This removes an indirect call overhead.
> > > > > 
> > > > > Signed-off-by: Li Hong <lihong.hi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > > > 
> > > > Looks no problem.
> > > > 
> > > > The reason why nilfs_segctor_init is present in that manner is
> > > > historical (just for your information. You don't have to mention this
> > > > reason).
> > > > 
> > > > I think you don't have to mention the indirect call overhead because
> > > > it's only triggered in the level of mount/unmount/remount and gcc will
> > > > inline it in the caller.
> > > ^^^^^^^^^^^^^^^^^^^^^^^^^
> > > No, there is no inline key word in nilfs_segctor_init().
> > 
> > > Unless you mean gcc will try to inline small procedures if possible.
> > 
> > I mean this ;)
> > 
> > By "objdump -D segment.o", you can actually see the function is
> > inlined in the caller.  It's a default thing nowadays unless
> > "noinline" keyword is specified.
> > 
> > Anyway, the only point I feel odd is the description.  Basically, I'd
> > like to apply this patch for cleanup.  May I remove the comment "this
> > removes an indrect call oveahead"?
> > 
> > Thanks,
> > Ryusuke Konishi
> > 
> > > Thanks,
> > > Li Hong
> > > 
> > > > Thanks,
> > > > Ryusuke Konishi
> > > > 
> > > > > ---
> > > > >  fs/nilfs2/segment.c |    9 +--------
> > > > >  1 files changed, 1 insertions(+), 8 deletions(-)
> > > > > 
> > > > > diff --git a/fs/nilfs2/segment.c b/fs/nilfs2/segment.c
> > > > > index f235fc0..514620d 100644
> > > > > --- a/fs/nilfs2/segment.c
> > > > > +++ b/fs/nilfs2/segment.c
> > > > > @@ -2684,13 +2684,6 @@ static void nilfs_segctor_kill_thread(struct nilfs_sc_info *sci)
> > > > >  	}
> > > > >  }
> > > > >  
> > > > > -static int nilfs_segctor_init(struct nilfs_sc_info *sci)
> > > > > -{
> > > > > -	sci->sc_seq_done = sci->sc_seq_request;
> > > > > -
> > > > > -	return nilfs_segctor_start_thread(sci);
> > > > > -}
> > > > > -
> > > > >  /*
> > > > >   * Setup & clean-up functions
> > > > >   */
> > > > > @@ -2814,7 +2807,7 @@ int nilfs_attach_segment_constructor(struct nilfs_sb_info *sbi)
> > > > >  		return -ENOMEM;
> > > > >  
> > > > >  	nilfs_attach_writer(nilfs, sbi);
> > > > > -	err = nilfs_segctor_init(NILFS_SC(sbi));
> > > > > +	err = nilfs_segctor_start_thread(NILFS_SC(sbi));
> > > > >  	if (err) {
> > > > >  		nilfs_detach_writer(nilfs, sbi);
> > > > >  		kfree(sbi->s_sc_info);
> > > > > -- 
> > > > > 1.6.3.3
> > > > > 
> > > > > --
> > > > > To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
> > > > > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] nilfs2: change sc_timer from a pointer to an embedded one in struct nilfs_sc_info

[Li Hong]

From 031b50dd4bc3c94687c8e587672aff2ffeefee9b Mon Sep 17 00:00:00 2001
From: Li Hong <lihong.hi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Date: Sat, 10 Apr 2010 23:25:39 +0800
Subject: [PATCH] nilfs2: change sc_timer from a pointer to an embedded one in struct nilfs_sc_info

In nilfs_segctor_thread(), timer is a local variable allocated on stack. Its
address can't be set to sci->sc_timer and passed in several procedures.

It works now by chance, just because other procedures are called by
nilfs_segctor_thread() directly or indirectly and the stack hasn't been
deallocated yet.

Signed-off-by: Li Hong <lihong.hi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 fs/nilfs2/segment.c |   31 +++++++++++++------------------
 fs/nilfs2/segment.h |    2 +-
 2 files changed, 14 insertions(+), 19 deletions(-)

diff --git a/fs/nilfs2/segment.c b/fs/nilfs2/segment.c
index 514620d..b523112 100644
--- a/fs/nilfs2/segment.c
+++ b/fs/nilfs2/segment.c
@@ -2158,9 +2158,9 @@ static int nilfs_segctor_do_construct(struct nilfs_sc_info *sci, int mode)
 static void nilfs_segctor_start_timer(struct nilfs_sc_info *sci)
 {
 	spin_lock(&sci->sc_state_lock);
-	if (sci->sc_timer && !(sci->sc_state & NILFS_SEGCTOR_COMMIT)) {
-		sci->sc_timer->expires = jiffies + sci->sc_interval;
-		add_timer(sci->sc_timer);
+	if (!(sci->sc_state & NILFS_SEGCTOR_COMMIT)) {
+		sci->sc_timer.expires = jiffies + sci->sc_interval;
+		add_timer(&sci->sc_timer);
 		sci->sc_state |= NILFS_SEGCTOR_COMMIT;
 	}
 	spin_unlock(&sci->sc_state_lock);
@@ -2365,9 +2365,7 @@ static void nilfs_segctor_accept(struct nilfs_sc_info *sci)
 	spin_lock(&sci->sc_state_lock);
 	sci->sc_seq_accepted = sci->sc_seq_request;
 	spin_unlock(&sci->sc_state_lock);
-
-	if (sci->sc_timer)
-		del_timer_sync(sci->sc_timer);
+	del_timer_sync(&sci->sc_timer);
 }
 
 /**
@@ -2393,9 +2391,9 @@ static void nilfs_segctor_notify(struct nilfs_sc_info *sci, int mode, int err)
 			sci->sc_flush_request &= ~FLUSH_DAT_BIT;
 
 		/* re-enable timer if checkpoint creation was not done */
-		if (sci->sc_timer && (sci->sc_state & NILFS_SEGCTOR_COMMIT) &&
-		    time_before(jiffies, sci->sc_timer->expires))
-			add_timer(sci->sc_timer);
+		if ((sci->sc_state & NILFS_SEGCTOR_COMMIT) &&
+		    time_before(jiffies, sci->sc_timer.expires))
+			add_timer(&sci->sc_timer);
 	}
 	spin_unlock(&sci->sc_state_lock);
 }
@@ -2574,13 +2572,10 @@ static int nilfs_segctor_thread(void *arg)
 {
 	struct nilfs_sc_info *sci = (struct nilfs_sc_info *)arg;
 	struct the_nilfs *nilfs = sci->sc_sbi->s_nilfs;
-	struct timer_list timer;
 	int timeout = 0;
 
-	init_timer(&timer);
-	timer.data = (unsigned long)current;
-	timer.function = nilfs_construction_timeout;
-	sci->sc_timer = &timer;
+	sci->sc_timer.data = (unsigned long)current;
+	sci->sc_timer.function = nilfs_construction_timeout;
 
 	/* start sync. */
 	sci->sc_task = current;
@@ -2629,7 +2624,7 @@ static int nilfs_segctor_thread(void *arg)
 			should_sleep = 0;
 		else if (sci->sc_state & NILFS_SEGCTOR_COMMIT)
 			should_sleep = time_before(jiffies,
-						   sci->sc_timer->expires);
+					sci->sc_timer.expires);
 
 		if (should_sleep) {
 			spin_unlock(&sci->sc_state_lock);
@@ -2638,7 +2633,7 @@ static int nilfs_segctor_thread(void *arg)
 		}
 		finish_wait(&sci->sc_wait_daemon, &wait);
 		timeout = ((sci->sc_state & NILFS_SEGCTOR_COMMIT) &&
-			   time_after_eq(jiffies, sci->sc_timer->expires));
+			   time_after_eq(jiffies, sci->sc_timer.expires));
 
 		if (nilfs_sb_dirty(nilfs) && nilfs_sb_need_update(nilfs))
 			set_nilfs_discontinued(nilfs);
@@ -2647,8 +2642,6 @@ static int nilfs_segctor_thread(void *arg)
 
  end_thread:
 	spin_unlock(&sci->sc_state_lock);
-	del_timer_sync(sci->sc_timer);
-	sci->sc_timer = NULL;
 
 	/* end sync. */
 	sci->sc_task = NULL;
@@ -2707,6 +2700,7 @@ static struct nilfs_sc_info *nilfs_segctor_new(struct nilfs_sb_info *sbi)
 	INIT_LIST_HEAD(&sci->sc_write_logs);
 	INIT_LIST_HEAD(&sci->sc_gc_inodes);
 	INIT_LIST_HEAD(&sci->sc_copied_buffers);
+	init_timer(&sci->sc_timer);
 
 	sci->sc_interval = HZ * NILFS_SC_DEFAULT_TIMEOUT;
 	sci->sc_mjcp_freq = HZ * NILFS_SC_DEFAULT_SR_FREQ;
@@ -2773,6 +2767,7 @@ static void nilfs_segctor_destroy(struct nilfs_sc_info *sci)
 
 	down_write(&sbi->s_nilfs->ns_segctor_sem);
 
+	del_timer_sync(&sci->sc_timer);
 	kfree(sci);
 }
 
diff --git a/fs/nilfs2/segment.h b/fs/nilfs2/segment.h
index 7aca765..dca1423 100644
--- a/fs/nilfs2/segment.h
+++ b/fs/nilfs2/segment.h
@@ -177,7 +177,7 @@ struct nilfs_sc_info {
 	unsigned long		sc_lseg_stime;	/* in 1/HZ seconds */
 	unsigned long		sc_watermark;
 
-	struct timer_list      *sc_timer;
+	struct timer_list	sc_timer;
 	struct task_struct     *sc_task;
 };
 
-- 
1.6.3.3

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

Re: [PATCH 3/3] nilfs2: change sc_timer from a pointer to an embedded one in struct nilfs_sc_info

[Ryusuke Konishi]

On Sun, 11 Apr 2010 17:08:15 +0800, Li Hong <lihong.hi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> From 031b50dd4bc3c94687c8e587672aff2ffeefee9b Mon Sep 17 00:00:00 2001
> From: Li Hong <lihong.hi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Date: Sat, 10 Apr 2010 23:25:39 +0800
> Subject: [PATCH] nilfs2: change sc_timer from a pointer to an embedded one in struct nilfs_sc_info
> 
> In nilfs_segctor_thread(), timer is a local variable allocated on stack. Its
> address can't be set to sci->sc_timer and passed in several procedures.
> 
> It works now by chance, just because other procedures are called by
> nilfs_segctor_thread() directly or indirectly and the stack hasn't been
> deallocated yet.
> 
> Signed-off-by: Li Hong <lihong.hi-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Applied, thanks you.

Ryusuke Konishi

> ---
>  fs/nilfs2/segment.c |   31 +++++++++++++------------------
>  fs/nilfs2/segment.h |    2 +-
>  2 files changed, 14 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/nilfs2/segment.c b/fs/nilfs2/segment.c
> index 514620d..b523112 100644
> --- a/fs/nilfs2/segment.c
> +++ b/fs/nilfs2/segment.c
> @@ -2158,9 +2158,9 @@ static int nilfs_segctor_do_construct(struct nilfs_sc_info *sci, int mode)
>  static void nilfs_segctor_start_timer(struct nilfs_sc_info *sci)
>  {
>  	spin_lock(&sci->sc_state_lock);
> -	if (sci->sc_timer && !(sci->sc_state & NILFS_SEGCTOR_COMMIT)) {
> -		sci->sc_timer->expires = jiffies + sci->sc_interval;
> -		add_timer(sci->sc_timer);
> +	if (!(sci->sc_state & NILFS_SEGCTOR_COMMIT)) {
> +		sci->sc_timer.expires = jiffies + sci->sc_interval;
> +		add_timer(&sci->sc_timer);
>  		sci->sc_state |= NILFS_SEGCTOR_COMMIT;
>  	}
>  	spin_unlock(&sci->sc_state_lock);
> @@ -2365,9 +2365,7 @@ static void nilfs_segctor_accept(struct nilfs_sc_info *sci)
>  	spin_lock(&sci->sc_state_lock);
>  	sci->sc_seq_accepted = sci->sc_seq_request;
>  	spin_unlock(&sci->sc_state_lock);
> -
> -	if (sci->sc_timer)
> -		del_timer_sync(sci->sc_timer);
> +	del_timer_sync(&sci->sc_timer);
>  }
>  
>  /**
> @@ -2393,9 +2391,9 @@ static void nilfs_segctor_notify(struct nilfs_sc_info *sci, int mode, int err)
>  			sci->sc_flush_request &= ~FLUSH_DAT_BIT;
>  
>  		/* re-enable timer if checkpoint creation was not done */
> -		if (sci->sc_timer && (sci->sc_state & NILFS_SEGCTOR_COMMIT) &&
> -		    time_before(jiffies, sci->sc_timer->expires))
> -			add_timer(sci->sc_timer);
> +		if ((sci->sc_state & NILFS_SEGCTOR_COMMIT) &&
> +		    time_before(jiffies, sci->sc_timer.expires))
> +			add_timer(&sci->sc_timer);
>  	}
>  	spin_unlock(&sci->sc_state_lock);
>  }
> @@ -2574,13 +2572,10 @@ static int nilfs_segctor_thread(void *arg)
>  {
>  	struct nilfs_sc_info *sci = (struct nilfs_sc_info *)arg;
>  	struct the_nilfs *nilfs = sci->sc_sbi->s_nilfs;
> -	struct timer_list timer;
>  	int timeout = 0;
>  
> -	init_timer(&timer);
> -	timer.data = (unsigned long)current;
> -	timer.function = nilfs_construction_timeout;
> -	sci->sc_timer = &timer;
> +	sci->sc_timer.data = (unsigned long)current;
> +	sci->sc_timer.function = nilfs_construction_timeout;
>  
>  	/* start sync. */
>  	sci->sc_task = current;
> @@ -2629,7 +2624,7 @@ static int nilfs_segctor_thread(void *arg)
>  			should_sleep = 0;
>  		else if (sci->sc_state & NILFS_SEGCTOR_COMMIT)
>  			should_sleep = time_before(jiffies,
> -						   sci->sc_timer->expires);
> +					sci->sc_timer.expires);
>  
>  		if (should_sleep) {
>  			spin_unlock(&sci->sc_state_lock);
> @@ -2638,7 +2633,7 @@ static int nilfs_segctor_thread(void *arg)
>  		}
>  		finish_wait(&sci->sc_wait_daemon, &wait);
>  		timeout = ((sci->sc_state & NILFS_SEGCTOR_COMMIT) &&
> -			   time_after_eq(jiffies, sci->sc_timer->expires));
> +			   time_after_eq(jiffies, sci->sc_timer.expires));
>  
>  		if (nilfs_sb_dirty(nilfs) && nilfs_sb_need_update(nilfs))
>  			set_nilfs_discontinued(nilfs);
> @@ -2647,8 +2642,6 @@ static int nilfs_segctor_thread(void *arg)
>  
>   end_thread:
>  	spin_unlock(&sci->sc_state_lock);
> -	del_timer_sync(sci->sc_timer);
> -	sci->sc_timer = NULL;
>  
>  	/* end sync. */
>  	sci->sc_task = NULL;
> @@ -2707,6 +2700,7 @@ static struct nilfs_sc_info *nilfs_segctor_new(struct nilfs_sb_info *sbi)
>  	INIT_LIST_HEAD(&sci->sc_write_logs);
>  	INIT_LIST_HEAD(&sci->sc_gc_inodes);
>  	INIT_LIST_HEAD(&sci->sc_copied_buffers);
> +	init_timer(&sci->sc_timer);
>  
>  	sci->sc_interval = HZ * NILFS_SC_DEFAULT_TIMEOUT;
>  	sci->sc_mjcp_freq = HZ * NILFS_SC_DEFAULT_SR_FREQ;
> @@ -2773,6 +2767,7 @@ static void nilfs_segctor_destroy(struct nilfs_sc_info *sci)
>  
>  	down_write(&sbi->s_nilfs->ns_segctor_sem);
>  
> +	del_timer_sync(&sci->sc_timer);
>  	kfree(sci);
>  }
>  
> diff --git a/fs/nilfs2/segment.h b/fs/nilfs2/segment.h
> index 7aca765..dca1423 100644
> --- a/fs/nilfs2/segment.h
> +++ b/fs/nilfs2/segment.h
> @@ -177,7 +177,7 @@ struct nilfs_sc_info {
>  	unsigned long		sc_lseg_stime;	/* in 1/HZ seconds */
>  	unsigned long		sc_watermark;
>  
> -	struct timer_list      *sc_timer;
> +	struct timer_list	sc_timer;
>  	struct task_struct     *sc_task;
>  };
>  
> -- 
> 1.6.3.3
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html