writeback: minor cleanups

PATCH In review Artem Bityutskiy 2 participants 8 messages Jul 12, 2010 – Jul 13, 2010

Subsystems (3)

Memory management VFS MM misc

lore

Share & tools

lore

Artem Bityutskiy

author

2010-07-12 07:53 UTC #0 lore raw

cover writeback: minor cleanups

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.

Artem Bityutskiy

author

2010-07-12 07:54 UTC #1 lore raw

PATCH 1/4 writeback: weed out unneeded code

From: Artem Bityutskiy <[email protected]>

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

---
 mm/backing-dev.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

mm/backing-dev.c +0 -6

index ac78a33..ff5669a 100644
@@ -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);
 }
 
 /*

Signature · 1 line

1.7.1.1

Artem Bityutskiy

author

2010-07-12 07:54 UTC #2 lore raw

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

In reply to Artem Bityutskiy

From: Artem Bityutskiy <[email protected]>

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

---
 mm/backing-dev.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

mm/backing-dev.c +8 -8

index ff5669a..431486e 100644
@@ -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;

Signature · 1 line

1.7.1.1

Artem Bityutskiy

author

2010-07-12 07:54 UTC #3 lore raw

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

In reply to Artem Bityutskiy

From: Artem Bityutskiy <[email protected]>

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

Signed-off-by Artem Bityutskiy <[email protected]>

---
 fs/fs-writeback.c           |   14 +++++++-------
 include/linux/backing-dev.h |    2 +-
 mm/backing-dev.c            |   20 ++++++++++----------
 3 files changed, 18 insertions(+), 18 deletions(-)

fs/fs-writeback.c +7 -7

index bf10cbf..1df249a 100644
@@ -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

include/linux/backing-dev.h +1 -1

index e536f3a..14648b9 100644
@@ -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 */

mm/backing-dev.c +10 ~~-10~~

index 431486e..bd36dc4 100644
@@ -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);
 	}
 }

Signature · 1 line

1.7.1.1

Artem Bityutskiy

author

2010-07-12 07:53 UTC #4 lore raw

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

In reply to Artem Bityutskiy

From: Artem Bityutskiy <[email protected]>

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

---
 fs/fs-writeback.c |    2 +-
 mm/backing-dev.c  |   10 +++++-----
 2 files changed, 6 insertions(+), 6 deletions(-)

fs/fs-writeback.c +1 -1

index 1df249a..a933419 100644
@@ -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);

mm/backing-dev.c +5 -5

index bd36dc4..15378db 100644
@@ -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.

Signature · 1 line

1.7.1.1

Christoph Hellwig

2010-07-12 15:33 UTC #5 lore raw

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

Quoted 9 lines · Artem Bityutskiy

From: Artem Bityutskiy <[email protected]>
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.

Artem Bityutskiy

author

2010-07-13 14:35 UTC #6 lore raw

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

Quoted 8 lines · Christoph Hellwig

Quoted 7 lines · Artem Bityutskiy
From: Artem Bityutskiy <[email protected]>
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.

Quoted 8 lines

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

Signature · 7 lines

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

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

Artem Bityutskiy

author

2010-07-12 15:16 UTC #7 lore raw

cover writeback: more clean-ups and fixes

In reply to 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.

Signature · 7 lines

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

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