sd: make error handling more robust

From: Tony Battersby <[email protected]>
Subject: [PATCH] [RFC] sd: make error handling more robust
To: "James Bottomley" <[email protected]>, "Luben Tuikov" <[email protected]>, [email protected]
Date: Thursday, January 31, 2008, 1:31 PM
I have a RAID that returns a medium error on a read command.
The
"information bytes valid" bit is set in the sense
data, but the
information bytes are zero:

CDB: 28 00 02 B0 62 00 00 00 02 00
Status: 02 (CHECK CONDITION)
Sense data:
F0 00 03 00 | 00 00 00 0A | 00 00 00 00 | 00 00 00 00 00 00

For HARDWARE_ERROR/MEDIUM_ERROR, sd_done assumes that the sense
information bytes contain the sector in error without sanity-checking
the value, so it ends up returning a completely bogus good_bytes value
greater than xfer_size.

This in turn causes the medium
error to be
ignored and corrupt data to be returned to the user application. This
problem was introduced by the patch "[SCSI] sd/scsi_lib simplify
sd_rw_intr and scsi_io_completion" in 2.6.18-rc1; with kernels 2.6.17
and before, the application correctly gets an I/O error instead. This
patch fixes this particular problem by checking that bad_lba falls
within a sensible range before using it.

For a read command that returns HARDWARE_ERROR/MEDIUM_ERROR, I also
added the ability to calculate the amount of good data returned using
the data transfer residual if set by the LLDD. If the both the sense
information bytes and the data transfer residual are valid, then they
are used to sanity-check each other.

The following code in sd_done doesn't seem right to me:

       if (driver_byte(result) != DRIVER_SENSE &&
           (!sense_valid || sense_deferred))
                goto out;

It would make more sense to use || rather than &&
for this test.

Even
better, this patch moves the check up higher so that the sense buffer
isn't even accessed unless driver_byte(result) & DRIVER_SENSE.

Finally, for the case of RECOVERED_ERROR/NO_SENSE, this patch adds a
check of the data transfer residual before assuming that the command
completed successfully.

I would like comments on the following:

sd_done doesn't check the data transfer residual for commands that
complete successfully. In the unlikely case that the drive returns good
status without transferring all the data (due to a SCSI bus problem or
disk firmware bug), the error won't be detected. I figured that it was
more likely for a LLDD to set resid incorrectly than for this unlikely
problem to happen, so I didn't add a check for it. Agreed?

Is the new is_sd_cmnd_read_or_write() function necessary? The original
code appears to use blk_fs_request(SCpnt->request) to determine if a
command is read or write. Should I drop is_sd_cmnd_read_or_write() and
use blk_fs_request() instead, or it it OK like this?

Does anyone object to the new data transfer residual checks for
non-read/write commands that return RECOVERED_ERROR/NO_SENSE? Should I
just drop this part of the patch for simplicity?

Negate original:

    if (driver_byte(result) == DRIVER_SENSE ||
        (sense_valid && sense_defered))
            Inspect sense.

Negate your proposed change "&&" ->
"||":

    if (driver_byte(result) == DRIVER_SENSE &&
         (sense_valid || sense_deferred))
                Inspect sense.

Serendipity, been working an issue for the past week where error reporting from aacraid's HARDWARE_ERROR when an array was marked DEAD in the Adapter was not propagating to MD. The trigger for this investigation was "AACRAID driver broken in 2.6.22.x (and beyond?) [WAS: Re: 2.6.22.16 MD raid1 doesn't mark removed disk faulty, MD thread goes UN]" as authored my Mike Snitzer.

I had attacked this yesterday by setting the deferred bit (0xF1) rather than invalidating the sense data (0x70) in the LLD (Patch to the aacraid would be forthcoming following some testing). The Valid bit in the SCSI layer sometimes (too many kernels, I am talking generically) throws away all the content and keys of the sense data, at least deferred appears to report that the info does not accompany the sense data.

The change needs to be SCSI compliant and possibly endure back-porting to other kernels. So where are we going to put this, in each respective LLD (invalidating info, but not the keys), in sd.c (where the bad_lba is calculated) or in scsi_error.c (where the info field is picked up)?

Sincerely -- Mark Salyzyn

= 2^32.

This is due to the fact that the LU/LLDD returned sense data with VALID = 1, and so Bad LBA is assumed correct. This is as per spec. At this moment given the situation you describe it seems that the LU/LLDD is behaving out of spec.

As to sanity checking, given the inequality above, and your peculiar target/LLDD returning broken sense data, a sanity check would be the following:

if (bad_lba < start_lba)

         goto out;

Added immediately before the computation of good_bytes.

Quoted 9 lines

The following code in sd_done doesn't seem right to me:

       if (driver_byte(result) != DRIVER_SENSE &&
           (!sense_valid || sense_deferred))
                goto out;

It would make more sense to use || rather than &&
for this test.

That's very easy to verify. Simply negate the test and see if what you get makes sense. Case in point:

Negate original:

    if (driver_byte(result) == DRIVER_SENSE ||
        (sense_valid && !sense_defered))
            Inspect sense.

Negate your proposed change "&&" -> "||":

    if (driver_byte(result) == DRIVER_SENSE &&
         (sense_valid && !sense_deferred))
                Inspect sense.

The reason to use the former, rather the latter, is that LLDDs don't always set the driver_byte in the result, and may only set the status byte. E.g. to indicate CHECK CONDITION and then using scsi_command_normalize_sense() to find out if sense data is present.

Quoted 6 lines

Finally, for the case of RECOVERED_ERROR/NO_SENSE, this patch adds a
check of the data transfer residual before assuming that the command
completed successfully.

You cannot do this for NO SENSE SK. You can do that for RECOVERED ERROR SK. The way to do this is to move "case RECOVERED_ERROR:" just above "case HARDWARE_ERROR:".

Luben

Quoted 7 lines · Luben Tuikov

This is due to the fact that the LU/LLDD returned sense data with VALID = 1, and so Bad LBA is assumed correct. This is as per spec. At this moment given the situation you describe it seems that the LU/LLDD is behaving out of spec.

I agree that my RAID (an external SCSI RAID connected via LSI Ultra320 HBA) is out-of-spec. It should set VALID = 0 if the sense information bytes are invalid. I also agree that my patch is geared toward making the kernel more robust when faced with out-of-spec devices.

Quoted 11 lines

As to sanity checking, given the inequality above, and your peculiar target/LLDD returning broken sense data, a sanity check would be the following:

if (bad_lba < start_lba)

         goto out;

Added immediately before the computation of good_bytes.

I agree that the simple check that you suggest would handle the particular case of my out-of-spec RAID. However, since we are sanity-checking bad_lba, why not also add a check for the upper bound too, in case there are other disks that are out-of-spec in a different way? I want to guarantee that sd_done() always returns a valid good_bytes value regardless of what the sense data contains.

Quoted 24 lines

Quoted 9 lines

The following code in sd_done doesn't seem right to me:

       if (driver_byte(result) != DRIVER_SENSE &&
           (!sense_valid || sense_deferred))
                goto out;

It would make more sense to use || rather than &&
for this test.

That's very easy to verify. Simply negate the test and see if what you get makes sense. Case in point:

Negate original:

    if (driver_byte(result) == DRIVER_SENSE ||
        (sense_valid && !sense_defered))
            Inspect sense.

Negate your proposed change "&&" -> "||":

    if (driver_byte(result) == DRIVER_SENSE &&
         (sense_valid && !sense_deferred))
                Inspect sense.

Quoted 8 lines

The reason to use the former, rather the latter, is that LLDDs don't always set the driver_byte in the result, and may only set the status byte. E.g. to indicate CHECK CONDITION and then using scsi_command_normalize_sense() to find out if sense data is present.

(Note: I edited the logic tests in the quote above.)

Here is the problem I see with the original: suppose the driver did retrieve the sense data and set driver_byte(result) to DRIVER_SENSE, but the sense data is invalid (corrupted on the wire, garbled by PCI, etc.). Then scsi_command_normalize_sense() will return sense_valid =
0. In that case, the original code will check the invalid sense data instead of bailing out (driver_byte(result) == DRIVER_SENSE, sense_valid == 0, sense_deferred == 0). Since scsi_command_normalize_sense() memsets the scsi_sense_hdr to 0, this error will be treated like SENSE_NO_SENSE, and sd_done will return good_bytes = xfer_size.

If LLDDs don't always set the driver byte,

and we can't trust that
"driver_byte(result) & DRIVER_SENSE" is set if sense data was retrieved, then I think the correct thing to do is to drop the check for it entirely. So we would have:

	if (result) {
		sense_valid = scsi_command_normalize_sense(SCpnt, &sshdr);
		if (sense_valid)
			sense_deferred = scsi_sense_is_deferred(&sshdr);
	}
	...
	if (!sense_valid || sense_deferred)
		goto out;

Quoted 7 lines

Quoted 6 lines

Finally, for the case of RECOVERED_ERROR/NO_SENSE, this patch adds a
check of the data transfer residual before assuming that the command
completed successfully.

Quoted 6 lines

You cannot do this for NO SENSE SK. You can do that for RECOVERED ERROR SK. The way to do this is to move "case RECOVERED_ERROR:" just above "case HARDWARE_ERROR:".

I think you may have misunderstood what I wrote. I am checking the "data transfer residual", which is set by the LLDD to indicate how much data was actually transferred over the wire to or from the device. It has nothing to do with sense data. The data transfer residual (if supported by the LLDD) should always be valid for any sense key, and even also for commands that complete successfully. For example, on a command that is supposed to write 128 KB, if the LLDD reports that it only transferred 64 KB to the drive, and the drive returns CHECK CONDITION, RECOVERED ERROR with VALID = 0, then we know that the drive could not have written more than 64 KB, since the LLDD never sent it all the data.

The only reason not to use the data transfer residual is if you are worried that there are LLDDs that don't calculate it correctly. I can vouch for a few LLDDs (aic7xxx, sym53c8xx, mptspi, mptfc, qla2xxx, and iscsi_tcp). I intentionally designed my patch to use the data transfer residual conservatively in case a LLDD got it wrong, so I am not too worried about it.

Also, I disagree about treating recovered error like hardware/medium error. Recovered error is supposed to mean "the last command completed successfully, with some recovery action performed by the device server". The other errors mean that the command didn't complete successfully. If VALID=1, then the sense information bytes indicate "the unsigned logical block address associated with the sense key". So for hardware/medium error, the sense info might indicate the bad LBA, but for recovered error, the sense info might indicate the recovered LBA. So I think that it would be a mistake to treat them the same.

Also, I disagree about treating recovered error like hardware/medium
error. Recovered error is supposed to mean "the last command completed
successfully, with some recovery action performed by the device
server".

Quoted 8 lines · Tony Battersby

Also, I disagree about treating recovered error like hardware/medium
error. Recovered error is supposed to mean "the last command completed
successfully, with some recovery action performed by the device
server".

Which then means that you agree with
commit 03aba2f7.

But the definition of RECOVERED ERROR immediately after what you quoted, adds:
"Details may be determined by examining the
additional sense bytes and the INFORMATION field."

Thus the patch I sent to you for you to try on
your hardware.

I disagree only with this part of the commit:

- good_bytes = (error_sector -
SCpnt->request->sector) << 9;
- if (good_bytes < 0 || good_bytes

Quoted 1 line

= this_count)

- good_bytes = 0;

So it removed the sanity-check on good_bytes, which broke error handling
for my out-of-spec RAID.

My patch adds the check back,
only doing it
before the multiplication by the sector size rather than after. That is
also why I wanted to add an upper-bound check, to make sure that sd_done
never returned good_bytes > xfer_size, but no one else agreed with that
level of paranoia.

I guess the question is: if a disk drive returns RECOVERED ERROR with
info_valid=1 and the sector number in the sense bytes, does that mean
that the disk completed the command successfully and transferred all the
data (and is reporting the sector number for information logging
purposes only), or does it mean that it stopped reading or writing at
the sector indicated in the sense data?

I can't really
say for sure, so
I will leave the debate to others.

BTW, your patch will result in sd_done returning good_bytes == 0 for the
case where sense_key == RECOVERED ERROR && info_valid == 0, which I
think is probably wrong.

In this case I would return
good_bytes == 0
for hardware/medium error and good_bytes == xfer_size for recovered error.

My hardware isn't returning "recovered error"
or "no sense" sense keys;

I was just trying to improve the handling of these cases while I was
looking at the function.
Thus, there is no point for me to
test your
full patch. My problem is now solved with the simplified patch I
already posted. If you want to push for the RECOVERED ERROR change,
then go right ahead with your own patch, but I'm done.

You then took hunk #2 (2 lines) of the patch I sent you and submitted it as your own, and then I acked "your" patch.

I think it would've been much clearer if you had singled out the problems you were seeing with your HW and sent a single problem with a single patch per single email.

I _really_ _really_ hope that you don't believe that I am trying to take
credit for your work. If you take another look, my original patch had
the following hunk:

(quoted diff) +8 -0

+
+		/* Make sure that bad_lba is one of the sectors that the
+		 * command was trying to access.
+		 */
+		if (bad_lba < start_lba ||
+		    bad_lba >= start_lba + xfer_size / sector_size)
+			goto out;
+

Your response patch had the following hunk:

drivers/scsi/sd.c +2 -0

+		if (bad_lba < start_lba)
+			goto out;

So I don't feel that it was dishonest for me to submit this as "my"
work. If you were offended, then I apologize.

Quoted 6 lines

I think it would've been much clearer if you had singled out the problems you were seeing with your HW and sent a single problem with a single patch per single email.

Agreed. Sometimes it is difficult to predict when something that seems
so straightforward will generate so much controversy.