Re: please revert kthread from loop.c

[Serge E. Hallyn]

Quoting Andrew Morton (akpm@xxxxxxxx):
> I'm not sure why it's all so tricky in there, really.  Loop is doing a
> pretty conventional stop, wakeup, stick-things-on-lists operation and we do
> that all over the kernel using pretty well-understood idioms.  But for some
> reason, loop is all difficult about it.  I wonder why.  hm.

Does this version, going back to using a completion - lo->lo_wait - seem
a touch simpler?

thanks,
-serge

From: "Serge E. Hallyn" <serue@xxxxxxxxxx>
Subject: [PATCH] kthread: convert loop.c to kthread

Convert loop.c from the deprecated kernel_thread to kthread.

This version goes back to using a completion for waking the
loop_thread when there is work to do.  Ending the loop thread
is done by first setting the state, then waking the thread
using the completion, and finally calling kthread_stop().
The loop_thread is either already awake serving requests,
or is awoken by loop_clr_fd()s complete.  It does one more
loop to serve pending requests, after which it sees it's
state is no longer running, and starts a short busyloop
while waiting to be reaped.

Signed-off-by: Serge Hallyn <serue@xxxxxxxxxx>

---

 drivers/block/loop.c |   73 +++++++++++++++++++-------------------------------
 include/linux/loop.h |    4 +--
 2 files changed, 30 insertions(+), 47 deletions(-)

5aa946838754cc8bd1582ca3f926cf51d7e21df8
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index ccaada1..8779194 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -72,6 +72,7 @@
 #include <linux/completion.h>
 #include <linux/highmem.h>
 #include <linux/gfp.h>
+#include <linux/kthread.h>
 
 #include <asm/uaccess.h>
 
@@ -524,13 +525,11 @@ static int loop_make_request(request_que
 		goto out;
 	lo->lo_pending++;
 	loop_add_bio(lo, old_bio);
+	complete(&lo->lo_wait);
 	spin_unlock_irq(&lo->lo_lock);
-	complete(&lo->lo_bh_done);
 	return 0;
 
 out:
-	if (lo->lo_pending == 0)
-		complete(&lo->lo_bh_done);
 	spin_unlock_irq(&lo->lo_lock);
 	bio_io_error(old_bio, old_bio->bi_size);
 	return 0;
@@ -576,8 +575,6 @@ static int loop_thread(void *data)
 	struct loop_device *lo = data;
 	struct bio *bio;
 
-	daemonize("loop%d", lo->lo_number);
-
 	/*
 	 * loop can be used in an encrypted device,
 	 * hence, it mustn't be stopped at all
@@ -587,47 +584,28 @@ static int loop_thread(void *data)
 
 	set_user_nice(current, -20);
 
-	lo->lo_state = Lo_bound;
-	lo->lo_pending = 1;
-
-	/*
-	 * complete it, we are running
-	 */
-	complete(&lo->lo_done);
-
 	for (;;) {
-		int pending;
-
-		if (wait_for_completion_interruptible(&lo->lo_bh_done))
-			continue;
-
 		spin_lock_irq(&lo->lo_lock);
+		while (lo->lo_pending) {
+			bio = loop_get_bio(lo);
+			lo->lo_pending--;
 
-		/*
-		 * could be completed because of tear-down, not pending work
-		 */
-		if (unlikely(!lo->lo_pending)) {
 			spin_unlock_irq(&lo->lo_lock);
-			break;
+			BUG_ON(!bio);
+			loop_handle_bio(lo, bio);
+			spin_lock_irq(&lo->lo_lock);
 		}
 
-		bio = loop_get_bio(lo);
-		lo->lo_pending--;
-		pending = lo->lo_pending;
+		if (lo->lo_state == Lo_rundown) {
+			spin_unlock_irq(&lo->lo_lock);
+			while (!kthread_should_stop());
+			break;
+		}
 		spin_unlock_irq(&lo->lo_lock);
 
-		BUG_ON(!bio);
-		loop_handle_bio(lo, bio);
-
-		/*
-		 * upped both for pending work and tear-down, lo_pending
-		 * will hit zero then
-		 */
-		if (unlikely(!pending))
-			break;
+		wait_for_completion_interruptible(&lo->lo_wait);
 	}
 
-	complete(&lo->lo_done);
 	return 0;
 }
 
@@ -846,10 +824,16 @@ static int loop_set_fd(struct loop_devic
 
 	set_blocksize(bdev, lo_blocksize);
 
-	error = kernel_thread(loop_thread, lo, CLONE_KERNEL);
-	if (error < 0)
+	lo->lo_thread = kthread_create(loop_thread, lo, "loop%d",
+						lo->lo_number);
+	if (IS_ERR(lo->lo_thread)) {
+		error = PTR_ERR(lo->lo_thread);
+		lo->lo_thread = NULL;
 		goto out_putf;
-	wait_for_completion(&lo->lo_done);
+	}
+	lo->lo_pending = 0;
+	lo->lo_state = Lo_bound;
+	wake_up_process(lo->lo_thread);
 	return 0;
 
  out_putf:
@@ -913,12 +897,10 @@ static int loop_clr_fd(struct loop_devic
 
 	spin_lock_irq(&lo->lo_lock);
 	lo->lo_state = Lo_rundown;
-	lo->lo_pending--;
-	if (!lo->lo_pending)
-		complete(&lo->lo_bh_done);
+	complete(&lo->lo_wait);
 	spin_unlock_irq(&lo->lo_lock);
 
-	wait_for_completion(&lo->lo_done);
+	kthread_stop(lo->lo_thread);
 
 	lo->lo_backing_file = NULL;
 
@@ -928,6 +910,7 @@ static int loop_clr_fd(struct loop_devic
 	lo->lo_device = NULL;
 	lo->lo_encryption = NULL;
 	lo->lo_offset = 0;
+	lo->lo_thread = NULL;
 	lo->lo_sizelimit = 0;
 	lo->lo_encrypt_key_size = 0;
 	lo->lo_flags = 0;
@@ -1293,8 +1276,8 @@ static int __init loop_init(void)
 		if (!lo->lo_queue)
 			goto out_mem4;
 		mutex_init(&lo->lo_ctl_mutex);
-		init_completion(&lo->lo_done);
-		init_completion(&lo->lo_bh_done);
+		init_completion(&lo->lo_wait);
+		lo->lo_thread = NULL;
 		lo->lo_number = i;
 		spin_lock_init(&lo->lo_lock);
 		disk->major = LOOP_MAJOR;
diff --git a/include/linux/loop.h b/include/linux/loop.h
index e76c761..c9a0a45 100644
--- a/include/linux/loop.h
+++ b/include/linux/loop.h
@@ -59,8 +59,8 @@ struct loop_device {
 	struct bio 		*lo_bio;
 	struct bio		*lo_biotail;
 	int			lo_state;
-	struct completion	lo_done;
-	struct completion	lo_bh_done;
+	struct completion	lo_wait;
+	struct task_struct	*lo_thread;
 	struct mutex		lo_ctl_mutex;
 	int			lo_pending;
 
-- 
1.1.6
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/