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");
+
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
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
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 ([email protected]) The developer of ARM Linux
http://www.arm.linux.org.uk/personal/aboutme.html
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
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");
+
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
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.
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
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
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
In article <[email protected]>, Jens Axboe <[email protected]> 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
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
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