please revert kthread from loop.c

please revert kthread from loop.c

[Hugh Dickins]

Please revert c7b2eff059fcc2d1b7085ee3d84b79fd657a537b
[PATCH] kthread: update loop.c to use kthread

It seems too little tested: "losetup -d /dev/loop0" fails with
EINVAL because nothing sets lo_thread; but even when you patch
loop_thread() to set lo->lo_thread = current, it can't survive
more than a few dozen iterations of the loop below (with a tmpfs
mounted on /tst): collapses with failed ioctl then BUG_ON(!bio).
I think the original lo_done completion was more subtle and safe
than the kthread conversion has allowed for.

j=0
cp /dev/zero /tst
while :
do
	let j=j+1
	echo "Doing pass $j"
	losetup /dev/loop0 /tst/zero
	mkfs -t ext2 -b 1024 /dev/loop0 >/dev/null 2>&1
	mount -t ext2 /dev/loop0 /mnt
	umount /mnt
	losetup -d /dev/loop0
done

Re: please revert kthread from loop.c

[Serge E. Hallyn]

Quoting Hugh Dickins (hugh@veritas.com):
> Please revert c7b2eff059fcc2d1b7085ee3d84b79fd657a537b
> [PATCH] kthread: update loop.c to use kthread
> 
> It seems too little tested: "losetup -d /dev/loop0" fails with
> EINVAL because nothing sets lo_thread; but even when you patch
> loop_thread() to set lo->lo_thread = current, it can't survive
> more than a few dozen iterations of the loop below (with a tmpfs
> mounted on /tst): collapses with failed ioctl then BUG_ON(!bio).
> I think the original lo_done completion was more subtle and safe
> than the kthread conversion has allowed for.
> 
> j=0
> cp /dev/zero /tst
> while :
> do
> 	let j=j+1
> 	echo "Doing pass $j"
> 	losetup /dev/loop0 /tst/zero
> 	mkfs -t ext2 -b 1024 /dev/loop0 >/dev/null 2>&1
> 	mount -t ext2 /dev/loop0 /mnt
> 	umount /mnt
> 	losetup -d /dev/loop0
> done

Very sorry.


Subject: [PATCH] kthread: convert loop.c to use kthread

Convert loop.c to use kthread in place of the deprecated
kernel_thread.

Update: Keep the lo_done completion to indicate when the
	loop_thread is ready.  Otherwise a user gets the
	go-ahead early and may start an ioctl before
	loop_thread is in fact ready.

	Also fix some other bugs including misnaming the thread,
	found by Andrew Morton, and not setting lo->thread as
	pointed out by Hugh Dickins.

	This version has passed parallel runs of the following
	script (on different devices of course), i.e.

	sh looptorture.sh 3 [ in screen 1 ]
	sh looptorture.sh 5 [ in screen 2 ]

	looptorture.sh contains the following script based
	on the one sent by Hugh Dickins earlier today:

	j=0
	if [ $# > 0 ]; then
	   num=$1
	else
	   num=0
	fi
	echo "starting loop ($num)"
	if [ ! -d /mnt/$num ]; then
		mkdir /mnt/$num
	fi
	dd if=/dev/zero of=/tst/zero$num bs=1M count=100
	while [ $j -lt 100 ]; do
		let j=j+1
		echo "Doing pass $j"
		losetup /dev/loop$num /tst/zero$num
		mkfs -t ext2 -b 1024 /dev/loop$num >/dev/null 2>&1
		mount -t ext2 /dev/loop$num /mnt/$num
		umount /mnt/$num
		losetup -d /dev/loop$num
	done

Signed-off-by: Serge Hallyn <serue@us.ibm.com>

---

 drivers/block/loop.c |   16 ++++++++++------
 include/linux/loop.h |    1 +
 2 files changed, 11 insertions(+), 6 deletions(-)

71c305041760195a3c96aedae52f7d8ce3982d72
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 3c74ea7..b3d1710 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -74,6 +74,7 @@
 #include <linux/completion.h>
 #include <linux/highmem.h>
 #include <linux/gfp.h>
+#include <linux/kthread.h>
 
 #include <asm/uaccess.h>
 
@@ -578,8 +579,7 @@ static int loop_thread(void *data)
 	struct loop_device *lo = data;
 	struct bio *bio;
 
-	daemonize("loop%d", lo->lo_number);
-
+	lo->lo_thread = current;
 	/*
 	 * loop can be used in an encrypted device,
 	 * hence, it mustn't be stopped at all
@@ -629,7 +629,6 @@ static int loop_thread(void *data)
 			break;
 	}
 
-	complete(&lo->lo_done);
 	return 0;
 }
 
@@ -747,6 +746,7 @@ static int loop_set_fd(struct loop_devic
 	int		lo_flags = 0;
 	int		error;
 	loff_t		size;
+	struct task_struct *tsk;
 
 	/* This is safe, since we have a reference from open(). */
 	__module_get(THIS_MODULE);
@@ -839,9 +839,11 @@ static int loop_set_fd(struct loop_devic
 
 	set_blocksize(bdev, lo_blocksize);
 
-	error = kernel_thread(loop_thread, lo, CLONE_KERNEL);
-	if (error < 0)
+	tsk = kthread_run(loop_thread, lo, "loop%d", lo->lo_number);
+	if (IS_ERR(tsk)) {
+		error = PTR_ERR(tsk);
 		goto out_putf;
+	}
 	wait_for_completion(&lo->lo_done);
 	return 0;
 
@@ -911,7 +913,7 @@ static int loop_clr_fd(struct loop_devic
 		complete(&lo->lo_bh_done);
 	spin_unlock_irq(&lo->lo_lock);
 
-	wait_for_completion(&lo->lo_done);
+	kthread_stop(lo->lo_thread);
 
 	lo->lo_backing_file = NULL;
 
@@ -921,6 +923,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;
@@ -1288,6 +1291,7 @@ static int __init loop_init(void)
 		if (!lo->lo_queue)
 			goto out_mem4;
 		mutex_init(&lo->lo_ctl_mutex);
+		lo->lo_thread = NULL;
 		init_completion(&lo->lo_done);
 		init_completion(&lo->lo_bh_done);
 		lo->lo_number = i;
diff --git a/include/linux/loop.h b/include/linux/loop.h
index e76c761..4bd4cde 100644
--- a/include/linux/loop.h
+++ b/include/linux/loop.h
@@ -59,6 +59,7 @@ struct loop_device {
 	struct bio 		*lo_bio;
 	struct bio		*lo_biotail;
 	int			lo_state;
+	struct task_struct	*lo_thread;
 	struct completion	lo_done;
 	struct completion	lo_bh_done;
 	struct mutex		lo_ctl_mutex;
-- 
1.1.6

Re: please revert kthread from loop.c

[Hugh Dickins]

On Tue, 27 Jun 2006, Serge E. Hallyn wrote:
> 
> Very sorry.

No problem, just something I happened to notice, that's all.

> Subject: [PATCH] kthread: convert loop.c to use kthread
> 
> Convert loop.c to use kthread in place of the deprecated
> kernel_thread.
> 
> Update: Keep the lo_done completion to indicate when the
> 	loop_thread is ready.  Otherwise a user gets the
> 	go-ahead early and may start an ioctl before
> 	loop_thread is in fact ready.
> 
> 	Also fix some other bugs including misnaming the thread,
> 	found by Andrew Morton, and not setting lo->thread as
> 	pointed out by Hugh Dickins.
> 
> 	This version has passed parallel runs of the following
> 	script (on different devices of course), i.e.

But not good for me.  Gets further e.g. 170 iterations,
but then hangs while kthread_stop waits for completion.

I haven't investigated further.  Is there really any reason
to be messing with what has worked well for so long here?

Hugh

Re: please revert kthread from loop.c

[Serge E. Hallyn]

Quoting Hugh Dickins (hugh@veritas.com):
> > 	This version has passed parallel runs of the following
> > 	script (on different devices of course), i.e.
> 
> But not good for me.  Gets further e.g. 170 iterations,
> but then hangs while kthread_stop waits for completion.

Confounded...

> I haven't investigated further.  Is there really any reason
> to be messing with what has worked well for so long here?

Only because loop.c can be compiled as a module, and kernel_thread
is slated to have it's EXPORT_SYMBOL removed.

-serge

Re: please revert kthread from loop.c

[Serge E. Hallyn]

> But not good for me.  Gets further e.g. 170 iterations,
> but then hangs while kthread_stop waits for completion.

After getting much more familiar with the code, here is a more invasive,
but pretty heavily tested patch.

> I haven't investigated further.  Is there really any reason
> to be messing with what has worked well for so long here?

If the EXPORT_SYMBOL(kernel_thread) is going to be removed, then
this code will have to be updated.  Otherwise, no, the code as is
doesn't use pids to reference tasks, so the original code would be
fine with me.

thanks,
-serge

From: Serge Hallyn <serue@us.ibm.com>
Subject: [PATCH] kthread: convert loop.c to kthread

Convert loop.c from the deprecated kernel_thread to kthread.
This patch also simplifies the code a bit.  It has passed
vigerous testing.  The following script, looptorture.sh,
which mounts and unmounts loopback device 1000 times,
completes in three terminals simultaneously on different
loop devices.  For example, started up screen, created
three screens, and typed each of the following lines into
a separate screen:
	sh loopback.sh 2
	sh loopback.sh 3
	sh loopback.sh 4

loopback.sh:
	j=0
	if [ $# > 0 ]; then
	   num=$1
	else
	   num="0"
	fi
	echo "starting loop ($num)"
	if [ ! -d /mnt/$num ]; then
		mkdir /mnt/$num
	fi
	dd if=/dev/zero of=/tst/zero$num bs=1M count=100
	while [ $j -lt 1000 ]; do
		let j=j+1
		echo "Doing pass $j"
		losetup /dev/loop$num /tst/zero$num
		mkfs -t ext2 -b 1024 /dev/loop$num >/dev/null 2>&1
		mount -t ext2 /dev/loop$num /mnt/$num
		echo hello > /mnt/$num/hw
		umount /mnt/$num
		losetup -d /dev/loop$num
	done

A partial kernel build on a loopback partition also worked fine.
I have not tested encrypted devices, or suspend to loopback device.

Changes since last attempt:
	Eliminated lo->lo_done and lo->lo_bh_done completions.
	The former is not needed because we can wake up the thread
	when we're ready.  The latter was used for waking the
	loop_thread up when there was something to do or it was
	time to quit, and we no longer need this since we simply
	wake the thread directly when there's data.

	The lo->pending count used to be set at one plus actual
	pending actions, and was reduced to 0 only when it was time
	to quit.  We now use kthread_should_stop() to find out whether
	we're done, though we do not actually stop until there are no
	pending actions.

Signed-off-by: Serge Hallyn <serue@us.ibm.com>

---

 drivers/block/loop.c |   71 ++++++++++++++++++--------------------------------
 include/linux/loop.h |    3 +-
 2 files changed, 27 insertions(+), 47 deletions(-)

b56590212f3050dbb6c630eeea3a44794c9c7439
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index ccaada1..bdad531 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>
 
@@ -525,12 +526,10 @@ static int loop_make_request(request_que
 	lo->lo_pending++;
 	loop_add_bio(lo, old_bio);
 	spin_unlock_irq(&lo->lo_lock);
-	complete(&lo->lo_bh_done);
+	wake_up_process(lo->lo_thread);
 	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 (kthread_should_stop()) {
+			spin_unlock_irq(&lo->lo_lock);
+			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;
+		__set_current_state(TASK_INTERRUPTIBLE);
+		schedule();
 	}
 
-	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,9 @@ 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);
 	spin_unlock_irq(&lo->lo_lock);
 
-	wait_for_completion(&lo->lo_done);
+	kthread_stop(lo->lo_thread);
 
 	lo->lo_backing_file = NULL;
 
@@ -928,6 +909,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 +1275,7 @@ 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);
+		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..51dee29 100644
--- a/include/linux/loop.h
+++ b/include/linux/loop.h
@@ -59,8 +59,7 @@ struct loop_device {
 	struct bio 		*lo_bio;
 	struct bio		*lo_biotail;
 	int			lo_state;
-	struct completion	lo_done;
-	struct completion	lo_bh_done;
+	struct task_struct	*lo_thread;
 	struct mutex		lo_ctl_mutex;
 	int			lo_pending;
 
-- 
1.1.6

Re: please revert kthread from loop.c

[Andrew Morton]

"Serge E. Hallyn" <serue@us.ibm.com> wrote:
>
> Convert loop.c from the deprecated kernel_thread to kthread.
>

I think you have a racelet here:

> +		}
>  		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;
> +		__set_current_state(TASK_INTERRUPTIBLE);
> +		schedule();
>  	}
>  
> -	complete(&lo->lo_done);
>  	return 0;
>  }


:		if (kthread_should_stop()) {
:			spin_unlock_irq(&lo->lo_lock);
:			break;
:		}
:		spin_unlock_irq(&lo->lo_lock);
:
:		__set_current_state(TASK_INTERRUPTIBLE);
:		schedule();
:

If the wake_up_process() is delivered before the __set_current_state(),
we'll miss the wakeup.

If so, this should plug it.  The same race is not possible against the
loop_set_fd() wakeup because the thread isn't running at that stage, yes?


diff -puN drivers/block/loop.c~kthread-convert-loopc-to-kthread-race-fix drivers/block/loop.c
--- a/drivers/block/loop.c~kthread-convert-loopc-to-kthread-race-fix
+++ a/drivers/block/loop.c
@@ -525,8 +525,8 @@ static int loop_make_request(request_que
 		goto out;
 	lo->lo_pending++;
 	loop_add_bio(lo, old_bio);
-	spin_unlock_irq(&lo->lo_lock);
 	wake_up_process(lo->lo_thread);
+	spin_unlock_irq(&lo->lo_lock);
 	return 0;
 
 out:
@@ -600,9 +600,8 @@ static int loop_thread(void *data)
 			spin_unlock_irq(&lo->lo_lock);
 			break;
 		}
-		spin_unlock_irq(&lo->lo_lock);
-
 		__set_current_state(TASK_INTERRUPTIBLE);
+		spin_unlock_irq(&lo->lo_lock);
 		schedule();
 	}
 
_

Re: please revert kthread from loop.c

[Serge E. Hallyn]

Quoting Andrew Morton (akpm@osdl.org):
> "Serge E. Hallyn" <serue@us.ibm.com> wrote:
> >
> > Convert loop.c from the deprecated kernel_thread to kthread.
> >
> 
> I think you have a racelet here:
> 
> > +		}
> >  		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;
> > +		__set_current_state(TASK_INTERRUPTIBLE);
> > +		schedule();
> >  	}
> >  
> > -	complete(&lo->lo_done);
> >  	return 0;
> >  }
> 
> 
> :		if (kthread_should_stop()) {
> :			spin_unlock_irq(&lo->lo_lock);
> :			break;
> :		}
> :		spin_unlock_irq(&lo->lo_lock);
> :
> :		__set_current_state(TASK_INTERRUPTIBLE);
> :		schedule();
> :
> 
> If the wake_up_process() is delivered before the __set_current_state(),
> we'll miss the wakeup.

Makes sense, and the patched kernel passes the parallel tests.

Thanks for the patch.

> If so, this should plug it.  The same race is not possible against the
> loop_set_fd() wakeup because the thread isn't running at that stage, yes?

Right, it's not yet running at loop_set_fd().  However what about
kthread_stop() called from loop_clr_fd()?  Unfortunately fixing
that seems hairy.  Need to think about it...

thanks,
-serge

Re: please revert kthread from loop.c

[Andrew Morton]

On Tue, 11 Jul 2006 22:26:47 -0500
"Serge E. Hallyn" <serue@us.ibm.com> wrote:

> > If so, this should plug it.  The same race is not possible against the
> > loop_set_fd() wakeup because the thread isn't running at that stage, yes?
> 
> Right, it's not yet running at loop_set_fd().  However what about
> kthread_stop() called from loop_clr_fd()?  Unfortunately fixing
> that seems hairy.  Need to think about it...

Yes, there does seem to be a little race there.

I think it would be sufficient to do


diff -puN drivers/block/loop.c~a drivers/block/loop.c
--- a/drivers/block/loop.c~a
+++ a/drivers/block/loop.c
@@ -602,7 +602,8 @@ static int loop_thread(void *data)
 		}
 		__set_current_state(TASK_INTERRUPTIBLE);
 		spin_unlock_irq(&lo->lo_lock);
-		schedule();
+		if (lo->state != Lo_rundown)
+			schedule();
 	}
 
 	return 0;
@@ -888,12 +889,11 @@ static int loop_clr_fd(struct loop_devic
 	if (filp == NULL)
 		return -EINVAL;
 
+	kthread_stop(lo->lo_thread);
 	spin_lock_irq(&lo->lo_lock);
 	lo->lo_state = Lo_rundown;
 	spin_unlock_irq(&lo->lo_lock);
 
-	kthread_stop(lo->lo_thread);
-
 	lo->lo_backing_file = NULL;
 
 	loop_release_xfer(lo);
_

where the tweak to loop_clr_fd() is just there to prevent loop_thread()
from going into a very brief busyloop.

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.

Re: please revert kthread from loop.c

[Serge E. Hallyn]

Quoting Andrew Morton (akpm@osdl.org):
> On Tue, 11 Jul 2006 22:26:47 -0500
> "Serge E. Hallyn" <serue@us.ibm.com> wrote:
> 
> > > If so, this should plug it.  The same race is not possible against the
> > > loop_set_fd() wakeup because the thread isn't running at that stage, yes?
> > 
> > Right, it's not yet running at loop_set_fd().  However what about
> > kthread_stop() called from loop_clr_fd()?  Unfortunately fixing
> > that seems hairy.  Need to think about it...
> 
> Yes, there does seem to be a little race there.
> 
> I think it would be sufficient to do
> 
> 
> diff -puN drivers/block/loop.c~a drivers/block/loop.c
> --- a/drivers/block/loop.c~a
> +++ a/drivers/block/loop.c
> @@ -602,7 +602,8 @@ static int loop_thread(void *data)
>  		}
>  		__set_current_state(TASK_INTERRUPTIBLE);
>  		spin_unlock_irq(&lo->lo_lock);
> -		schedule();
> +		if (lo->state != Lo_rundown)
> +			schedule();
>  	}
>  
>  	return 0;
> @@ -888,12 +889,11 @@ static int loop_clr_fd(struct loop_devic
>  	if (filp == NULL)
>  		return -EINVAL;
>  
> +	kthread_stop(lo->lo_thread);
>  	spin_lock_irq(&lo->lo_lock);
>  	lo->lo_state = Lo_rundown;
>  	spin_unlock_irq(&lo->lo_lock);
>  
> -	kthread_stop(lo->lo_thread);
> -
>  	lo->lo_backing_file = NULL;
>  
>  	loop_release_xfer(lo);
> _
> 
> where the tweak to loop_clr_fd() is just there to prevent loop_thread()
> from going into a very brief busyloop.

Why does this fix the problem?  Can't the wake_up_process() in
kthread_stop() still happen right before loop_thread's schedule()?

This also means that after loop_thread() has decided to stop,
make_request() has a chance to make a few more requests.  It
will see lo->lo_state as bound, assume all is well, but when it goes to
wake_up_thread(), the thread will have been put_task_struct()d.

If I'm not entirely wrong above, how about the following alternate fix?  
Unfortunately I guess it doesn't stop the brief busyloop...

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

Perhaps I should give completions another go.

thanks,
-serge

Subject: [PATCH 3/3] kthread: fix loop.c race at thread stop

The wake_up_process() from kthread_stop() could happen
between loop_thread's __set_current_state(TASK_INTERRUPTIBLE)
and schedule().  But we can't put kthread_stop() under the
spin_lock like we did the wake_up_process() in make_request().

So turn the thread stopping into a two-phase process.  Do
a wake_up_process() under spin_lock after setting the
lo_state to Lo_rundown, after which the loop_thread no
long sleeps.

Signed-off-by: Serge Hallyn <serue@us.ibm.com>

---

 drivers/block/loop.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

e972f09b6ca27a7ac3421ab49bde6dba33fca62c
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index f944536..df38e05 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -600,9 +600,13 @@ static int loop_thread(void *data)
 			spin_unlock_irq(&lo->lo_lock);
 			break;
 		}
-		__set_current_state(TASK_INTERRUPTIBLE);
+		if (lo->lo_state != Lo_rundown)
+			__set_current_state(TASK_INTERRUPTIBLE);
 		spin_unlock_irq(&lo->lo_lock);
-		schedule();
+		if (lo->lo_state != Lo_rundown)
+			schedule();
+		else
+			__set_current_state(TASK_UNINTERRUPTIBLE);
 	}
 
 	return 0;
@@ -896,6 +900,7 @@ static int loop_clr_fd(struct loop_devic
 
 	spin_lock_irq(&lo->lo_lock);
 	lo->lo_state = Lo_rundown;
+	wake_up_process(lo->lo_thread);
 	spin_unlock_irq(&lo->lo_lock);
 
 	kthread_stop(lo->lo_thread);
-- 
1.1.6

Re: please revert kthread from loop.c

[Serge E. Hallyn]

Quoting Andrew Morton (akpm@osdl.org):
> 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@us.ibm.com>
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@us.ibm.com>

---

 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

Re: please revert kthread from loop.c

[Andrew Morton]

On Wed, 12 Jul 2006 18:02:29 -0500
"Serge E. Hallyn" <serue@us.ibm.com> wrote:

> +		if (lo->lo_state == Lo_rundown) {
> +			spin_unlock_irq(&lo->lo_lock);
> +			while (!kthread_should_stop());

eww.

A schedule_timeout_uninterruptible(1) or even cpu_relax() would be less
sinful, but still unpleasant.

It's strange that the problem of kthread_stop(already_exitted_task) hasn't
occurred before.

Again: why is this so hard?  It shouldn't be.  Perhaps because loop is
using completions in bizarre ways where it should be using
wake_up_process(), wait_event(), etc.

Re: please revert kthread from loop.c

[Serge E. Hallyn]

Quoting Andrew Morton (akpm@osdl.org):

> Again: why is this so hard?  It shouldn't be.  Perhaps because loop is
> using completions in bizarre ways where it should be using
> wake_up_process(), wait_event(), etc.

Ah.

wait_event() actually seems like the way to go - I'll try to follow the
example in fs/ocfs2/journal.c.

Still I'd also like to patch kthread to correctly handle an already
exited thread.  Would that be acceptable, or is requiring the thread not
to exit prematurely considered desirable?

thanks,
-serge

Re: please revert kthread from loop.c

[Andrew Morton]

On Thu, 13 Jul 2006 08:36:02 -0500
"Serge E. Hallyn" <serue@us.ibm.com> wrote:

> Quoting Andrew Morton (akpm@osdl.org):
> 
> > Again: why is this so hard?  It shouldn't be.  Perhaps because loop is
> > using completions in bizarre ways where it should be using
> > wake_up_process(), wait_event(), etc.
> 
> Ah.
> 
> wait_event() actually seems like the way to go - I'll try to follow the
> example in fs/ocfs2/journal.c.

I suspect quite a lot of changes to loop.c would fall out.  For a start, in
a sufficiently-simplified implementation lo_pending would perhaps go away -
just test the NULLness of the top of the list of BIOs.

> Still I'd also like to patch kthread to correctly handle an already
> exited thread.  Would that be acceptable, or is requiring the thread not
> to exit prematurely considered desirable?

That would seem sensible, but I don't immediately see how to do it
non-racily without changing the API or by adding a `struct completion' to
the task_struct.  Because the task might be exitting-but-not-exitted, and
still using resources which the kthread_stop() caller wants to release.

Re: please revert kthread from loop.c

[Serge E. Hallyn]

Quoting Andrew Morton (akpm@osdl.org):
> On Thu, 13 Jul 2006 08:36:02 -0500
> "Serge E. Hallyn" <serue@us.ibm.com> wrote:
> 
> > Quoting Andrew Morton (akpm@osdl.org):
> > 
> > > Again: why is this so hard?  It shouldn't be.  Perhaps because loop is
> > > using completions in bizarre ways where it should be using
> > > wake_up_process(), wait_event(), etc.
> > 
> > Ah.
> > 
> > wait_event() actually seems like the way to go - I'll try to follow the
> > example in fs/ocfs2/journal.c.
> 
> I suspect quite a lot of changes to loop.c would fall out.  For a start, in
> a sufficiently-simplified implementation lo_pending would perhaps go away -
> just test the NULLness of the top of the list of BIOs.

True - here is an attempt at that:

Subject: [PATCH] kthread: convert loop.c to kthread
From: Serge E. Hallyn <hallyn@sergelap.(none)>
Date: 1153345222 -0500

Convert loop.c from the deprecated kernel_thread to kthread.
This patch simplifies the code quite a bit and passes similar
testing to the previous submission on both emulated x86 and
s390.

Changes since last submission:
	switched to using a rather simple loop based on
	wait_event_interruptible.

Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>

---

 drivers/block/loop.c |   69 +++++++++++++++++---------------------------------
 include/linux/loop.h |    5 +---
 2 files changed, 26 insertions(+), 48 deletions(-)

c5f5e9d7016d6fb2b43aa4b13aea302ac367a28c
diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 7b3b94d..0468785 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>
 
@@ -522,15 +523,12 @@ static int loop_make_request(request_que
 		goto out;
 	if (unlikely(rw == WRITE && (lo->lo_flags & LO_FLAGS_READ_ONLY)))
 		goto out;
-	lo->lo_pending++;
 	loop_add_bio(lo, old_bio);
+	wake_up(&lo->lo_event);
 	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;
@@ -570,14 +568,18 @@ static inline void loop_handle_bio(struc
  * to avoid blocking in our make_request_fn. it also does loop decrypting
  * on reads for block backed loop, as that is too heavy to do from
  * b_end_io context where irqs may be disabled.
+ *
+ * Loop explanation:  loop_clr_fd() sets lo_state to Lo_rundown before
+ * calling kthread_stop().  Therefore once kthread_should_stop() is
+ * true, make_request will not place any more requests.  Therefore
+ * once kthread_should_stop() is true and lo_bio is NULL, we are
+ * done with the loop.
  */
 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 +589,21 @@ 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);
+	while (!kthread_should_stop() || lo->lo_bio) {
 
-	for (;;) {
-		int pending;
+		wait_event_interruptible(lo->lo_event,
+				lo->lo_bio || kthread_should_stop());
 
-		if (wait_for_completion_interruptible(&lo->lo_bh_done))
+		if (!lo->lo_bio)
 			continue;
-
 		spin_lock_irq(&lo->lo_lock);
-
-		/*
-		 * could be completed because of tear-down, not pending work
-		 */
-		if (unlikely(!lo->lo_pending)) {
-			spin_unlock_irq(&lo->lo_lock);
-			break;
-		}
-
 		bio = loop_get_bio(lo);
-		lo->lo_pending--;
-		pending = lo->lo_pending;
 		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;
 	}
 
-	complete(&lo->lo_done);
 	return 0;
 }
 
@@ -837,10 +813,15 @@ 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_state = Lo_bound;
+	wake_up_process(lo->lo_thread);
 	return 0;
 
  out_putf:
@@ -904,12 +885,9 @@ 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);
 	spin_unlock_irq(&lo->lo_lock);
 
-	wait_for_completion(&lo->lo_done);
+	kthread_stop(lo->lo_thread);
 
 	lo->lo_backing_file = NULL;
 
@@ -922,6 +900,7 @@ static int loop_clr_fd(struct loop_devic
 	lo->lo_sizelimit = 0;
 	lo->lo_encrypt_key_size = 0;
 	lo->lo_flags = 0;
+	lo->lo_thread = NULL;
 	memset(lo->lo_encrypt_key, 0, LO_KEY_SIZE);
 	memset(lo->lo_crypt_name, 0, LO_NAME_SIZE);
 	memset(lo->lo_file_name, 0, LO_NAME_SIZE);
@@ -1284,9 +1263,9 @@ 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);
 		lo->lo_number = i;
+		lo->lo_thread = NULL;
+		init_waitqueue_head(&lo->lo_event);
 		spin_lock_init(&lo->lo_lock);
 		disk->major = LOOP_MAJOR;
 		disk->first_minor = i;
diff --git a/include/linux/loop.h b/include/linux/loop.h
index e76c761..191a595 100644
--- a/include/linux/loop.h
+++ b/include/linux/loop.h
@@ -59,10 +59,9 @@ struct loop_device {
 	struct bio 		*lo_bio;
 	struct bio		*lo_biotail;
 	int			lo_state;
-	struct completion	lo_done;
-	struct completion	lo_bh_done;
 	struct mutex		lo_ctl_mutex;
-	int			lo_pending;
+	struct task_struct	*lo_thread;
+	wait_queue_head_t	lo_event;
 
 	request_queue_t		*lo_queue;
 };
-- 
1.1.6

Re: please revert kthread from loop.c

[Hugh Dickins]

On Tue, 11 Jul 2006, Serge E. Hallyn wrote:
> > But not good for me.  Gets further e.g. 170 iterations,
> > but then hangs while kthread_stop waits for completion.
> 
> After getting much more familiar with the code, here is a more invasive,
> but pretty heavily tested patch.

I didn't study your patch in detail: as you say, more invasive,
but if it really does the job then it's an improvement, removing
some mystery from loop_thread().  And it does work fine for me -
well, I tested with your next little race addition, and Andrew's
on top of that (with lo->state typo fixed to lo->lo_state); but
I haven't tried your latest refinement from today.

Thanks,
Hugh