[PATCH] Always pass result and sense on request completion

[PATCH] Always pass result and sense on request completion

[Hannes Reinecke]

Currently we're passing the SCSI result and sense
code only for BLK_PC commands. However, some
instances up the stack might be interested
in them, too. So we can as well pass the
result and a possible sense code with every
request.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/scsi_lib.c |   28 ++++++++++++----------------
 1 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index cc0a06f..10aa084 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -722,23 +722,19 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 			sense_deferred = scsi_sense_is_deferred(&sshdr);
 	}
 
-	if (blk_pc_request(req)) { /* SG_IO ioctl from block level */
-		req->errors = result;
-		if (result) {
-			if (sense_valid && req->sense) {
-				/*
-				 * SG_IO wants current and deferred errors
-				 */
-				int len = 8 + cmd->sense_buffer[7];
+	req->errors = result;
+	if (sense_valid && req->sense) {
+		int len = 8 + cmd->sense_buffer[7];
+
+		if (len > SCSI_SENSE_BUFFERSIZE)
+			len = SCSI_SENSE_BUFFERSIZE;
+		memcpy(req->sense, cmd->sense_buffer,  len);
+		req->sense_len = len;
+	}
 
-				if (len > SCSI_SENSE_BUFFERSIZE)
-					len = SCSI_SENSE_BUFFERSIZE;
-				memcpy(req->sense, cmd->sense_buffer,  len);
-				req->sense_len = len;
-			}
-			if (!sense_deferred)
-				error = -EIO;
-		}
+	if (blk_pc_request(req)) { /* SG_IO ioctl from block level */
+		if ((result) && (!sense_deferred))
+			error = -EIO;
 
 		req->resid_len = scsi_get_resid(cmd);
 
-- 
1.5.3.2

Re: [PATCH] Always pass result and sense on request completion

[Hannes Reinecke]

Hi James,

would you mind commenting on this patch?
We really need this if we ever want to be able to do proper error code
handling from multipath.

Hannes Reinecke wrote:
> Currently we're passing the SCSI result and sense
> code only for BLK_PC commands. However, some
> instances up the stack might be interested
> in them, too. So we can as well pass the
> result and a possible sense code with every
> request.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/scsi/scsi_lib.c |   28 ++++++++++++----------------
>  1 files changed, 12 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index cc0a06f..10aa084 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -722,23 +722,19 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
>  			sense_deferred = scsi_sense_is_deferred(&sshdr);
>  	}
>  
> -	if (blk_pc_request(req)) { /* SG_IO ioctl from block level */
> -		req->errors = result;
> -		if (result) {
> -			if (sense_valid && req->sense) {
> -				/*
> -				 * SG_IO wants current and deferred errors
> -				 */
> -				int len = 8 + cmd->sense_buffer[7];
> +	req->errors = result;
> +	if (sense_valid && req->sense) {
> +		int len = 8 + cmd->sense_buffer[7];
> +
> +		if (len > SCSI_SENSE_BUFFERSIZE)
> +			len = SCSI_SENSE_BUFFERSIZE;
> +		memcpy(req->sense, cmd->sense_buffer,  len);
> +		req->sense_len = len;
> +	}
>  
> -				if (len > SCSI_SENSE_BUFFERSIZE)
> -					len = SCSI_SENSE_BUFFERSIZE;
> -				memcpy(req->sense, cmd->sense_buffer,  len);
> -				req->sense_len = len;
> -			}
> -			if (!sense_deferred)
> -				error = -EIO;
> -		}
> +	if (blk_pc_request(req)) { /* SG_IO ioctl from block level */
> +		if ((result) && (!sense_deferred))
> +			error = -EIO;
>  
>  		req->resid_len = scsi_get_resid(cmd);
>  

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Markus Rex, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Always pass result and sense on request completion

[James Bottomley]

On Thu, 2009-12-10 at 10:49 +0100, Hannes Reinecke wrote:
> Hi James,
> 
> would you mind commenting on this patch?
> We really need this if we ever want to be able to do proper error code
> handling from multipath.

OK, not keen on the way you're setting req->errors.

Our big problem with FS requests is deferred or corrected errors
(basically we never want the FS to think we had a problem with them).  I
think it's currently OK because block tends to believe the returned
error code rather than req->errors ... I'd just hate it if that changed
and suddenly lots of stuff broke.

I think you're just looking for the sense data, so could you look at
that and not set req->errors?

James

Re: [dm-devel] Re: [PATCH] Always pass result and sense on request completion

[Mike Christie]

James Bottomley wrote:
> On Thu, 2009-12-10 at 10:49 +0100, Hannes Reinecke wrote:
>> Hi James,
>>
>> would you mind commenting on this patch?
>> We really need this if we ever want to be able to do proper error code
>> handling from multipath.
> 
> OK, not keen on the way you're setting req->errors.
> 
> Our big problem with FS requests is deferred or corrected errors
> (basically we never want the FS to think we had a problem with them).  I
> think it's currently OK because block tends to believe the returned
> error code rather than req->errors ... I'd just hate it if that changed
> and suddenly lots of stuff broke.
> 
> I think you're just looking for the sense data, so could you look at
> that and not set req->errors?
> 

For the specific bug Hannes is fixing we only need the sense code, so 
that would work. If you also pass info like the host_byte bits then we 
can do something like fail a path right away for a DID_TRANSPORT* or 
DID_NO_CONNECT failure, but then for other errors do something else. 
RAID could also not fail the drive on transport errors, and do something 
different too.

Re: [dm-devel] Re: [PATCH] Always pass result and sense on request completion

[James Bottomley]

On Thu, 2009-12-10 at 11:03 -0600, Mike Christie wrote:
> James Bottomley wrote:
> > On Thu, 2009-12-10 at 10:49 +0100, Hannes Reinecke wrote:
> >> Hi James,
> >>
> >> would you mind commenting on this patch?
> >> We really need this if we ever want to be able to do proper error code
> >> handling from multipath.
> > 
> > OK, not keen on the way you're setting req->errors.
> > 
> > Our big problem with FS requests is deferred or corrected errors
> > (basically we never want the FS to think we had a problem with them).  I
> > think it's currently OK because block tends to believe the returned
> > error code rather than req->errors ... I'd just hate it if that changed
> > and suddenly lots of stuff broke.
> > 
> > I think you're just looking for the sense data, so could you look at
> > that and not set req->errors?
> > 
> 
> For the specific bug Hannes is fixing we only need the sense code, so 
> that would work. If you also pass info like the host_byte bits then we 
> can do something like fail a path right away for a DID_TRANSPORT* or 
> DID_NO_CONNECT failure, but then for other errors do something else. 
> RAID could also not fail the drive on transport errors, and do something 
> different too.

So I have no trouble setting req->errors for genuine error cases (like
DID_NO_CONNECT).  I just don't want to set it for errors which are
deferred or corrected and then have something further up the stack do
the wrong thing because it thinks there was an error when, in fact,
there wasn't.

James

Re: [PATCH] Always pass result and sense on request completion

[Boaz Harrosh]

On 12/10/2009 06:44 PM, James Bottomley wrote:
> On Thu, 2009-12-10 at 10:49 +0100, Hannes Reinecke wrote:
>> Hi James,
>>
>> would you mind commenting on this patch?
>> We really need this if we ever want to be able to do proper error code
>> handling from multipath.
> 
> OK, not keen on the way you're setting req->errors.
> 
> Our big problem with FS requests is deferred or corrected errors
> (basically we never want the FS to think we had a problem with them).  I
> think it's currently OK because block tends to believe the returned
> error code rather than req->errors ... I'd just hate it if that changed
> and suddenly lots of stuff broke.
> 

>From what I understood, req->errors is for private block-driver use and has
no meaning to the block layer. It expects a translation of req->errors to
a Linux error code passed to the blk_end_request which will be set to the
bio and passed to the async_done function. Only the block-driver understand
the format of req->errors.

Perhaps we must make sure there is an agreement between
	(returned-error == 0) == (req->errors == 0)

I know scsi-ml makes sure of that, so should the device manager.

> I think you're just looking for the sense data, so could you look at
> that and not set req->errors?
> 

I agree that the req->errors bits where not understood outside of scsi
up till now. Is multipath only compatible over scsi block devices?

> James
> 

Boaz

Re: Re: [PATCH] Always pass result and sense on request completion

[Mike Christie]

Boaz Harrosh wrote:
> 
> I agree that the req->errors bits where not understood outside of scsi
> up till now. Is multipath only compatible over scsi block devices?
> 

It works with any device that has a request_queue and implements 
queueing functions like the request_fn. It should work with DASD I think.