This patch adds 2 new interfaces for request completion:
o blk_end_request() : called without queue lock
o __blk_end_request() : called with queue lock held
Some device drivers call some generic functions below between
end_that_request_{first/chunk} and end_that_request_last().
o add_disk_randomness()
o blk_queue_end_tag()
o blkdev_dequeue_request()
These are called in the blk_end_request() as a part of generic
request completion.
So all device drivers become to call above functions.
"Normal" drivers can be converted to use blk_end_request()
in a standard way shown below.
a) end_that_request_{chunk/first}
spin_lock_irqsave()
(add_disk_randomness(), blk_queue_end_tag(), blkdev_dequeue_request())
end_that_request_last()
spin_unlock_irqrestore()
=> blk_end_request()
b) spin_lock_irqsave()
end_that_request_{chunk/first}
(add_disk_randomness(), blk_queue_end_tag(), blkdev_dequeue_request())
end_that_request_last()
spin_unlock_irqrestore()
=> spin_lock_irqsave()
__blk_end_request()
spin_unlock_irqsave()
c) end_that_request_last()
=> __blk_end_request()
Signed-off-by: Kiyoshi Ueda <[email protected]>
Signed-off-by: Jun'ichi Nomura <[email protected]>
---
block/ll_rw_blk.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/blkdev.h | 2 +
2 files changed, 69 insertions(+)
Index: 2.6.24-rc3-mm2/block/ll_rw_blk.c
===================================================================
--- 2.6.24-rc3-mm2.orig/block/ll_rw_blk.c
+++ 2.6.24-rc3-mm2/block/ll_rw_blk.c
@@ -3769,6 +3769,73 @@ void end_request(struct request *req, in
}
EXPORT_SYMBOL(end_request);
+static void complete_request(struct request *rq, int uptodate)
+{
+ if (blk_rq_tagged(rq))
+ blk_queue_end_tag(rq->q, rq);
+
+ /* rq->queuelist of dequeued request should be list_empty() */
+ if (!list_empty(&rq->queuelist))
+ blkdev_dequeue_request(rq);
+
+ end_that_request_last(rq, uptodate);
+}
+
+/**
+ * blk_end_request - Helper function for drivers to complete the request.
+ * @rq: the request being processed
+ * @uptodate: 1 for success, 0 for I/O error, < 0 for specific error
+ * @nr_bytes: number of bytes to complete
+ *
+ * Description:
+ * Ends I/O on a number of bytes attached to @rq.
+ * If @rq has leftover, sets it up for the next range of segments.
+ *
+ * Return:
+ * 0 - we are done with this request
+ * 1 - still buffers pending for this request
+ **/
+int blk_end_request(struct request *rq, int uptodate, int nr_bytes)
+{
+ struct request_queue *q = rq->q;
+ unsigned long flags = 0UL;
+
+ if (blk_fs_request(rq) || blk_pc_request(rq)) {
+ if (__end_that_request_first(rq, uptodate, nr_bytes))
+ return 1;
+ }
+
+ add_disk_randomness(rq->rq_disk);
+
+ spin_lock_irqsave(q->queue_lock, flags);
+ complete_request(rq, uptodate);
+ spin_unlock_irqrestore(q->queue_lock, flags);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(blk_end_request);
+
+/**
+ * __blk_end_request - Helper function for drivers to complete the request.
+ *
+ * Description:
+ * Must be called with queue lock held unlike blk_end_request().
+ **/
+int __blk_end_request(struct request *rq, int uptodate, int nr_bytes)
+{
+ if (blk_fs_request(rq) || blk_pc_request(rq)) {
+ if (__end_that_request_first(rq, uptodate, nr_bytes))
+ return 1;
+ }
+
+ add_disk_randomness(rq->rq_disk);
+
+ complete_request(rq, uptodate);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(__blk_end_request);
+
static void blk_rq_bio_prep(struct request_queue *q, struct request *rq,
struct bio *bio)
{
Index: 2.6.24-rc3-mm2/include/linux/blkdev.h
===================================================================
--- 2.6.24-rc3-mm2.orig/include/linux/blkdev.h
+++ 2.6.24-rc3-mm2/include/linux/blkdev.h
@@ -725,6 +725,8 @@ static inline void blk_run_address_space
* for parts of the original function. This prevents
* code duplication in drivers.
*/
+extern int blk_end_request(struct request *rq, int uptodate, int nr_bytes);
+extern int __blk_end_request(struct request *rq, int uptodate, int nr_bytes);
extern int end_that_request_first(struct request *, int, int);
extern int end_that_request_chunk(struct request *, int, int);
extern void end_that_request_last(struct request *, int);
On Sat, Dec 01 2007 at 1:24 +0200, Kiyoshi Ueda <[email protected]> wrote:
> This patch adds 2 new interfaces for request completion:
> o blk_end_request() : called without queue lock
> o __blk_end_request() : called with queue lock held
>
> Some device drivers call some generic functions below between
> end_that_request_{first/chunk} and end_that_request_last().
> o add_disk_randomness()
> o blk_queue_end_tag()
> o blkdev_dequeue_request()
> These are called in the blk_end_request() as a part of generic
> request completion.
> So all device drivers become to call above functions.
>
> "Normal" drivers can be converted to use blk_end_request()
> in a standard way shown below.
>
> a) end_that_request_{chunk/first}
> spin_lock_irqsave()
> (add_disk_randomness(), blk_queue_end_tag(), blkdev_dequeue_request())
> end_that_request_last()
> spin_unlock_irqrestore()
> => blk_end_request()
>
> b) spin_lock_irqsave()
> end_that_request_{chunk/first}
> (add_disk_randomness(), blk_queue_end_tag(), blkdev_dequeue_request())
> end_that_request_last()
> spin_unlock_irqrestore()
> => spin_lock_irqsave()
> __blk_end_request()
> spin_unlock_irqsave()
>
> c) end_that_request_last()
> => __blk_end_request()
>
> Signed-off-by: Kiyoshi Ueda <[email protected]>
> Signed-off-by: Jun'ichi Nomura <[email protected]>
comments in body
> ---
> block/ll_rw_blk.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++
> include/linux/blkdev.h | 2 +
> 2 files changed, 69 insertions(+)
>
> Index: 2.6.24-rc3-mm2/block/ll_rw_blk.c
> ===================================================================
> --- 2.6.24-rc3-mm2.orig/block/ll_rw_blk.c
> +++ 2.6.24-rc3-mm2/block/ll_rw_blk.c
> @@ -3769,6 +3769,73 @@ void end_request(struct request *req, in
> }
> EXPORT_SYMBOL(end_request);
>
> +static void complete_request(struct request *rq, int uptodate)
> +{
> + if (blk_rq_tagged(rq))
> + blk_queue_end_tag(rq->q, rq);
> +
> + /* rq->queuelist of dequeued request should be list_empty() */
> + if (!list_empty(&rq->queuelist))
> + blkdev_dequeue_request(rq);
> +
> + end_that_request_last(rq, uptodate);
> +}
> +
> +/**
> + * blk_end_request - Helper function for drivers to complete the request.
> + * @rq: the request being processed
> + * @uptodate: 1 for success, 0 for I/O error, < 0 for specific error
> + * @nr_bytes: number of bytes to complete
> + *
> + * Description:
> + * Ends I/O on a number of bytes attached to @rq.
> + * If @rq has leftover, sets it up for the next range of segments.
> + *
> + * Return:
> + * 0 - we are done with this request
> + * 1 - still buffers pending for this request
> + **/
> +int blk_end_request(struct request *rq, int uptodate, int nr_bytes)
I always hated that uptodate boolean with possible negative error value.
I guess it was done for backward compatibility of then users of
end_that_request_first(). But since you are introducing a new API then
this is not the case. Just have regular status int where 0 means ALL_OK
and negative value means error code.
Just my $0.02.
> +{
> + struct request_queue *q = rq->q;
> + unsigned long flags = 0UL;
> +
> + if (blk_fs_request(rq) || blk_pc_request(rq)) {
> + if (__end_that_request_first(rq, uptodate, nr_bytes))
> + return 1;
> + }
> +
> + add_disk_randomness(rq->rq_disk);
> +
> + spin_lock_irqsave(q->queue_lock, flags);
> + complete_request(rq, uptodate);
> + spin_unlock_irqrestore(q->queue_lock, flags);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(blk_end_request);
> +
> +/**
> + * __blk_end_request - Helper function for drivers to complete the request.
> + *
> + * Description:
> + * Must be called with queue lock held unlike blk_end_request().
> + **/
> +int __blk_end_request(struct request *rq, int uptodate, int nr_bytes)
> +{
> + if (blk_fs_request(rq) || blk_pc_request(rq)) {
> + if (__end_that_request_first(rq, uptodate, nr_bytes))
> + return 1;
> + }
> +
> + add_disk_randomness(rq->rq_disk);
> +
> + complete_request(rq, uptodate);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(__blk_end_request);
> +
> static void blk_rq_bio_prep(struct request_queue *q, struct request *rq,
> struct bio *bio)
> {
> Index: 2.6.24-rc3-mm2/include/linux/blkdev.h
> ===================================================================
> --- 2.6.24-rc3-mm2.orig/include/linux/blkdev.h
> +++ 2.6.24-rc3-mm2/include/linux/blkdev.h
> @@ -725,6 +725,8 @@ static inline void blk_run_address_space
> * for parts of the original function. This prevents
> * code duplication in drivers.
> */
> +extern int blk_end_request(struct request *rq, int uptodate, int nr_bytes);
> +extern int __blk_end_request(struct request *rq, int uptodate, int nr_bytes);
> extern int end_that_request_first(struct request *, int, int);
> extern int end_that_request_chunk(struct request *, int, int);
> extern void end_that_request_last(struct request *, int);
I don't like it that you have two Identical but slightly different implementations
I wish you would do an internal-with-flags implementation and then API ones can
call the internal one. Or maybe just hold the spin_lock just a bit longer and have
one call the other. To prove my case see how hard it is to add new code
like with the bidi patch, where you need to add exact same code in 3 places.
(OK only 2 places actually, if _callback is gone)
I want to say that this is grate work and thanks for doing this, I like this code
a lot.
Boaz
Hi Boaz,
On Tue, 04 Dec 2007 15:56:32 +0200, Boaz Harrosh <[email protected]> wrote:
> > +int blk_end_request(struct request *rq, int uptodate, int nr_bytes)
> > +{
> > + struct request_queue *q = rq->q;
> > + unsigned long flags = 0UL;
> > +
> > + if (blk_fs_request(rq) || blk_pc_request(rq)) {
> > + if (__end_that_request_first(rq, uptodate, nr_bytes))
> > + return 1;
> > + }
> > +
> > + add_disk_randomness(rq->rq_disk);
> > +
> > + spin_lock_irqsave(q->queue_lock, flags);
> > + complete_request(rq, uptodate);
> > + spin_unlock_irqrestore(q->queue_lock, flags);
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(blk_end_request);
> > +
> > +/**
> > + * __blk_end_request - Helper function for drivers to complete the request.
> > + *
> > + * Description:
> > + * Must be called with queue lock held unlike blk_end_request().
> > + **/
> > +int __blk_end_request(struct request *rq, int uptodate, int nr_bytes)
> > +{
> > + if (blk_fs_request(rq) || blk_pc_request(rq)) {
> > + if (__end_that_request_first(rq, uptodate, nr_bytes))
> > + return 1;
> > + }
> > +
> > + add_disk_randomness(rq->rq_disk);
> > +
> > + complete_request(rq, uptodate);
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(__blk_end_request);
>
> I don't like it that you have two Identical but slightly different
> implementations I wish you would do an internal-with-flags
> implementation and then API ones can call the internal one. Or maybe
> just hold the spin_lock just a bit longer and have one call the other.
> To prove my case see how hard it is to add new code like with
> the bidi patch, where you need to add exact same code in 3 places.
> (OK only 2 places actually, if _callback is gone)
As for the internal-with-flags implementation, I once proposed
something like below but it was rejected by Jens.
(http://marc.info/?l=linux-kernel&m=118880584720600&w=2)
----------------------------------------------------------------------
static int internal_function(rq, needlock)
{
end_that_request_chunk(rq);
if (needlock)
spin_lock_irqsave();
end_that_request_last(rq);
if (needlock)
spin_unlock_irqrestore();
}
int blk_end_request(rq)
{
return internal_function(rq, 1);
}
int __blk_end_request(rq)
{
return internal_function(rq, 0);
}
----------------------------------------------------------------------
As for the holding-queue-lock-longer implementation,
end_that_request_chunk() completes bios in the request and it can
reaches filesystem layer and may take time.
I guess many drivers like scsi are calling end_that_request_chunk()
without queue's lock because of the reason above.
I'll try to remove the duplication again by another patch-set
after blk_end_request interfaces are merged.
So I would like to leave the duplication for now.
Is it acceptable for you?
Thanks,
Kiyoshi Ueda
Hi Boaz and Jens,
On Tue, 04 Dec 2007 15:56:32 +0200, Boaz Harrosh <[email protected]> wrote:
> > +/**
> > + * blk_end_request - Helper function for drivers to complete the request.
> > + * @rq: the request being processed
> > + * @uptodate: 1 for success, 0 for I/O error, < 0 for specific error
> > + * @nr_bytes: number of bytes to complete
> > + *
> > + * Description:
> > + * Ends I/O on a number of bytes attached to @rq.
> > + * If @rq has leftover, sets it up for the next range of segments.
> > + *
> > + * Return:
> > + * 0 - we are done with this request
> > + * 1 - still buffers pending for this request
> > + **/
> > +int blk_end_request(struct request *rq, int uptodate, int nr_bytes)
>
> I always hated that uptodate boolean with possible negative error value.
> I guess it was done for backward compatibility of then users of
> end_that_request_first(). But since you are introducing a new API then
> this is not the case. Just have regular status int where 0 means ALL_OK
> and negative value means error code.
> Just my $0.02.
Thank you for the comment.
I think it's quite reasonable.
By doing that, we don't need end_io_error() anymore.
Jens,
What do you think?
If you agree with the interface change above, I would prefer to
separate the patch-set from blk_end_request patch-set like below:
o blk_end_request: remove end_that_request_*
o change interface of 'uptodate' in blk_end_request to 'error'
It makes the purpose of blk_end_request patch-set clear
(and also, each patch of device drivers could be smaller).
But, it doubles your merging work. So if you would like to get
the changes at once, I'll merge them into blk_end_request patch-set.
As for the patch inclusion, do you push the driver changes to Linus
all at once? Or should I ask each maintainer to take the patch?
Thanks,
Kiyoshi Ueda
On Tue, Dec 04 2007, Kiyoshi Ueda wrote:
> Hi Boaz and Jens,
>
> On Tue, 04 Dec 2007 15:56:32 +0200, Boaz Harrosh <[email protected]> wrote:
> > > +/**
> > > + * blk_end_request - Helper function for drivers to complete the request.
> > > + * @rq: the request being processed
> > > + * @uptodate: 1 for success, 0 for I/O error, < 0 for specific error
> > > + * @nr_bytes: number of bytes to complete
> > > + *
> > > + * Description:
> > > + * Ends I/O on a number of bytes attached to @rq.
> > > + * If @rq has leftover, sets it up for the next range of segments.
> > > + *
> > > + * Return:
> > > + * 0 - we are done with this request
> > > + * 1 - still buffers pending for this request
> > > + **/
> > > +int blk_end_request(struct request *rq, int uptodate, int nr_bytes)
> >
> > I always hated that uptodate boolean with possible negative error value.
> > I guess it was done for backward compatibility of then users of
> > end_that_request_first(). But since you are introducing a new API then
> > this is not the case. Just have regular status int where 0 means ALL_OK
> > and negative value means error code.
> > Just my $0.02.
>
> Thank you for the comment.
> I think it's quite reasonable.
> By doing that, we don't need end_io_error() anymore.
>
>
> Jens,
> What do you think?
I agree, the uptodate usage right now is horrible.
> If you agree with the interface change above, I would prefer to
> separate the patch-set from blk_end_request patch-set like below:
> o blk_end_request: remove end_that_request_*
> o change interface of 'uptodate' in blk_end_request to 'error'
> It makes the purpose of blk_end_request patch-set clear
> (and also, each patch of device drivers could be smaller).
> But, it doubles your merging work. So if you would like to get
> the changes at once, I'll merge them into blk_end_request patch-set.
Twice the merging is not an issue for me.
> As for the patch inclusion, do you push the driver changes to Linus
> all at once? Or should I ask each maintainer to take the patch?
Lets just try to get as many maintainer acks as possible, since the
patches need to go in together.
--
Jens Axboe