block: fix barrier error transmission

There's a problem with barriers in SCSI. They're prepared as a SYNCHRONIZE_CACHE REQ_TYPE_BLOCK_PC command. However, our processing model dictates that we always return no block error but set req->errors to the SCSI result. The upshot is that if we get an error on a flush barrier, it's never seen by the block layer.

This patch attempts to get the block layer to see such errors. Note, it's still not quite right. There are possible sense errors that could indicate success of the command (like deferred errors) that this will treat as failures. I suppose we could process the sense in scsi and try to return a first pass error even for REQ_TYPE_BLOCK_PC ... I'm just not sure what that will do to the other users.

James

---

block/blk-barrier.c +0 -0

index 55c5f1f..3a3947c 100644

block/blk-barrier.c void blk_ordered_complete_seq(struct request_queue *q, unsigned seq, int error) · line 121

Quoted 13 lines

block/blk-barrier.c +2 -0

index 55c5f1f..3a3947c 100644
@@ -114,18 +114,24 @@ void blk_ordered_complete_seq(struct request_queue *q, unsigned seq, int error)
 
 static void pre_flush_end_io(struct request *rq, int error)
 {
+	error = rq->errors ? -EIO : error;
+
 	elv_completed_request(rq->q, rq);
 	blk_ordered_complete_seq(rq->q, QUEUE_ORDSEQ_PREFLUSH, error);
 }

It's a bit of a hack, SCSI really should pass the error value back instead of fiddling around with possibly perhaps finding it in ->errors. And please don't use these ?: constructs, in this case it doesn't even make a lot of sense and a

        if (rq->errors)
                error = -EIO;

would have been much cleaner ;-)

So my question is why does the model not allow you to return the error properly?

Quoted 19 lines · Jens Axboe

block/blk-barrier.c void blk_ordered_complete_seq(struct request_queue *q, unsigned seq, int error) · line 121

Quoted 13 lines

block/blk-barrier.c +2 -0

index 55c5f1f..3a3947c 100644
@@ -114,18 +114,24 @@ void blk_ordered_complete_seq(struct request_queue *q, unsigned seq, int error)
 
 static void pre_flush_end_io(struct request *rq, int error)
 {
+	error = rq->errors ? -EIO : error;
+
 	elv_completed_request(rq->q, rq);
 	blk_ordered_complete_seq(rq->q, QUEUE_ORDSEQ_PREFLUSH, error);
 }

It's a bit of a hack, SCSI really should pass the error value back instead of fiddling around with possibly perhaps finding it in ->errors. And please don't use these ?: constructs, in this case it doesn't even make a lot of sense and a

Quoted 7 lines

        if (rq->errors)
                error = -EIO;

would have been much cleaner ;-)

So my question is why does the model not allow you to return the error properly?

I thought it was the sg_io that would be the problem, but apparently on further research, it simply discards the error as does scsi_execute_req.

I suppose that's a strong enough reason to try returning an error ... I'm just a bit leery this close to a release.

I think this will work ... it just really needs quite a bit of testing ...

James

---

drivers/scsi/scsi_lib.c +4 -6

index 1b199e1..67f412b 100644
@@ -839,7 +839,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 	int this_count = scsi_bufflen(cmd);
 	struct request_queue *q = cmd->device->request_queue;
 	struct request *req = cmd->request;
-	int clear_errors = 1;
+	int error = 0;
 	struct scsi_sense_hdr sshdr;
 	int sense_valid = 0;
 	int sense_deferred = 0;

@@ -853,7 +853,6 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 	if (blk_pc_request(req)) { /* SG_IO ioctl from block level */
 		req->errors = result;
 		if (result) {
-			clear_errors = 0;
 			if (sense_valid && req->sense) {
 				/*
 				 * SG_IO wants current and deferred errors

@@ -865,6 +864,8 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 				memcpy(req->sense, cmd->sense_buffer,  len);
 				req->sense_len = len;
 			}
+			if (!sense_deferred)
+				error = -EIO;
 		}
 		if (scsi_bidi_cmnd(cmd)) {
 			/* will also release_buffers */

@@ -885,14 +886,11 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 				      "%d bytes done.\n",
 				      req->nr_sectors, good_bytes));
 
-	if (clear_errors)
-		req->errors = 0;
-
 	/* A number of bytes were successfully read.  If there
 	 * are leftovers and there is some kind of error
 	 * (result != 0), retry the rest.
 	 */
-	if (scsi_end_request(cmd, 0, good_bytes, result == 0) == NULL)
+	if (scsi_end_request(cmd, error, good_bytes, result == 0) == NULL)
 		return;
 
 	/* good_bytes = 0, or (inclusive) there were leftovers and

Quoted 31 lines · James Bottomley

Quoted 19 lines · Jens Axboe

block/blk-barrier.c void blk_ordered_complete_seq(struct request_queue *q, unsigned seq, int error) · line 121

Quoted 13 lines

block/blk-barrier.c +2 -0

index 55c5f1f..3a3947c 100644
@@ -114,18 +114,24 @@ void blk_ordered_complete_seq(struct request_queue *q, unsigned seq, int error)
 
 static void pre_flush_end_io(struct request *rq, int error)
 {
+	error = rq->errors ? -EIO : error;
+
 	elv_completed_request(rq->q, rq);
 	blk_ordered_complete_seq(rq->q, QUEUE_ORDSEQ_PREFLUSH, error);
 }

It's a bit of a hack, SCSI really should pass the error value back instead of fiddling around with possibly perhaps finding it in ->errors. And please don't use these ?: constructs, in this case it doesn't even make a lot of sense and a

Quoted 7 lines

        if (rq->errors)
                error = -EIO;

would have been much cleaner ;-)

So my question is why does the model not allow you to return the error properly?

I thought it was the sg_io that would be the problem, but apparently on further research, it simply discards the error as does scsi_execute_req.

Quoted 5 lines

I suppose that's a strong enough reason to try returning an error ... I'm just a bit leery this close to a release.

I think this will work ... it just really needs quite a bit of testing ...

This looks much better, but I'm with you on the danger of applying something like this so close to a release...

Now, this isn't a regression, but it also impacts barrier reliability and as such it's a big nasty to leave this open for another release.

Quoted 32 lines · Jens Axboe

Quoted 31 lines · James Bottomley

Quoted 19 lines · Jens Axboe

block/blk-barrier.c void blk_ordered_complete_seq(struct request_queue *q, unsigned seq, int error) · line 121

Quoted 13 lines

block/blk-barrier.c +2 -0

index 55c5f1f..3a3947c 100644
@@ -114,18 +114,24 @@ void blk_ordered_complete_seq(struct request_queue *q, unsigned seq, int error)
 
 static void pre_flush_end_io(struct request *rq, int error)
 {
+	error = rq->errors ? -EIO : error;
+
 	elv_completed_request(rq->q, rq);
 	blk_ordered_complete_seq(rq->q, QUEUE_ORDSEQ_PREFLUSH, error);
 }

It's a bit of a hack, SCSI really should pass the error value back instead of fiddling around with possibly perhaps finding it in ->errors. And please don't use these ?: constructs, in this case it doesn't even make a lot of sense and a

Quoted 7 lines

        if (rq->errors)
                error = -EIO;

would have been much cleaner ;-)

So my question is why does the model not allow you to return the error properly?

I thought it was the sg_io that would be the problem, but apparently on further research, it simply discards the error as does scsi_execute_req.

Quoted 8 lines

Quoted 5 lines

I suppose that's a strong enough reason to try returning an error ... I'm just a bit leery this close to a release.

I think this will work ... it just really needs quite a bit of testing ...

This looks much better, but I'm with you on the danger of applying something like this so close to a release...

Yes.

Quoted 2 lines

Now, this isn't a regression, but it also impacts barrier reliability and as such it's a big nasty to leave this open for another release.

Yes, I agree ... let's put it in after 2.6.25 (so into scsi-misc) but if no problems turn up by -rc2 say, I'll send it as a backport to stable 2.6.25.X. That way we don't have to wait out the entire release cycle for users to see the benefit.