On Sun, 16 Apr 2000, Manfred Spraul wrote:
>
>+ request_queue_t *q = &blk_dev[DAC960_MAJOR +
>Controller->ControllerNumber].request_queue;
>[...]
>- spin_unlock(&io_request_lock);
>+ spin_unlock(&q->request_lock);
>
>Are you sure that you don't introduce lots of SMP races, e.g. in drivers
>that support multiple majors?
Also drivers with multiple "queues" should be checked.
In all cases I'd prefer to get the patches separated in little pieces. The
most interesting part is the get_request complexity fixage without the
per-queue freelist but with two gloabl freelists. The other parts are more
scalability changes than real fixes and they are completly orthogonal with
the get_request fixage. get_request complexity always bitten us also doing
I/O to only one disk in the system and fixing that things has more prio
IMHO :).
About the per-queue freelist thing, to allocate the requests I suggest to
create a slab cache and allocating each request from the cache. The
current big kmalloc wouldn't be reliable with modules.
I noticed a problem in the patch in nbd.c:
+++ linux/drivers/block/nbd.c Thu Apr 13 22:24:12 2000
@@ -325,22 +325,21 @@
#endif
req->errors = 0;
blkdev_dequeue_request(req);
- spin_unlock_irq(&io_request_lock);
+ spin_unlock_irq(&q->request_lock);
down (&lo->queue_lock);
+ spin_lock_irq(&q->request_lock);
list_add(&req->queue, &lo->queue_head);
nbd_send_req(lo->sock, req); /* Why does this block?
*/
the lo->queue_head is a nbd internal queue and it's serialized by the
down(&lo->queue_lock) so you don't need and you must not grab the
io_request_lock there. The current code is correct.
Andrea
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.rutgers.edu
Please read the FAQ at http://www.tux.org/lkml/
This archive was generated by hypermail 2b29 : Sun Apr 23 2000 - 21:00:09 EST