Re: [PATCH] libata error handling fixes (ATAPI)

[Jens Axboe <axboe@suse.de>]

On Wed, Nov 16 2005, Jens Axboe wrote:
> which of course didn't work, so it was changed to the above which then
> broke the assumption of what type of requests we expect to see in
> ide_softirq_done(). We can't generically handle this case, so it's
> probably best to just add this logic to __ide_end_request() - it's just
> another case for _not_ using the blk_complete_request() path, just like
> the partial case.

How about something like this? It requires blk_complete_request() calls
to be full completions of the request (or what is left of it). It cleans
up the calls a lot and fixes the wrong comment for IDE.

diff --git a/block/ll_rw_blk.c b/block/ll_rw_blk.c
index 82b7290..8e6dd9f 100644
--- a/block/ll_rw_blk.c
+++ b/block/ll_rw_blk.c
@@ -3223,33 +3223,20 @@ static struct notifier_block __devinitda
 /**
  * blk_complete_request - end I/O on a request
  * @req:      the request being processed
- * @uptodate: 1 for success, 0 for I/O error, < 0 for specific error
- * @nbytes:   number of bytes to end I/O on
  *
  * Description:
- *     Ends I/O on a number of sectors attached to @req, and sets it up
- *     for the next range of segments (if any) in the cluster. This variant
- *     completes the request out-of-order through a softirq handler. The user
- *     must have registered a completion callback through
- *     blk_queue_softirq_fn() at queue init time.
- *
- * Return:
- *     0 - we are done with this request, call end_that_request_last()
- *     1 - still buffers pending for this request
+ *     Ends all I/O on a request. It does not handle partial completions,
+ *     unless the driver actually implements this in its completionc callback
+ *     through requeueing. Theh actual completion happens out-of-order,
+ *     through a softirq handler. The user must have registered a completion
+ *     callback through blk_queue_softirq_done().
  **/
-int blk_complete_request(struct request *req, int uptodate, unsigned int nbytes,
-			 int partial_ok)
+
+void blk_complete_request(struct request *req)
 {
 	struct list_head *cpu_list;
 	unsigned long flags;
 
-	/*
-	 * for partial completions, fall back to normal end io handling.
-	 */
-	if (unlikely(!partial_ok && !rq_all_done(req, nbytes)))
-		if (end_that_request_chunk(req, uptodate, nbytes))
-			return 1;
-
 	BUG_ON(!req->q->softirq_done_fn);
 		
 	local_irq_save(flags);
@@ -3259,7 +3246,6 @@ int blk_complete_request(struct request 
 	raise_softirq_irqoff(BLOCK_SOFTIRQ);
 
 	local_irq_restore(flags);
-	return 0;
 }
 
 EXPORT_SYMBOL(blk_complete_request);
diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
index f114106..1129bfb 100644
--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -58,22 +58,9 @@
 void ide_softirq_done(struct request *rq)
 {
 	request_queue_t *q = rq->q;
-	int nbytes, uptodate = 1;
 
 	add_disk_randomness(rq->rq_disk);
-
-	/*
-	 * we know it's either an FS or PC request
-	 */
-	if (blk_fs_request(rq))
-		nbytes = rq->hard_nr_sectors << 9;
-	else
-		nbytes = rq->data_len;
-
-	if (rq->errors < 0)
-		uptodate = rq->errors;
-
-	end_that_request_chunk(rq, uptodate, nbytes);
+	end_that_request_chunk(rq, rq->errors, rq->data_len);
 
 	spin_lock_irq(q->queue_lock);
 	end_that_request_last(rq);
@@ -83,6 +70,9 @@ void ide_softirq_done(struct request *rq
 int __ide_end_request(ide_drive_t *drive, struct request *rq, int uptodate,
 		      int nr_sectors)
 {
+	unsigned int nbytes;
+	int ret = 1;
+
 	BUG_ON(!(rq->flags & REQ_STARTED));
 
 	/*
@@ -104,13 +94,30 @@ int __ide_end_request(ide_drive_t *drive
 		HWGROUP(drive)->hwif->ide_dma_on(drive);
 	}
 
-	if (!blk_complete_request(rq, uptodate, nr_sectors << 9, 0)) {
+	/*
+	 * For partial completions (or non fs/pc requests), use the regular
+	 * direct completion path.
+	 */
+	nbytes = nr_sectors << 9;
+	if (rq_all_done(rq, nbytes)) {
+		rq->errors = uptodate;
+		rq->data_len = nbytes;
+		blk_complete_request(rq);
+		ret = 0;
+	} else {
+		if (!end_that_request_first(rq, uptodate, nr_sectors)) {
+			add_disk_randomness(rq->rq_disk);
+			end_that_request_last(rq);
+			ret = 0;
+		}
+	}
+
+	if (!ret) {
 		blkdev_dequeue_request(rq);
 		HWGROUP(drive)->rq = NULL;
-		return 0;
 	}
 
-	return 1;
+	return ret;
 }
 EXPORT_SYMBOL(__ide_end_request);
 
diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 4044ba4..bf902cf 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -751,6 +751,8 @@ static void scsi_done(struct scsi_cmnd *
  * isn't running --- used by scsi_times_out */
 void __scsi_done(struct scsi_cmnd *cmd)
 {
+	struct request *rq = cmd->request;
+
 	/*
 	 * Set the serial numbers back to zero
 	 */
@@ -760,14 +762,14 @@ void __scsi_done(struct scsi_cmnd *cmd)
 	if (cmd->result)
 		atomic_inc(&cmd->device->ioerr_cnt);
 
-	BUG_ON(!cmd->request);
+	BUG_ON(!rq);
 
 	/*
 	 * The uptodate/nbytes values don't matter, as we allow partial
 	 * completes and thus will check this in the softirq callback
 	 */
-	cmd->request->completion_data = cmd;
-	blk_complete_request(cmd->request, 0, 0, 1);
+	rq->completion_data = cmd;
+	blk_complete_request(rq);
 }
 
 /*
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 588b9cc..3ff7c3f 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -613,7 +613,7 @@ extern int end_that_request_first(struct
 extern int end_that_request_chunk(struct request *, int, int);
 extern void end_that_request_last(struct request *);
 extern void end_request(struct request *req, int uptodate);
-extern int blk_complete_request(struct request *, int, unsigned int, int);
+extern void blk_complete_request(struct request *);
 
 static inline int rq_all_done(struct request *rq, unsigned int nr_bytes)
 {

-- 
Jens Axboe