Re: [PATCH 01/28] blk_end_request: add new request completion interface (take 3)

2007-12-05 Thread Jens Axboe 
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

-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 01/28] blk_end_request: add new request completion interface (take 3)

2007-12-04 Thread Boaz Harrosh 
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

Re: [PATCH 01/28] blk_end_request: add new request completion interface (take 3)

2007-12-04 Thread Kiyoshi Ueda 
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-kernelm=118880584720600w=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
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 01/28] blk_end_request: add new request completion interface (take 3)

2007-12-04 Thread 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
-
To unsubscribe from this list: send the line unsubscribe linux-scsi in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html