linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* NBD Cleanup patch and bugfix in ll_rw_blk.c
@ 2001-02-25 19:57 Steve Whitehouse
  2001-02-25 20:55 ` Andrea Arcangeli
                   ` (2 more replies)
  0 siblings, 3 replies; 14+ messages in thread
From: Steve Whitehouse @ 2001-02-25 19:57 UTC (permalink / raw)
  To: torvalds; +Cc: pavel, linux-kernel

Hi,

Here is a new version of the patch I recently sent to the list with some 
NBD cleanups and a bug fix in ll_rw_blk.c. The changes to NBD have Pavel 
Machek's approval as I've left out the two changes as he suggested.

The bug fix in ll_rw_blk.c prevents hangs when using block devices which
don't have plugging functions,

Steve.

------------------------------------------------------------------------------

diff -u -r linux-2.4.2/drivers/block/ll_rw_blk.c linux/drivers/block/ll_rw_blk.c
--- linux-2.4.2/drivers/block/ll_rw_blk.c	Thu Feb 22 19:46:23 2001
+++ linux/drivers/block/ll_rw_blk.c	Sun Feb 25 19:35:43 2001
@@ -588,6 +588,9 @@
 	 * inserted at elevator_merge time
 	 */
 	list_add(&req->queue, insert_here);
+
+	if (!q->plugged && insert_here == &q->queue_head)
+		q->request_fn(q);
 }
 
 void inline blk_refill_freelist(request_queue_t *q, int rw)
diff -u -r linux-2.4.2/drivers/block/nbd.c linux/drivers/block/nbd.c
--- linux-2.4.2/drivers/block/nbd.c	Mon Oct 30 22:30:33 2000
+++ linux/drivers/block/nbd.c	Sun Feb 25 19:35:43 2001
@@ -29,7 +29,7 @@
 #include <linux/major.h>
 
 #include <linux/module.h>
-
+#include <linux/init.h>
 #include <linux/sched.h>
 #include <linux/fs.h>
 #include <linux/stat.h>
@@ -149,12 +149,13 @@
 {
 	int result;
 	struct nbd_request request;
+	unsigned long size = req->current_nr_sectors << 9;
 
 	DEBUG("NBD: sending control, ");
 	request.magic = htonl(NBD_REQUEST_MAGIC);
 	request.type = htonl(req->cmd);
 	request.from = cpu_to_be64( (u64) req->sector << 9);
-	request.len = htonl(req->current_nr_sectors << 9);
+	request.len = htonl(size);
 	memcpy(request.handle, &req, sizeof(req));
 
 	result = nbd_xmit(1, sock, (char *) &request, sizeof(request));
@@ -163,7 +164,7 @@
 
 	if (req->cmd == WRITE) {
 		DEBUG("data, ");
-		result = nbd_xmit(1, sock, req->buffer, req->current_nr_sectors << 9);
+		result = nbd_xmit(1, sock, req->buffer, size);
 		if (result <= 0)
 			FAIL("Send data failed.");
 	}
@@ -475,11 +476,7 @@
  *  (Just smiley confuses emacs :-)
  */
 
-#ifdef MODULE
-#define nbd_init init_module
-#endif
-
-int nbd_init(void)
+int __init nbd_init(void)
 {
 	int i;
 
@@ -526,8 +523,7 @@
 	return 0;
 }
 
-#ifdef MODULE
-void cleanup_module(void)
+void __exit nbd_cleanup(void)
 {
 	devfs_unregister (devfs_handle);
 	blk_cleanup_queue(BLK_DEFAULT_QUEUE(MAJOR_NR));
@@ -537,4 +533,9 @@
 	else
 		printk("nbd: module cleaned up.\n");
 }
-#endif
+
+module_init(nbd_init);
+module_exit(nbd_cleanup);
+
+MODULE_DESCRIPTION("Network Block Device");
+

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: NBD Cleanup patch and bugfix in ll_rw_blk.c
  2001-02-25 19:57 NBD Cleanup patch and bugfix in ll_rw_blk.c Steve Whitehouse
@ 2001-02-25 20:55 ` Andrea Arcangeli
  2001-02-25 20:59   ` Jens Axboe
  2001-02-25 22:39 ` Russell King
  2001-02-28 19:41 ` Linus Torvalds
  2 siblings, 1 reply; 14+ messages in thread
From: Andrea Arcangeli @ 2001-02-25 20:55 UTC (permalink / raw)
  To: Steve Whitehouse; +Cc: torvalds, pavel, linux-kernel

On Sun, Feb 25, 2001 at 07:57:29PM +0000, Steve Whitehouse wrote:
> The bug fix in ll_rw_blk.c prevents hangs when using block devices which
> don't have plugging functions,

It looks the right fix (better than 2.4.0 that didn't had such bug but that was
recalling the request_fn at every inserction in the queue).

Thanks,
Andrea

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: NBD Cleanup patch and bugfix in ll_rw_blk.c
  2001-02-25 20:55 ` Andrea Arcangeli
@ 2001-02-25 20:59   ` Jens Axboe
  0 siblings, 0 replies; 14+ messages in thread
From: Jens Axboe @ 2001-02-25 20:59 UTC (permalink / raw)
  To: Andrea Arcangeli; +Cc: Steve Whitehouse, torvalds, pavel, linux-kernel

On Sun, Feb 25 2001, Andrea Arcangeli wrote:
> On Sun, Feb 25, 2001 at 07:57:29PM +0000, Steve Whitehouse wrote:
> > The bug fix in ll_rw_blk.c prevents hangs when using block devices which
> > don't have plugging functions,
> 
> It looks the right fix (better than 2.4.0 that didn't had such bug but
> that was recalling the request_fn at every inserction in the queue).

Indeed, the removal was too quick and forgot private plugs.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: NBD Cleanup patch and bugfix in ll_rw_blk.c
  2001-02-25 19:57 NBD Cleanup patch and bugfix in ll_rw_blk.c Steve Whitehouse
  2001-02-25 20:55 ` Andrea Arcangeli
@ 2001-02-25 22:39 ` Russell King
  2001-02-25 22:49   ` Jens Axboe
  2001-02-28 19:41 ` Linus Torvalds
  2 siblings, 1 reply; 14+ messages in thread
From: Russell King @ 2001-02-25 22:39 UTC (permalink / raw)
  To: Steve Whitehouse; +Cc: torvalds, pavel, linux-kernel

On Sun, Feb 25, 2001 at 07:57:29PM +0000, Steve Whitehouse wrote:
> -int nbd_init(void)
> +int __init nbd_init(void)

> -void cleanup_module(void)
> +void __exit nbd_cleanup(void)

> +
> +module_init(nbd_init);
> +module_exit(nbd_cleanup);

If you're using module_init/module_exit, shouldn't nbd_init/nbd_cleanup
be declared statically?

--
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: NBD Cleanup patch and bugfix in ll_rw_blk.c
  2001-02-25 22:39 ` Russell King
@ 2001-02-25 22:49   ` Jens Axboe
  2001-02-25 23:02     ` Steve Whitehouse
  0 siblings, 1 reply; 14+ messages in thread
From: Jens Axboe @ 2001-02-25 22:49 UTC (permalink / raw)
  To: Russell King; +Cc: Steve Whitehouse, torvalds, pavel, linux-kernel

On Sun, Feb 25 2001, Russell King wrote:
> On Sun, Feb 25, 2001 at 07:57:29PM +0000, Steve Whitehouse wrote:
> > -int nbd_init(void)
> > +int __init nbd_init(void)
> 
> > -void cleanup_module(void)
> > +void __exit nbd_cleanup(void)
> 
> > +
> > +module_init(nbd_init);
> > +module_exit(nbd_cleanup);
> 
> If you're using module_init/module_exit, shouldn't nbd_init/nbd_cleanup
> be declared statically?

And more importantly, the init calls from ll_rw_blk.c:blk_dev_init()
should be removed too.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: NBD Cleanup patch and bugfix in ll_rw_blk.c
  2001-02-25 22:49   ` Jens Axboe
@ 2001-02-25 23:02     ` Steve Whitehouse
  0 siblings, 0 replies; 14+ messages in thread
From: Steve Whitehouse @ 2001-02-25 23:02 UTC (permalink / raw)
  To: Jens Axboe; +Cc: Russell King, torvalds, pavel, linux-kernel

Hi,

Oops, sorry. Updated patch below. Jens & Russell: Thanks for pointing
this out,

Steve.

> 
> On Sun, Feb 25 2001, Russell King wrote:
> > On Sun, Feb 25, 2001 at 07:57:29PM +0000, Steve Whitehouse wrote:
> > > -int nbd_init(void)
> > > +int __init nbd_init(void)
> > 
> > > -void cleanup_module(void)
> > > +void __exit nbd_cleanup(void)
> > 
> > > +
> > > +module_init(nbd_init);
> > > +module_exit(nbd_cleanup);
> > 
> > If you're using module_init/module_exit, shouldn't nbd_init/nbd_cleanup
> > be declared statically?
> 
> And more importantly, the init calls from ll_rw_blk.c:blk_dev_init()
> should be removed too.
> 
> -- 
> Jens Axboe
> 

-----------------------------------------------------------------------------

diff -u -r linux-2.4.2/drivers/block/ll_rw_blk.c linux/drivers/block/ll_rw_blk.c
--- linux-2.4.2/drivers/block/ll_rw_blk.c	Thu Feb 22 19:46:23 2001
+++ linux/drivers/block/ll_rw_blk.c	Sun Feb 25 23:05:30 2001
@@ -588,6 +588,9 @@
 	 * inserted at elevator_merge time
 	 */
 	list_add(&req->queue, insert_here);
+
+	if (!q->plugged && insert_here == &q->queue_head)
+		q->request_fn(q);
 }
 
 void inline blk_refill_freelist(request_queue_t *q, int rw)
@@ -1320,9 +1323,6 @@
 #endif
 #ifdef CONFIG_DDV
 	ddv_init();
-#endif
-#ifdef CONFIG_BLK_DEV_NBD
-	nbd_init();
 #endif
 #ifdef CONFIG_MDISK
 	mdisk_init();
diff -u -r linux-2.4.2/drivers/block/nbd.c linux/drivers/block/nbd.c
--- linux-2.4.2/drivers/block/nbd.c	Mon Oct 30 22:30:33 2000
+++ linux/drivers/block/nbd.c	Sun Feb 25 23:05:19 2001
@@ -29,7 +29,7 @@
 #include <linux/major.h>
 
 #include <linux/module.h>
-
+#include <linux/init.h>
 #include <linux/sched.h>
 #include <linux/fs.h>
 #include <linux/stat.h>
@@ -149,12 +149,13 @@
 {
 	int result;
 	struct nbd_request request;
+	unsigned long size = req->current_nr_sectors << 9;
 
 	DEBUG("NBD: sending control, ");
 	request.magic = htonl(NBD_REQUEST_MAGIC);
 	request.type = htonl(req->cmd);
 	request.from = cpu_to_be64( (u64) req->sector << 9);
-	request.len = htonl(req->current_nr_sectors << 9);
+	request.len = htonl(size);
 	memcpy(request.handle, &req, sizeof(req));
 
 	result = nbd_xmit(1, sock, (char *) &request, sizeof(request));
@@ -163,7 +164,7 @@
 
 	if (req->cmd == WRITE) {
 		DEBUG("data, ");
-		result = nbd_xmit(1, sock, req->buffer, req->current_nr_sectors << 9);
+		result = nbd_xmit(1, sock, req->buffer, size);
 		if (result <= 0)
 			FAIL("Send data failed.");
 	}
@@ -475,11 +476,7 @@
  *  (Just smiley confuses emacs :-)
  */
 
-#ifdef MODULE
-#define nbd_init init_module
-#endif
-
-int nbd_init(void)
+static int __init nbd_init(void)
 {
 	int i;
 
@@ -526,8 +523,7 @@
 	return 0;
 }
 
-#ifdef MODULE
-void cleanup_module(void)
+static void __exit nbd_cleanup(void)
 {
 	devfs_unregister (devfs_handle);
 	blk_cleanup_queue(BLK_DEFAULT_QUEUE(MAJOR_NR));
@@ -537,4 +533,9 @@
 	else
 		printk("nbd: module cleaned up.\n");
 }
-#endif
+
+module_init(nbd_init);
+module_exit(nbd_cleanup);
+
+MODULE_DESCRIPTION("Network Block Device");
+

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: NBD Cleanup patch and bugfix in ll_rw_blk.c
  2001-02-25 19:57 NBD Cleanup patch and bugfix in ll_rw_blk.c Steve Whitehouse
  2001-02-25 20:55 ` Andrea Arcangeli
  2001-02-25 22:39 ` Russell King
@ 2001-02-28 19:41 ` Linus Torvalds
  2001-02-28 21:27   ` Steve Whitehouse
  2 siblings, 1 reply; 14+ messages in thread
From: Linus Torvalds @ 2001-02-28 19:41 UTC (permalink / raw)
  To: Steve Whitehouse; +Cc: pavel, Jens Axboe, linux-kernel



On Sun, 25 Feb 2001, Steve Whitehouse wrote:
> 
> Here is a new version of the patch I recently sent to the list with some 
> NBD cleanups and a bug fix in ll_rw_blk.c. The changes to NBD have Pavel 
> Machek's approval as I've left out the two changes as he suggested.
> 
> The bug fix in ll_rw_blk.c prevents hangs when using block devices which
> don't have plugging functions,

I'm convinced that the right fix is to just make everybody have plugging
functions.

Right now, who doesn't? The fix is unbelievably ugly, AND can break real
drivers that _do_ have plugging functions (where they get surprised by
having their request function called several times per plug just because 
somebody unplugged them and new requests came in).

Just fix ndb instead.

		Linus


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: NBD Cleanup patch and bugfix in ll_rw_blk.c
  2001-02-28 19:41 ` Linus Torvalds
@ 2001-02-28 21:27   ` Steve Whitehouse
  2001-02-28 21:37     ` Jens Axboe
  2001-02-28 23:29     ` Linus Torvalds
  0 siblings, 2 replies; 14+ messages in thread
From: Steve Whitehouse @ 2001-02-28 21:27 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: pavel, Jens Axboe, linux-kernel

Hi,

> 
> 
> 
> On Sun, 25 Feb 2001, Steve Whitehouse wrote:
> > 
> > Here is a new version of the patch I recently sent to the list with some 
> > NBD cleanups and a bug fix in ll_rw_blk.c. The changes to NBD have Pavel 
> > Machek's approval as I've left out the two changes as he suggested.
> > 
> > The bug fix in ll_rw_blk.c prevents hangs when using block devices which
> > don't have plugging functions,
> 
> I'm convinced that the right fix is to just make everybody have plugging
> functions.
> 
I'm working on that. Once I've discovered why enabling plugging causes nbd
to hang, then I'll send a patch assuming nobody beats me to it :-)

> Right now, who doesn't? The fix is unbelievably ugly, AND can break real
> drivers that _do_ have plugging functions (where they get surprised by
> having their request function called several times per plug just because 
> somebody unplugged them and new requests came in).
> 
> Just fix ndb instead.
> 
> 		Linus
> 
I tested the patch with a printk() which printed whenever the new call to the
request function was triggered. It didn't happen once in normal fs use
with ext2 on a scsi disk. From the code I think its not even possible for
this to be called at all for a device which has plugging. For a plugged
device when I/O comes in, there appear to be only two cases:

 - Device queue empty. Device gets plugged. New request_fn call not called
 - Device queue not empty. New I/O added to back of queue. New request_fn
   not called (it only gets called when the I/O is added to the front of
   the queue).

I think nbd is the only device which doesn't use plugging at the
moment (from a quick grep of the kernel source),

Steve.


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: NBD Cleanup patch and bugfix in ll_rw_blk.c
  2001-02-28 21:27   ` Steve Whitehouse
@ 2001-02-28 21:37     ` Jens Axboe
  2001-02-28 23:29     ` Linus Torvalds
  1 sibling, 0 replies; 14+ messages in thread
From: Jens Axboe @ 2001-02-28 21:37 UTC (permalink / raw)
  To: Steve Whitehouse; +Cc: Linus Torvalds, pavel, linux-kernel

On Wed, Feb 28 2001, Steve Whitehouse wrote:
> > > The bug fix in ll_rw_blk.c prevents hangs when using block devices which
> > > don't have plugging functions,
> > 
> > I'm convinced that the right fix is to just make everybody have plugging
> > functions.
> > 
> I'm working on that. Once I've discovered why enabling plugging causes nbd
> to hang, then I'll send a patch assuming nobody beats me to it :-)

Does anyone actually know why? Pavel didn't seem to, so maybe it
was just disabled on a hunch?

> I tested the patch with a printk() which printed whenever the new call to the
> request function was triggered. It didn't happen once in normal fs use
> with ext2 on a scsi disk. From the code I think its not even possible for
> this to be called at all for a device which has plugging. For a plugged
> device when I/O comes in, there appear to be only two cases:
> 
>  - Device queue empty. Device gets plugged. New request_fn call not called
>  - Device queue not empty. New I/O added to back of queue. New request_fn
>    not called (it only gets called when the I/O is added to the front of
>    the queue).

Exactly right.

> I think nbd is the only device which doesn't use plugging at the
> moment (from a quick grep of the kernel source),

Probably right -- loop used to disable plugging to disallow merging
and tq_disk deadlocks, but now (-ac) it's a make_request style
driver instead.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: NBD Cleanup patch and bugfix in ll_rw_blk.c
  2001-02-28 21:27   ` Steve Whitehouse
  2001-02-28 21:37     ` Jens Axboe
@ 2001-02-28 23:29     ` Linus Torvalds
  2001-03-01  0:07       ` Jens Axboe
  1 sibling, 1 reply; 14+ messages in thread
From: Linus Torvalds @ 2001-02-28 23:29 UTC (permalink / raw)
  To: Steve Whitehouse; +Cc: pavel, Jens Axboe, linux-kernel



On Wed, 28 Feb 2001, Steve Whitehouse wrote:
> 
> I tested the patch with a printk() which printed whenever the new call to the
> request function was triggered. It didn't happen once in normal fs use
> with ext2 on a scsi disk.

As far as I can tell, the patch will trigger only for a not-empty request
list, where the elevator decides to put the new request at the head of the
queue.

Which is probably unlikely (and with the current elevator it might even be
impossible). But it does not look impossible in theory. And I'd really
prefer _not_ to have something that
 - looks completely bogus from a design standpoint
 - might be buggy under some rather unlikely circumstances

Reading them together they become "might cause disk corruption in some
really hard-to-trigger circumstances". No, thank you.

Note that I suspect that all current drivers (or at least the common ones)
have protection against being called multiple times, simply because 2.2.x
used to do it. Which again means that you'd probably never see problems
in practice. But that doesn't make it _right_. 

		Linus


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: NBD Cleanup patch and bugfix in ll_rw_blk.c
  2001-02-28 23:29     ` Linus Torvalds
@ 2001-03-01  0:07       ` Jens Axboe
  2001-03-01  3:14         ` Linus Torvalds
       [not found]         ` <200103010314.TAA06827@penguin.transmeta.com>
  0 siblings, 2 replies; 14+ messages in thread
From: Jens Axboe @ 2001-03-01  0:07 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Steve Whitehouse, pavel, linux-kernel

On Wed, Feb 28 2001, Linus Torvalds wrote:
> As far as I can tell, the patch will trigger only for a not-empty request
> list, where the elevator decides to put the new request at the head of the
> queue.
> 
> Which is probably unlikely (and with the current elevator it might even be
> impossible). But it does not look impossible in theory. And I'd really
> prefer _not_ to have something that
>  - looks completely bogus from a design standpoint
>  - might be buggy under some rather unlikely circumstances
> 
> Reading them together they become "might cause disk corruption in some
> really hard-to-trigger circumstances". No, thank you.
> 
> Note that I suspect that all current drivers (or at least the common ones)
> have protection against being called multiple times, simply because 2.2.x
> used to do it. Which again means that you'd probably never see problems
> in practice. But that doesn't make it _right_. 

I think most of the "we want to disable plugging" behaviour stems
from the way task queues behave. Once somebody starts a tq_disk
run, the list is fried and walked one by one. Both old loop
and nbd drop the io_request_lock and block, possibly waiting
for I/O to be done (at least the loop case, don't know about
ndb). But this I/O won't be done just because the target plug every
now and then just happens to be queued behind the nbd/loop one and a new
tq_disk run won't start it.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: NBD Cleanup patch and bugfix in ll_rw_blk.c
  2001-03-01  0:07       ` Jens Axboe
@ 2001-03-01  3:14         ` Linus Torvalds
       [not found]         ` <200103010314.TAA06827@penguin.transmeta.com>
  1 sibling, 0 replies; 14+ messages in thread
From: Linus Torvalds @ 2001-03-01  3:14 UTC (permalink / raw)
  To: linux-kernel

In article <20010301010751.Y21518@suse.de>, Jens Axboe  <axboe@suse.de> wrote:
>
>I think most of the "we want to disable plugging" behaviour stems
>from the way task queues behave. Once somebody starts a tq_disk
>run, the list is fried and walked one by one. Both old loop
>and nbd drop the io_request_lock and block, possibly waiting
>for I/O to be done (at least the loop case, don't know about
>ndb). But this I/O won't be done just because the target plug every
>now and then just happens to be queued behind the nbd/loop one and a new
>tq_disk run won't start it.

Ugh. 

How about loop/ndb intercepting the damn requests at the "elevator"
layer - that way you see every one of them, and the actual request
function might as well just be a no-op?

		Linus

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: NBD Cleanup patch and bugfix in ll_rw_blk.c
       [not found]         ` <200103010314.TAA06827@penguin.transmeta.com>
@ 2001-03-01 13:39           ` Jens Axboe
  2001-03-01 14:49             ` Jens Axboe
  0 siblings, 1 reply; 14+ messages in thread
From: Jens Axboe @ 2001-03-01 13:39 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Linux Kernel

On Wed, Feb 28 2001, Linus Torvalds wrote:
> >I think most of the "we want to disable plugging" behaviour stems
> >from the way task queues behave. Once somebody starts a tq_disk
> >run, the list is fried and walked one by one. Both old loop
> >and nbd drop the io_request_lock and block, possibly waiting
> >for I/O to be done (at least the loop case, don't know about
> >ndb). But this I/O won't be done just because the target plug every
> >now and then just happens to be queued behind the nbd/loop one and a new
> >tq_disk run won't start it.
> 
> Ugh. 
> 
> How about loop/ndb intercepting the damn requests at the "elevator"
> layer - that way you see every one of them, and the actual request
> function might as well just be a no-op?

I've suggested that before, turn ndb into ndb_make_request style
driver and all of this will disappear. I'll give it a shot.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: NBD Cleanup patch and bugfix in ll_rw_blk.c
  2001-03-01 13:39           ` Jens Axboe
@ 2001-03-01 14:49             ` Jens Axboe
  0 siblings, 0 replies; 14+ messages in thread
From: Jens Axboe @ 2001-03-01 14:49 UTC (permalink / raw)
  To: Linus Torvalds; +Cc: Steve Whitehouse, Linux Kernel

[-- Attachment #1: Type: text/plain, Size: 286 bytes --]

On Thu, Mar 01 2001, Jens Axboe wrote:
> I've suggested that before, turn ndb into ndb_make_request style
> driver and all of this will disappear. I'll give it a shot.

Here's a first shot, compile tested but that's it. Steve, does this
work for you? Against 2.4.2-ac7

-- 
Jens Axboe


[-- Attachment #2: nbd-1 --]
[-- Type: text/plain, Size: 12091 bytes --]

diff -ur --exclude-from /home/axboe/cdrom/exclude /opt/kernel/linux-2.4.2-ac7/drivers/block/nbd.c linux/drivers/block/nbd.c
--- /opt/kernel/linux-2.4.2-ac7/drivers/block/nbd.c	Thu Mar  1 15:46:14 2001
+++ linux/drivers/block/nbd.c	Thu Mar  1 15:44:57 2001
@@ -24,7 +24,6 @@
  *          structure with userland
  */
 
-#undef	NBD_PLUGGABLE
 #define PARANOIA
 #include <linux/major.h>
 
@@ -44,6 +43,8 @@
 #include <asm/uaccess.h>
 #include <asm/types.h>
 
+static kmem_cache_t *nbd_cachep;
+
 #define MAJOR_NR NBD_MAJOR
 #include <linux/nbd.h>
 
@@ -66,8 +67,6 @@
 static int requests_out;
 #endif
 
-static void nbd_plug_device(request_queue_t *q, kdev_t dev) { }
-
 static int nbd_open(struct inode *inode, struct file *file)
 {
 	int dev;
@@ -145,52 +144,52 @@
 
 #define FAIL( s ) { printk( KERN_ERR "NBD: " s "(result %d)\n", result ); goto error_out; }
 
-void nbd_send_req(struct socket *sock, struct request *req)
+void nbd_send_req(struct socket *sock, struct nbd_bh *nbh)
 {
 	int result;
 	struct nbd_request request;
-	unsigned long size = req->current_nr_sectors << 9;
+	struct buffer_head *bh = nbh->bh;
 
 	DEBUG("NBD: sending control, ");
 	request.magic = htonl(NBD_REQUEST_MAGIC);
-	request.type = htonl(req->cmd);
-	request.from = cpu_to_be64( (u64) req->sector << 9);
-	request.len = htonl(size);
-	memcpy(request.handle, &req, sizeof(req));
+	request.type = htonl(nbh->cmd);
+	request.from = cpu_to_be64( (u64) bh->b_rsector << 9);
+	request.len = htonl(bh->b_size);
+	memcpy(request.handle, &nbh, sizeof(nbh));
 
 	result = nbd_xmit(1, sock, (char *) &request, sizeof(request));
 	if (result <= 0)
 		FAIL("Sendmsg failed for control.");
 
-	if (req->cmd == WRITE) {
+	if (nbh->cmd == WRITE) {
 		DEBUG("data, ");
-		result = nbd_xmit(1, sock, req->buffer, size);
+		result = nbd_xmit(1, sock, bh->b_data, bh->b_size);
 		if (result <= 0)
 			FAIL("Send data failed.");
 	}
 	return;
 
       error_out:
-	req->errors++;
+	nbh->errors++;
 }
 
 #define HARDFAIL( s ) { printk( KERN_ERR "NBD: " s "(result %d)\n", result ); lo->harderror = result; return NULL; }
-struct request *nbd_read_stat(struct nbd_device *lo)
+struct nbd_bh *nbd_read_stat(struct nbd_device *lo)
 		/* NULL returned = something went wrong, inform userspace       */ 
 {
 	int result;
 	struct nbd_reply reply;
-	struct request *xreq, *req;
+	struct nbd_bh *xnbh, *nbh;
 
 	DEBUG("reading control, ");
 	reply.magic = 0;
 	result = nbd_xmit(0, lo->sock, (char *) &reply, sizeof(reply));
 	if (result <= 0)
 		HARDFAIL("Recv control failed.");
-	memcpy(&xreq, reply.handle, sizeof(xreq));
-	req = blkdev_entry_prev_request(&lo->queue_head);
+	memcpy(&xnbh, reply.handle, sizeof(xnbh));
+	nbh = list_entry(lo->queue_head.prev, struct nbd_bh, queue);
 
-	if (xreq != req)
+	if (xnbh != nbh)
 		FAIL("Unexpected handle received.\n");
 
 	DEBUG("ok, ");
@@ -198,41 +197,37 @@
 		HARDFAIL("Not enough magic.");
 	if (ntohl(reply.error))
 		FAIL("Other side returned error.");
-	if (req->cmd == READ) {
+	if (nbh->cmd == READ) {
 		DEBUG("data, ");
-		result = nbd_xmit(0, lo->sock, req->buffer, req->current_nr_sectors << 9);
+		result = nbd_xmit(0, lo->sock, nbh->bh->b_data,nbh->bh->b_size);
 		if (result <= 0)
 			HARDFAIL("Recv data failed.");
 	}
 	DEBUG("done.\n");
-	return req;
+	return nbh;
 
 /* Can we get here? Yes, if other side returns error */
       error_out:
-	req->errors++;
-	return req;
+	nbh->errors++;
+	return nbh;
 }
 
 void nbd_do_it(struct nbd_device *lo)
 {
-	struct request *req;
-	int dequeued;
+	struct nbd_bh *nbh;
 
 	down (&lo->queue_lock);
 	while (1) {
 		up (&lo->queue_lock);
-		req = nbd_read_stat(lo);
+		nbh = nbd_read_stat(lo);
 		down (&lo->queue_lock);
 
-		if (!req) {
+		if (!nbh) {
 			printk(KERN_ALERT "req should never be null\n" );
 			goto out;
 		}
 #ifdef PARANOIA
-		if (req != blkdev_entry_prev_request(&lo->queue_head)) {
-			printk(KERN_ALERT "NBD: I have problem...\n");
-		}
-		if (lo != &nbd_dev[MINOR(req->rq_dev)]) {
+		if (lo != &nbd_dev[MINOR(nbh->bh->b_rdev)]) {
 			printk(KERN_ALERT "NBD: request corrupted!\n");
 			continue;
 		}
@@ -241,14 +236,12 @@
 			goto out;
 		}
 #endif
-		list_del(&req->queue);
+		list_del(&nbh->queue);
 		up (&lo->queue_lock);
 		
-		dequeued = nbd_end_request(req);
+		nbd_end_request(nbh);
 
 		down (&lo->queue_lock);
-		if (!dequeued)
-			list_add(&req->queue, &lo->queue_head);
 	}
  out:
 	up (&lo->queue_lock);
@@ -256,8 +249,7 @@
 
 void nbd_clear_que(struct nbd_device *lo)
 {
-	struct request *req;
-	int dequeued;
+	struct nbd_bh *nbh;
 
 #ifdef PARANOIA
 	if (lo->magic != LO_MAGIC) {
@@ -267,29 +259,43 @@
 #endif
 
 	while (!list_empty(&lo->queue_head)) {
-		req = blkdev_entry_prev_request(&lo->queue_head);
+		nbh = list_entry(lo->queue_head.prev, struct nbd_bh, queue);
 #ifdef PARANOIA
-		if (!req) {
-			printk( KERN_ALERT "NBD: panic, panic, panic\n" );
-			break;
-		}
-		if (lo != &nbd_dev[MINOR(req->rq_dev)]) {
+		if (lo != &nbd_dev[MINOR(nbh->bh->b_rdev)]) {
 			printk(KERN_ALERT "NBD: request corrupted when clearing!\n");
 			continue;
 		}
 #endif
-		req->errors++;
-		list_del(&req->queue);
+		nbh->errors++;
+		list_del(&nbh->queue);
 		up(&lo->queue_lock);
 
-		dequeued = nbd_end_request(req);
+		nbd_end_request(nbh);
 
 		down(&lo->queue_lock);
-		if (!dequeued)
-			list_add(&req->queue, &lo->queue_head);
 	}
 }
 
+static struct nbd_bh *nbd_alloc_bh(struct buffer_head *bh, int rw)
+{
+	struct nbd_bh *nbh;
+
+	do {
+		nbh = kmem_cache_alloc(nbd_cachep, SLAB_BUFFER);
+		if (nbh)
+			break;
+
+		run_task_queue(&tq_disk);
+		schedule_timeout(HZ);
+	} while (1);
+
+	nbh->bh = bh;
+	nbh->cmd = rw;
+	nbh->errors = 0;
+	INIT_LIST_HEAD(&nbh->queue);
+	return nbh;
+}
+
 /*
  * We always wait for result of write, for now. It would be nice to make it optional
  * in future
@@ -300,53 +306,45 @@
 #undef FAIL
 #define FAIL( s ) { printk( KERN_ERR "NBD, minor %d: " s "\n", dev ); goto error_out; }
 
-static void do_nbd_request(request_queue_t * q)
+static int nbd_make_request(request_queue_t *q, int rw, struct buffer_head *bh)
 {
-	struct request *req;
 	int dev = 0;
 	struct nbd_device *lo;
+	struct nbd_bh *nbh;
 
-	while (!QUEUE_EMPTY) {
-		req = CURRENT;
+	dev = MINOR(bh->b_rdev);
 #ifdef PARANOIA
-		if (!req)
-			FAIL("que not empty but no request?");
-#endif
-		dev = MINOR(req->rq_dev);
-#ifdef PARANOIA
-		if (dev >= MAX_NBD)
-			FAIL("Minor too big.");		/* Probably can not happen */
+	if (dev >= MAX_NBD)
+		FAIL("Minor too big.");		/* Probably can not happen */
 #endif
-		lo = &nbd_dev[dev];
-		if (!lo->file)
-			FAIL("Request when not-ready.");
-		if ((req->cmd == WRITE) && (lo->flags & NBD_READ_ONLY))
-			FAIL("Write on read-only");
+	lo = &nbd_dev[dev];
+	if (!lo->file)
+		FAIL("Request when not-ready.");
+	if ((rw == WRITE) && (lo->flags & NBD_READ_ONLY))
+		FAIL("Write on read-only");
 #ifdef PARANOIA
-		if (lo->magic != LO_MAGIC)
-			FAIL("nbd[] is not magical!");
-		requests_in++;
+	if (lo->magic != LO_MAGIC)
+		FAIL("nbd[] is not magical!");
+	requests_in++;
 #endif
-		req->errors = 0;
-		blkdev_dequeue_request(req);
-		spin_unlock_irq(&io_request_lock);
 
-		down (&lo->queue_lock);
-		list_add(&req->queue, &lo->queue_head);
-		nbd_send_req(lo->sock, req);	/* Why does this block?         */
-		up (&lo->queue_lock);
+	/*
+	 * not an error, but we don't want to do read-aheads
+	 */
+	if (rw == READA)
+		goto error_out;
 
-		spin_lock_irq(&io_request_lock);
-		continue;
+	nbh = nbd_alloc_bh(bh, rw);
 
-	      error_out:
-		req->errors++;
-		blkdev_dequeue_request(req);
-		spin_unlock(&io_request_lock);
-		nbd_end_request(req);
-		spin_lock(&io_request_lock);
-	}
-	return;
+	down (&lo->queue_lock);
+	list_add(&nbh->queue, &lo->queue_head);
+	nbd_send_req(lo->sock, nbh);	/* Why does this block?         */
+	up (&lo->queue_lock);
+	return 0;
+
+error_out:
+	bh->b_end_io(bh, test_bit(BH_Uptodate, &bh->b_state));
+	return 0;
 }
 
 static int nbd_ioctl(struct inode *inode, struct file *file,
@@ -354,7 +352,7 @@
 {
 	struct nbd_device *lo;
 	int dev, error, temp;
-	struct request sreq ;
+	struct nbd_bh snbh;
 
 	/* Anyone capable of this syscall can do *real bad* things */
 
@@ -370,9 +368,9 @@
 	switch (cmd) {
 	case NBD_DISCONNECT:
 	        printk("NBD_DISCONNECT\n") ;
-                sreq.cmd=2 ; /* shutdown command */
+                snbh.cmd=2 ; /* shutdown command */
                 if (!lo->sock) return -EINVAL ;
-                nbd_send_req(lo->sock,&sreq) ;
+                nbd_send_req(lo->sock, &snbh);
                 return 0 ;
  
 	case NBD_CLEAR_SOCK:
@@ -476,16 +474,25 @@
  *  (Just smiley confuses emacs :-)
  */
 
-static int __init nbd_init(void)
+int __init nbd_init(void)
 {
 	int i;
 
+	nbd_cachep = kmem_cache_create("nbd_requests", sizeof(struct nbd_bh),
+					0, SLAB_HWCACHE_ALIGN, NULL, NULL);
+	if (nbd_cachep == NULL) {
+		printk("nbd: unable to setup slab cache\n");
+		return -ENOMEM;
+	}
+
 	if (sizeof(struct nbd_request) != 28) {
+		kmem_cache_destroy(nbd_cachep);
 		printk(KERN_CRIT "Sizeof nbd_request needs to be 28 in order to work!\n" );
 		return -EIO;
 	}
 
 	if (register_blkdev(MAJOR_NR, "nbd", &nbd_fops)) {
+		kmem_cache_destroy(nbd_cachep);
 		printk("Unable to get major number %d for NBD\n",
 		       MAJOR_NR);
 		return -EIO;
@@ -495,11 +502,7 @@
 #endif
 	blksize_size[MAJOR_NR] = nbd_blksizes;
 	blk_size[MAJOR_NR] = nbd_sizes;
-	blk_init_queue(BLK_DEFAULT_QUEUE(MAJOR_NR), do_nbd_request);
-#ifndef NBD_PLUGGABLE
-	blk_queue_pluggable(BLK_DEFAULT_QUEUE(MAJOR_NR), nbd_plug_device);
-#endif
-	blk_queue_headactive(BLK_DEFAULT_QUEUE(MAJOR_NR), 0);
+	blk_queue_make_request(BLK_DEFAULT_QUEUE(MAJOR_NR), nbd_make_request);
 	for (i = 0; i < MAX_NBD; i++) {
 		nbd_dev[i].refcnt = 0;
 		nbd_dev[i].file = NULL;
@@ -526,7 +529,7 @@
 static void __exit nbd_cleanup(void)
 {
 	devfs_unregister (devfs_handle);
-	blk_cleanup_queue(BLK_DEFAULT_QUEUE(MAJOR_NR));
+	kmem_cache_destroy(nbd_cachep);
 
 	if (unregister_blkdev(MAJOR_NR, "nbd") != 0)
 		printk("nbd: cleanup_module failed\n");
diff -ur --exclude-from /home/axboe/cdrom/exclude /opt/kernel/linux-2.4.2-ac7/include/linux/blk.h linux/include/linux/blk.h
--- /opt/kernel/linux-2.4.2-ac7/include/linux/blk.h	Thu Mar  1 15:46:17 2001
+++ linux/include/linux/blk.h	Thu Mar  1 15:44:30 2001
@@ -291,12 +291,6 @@
 #define DEVICE_REQUEST do_mfm_request
 #define DEVICE_NR(device) (MINOR(device) >> 6)
 
-#elif (MAJOR_NR == NBD_MAJOR)
-
-#define DEVICE_NAME "nbd"
-#define DEVICE_REQUEST do_nbd_request
-#define DEVICE_NR(device) (MINOR(device))
-
 #elif (MAJOR_NR == MDISK_MAJOR)
 
 #define DEVICE_NAME "mdisk"
diff -ur --exclude-from /home/axboe/cdrom/exclude /opt/kernel/linux-2.4.2-ac7/include/linux/nbd.h linux/include/linux/nbd.h
--- /opt/kernel/linux-2.4.2-ac7/include/linux/nbd.h	Mon Dec 11 21:50:42 2000
+++ linux/include/linux/nbd.h	Thu Mar  1 15:44:05 2001
@@ -22,8 +22,6 @@
 #include <linux/locks.h>
 #include <asm/semaphore.h>
 
-#define LOCAL_END_REQUEST
-
 #include <linux/blk.h>
 
 #ifdef PARANOIA
@@ -31,35 +29,31 @@
 extern int requests_out;
 #endif
 
+/*
+ * a bit of a hack...
+ */
+struct nbd_bh
+{
+	struct buffer_head *bh;
+	int cmd, errors;
+	struct list_head queue;
+};
+
 static int 
-nbd_end_request(struct request *req)
+nbd_end_request(struct nbd_bh *nbh)
 {
-	unsigned long flags;
-	int ret = 0;
+	struct buffer_head *bh = nbh->bh;
 
 #ifdef PARANOIA
 	requests_out++;
 #endif
-	/*
-	 * This is a very dirty hack that we have to do to handle
-	 * merged requests because end_request stuff is a bit
-	 * broken. The fact we have to do this only if there
-	 * aren't errors looks even more silly.
-	 */
-	if (!req->errors) {
-		req->sector += req->current_nr_sectors;
-		req->nr_sectors -= req->current_nr_sectors;
-	}
-
-	spin_lock_irqsave(&io_request_lock, flags);
-	if (end_that_request_first( req, !req->errors, "nbd" ))
-		goto out;
-	ret = 1;
-	end_that_request_last( req );
-
-out:
-	spin_unlock_irqrestore(&io_request_lock, flags);
-	return ret;
+
+	if (!bh)
+		BUG();
+
+	bh->b_end_io(bh, !nbh->errors);
+	kmem_cache_free(nbd_cachep, nbh);
+	return 1;
 }
 
 #define MAX_NBD 128

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2001-03-01 14:49 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2001-02-25 19:57 NBD Cleanup patch and bugfix in ll_rw_blk.c Steve Whitehouse
2001-02-25 20:55 ` Andrea Arcangeli
2001-02-25 20:59   ` Jens Axboe
2001-02-25 22:39 ` Russell King
2001-02-25 22:49   ` Jens Axboe
2001-02-25 23:02     ` Steve Whitehouse
2001-02-28 19:41 ` Linus Torvalds
2001-02-28 21:27   ` Steve Whitehouse
2001-02-28 21:37     ` Jens Axboe
2001-02-28 23:29     ` Linus Torvalds
2001-03-01  0:07       ` Jens Axboe
2001-03-01  3:14         ` Linus Torvalds
     [not found]         ` <200103010314.TAA06827@penguin.transmeta.com>
2001-03-01 13:39           ` Jens Axboe
2001-03-01 14:49             ` Jens Axboe

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).