[PATCH 0/4] writeback: minor cleanups

[PATCH 0/4] writeback: minor cleanups

[Artem Bityutskiy]

Jens,

I have few clean-ups for your write-back code. I'm not sure you hate
this kind of clean-ups or not, but I found the code to be a bit more
consistent when we do not mix 'task' and 'thread' words everywhere.

P.S. Christoph's RFC clean-up will conflict with this, but we all
     seem to stay unconvinced that it is worth it, so I think it is OK
     to do smaller clean-ups so far.

Artem.

[PATCH 1/4] writeback: weed out unneeded code

[Artem Bityutskiy]

From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>

The only user of 'bdi_add_to_pending()' is the default BDI forker
task ('bdi_forker_task()'). And 'bdi_add_to_pending()' wakes up
the forker BDI task, i.e., itself, which is unnecessary. The patch
weeds out the unneeded 'wake_up_process()' call.

Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
---
 mm/backing-dev.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index ac78a33..ff5669a 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -421,12 +421,6 @@ static void bdi_add_to_pending(struct rcu_head *head)
 	spin_lock(&bdi_lock);
 	list_add_tail(&bdi->bdi_list, &bdi_pending_list);
 	spin_unlock(&bdi_lock);
-
-	/*
-	 * We are now on the pending list, wake up bdi_forker_task()
-	 * to finish the job and add us back to the active bdi_list
-	 */
-	wake_up_process(default_backing_dev_info.wb.task);
 }
 
 /*
-- 
1.7.1.1

[PATCH 2/4] writeback: harmonize writeback threads and tasks - 1

[Artem Bityutskiy]

From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>

The write-back code mixes words "thread" and "task" for the
same things - this is not a problem, but this is still an
inconsistency which makes it a bit difficult to read the code.
It is better to use term "thread" consistently everywhere.

This patch renames:

'bdi_add_default_flusher_task()' -> 'bdi_add_default_flusher_thread()'
'bdi_forker_task()' -> 'bdi_forker_thread()'

This is just more consistent, because the per-bdi flushers are
'bdi_writeback_thread()'.

Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
---
 mm/backing-dev.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index ff5669a..431486e 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -50,7 +50,7 @@ static struct timer_list sync_supers_timer;
 static int bdi_sync_supers(void *);
 static void sync_supers_timer_fn(unsigned long);
 
-static void bdi_add_default_flusher_task(struct backing_dev_info *bdi);
+static void bdi_add_default_flusher_thread(struct backing_dev_info *bdi);
 
 #ifdef CONFIG_DEBUG_FS
 #include <linux/debugfs.h>
@@ -279,7 +279,7 @@ static void bdi_flush_io(struct backing_dev_info *bdi)
 }
 
 /*
- * kupdated() used to do this. We cannot do it from the bdi_forker_task()
+ * kupdated() used to do this. We cannot do it from the bdi_forker_thread()
  * or we risk deadlocking on ->s_umount. The longer term solution would be
  * to implement sync_supers_bdi() or similar and simply do it from the
  * bdi writeback tasks individually.
@@ -318,7 +318,7 @@ static void sync_supers_timer_fn(unsigned long unused)
 	bdi_arm_supers_timer();
 }
 
-static int bdi_forker_task(void *ptr)
+static int bdi_forker_thread(void *ptr)
 {
 	struct bdi_writeback *me = ptr;
 
@@ -354,7 +354,7 @@ static int bdi_forker_task(void *ptr)
 			    !bdi_has_dirty_io(bdi))
 				continue;
 
-			bdi_add_default_flusher_task(bdi);
+			bdi_add_default_flusher_thread(bdi);
 		}
 
 		set_current_state(TASK_INTERRUPTIBLE);
@@ -427,7 +427,7 @@ static void bdi_add_to_pending(struct rcu_head *head)
  * Add the default flusher task that gets created for any bdi
  * that has dirty data pending writeout
  */
-void static bdi_add_default_flusher_task(struct backing_dev_info *bdi)
+void static bdi_add_default_flusher_thread(struct backing_dev_info *bdi)
 {
 	if (!bdi_cap_writeback_dirty(bdi))
 		return;
@@ -441,8 +441,8 @@ void static bdi_add_default_flusher_task(struct backing_dev_info *bdi)
 	/*
 	 * Check with the helper whether to proceed adding a task. Will only
 	 * abort if we two or more simultanous calls to
-	 * bdi_add_default_flusher_task() occured, further additions will block
-	 * waiting for previous additions to finish.
+	 * bdi_add_default_flusher_thread() occured, further additions will
+	 * block waiting for previous additions to finish.
 	 */
 	if (!test_and_set_bit(BDI_pending, &bdi->state)) {
 		list_del_rcu(&bdi->bdi_list);
@@ -500,7 +500,7 @@ int bdi_register(struct backing_dev_info *bdi, struct device *parent,
 	if (bdi_cap_flush_forker(bdi)) {
 		struct bdi_writeback *wb = &bdi->wb;
 
-		wb->task = kthread_run(bdi_forker_task, wb, "bdi-%s",
+		wb->task = kthread_run(bdi_forker_thread, wb, "bdi-%s",
 						dev_name(dev));
 		if (IS_ERR(wb->task)) {
 			wb->task = NULL;
-- 
1.7.1.1

[PATCH 3/4] writeback: harmonize writeback threads and tasks - 2

[Artem Bityutskiy]

From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>

The write-back code mixes words "thread" and "task" for the
same things - this is not a problem, but this is still an
inconsistency which makes it a bit difficult to read the code.
It is better to use term "thread" consistently everywhere.

This patch renames the 'task' field in 'struct bdi_writeback'
into 'thread' for consistency reasons.

Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
---
 fs/fs-writeback.c           |   14 +++++++-------
 include/linux/backing-dev.h |    2 +-
 mm/backing-dev.c            |   20 ++++++++++----------
 3 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index bf10cbf..1df249a 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -84,14 +84,14 @@ static void bdi_queue_work(struct backing_dev_info *bdi,
 	 * If the default thread isn't there, make sure we add it. When
 	 * it gets created and wakes up, we'll run this work.
 	 */
-	if (unlikely(!bdi->wb.task)) {
+	if (unlikely(!bdi->wb.thread)) {
 		trace_writeback_nothread(bdi, work);
-		wake_up_process(default_backing_dev_info.wb.task);
+		wake_up_process(default_backing_dev_info.wb.thread);
 	} else {
 		struct bdi_writeback *wb = &bdi->wb;
 
-		if (wb->task)
-			wake_up_process(wb->task);
+		if (wb->thread)
+			wake_up_process(wb->thread);
 	}
 }
 
@@ -107,9 +107,9 @@ __bdi_start_writeback(struct backing_dev_info *bdi, long nr_pages,
 	 */
 	work = kzalloc(sizeof(*work), GFP_ATOMIC);
 	if (!work) {
-		if (bdi->wb.task) {
+		if (bdi->wb.thread) {
 			trace_writeback_nowork(bdi);
-			wake_up_process(bdi->wb.task);
+			wake_up_process(bdi->wb.thread);
 		}
 		return;
 	}
@@ -862,7 +862,7 @@ int bdi_writeback_thread(void *data)
 		try_to_freeze();
 	}
 
-	wb->task = NULL;
+	wb->thread = NULL;
 
 	/*
 	 * Flush any work that raced with us exiting. No new work
diff --git a/include/linux/backing-dev.h b/include/linux/backing-dev.h
index e536f3a..14648b9 100644
--- a/include/linux/backing-dev.h
+++ b/include/linux/backing-dev.h
@@ -50,7 +50,7 @@ struct bdi_writeback {
 
 	unsigned long last_old_flush;		/* last old data flush */
 
-	struct task_struct	*task;		/* writeback task */
+	struct task_struct	*thread;	/* writeback thread */
 	struct list_head	b_dirty;	/* dirty inodes */
 	struct list_head	b_io;		/* parked for writeback */
 	struct list_head	b_more_io;	/* parked for more writeback */
diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index 431486e..bd36dc4 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -348,7 +348,7 @@ static int bdi_forker_thread(void *ptr)
 		 * a thread registered. If so, set that up.
 		 */
 		list_for_each_entry_safe(bdi, tmp, &bdi_list, bdi_list) {
-			if (bdi->wb.task)
+			if (bdi->wb.thread)
 				continue;
 			if (list_empty(&bdi->work_list) &&
 			    !bdi_has_dirty_io(bdi))
@@ -384,7 +384,7 @@ static int bdi_forker_thread(void *ptr)
 		spin_unlock_bh(&bdi_lock);
 
 		wb = &bdi->wb;
-		wb->task = kthread_run(bdi_writeback_thread, wb, "flush-%s",
+		wb->thread = kthread_run(bdi_writeback_thread, wb, "flush-%s",
 					dev_name(bdi->dev));
 		/*
 		 * If task creation fails, then readd the bdi to
@@ -392,8 +392,8 @@ static int bdi_forker_thread(void *ptr)
 		 * from this forker thread. That will free some memory
 		 * and we can try again.
 		 */
-		if (IS_ERR(wb->task)) {
-			wb->task = NULL;
+		if (IS_ERR(wb->thread)) {
+			wb->thread = NULL;
 
 			/*
 			 * Add this 'bdi' to the back, so we get
@@ -500,10 +500,10 @@ int bdi_register(struct backing_dev_info *bdi, struct device *parent,
 	if (bdi_cap_flush_forker(bdi)) {
 		struct bdi_writeback *wb = &bdi->wb;
 
-		wb->task = kthread_run(bdi_forker_thread, wb, "bdi-%s",
+		wb->thread = kthread_run(bdi_forker_thread, wb, "bdi-%s",
 						dev_name(dev));
-		if (IS_ERR(wb->task)) {
-			wb->task = NULL;
+		if (IS_ERR(wb->thread)) {
+			wb->thread = NULL;
 			ret = -ENOMEM;
 
 			bdi_remove_from_list(bdi);
@@ -550,9 +550,9 @@ static void bdi_wb_shutdown(struct backing_dev_info *bdi)
 	 * unfreeze of the thread before calling kthread_stop(), otherwise
 	 * it would never exet if it is currently stuck in the refrigerator.
 	 */
-	if (bdi->wb.task) {
-		thaw_process(bdi->wb.task);
-		kthread_stop(bdi->wb.task);
+	if (bdi->wb.thread) {
+		thaw_process(bdi->wb.thread);
+		kthread_stop(bdi->wb.thread);
 	}
 }
 
-- 
1.7.1.1

[PATCH 4/4] writeback: harmonize writeback threads and tasks - 3

[Artem Bityutskiy]

From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>

The write-back code mixes words "thread" and "task" for the
same things - this is not a problem, but this is still an
inconsistency which makes it a bit difficult to read the code.
It is better to use term "thread" consistently everywhere.

This patch amends commentaries and makes them refer the forker thread and the
write-back threads as "threads", not "tasks".

Signed-off-by: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
---
 fs/fs-writeback.c |    2 +-
 mm/backing-dev.c  |   10 +++++-----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
index 1df249a..a933419 100644
--- a/fs/fs-writeback.c
+++ b/fs/fs-writeback.c
@@ -840,7 +840,7 @@ int bdi_writeback_thread(void *data)
 
 			/*
 			 * Longest period of inactivity that we tolerate. If we
-			 * see dirty data again later, the task will get
+			 * see dirty data again later, the thread will get
 			 * recreated automatically.
 			 */
 			max_idle = max(5UL * 60 * HZ, wait_jiffies);
diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index bd36dc4..15378db 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -282,7 +282,7 @@ static void bdi_flush_io(struct backing_dev_info *bdi)
  * kupdated() used to do this. We cannot do it from the bdi_forker_thread()
  * or we risk deadlocking on ->s_umount. The longer term solution would be
  * to implement sync_supers_bdi() or similar and simply do it from the
- * bdi writeback tasks individually.
+ * bdi writeback thread individually.
  */
 static int bdi_sync_supers(void *unused)
 {
@@ -376,7 +376,7 @@ static int bdi_forker_thread(void *ptr)
 
 		/*
 		 * This is our real job - check for pending entries in
-		 * bdi_pending_list, and create the tasks that got added
+		 * bdi_pending_list, and create the threads that got added
 		 */
 		bdi = list_entry(bdi_pending_list.next, struct backing_dev_info,
 				 bdi_list);
@@ -387,7 +387,7 @@ static int bdi_forker_thread(void *ptr)
 		wb->thread = kthread_run(bdi_writeback_thread, wb, "flush-%s",
 					dev_name(bdi->dev));
 		/*
-		 * If task creation fails, then readd the bdi to
+		 * If thread creation fails, then readd the bdi to
 		 * the pending list and force writeout of the bdi
 		 * from this forker thread. That will free some memory
 		 * and we can try again.
@@ -424,7 +424,7 @@ static void bdi_add_to_pending(struct rcu_head *head)
 }
 
 /*
- * Add the default flusher task that gets created for any bdi
+ * Add the default flusher thread that gets created for any bdi
  * that has dirty data pending writeout
  */
 void static bdi_add_default_flusher_thread(struct backing_dev_info *bdi)
@@ -439,7 +439,7 @@ void static bdi_add_default_flusher_thread(struct backing_dev_info *bdi)
 	}
 
 	/*
-	 * Check with the helper whether to proceed adding a task. Will only
+	 * Check with the helper whether to proceed adding a thread. Will only
 	 * abort if we two or more simultanous calls to
 	 * bdi_add_default_flusher_thread() occured, further additions will
 	 * block waiting for previous additions to finish.
-- 
1.7.1.1

[PATCH 0/3] writeback: more clean-ups and fixes

[Artem Bityutskiy]

And on top of those patches I've implemented the following 3 patches
which do further clean-up and fix one of the races I see. There is more
work, but I am sending this early to get an early feedback.

Note, I only gave few test to my patches - booted Fedora, made sure
bdi writeback threads are created/deleted/doing writeback, hotplugged
external USB drive, saw writeback thread created, unplugged and saw it
be removed. Did some work while running a kernel with these patches.

The patches are against your for-2.6.36 branch.

My long-term plan is to get rid of unneeded wake-ups, but I'm still just
reading your code and learning, and clean-up / fix things I spot.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

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

Re: [PATCH 4/4] writeback: harmonize writeback threads and tasks - 3

[Christoph Hellwig]

On Mon, Jul 12, 2010 at 10:49:54AM +0300, Artem Bityutskiy wrote:
> From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> 
> The write-back code mixes words "thread" and "task" for the
> same things - this is not a problem, but this is still an
> inconsistency which makes it a bit difficult to read the code.
> It is better to use term "thread" consistently everywhere.
> 
> This patch amends commentaries and makes them refer the forker thread and the
> write-back threads as "threads", not "tasks".

A convention I tend to use and I've seen in various places is to always
use _task for the storage of the task_struct pointer, and thread
everywhere else.  This especially helps with having foo_thread for the
actual thread and foo_task for a global variable keeping the
task_struct pointer.

Re: [PATCH 4/4] writeback: harmonize writeback threads and tasks - 3

[Artem Bityutskiy]

On Mon, 2010-07-12 at 11:33 -0400, Christoph Hellwig wrote:
> On Mon, Jul 12, 2010 at 10:49:54AM +0300, Artem Bityutskiy wrote:
> > From: Artem Bityutskiy <Artem.Bityutskiy@nokia.com>
> > 
> > The write-back code mixes words "thread" and "task" for the
> > same things - this is not a problem, but this is still an
> > inconsistency which makes it a bit difficult to read the code.
> > It is better to use term "thread" consistently everywhere.
> > 
> > This patch amends commentaries and makes them refer the forker thread and the
> > write-back threads as "threads", not "tasks".
> 
> A convention I tend to use and I've seen in various places is to always
> use _task for the storage of the task_struct pointer, and thread
> everywhere else.  This especially helps with having foo_thread for the
> actual thread and foo_task for a global variable keeping the
> task_struct pointer.

OK, thanks for feed-back, then I'll drop the patch which renames
bdi->wb.task to bdi->wb.thread. Namely, this one:

[PATCH 3/4] writeback: harmonize writeback threads and tasks - 2

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

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