Re: [PATCH 11/20] nbd: request_fn fixup
On Tue, Sep 12 2006, Jeff Garzik wrote:
Jens Axboe wrote:
Generally the block device rule is that once you are invoked due to an
unplug (or whatever) event, it is the responsibility of the block device
to run the queue until it's done. So if you bail out of queue handling
for whatever reason (might be resource starvation in hard- or software),
you must make sure to reenter queue handling since the device will not
get replugged while it has requests pending. Unless you run into some
software resource shortage, running of the queue is done
deterministically when you know resources are available (ie an io
completes). The device plugging itself is only ever done when you
encounter a shortage outside of your control (memory shortage, for
instance) _and_ you don't already have pending work where you can invoke
queueing from again.
Or he could employ the blk_{start,stop}_queue() functions, if that model
is easier for the driver (and brain).
Definitely, yes.
--
Jens Axboe
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/20] nbd: request_fn fixup
On Tue, Sep 12 2006, Peter Zijlstra wrote: @@ -463,10 +465,13 @@ static void do_nbd_request(request_queue error_out: req-errors++; - spin_unlock(q-queue_lock); - nbd_end_request(req); - spin_lock(q-queue_lock); + __nbd_end_request(req); } + /* + * q-queue_lock has been dropped, this opens up a race + * plug the device to close it. + */ + blk_plug_device(q); return; } This looks wrong, I wonder if this only fixes things for you because it happens to reinvoke the request handler after the timeout occurs? Your comment doesn't really describe what you think is going on, please describe in detail what you think is happening here that the plugging supposedly solves. Generally the block device rule is that once you are invoked due to an unplug (or whatever) event, it is the responsibility of the block device to run the queue until it's done. So if you bail out of queue handling for whatever reason (might be resource starvation in hard- or software), you must make sure to reenter queue handling since the device will not get replugged while it has requests pending. Unless you run into some software resource shortage, running of the queue is done deterministically when you know resources are available (ie an io completes). The device plugging itself is only ever done when you encounter a shortage outside of your control (memory shortage, for instance) _and_ you don't already have pending work where you can invoke queueing from again. -- Jens Axboe - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/20] nbd: request_fn fixup
Jens Axboe wrote:
Generally the block device rule is that once you are invoked due to an
unplug (or whatever) event, it is the responsibility of the block device
to run the queue until it's done. So if you bail out of queue handling
for whatever reason (might be resource starvation in hard- or software),
you must make sure to reenter queue handling since the device will not
get replugged while it has requests pending. Unless you run into some
software resource shortage, running of the queue is done
deterministically when you know resources are available (ie an io
completes). The device plugging itself is only ever done when you
encounter a shortage outside of your control (memory shortage, for
instance) _and_ you don't already have pending work where you can invoke
queueing from again.
Or he could employ the blk_{start,stop}_queue() functions, if that model
is easier for the driver (and brain).
Jeff
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 11/20] nbd: request_fn fixup
Dropping the queue_lock opens up a nasty race, fix this race by
plugging the device when we're done.
Also includes a small cleanup.
Signed-off-by: Peter Zijlstra [EMAIL PROTECTED]
CC: Pavel Machek [EMAIL PROTECTED]
---
drivers/block/nbd.c | 67 ++--
1 file changed, 49 insertions(+), 18 deletions(-)
Index: linux-2.6/drivers/block/nbd.c
===
--- linux-2.6.orig/drivers/block/nbd.c 2006-09-07 17:20:52.0 +0200
+++ linux-2.6/drivers/block/nbd.c 2006-09-07 17:35:05.0 +0200
@@ -97,20 +97,24 @@ static const char *nbdcmd_to_ascii(int c
}
#endif /* NDEBUG */
-static void nbd_end_request(struct request *req)
+static void __nbd_end_request(struct request *req)
{
int uptodate = (req-errors == 0) ? 1 : 0;
- request_queue_t *q = req-q;
- unsigned long flags;
dprintk(DBG_BLKDEV, %s: request %p: %s\n, req-rq_disk-disk_name,
req, uptodate? done: failed);
- spin_lock_irqsave(q-queue_lock, flags);
- if (!end_that_request_first(req, uptodate, req-nr_sectors)) {
+ if (!end_that_request_first(req, uptodate, req-nr_sectors))
end_that_request_last(req, uptodate);
- }
- spin_unlock_irqrestore(q-queue_lock, flags);
+}
+
+static void nbd_end_request(struct request *req)
+{
+ request_queue_t *q = req-q;
+
+ spin_lock_irq(q-queue_lock);
+ __nbd_end_request(req);
+ spin_unlock_irq(q-queue_lock);
}
/*
@@ -435,10 +439,8 @@ static void do_nbd_request(request_queue
mutex_unlock(lo-tx_lock);
printk(KERN_ERR %s: Attempted send on closed socket\n,
lo-disk-disk_name);
- req-errors++;
- nbd_end_request(req);
spin_lock_irq(q-queue_lock);
- continue;
+ goto error_out;
}
lo-active_req = req;
@@ -463,10 +465,13 @@ static void do_nbd_request(request_queue
error_out:
req-errors++;
- spin_unlock(q-queue_lock);
- nbd_end_request(req);
- spin_lock(q-queue_lock);
+ __nbd_end_request(req);
}
+ /*
+* q-queue_lock has been dropped, this opens up a race
+* plug the device to close it.
+*/
+ blk_plug_device(q);
return;
}
--
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
