nilfs2: remove nilfs_segctor_init() in segment.c

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 <[email protected]>

---
 fs/nilfs2/segment.c |   31 +++++++++++++------------------
 fs/nilfs2/segment.h |    2 +-
 2 files changed, 14 insertions(+), 19 deletions(-)

fs/nilfs2/segment.c +13 -18

index dd3c4d5..310979a 100644
@@ -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;

fs/nilfs2/segment.h +1 -1

index 7aca765..dca1423 100644
@@ -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;
 };
 

Signature · 6 lines

1.6.3.3

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi,

Quoted 8 lines · 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 <[email protected]>

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

Quoted 111 lines

---
 fs/nilfs2/segment.c |   31 +++++++++++++------------------
 fs/nilfs2/segment.h |    2 +-
 2 files changed, 14 insertions(+), 19 deletions(-)

fs/nilfs2/segment.c +13 -18

index dd3c4d5..310979a 100644
@@ -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;

fs/nilfs2/segment.h +1 -1

index 7aca765..dca1423 100644
@@ -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;
 };
 

Quoted 7 lines

Signature · 6 lines

1.6.3.3

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

From 031b50dd4bc3c94687c8e587672aff2ffeefee9b Mon Sep 17 00:00:00 2001
From: Li Hong <[email protected]>
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 <[email protected]>

---
 fs/nilfs2/segment.c |   31 +++++++++++++------------------
 fs/nilfs2/segment.h |    2 +-
 2 files changed, 14 insertions(+), 19 deletions(-)

fs/nilfs2/segment.c +13 -18

index 514620d..b523112 100644
@@ -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);
 }
 

fs/nilfs2/segment.h +1 -1

index 7aca765..dca1423 100644
@@ -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;
 };
 

Signature · 1 line

1.6.3.3

From 466fbbc10dabc8a14e292ee19636fc534036a8b7 Mon Sep 17 00:00:00 2001
From: Li Hong <[email protected]>
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 <[email protected]>

---
 fs/nilfs2/segment.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

fs/nilfs2/segment.c +2 -3

index 514620d..dd3c4d5 100644
@@ -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);

Signature · 6 lines

1.6.3.3

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 5 lines · Li Hong

From 466fbbc10dabc8a14e292ee19636fc534036a8b7 Mon Sep 17 00:00:00 2001
From: Li Hong <[email protected]>
Date: Sat, 10 Apr 2010 22:26:04 +0800
Subject: [PATCH 2/3] nilfs2: nilfs_segctor_destroy() should do a full job

Quoted 4 lines

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 <[email protected]>

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

Quoted 31 lines

---
 fs/nilfs2/segment.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

fs/nilfs2/segment.c +2 -3

index 514620d..dd3c4d5 100644
@@ -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);

Signature · 1 line

1.6.3.3

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 9 lines · Ryusuke Konishi

Quoted 8 lines · Li Hong

From 466fbbc10dabc8a14e292ee19636fc534036a8b7 Mon Sep 17 00:00:00 2001
From: Li Hong <[email protected]>
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.

Quoted 4 lines

Quoted 1 line

Signed-off-by Li Hong <[email protected]>

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

Quoted 35 lines

nilfs_segctor_destroy needs some cleanup.

Ryusuke Konishi

Quoted 31 lines

---
 fs/nilfs2/segment.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

fs/nilfs2/segment.c +2 -3

index 514620d..dd3c4d5 100644
@@ -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);

Signature · 1 line

1.6.3.3

Quoted 4 lines

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in the body of a message to [email protected] 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 10 lines · Li Hong

Quoted 9 lines · Ryusuke Konishi

Quoted 8 lines · Li Hong

From 466fbbc10dabc8a14e292ee19636fc534036a8b7 Mon Sep 17 00:00:00 2001
From: Li Hong <[email protected]>
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.

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

Signed-off-by Li Hong <[email protected]>

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

Quoted 42 lines

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

Quoted 35 lines

nilfs_segctor_destroy needs some cleanup.

Ryusuke Konishi

Quoted 31 lines

---
 fs/nilfs2/segment.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)

fs/nilfs2/segment.c +2 -3

index 514620d..dd3c4d5 100644
@@ -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);

Signature · 1 line

1.6.3.3

Quoted 8 lines

Quoted 4 lines

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in the body of a message to [email protected] 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

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 <[email protected]>
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 <[email protected]>

---
 fs/nilfs2/segment.c |    9 +--------
 1 files changed, 1 insertions(+), 8 deletions(-)

fs/nilfs2/segment.c +1 -8

index f235fc0..514620d 100644
@@ -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);

Signature · 6 lines

1.6.3.3

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi,

Quoted 7 lines · 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

Ok, I'll look into each of them.

Quoted 7 lines

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

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

Quoted 7 lines

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 <[email protected]>

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

Quoted 34 lines

---
 fs/nilfs2/segment.c |    9 +--------
 1 files changed, 1 insertions(+), 8 deletions(-)

fs/nilfs2/segment.c +1 -8

index f235fc0..514620d 100644
@@ -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);

Signature · 1 line

1.6.3.3

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 33 lines · Ryusuke Konishi

Hi,

Quoted 7 lines · 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

Ok, I'll look into each of them.

Quoted 7 lines

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

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

Quoted 7 lines

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 <[email protected]>

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).

Quoted 3 lines

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

Quoted 37 lines

Thanks,
Ryusuke Konishi

Quoted 34 lines

---
 fs/nilfs2/segment.c |    9 +--------
 1 files changed, 1 insertions(+), 8 deletions(-)

fs/nilfs2/segment.c +1 -8

index f235fc0..514620d 100644
@@ -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);

Signature · 1 line

1.6.3.3

Quoted 4 lines

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 34 lines · Li Hong

Quoted 33 lines · Ryusuke Konishi

Hi,

Quoted 7 lines · 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

Ok, I'll look into each of them.

Quoted 7 lines

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

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

Quoted 7 lines

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 <[email protected]>

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).

Quoted 7 lines

Quoted 3 lines

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

Quoted 40 lines

Thanks,
Li Hong

Quoted 37 lines

Thanks,
Ryusuke Konishi

Quoted 34 lines

---
 fs/nilfs2/segment.c |    9 +--------
 1 files changed, 1 insertions(+), 8 deletions(-)

fs/nilfs2/segment.c +1 -8

index f235fc0..514620d 100644
@@ -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);

Signature · 1 line

1.6.3.3

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Yes, go ahead :)

Thanks,
Li Hong

Quoted 96 lines · Li Hong

Quoted 34 lines · Ryusuke Konishi

Quoted 33 lines

Hi,

Quoted 7 lines · 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

Ok, I'll look into each of them.

Quoted 7 lines

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

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

Quoted 7 lines

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 <[email protected]>

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).

Quoted 7 lines

Quoted 3 lines

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

Quoted 40 lines

Thanks,
Li Hong

Quoted 37 lines

Thanks,
Ryusuke Konishi

Quoted 34 lines

---
 fs/nilfs2/segment.c |    9 +--------
 1 files changed, 1 insertions(+), 8 deletions(-)

fs/nilfs2/segment.c +1 -8

index f235fc0..514620d 100644
@@ -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);

Signature · 1 line

1.6.3.3

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html