Re: [PATCH 24/28] blk_end_request: changing ide normal caller (take 3)
Hi Bartlomiej, On Tue, 4 Dec 2007 14:47:00 +0100, Bartlomiej Zolnierkiewicz wrote: > > > Hmmm, this seems to change the old behavior (the request should > > > be dequeued from the queue only if 'dequeue' variable is set) > > > and AFAIR some error handling code (in ide-cd?) depends on the > > > old behavior so please revisit this patch. > > > > blk_end_request() takes care of the dequeue like below, > > so I think no problem. (Please see PATCH 01) > > > > > + /* rq->queuelist of dequeued request should be list_empty() */ > > > + if (!list_empty(&rq->queuelist)) > > > + blkdev_dequeue_request(rq); > > > > In the case of ide-cd, > > o 'dequeue' variable is 1 only when the request is still linked > > to the queue (i.e. rq->queuelist is not empty) > > o 'dequeue' variable is 0 only when the request has already been > > removed from the queue (i.e. rq->queuelist is empty) > > So blk_end_request() can handle it correctly. > > It would be helpful to add the above explanation to a patch description. > > > If there are any drivers which don't want dequeue the queued request, > > the code above would not work. > > But, as far as I investigated, I have never seen such a requirement > > in device drivers. > > > > Do you think that ide may still gets a problem for the 'dequeue'? > > Everything seems to be fine now. > > Acked-by: Bartlomiej Zolnierkiewicz <[EMAIL PROTECTED]> Thank you for the check. OK, I'll add the explanation about the 'dequeue' to patch description. Thanks, Kiyoshi Ueda -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 24/28] blk_end_request: changing ide normal caller (take 3)
On Tuesday 04 December 2007, Kiyoshi Ueda wrote: > Hi Bartlomiej, > > On Sat, 1 Dec 2007 23:53:05 +0100, Bartlomiej Zolnierkiewicz <[EMAIL > PROTECTED]> wrote: > > On Saturday 01 December 2007, Kiyoshi Ueda wrote: > > > This patch converts "normal" parts of ide to use blk_end_request(). > > > > > > Signed-off-by: Kiyoshi Ueda <[EMAIL PROTECTED]> > > > Signed-off-by: Jun'ichi Nomura <[EMAIL PROTECTED]> > > > --- > > > drivers/ide/ide-cd.c |6 +++--- > > > drivers/ide/ide-io.c | 17 ++--- > > > 2 files changed, 9 insertions(+), 14 deletions(-) > > > > [...] > > > > > Index: 2.6.24-rc3-mm2/drivers/ide/ide-io.c > > > === > > > --- 2.6.24-rc3-mm2.orig/drivers/ide/ide-io.c > > > +++ 2.6.24-rc3-mm2/drivers/ide/ide-io.c > > > @@ -78,14 +78,9 @@ static int __ide_end_request(ide_drive_t > > > ide_dma_on(drive); > > > } > > > > > > - if (!end_that_request_chunk(rq, uptodate, nr_bytes)) { > > > - add_disk_randomness(rq->rq_disk); > > > - if (dequeue) { > > > - if (!list_empty(&rq->queuelist)) > > > - blkdev_dequeue_request(rq); > > > + if (!__blk_end_request(rq, uptodate, nr_bytes)) { > > > + if (dequeue) > > > HWGROUP(drive)->rq = NULL; > > > - } > > > - end_that_request_last(rq, uptodate); > > > ret = 0; > > > } > > > > Hmmm, this seems to change the old behavior (the request should > > be dequeued from the queue only if 'dequeue' variable is set) > > and AFAIR some error handling code (in ide-cd?) depends on the > > old behavior so please revisit this patch. > > blk_end_request() takes care of the dequeue like below, > so I think no problem. (Please see PATCH 01) > > > + /* rq->queuelist of dequeued request should be list_empty() */ > > + if (!list_empty(&rq->queuelist)) > > + blkdev_dequeue_request(rq); > > In the case of ide-cd, > o 'dequeue' variable is 1 only when the request is still linked > to the queue (i.e. rq->queuelist is not empty) > o 'dequeue' variable is 0 only when the request has already been > removed from the queue (i.e. rq->queuelist is empty) > So blk_end_request() can handle it correctly. It would be helpful to add the above explanation to a patch description. > If there are any drivers which don't want dequeue the queued request, > the code above would not work. > But, as far as I investigated, I have never seen such a requirement > in device drivers. > > Do you think that ide may still gets a problem for the 'dequeue'? Everything seems to be fine now. Acked-by: Bartlomiej Zolnierkiewicz <[EMAIL PROTECTED]> -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 24/28] blk_end_request: changing ide normal caller (take 3)
Hi Bartlomiej, On Sat, 1 Dec 2007 23:53:05 +0100, Bartlomiej Zolnierkiewicz <[EMAIL PROTECTED]> wrote: > On Saturday 01 December 2007, Kiyoshi Ueda wrote: > > This patch converts "normal" parts of ide to use blk_end_request(). > > > > Signed-off-by: Kiyoshi Ueda <[EMAIL PROTECTED]> > > Signed-off-by: Jun'ichi Nomura <[EMAIL PROTECTED]> > > --- > > drivers/ide/ide-cd.c |6 +++--- > > drivers/ide/ide-io.c | 17 ++--- > > 2 files changed, 9 insertions(+), 14 deletions(-) > > [...] > > > Index: 2.6.24-rc3-mm2/drivers/ide/ide-io.c > > === > > --- 2.6.24-rc3-mm2.orig/drivers/ide/ide-io.c > > +++ 2.6.24-rc3-mm2/drivers/ide/ide-io.c > > @@ -78,14 +78,9 @@ static int __ide_end_request(ide_drive_t > > ide_dma_on(drive); > > } > > > > - if (!end_that_request_chunk(rq, uptodate, nr_bytes)) { > > - add_disk_randomness(rq->rq_disk); > > - if (dequeue) { > > - if (!list_empty(&rq->queuelist)) > > - blkdev_dequeue_request(rq); > > + if (!__blk_end_request(rq, uptodate, nr_bytes)) { > > + if (dequeue) > > HWGROUP(drive)->rq = NULL; > > - } > > - end_that_request_last(rq, uptodate); > > ret = 0; > > } > > Hmmm, this seems to change the old behavior (the request should > be dequeued from the queue only if 'dequeue' variable is set) > and AFAIR some error handling code (in ide-cd?) depends on the > old behavior so please revisit this patch. blk_end_request() takes care of the dequeue like below, so I think no problem. (Please see PATCH 01) > + /* rq->queuelist of dequeued request should be list_empty() */ > + if (!list_empty(&rq->queuelist)) > + blkdev_dequeue_request(rq); In the case of ide-cd, o 'dequeue' variable is 1 only when the request is still linked to the queue (i.e. rq->queuelist is not empty) o 'dequeue' variable is 0 only when the request has already been removed from the queue (i.e. rq->queuelist is empty) So blk_end_request() can handle it correctly. If there are any drivers which don't want dequeue the queued request, the code above would not work. But, as far as I investigated, I have never seen such a requirement in device drivers. Do you think that ide may still gets a problem for the 'dequeue'? Thanks, Kiyoshi Ueda -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 24/28] blk_end_request: changing ide normal caller (take 3)
On Saturday 01 December 2007, Kiyoshi Ueda wrote: > This patch converts "normal" parts of ide to use blk_end_request(). > > Signed-off-by: Kiyoshi Ueda <[EMAIL PROTECTED]> > Signed-off-by: Jun'ichi Nomura <[EMAIL PROTECTED]> > --- > drivers/ide/ide-cd.c |6 +++--- > drivers/ide/ide-io.c | 17 ++--- > 2 files changed, 9 insertions(+), 14 deletions(-) [...] > Index: 2.6.24-rc3-mm2/drivers/ide/ide-io.c > === > --- 2.6.24-rc3-mm2.orig/drivers/ide/ide-io.c > +++ 2.6.24-rc3-mm2/drivers/ide/ide-io.c > @@ -78,14 +78,9 @@ static int __ide_end_request(ide_drive_t > ide_dma_on(drive); > } > > - if (!end_that_request_chunk(rq, uptodate, nr_bytes)) { > - add_disk_randomness(rq->rq_disk); > - if (dequeue) { > - if (!list_empty(&rq->queuelist)) > - blkdev_dequeue_request(rq); > + if (!__blk_end_request(rq, uptodate, nr_bytes)) { > + if (dequeue) > HWGROUP(drive)->rq = NULL; > - } > - end_that_request_last(rq, uptodate); > ret = 0; > } Hmmm, this seems to change the old behavior (the request should be dequeued from the queue only if 'dequeue' variable is set) and AFAIR some error handling code (in ide-cd?) depends on the old behavior so please revisit this patch. Thanks, Bart -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 24/28] blk_end_request: changing ide normal caller (take 3)
This patch converts "normal" parts of ide to use blk_end_request(). Signed-off-by: Kiyoshi Ueda <[EMAIL PROTECTED]> Signed-off-by: Jun'ichi Nomura <[EMAIL PROTECTED]> --- drivers/ide/ide-cd.c |6 +++--- drivers/ide/ide-io.c | 17 ++--- 2 files changed, 9 insertions(+), 14 deletions(-) Index: 2.6.24-rc3-mm2/drivers/ide/ide-cd.c === --- 2.6.24-rc3-mm2.orig/drivers/ide/ide-cd.c +++ 2.6.24-rc3-mm2/drivers/ide/ide-cd.c @@ -655,9 +655,9 @@ static void cdrom_end_request (ide_drive BUG(); } else { spin_lock_irqsave(&ide_lock, flags); - end_that_request_chunk(failed, 0, - failed->data_len); - end_that_request_last(failed, 0); + if (__blk_end_request(failed, 0, + failed->data_len)) + BUG(); spin_unlock_irqrestore(&ide_lock, flags); } } else Index: 2.6.24-rc3-mm2/drivers/ide/ide-io.c === --- 2.6.24-rc3-mm2.orig/drivers/ide/ide-io.c +++ 2.6.24-rc3-mm2/drivers/ide/ide-io.c @@ -78,14 +78,9 @@ static int __ide_end_request(ide_drive_t ide_dma_on(drive); } - if (!end_that_request_chunk(rq, uptodate, nr_bytes)) { - add_disk_randomness(rq->rq_disk); - if (dequeue) { - if (!list_empty(&rq->queuelist)) - blkdev_dequeue_request(rq); + if (!__blk_end_request(rq, uptodate, nr_bytes)) { + if (dequeue) HWGROUP(drive)->rq = NULL; - } - end_that_request_last(rq, uptodate); ret = 0; } @@ -290,9 +285,9 @@ static void ide_complete_pm_request (ide drive->blocked = 0; blk_start_queue(drive->queue); } - blkdev_dequeue_request(rq); HWGROUP(drive)->rq = NULL; - end_that_request_last(rq, 1); + if (__blk_end_request(rq, 1, 0)) + BUG(); spin_unlock_irqrestore(&ide_lock, flags); } @@ -402,10 +397,10 @@ void ide_end_drive_cmd (ide_drive_t *dri } spin_lock_irqsave(&ide_lock, flags); - blkdev_dequeue_request(rq); HWGROUP(drive)->rq = NULL; rq->errors = err; - end_that_request_last(rq, !rq->errors); + if (__blk_end_request(rq, !rq->errors, 0)) + BUG(); spin_unlock_irqrestore(&ide_lock, flags); } - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/