SCSI: Correct UA being ignored when notifying of media-changed

RFC PATCH In review Rob Evers 4 participants 9 messages Aug 3, 2011 – Aug 11, 2011 SCSI

lore

Share & tools

lore

Rob Evers

author

2011-08-03 13:39 UTC #0 lore raw

RFC PATCH 1/1 SCSI: Correct UA being ignored when notifying of media-changed

Unit attention commands are getting ignored when a dvd is replaced, resulting in errors when accessing the new dvd.

This problem occurred when a system is booted in rescue mode via dvd and the dvd is then replaced with another dvd. Subsequent commands to a filesystem on the dvd, such as 'fsck' and 'restore' report errors.

Solution suggested by Mike Christie.

Signed-off-by Rob Evers <[email protected]>

---
 drivers/scsi/scsi_error.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

drivers/scsi/scsi_error.c +4 -0

index a4b9cdb..725cc11 100644
@@ -294,6 +294,10 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
 		 */
 		if (scmd->device->expecting_cc_ua) {
 			scmd->device->expecting_cc_ua = 0;
+				if (sdev->removable) {
+					sdev->changed = 1;
+					return SUCCESS;
+				}
 			return NEEDS_RETRY;
 		}
 		/*

Signature · 1 line

1.7.1

Mike Christie

2011-08-03 22:06 UTC #1 lore raw

Quoted 20 lines · Rob Evers

Unit attention commands are getting ignored when a dvd is replaced, resulting in errors when accessing the new dvd.
This problem occurred when a system is booted in rescue mode via dvd and the dvd is then replaced with another dvd. Subsequent commands to a filesystem on the dvd, such as 'fsck' and 'restore' report errors.
Solution suggested by Mike Christie.
Signed-off-by Rob Evers <[email protected]>
---
 drivers/scsi/scsi_error.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)
drivers/scsi/scsi_error.c +0 -0
index a4b9cdb..725cc11 100644

drivers/scsi/scsi_error.c static int scsi_check_sense(struct scsi_cmnd *scmd) · line 300

Quoted 7 lines

drivers/scsi/scsi_error.c +4 -0

@@ -294,6 +294,10 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
 		 */
 		if (scmd->device->expecting_cc_ua) {
 			scmd->device->expecting_cc_ua = 0;
+				if (sdev->removable) {
+					sdev->changed = 1;
+					return SUCCESS;
+				}

I think this 'if' block got tabbed over once too many times.

Quoted 3 lines

drivers/scsi/scsi_error.c +0 -0

 			return NEEDS_RETRY;
 		}
 		/*

James Bottomley

maintainer

2011-08-03 22:22 UTC #2 lore raw

In reply to Rob Evers

Quoted 20 lines · Rob Evers

Unit attention commands are getting ignored when a dvd is replaced, resulting in errors when accessing the new dvd.
This problem occurred when a system is booted in rescue mode via dvd and the dvd is then replaced with another dvd. Subsequent commands to a filesystem on the dvd, such as 'fsck' and 'restore' report errors.
Solution suggested by Mike Christie.
Signed-off-by Rob Evers <[email protected]>
---
 drivers/scsi/scsi_error.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)
drivers/scsi/scsi_error.c +0 -0
index a4b9cdb..725cc11 100644

drivers/scsi/scsi_error.c static int scsi_check_sense(struct scsi_cmnd *scmd) · line 300

Quoted 7 lines

drivers/scsi/scsi_error.c +4 -0

@@ -294,6 +294,10 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
 		 */
 		if (scmd->device->expecting_cc_ua) {
 			scmd->device->expecting_cc_ua = 0;
+				if (sdev->removable) {
+					sdev->changed = 1;
+					return SUCCESS;
+				}

Are you sure about this? We only set expecting_cc_ua if the device was reset in error handling ... and there we definitely *don't* want to trigger a medium change.

Surely we want this at the end of the UA handling clause?

James

Mike Christie

2011-08-03 22:31 UTC #3 lore raw

Quoted 16 lines · James Bottomley

Quoted 15 lines · Rob Evers
Unit attention commands are getting ignored when a dvd is replaced, resulting in errors when accessing the new dvd.
This problem occurred when a system is booted in rescue mode via dvd and the dvd is then replaced with another dvd. Subsequent commands to a filesystem on the dvd, such as 'fsck' and 'restore' report errors.
Solution suggested by Mike Christie.
Signed-off-by Rob Evers <[email protected]>
---
 drivers/scsi/scsi_error.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

Quoted 17 lines

drivers/scsi/scsi_error.c static int scsi_check_sense(struct scsi_cmnd *scmd) · line 300
Quoted 12 lines
drivers/scsi/scsi_error.c +4 -0
index a4b9cdb..725cc11 100644
@@ -294,6 +294,10 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
 		 */
 		if (scmd->device->expecting_cc_ua) {
 			scmd->device->expecting_cc_ua = 0;
+				if (sdev->removable) {
+					sdev->changed = 1;
+					return SUCCESS;
+				}
Are you sure about this? We only set expecting_cc_ua if the device was reset in error handling ... and there we definitely *don't* want to trigger a medium change.

What is happening is that we have a quirky device, and when you switch CD/DVDs the first command times out, so the scsi error handler runs. That is why the above code was modified. Before the patch the scsi layer would just retry the IO, and sr would not see the sense and so it would not detect the media changed. So the above code was meant to work like UA code in scsi_io_completion.

James Bottomley

maintainer

2011-08-03 22:36 UTC #4 lore raw

Quoted 35 lines · Mike Christie

Quoted 16 lines · James Bottomley
Quoted 15 lines · Rob Evers
Unit attention commands are getting ignored when a dvd is replaced, resulting in errors when accessing the new dvd.
This problem occurred when a system is booted in rescue mode via dvd and the dvd is then replaced with another dvd. Subsequent commands to a filesystem on the dvd, such as 'fsck' and 'restore' report errors.
Solution suggested by Mike Christie.
Signed-off-by Rob Evers <[email protected]>
---
 drivers/scsi/scsi_error.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)
Quoted 17 lines
drivers/scsi/scsi_error.c static int scsi_check_sense(struct scsi_cmnd *scmd) · line 300
Quoted 12 lines
drivers/scsi/scsi_error.c +4 -0
index a4b9cdb..725cc11 100644
@@ -294,6 +294,10 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
 		 */
 		if (scmd->device->expecting_cc_ua) {
 			scmd->device->expecting_cc_ua = 0;
+				if (sdev->removable) {
+					sdev->changed = 1;
+					return SUCCESS;
+				}
Are you sure about this? We only set expecting_cc_ua if the device was reset in error handling ... and there we definitely *don't* want to trigger a medium change.

Quoted 6 lines

What is happening is that we have a quirky device, and when you switch CD/DVDs the first command times out, so the scsi error handler runs. That is why the above code was modified. Before the patch the scsi layer would just retry the IO, and sr would not see the sense and so it would not detect the media changed. So the above code was meant to work like UA code in scsi_io_completion.

The SCSI standard requires all SCSI devices to return UA after a reset (that's the reason for the code). So, if I put this patch in, every removable device would be flagged as having changed media if we go into an error handling situation and reset it. I don't think that's a good idea to fix a single non-standard CD.

James

Mike Christie

2011-08-03 23:11 UTC #5 lore raw

Quoted 43 lines · James Bottomley

Quoted 35 lines · Mike Christie
Quoted 16 lines · James Bottomley
Quoted 15 lines · Rob Evers
Unit attention commands are getting ignored when a dvd is replaced, resulting in errors when accessing the new dvd.
This problem occurred when a system is booted in rescue mode via dvd and the dvd is then replaced with another dvd. Subsequent commands to a filesystem on the dvd, such as 'fsck' and 'restore' report errors.
Solution suggested by Mike Christie.
Signed-off-by Rob Evers <[email protected]>
---
 drivers/scsi/scsi_error.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)
Quoted 17 lines
drivers/scsi/scsi_error.c static int scsi_check_sense(struct scsi_cmnd *scmd) · line 300
Quoted 12 lines
drivers/scsi/scsi_error.c +4 -0
index a4b9cdb..725cc11 100644
@@ -294,6 +294,10 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
 		 */
 		if (scmd->device->expecting_cc_ua) {
 			scmd->device->expecting_cc_ua = 0;
+				if (sdev->removable) {
+					sdev->changed = 1;
+					return SUCCESS;
+				}
Are you sure about this? We only set expecting_cc_ua if the device was reset in error handling ... and there we definitely *don't* want to trigger a medium change.
Quoted 6 lines
What is happening is that we have a quirky device, and when you switch CD/DVDs the first command times out, so the scsi error handler runs. That is why the above code was modified. Before the patch the scsi layer would just retry the IO, and sr would not see the sense and so it would not detect the media changed. So the above code was meant to work like UA code in scsi_io_completion.

Quoted 6 lines

The SCSI standard requires all SCSI devices to return UA after a reset (that's the reason for the code). So, if I put this patch in, every removable device would be flagged as having changed media if we go into an error handling situation and reset it. I don't think that's a good idea to fix a single non-standard CD.

Yeah, ok. Agree with you.

TARUISI Hiroaki

2011-08-08 22:50 UTC #6 lore raw

In reply to James Bottomley

Hi James,

I posted a patch (as below) last year about this.
http://www.kerneltrap.org/mailarchive/linux-scsi/2010/12/22/6887895

I intended to distinguish UA caused by media change from UA caused by other reasons like resetting a device. First, it should be notified to upper layer driver, sr, but the other must be retried as scsi_error.c coded.

At least SCSI driver should not squash all UAs, I think.

Signature · 1 line

taruisi

Quoted 59 lines · James Bottomley

Quoted 35 lines · Mike Christie
Quoted 16 lines · James Bottomley
Quoted 15 lines · Rob Evers
Unit attention commands are getting ignored when a dvd is replaced, resulting in errors when accessing the new dvd.
This problem occurred when a system is booted in rescue mode via dvd and the dvd is then replaced with another dvd. Subsequent commands to a filesystem on the dvd, such as 'fsck' and 'restore' report errors.
Solution suggested by Mike Christie.
Signed-off-by Rob Evers <[email protected]>
---
 drivers/scsi/scsi_error.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)
Quoted 17 lines
drivers/scsi/scsi_error.c static int scsi_check_sense(struct scsi_cmnd *scmd) · line 300
Quoted 12 lines
drivers/scsi/scsi_error.c +4 -0
index a4b9cdb..725cc11 100644
@@ -294,6 +294,10 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
 		 */
 		if (scmd->device->expecting_cc_ua) {
 			scmd->device->expecting_cc_ua = 0;
+				if (sdev->removable) {
+					sdev->changed = 1;
+					return SUCCESS;
+				}
Are you sure about this? We only set expecting_cc_ua if the device was reset in error handling ... and there we definitely *don't* want to trigger a medium change.
Quoted 6 lines
What is happening is that we have a quirky device, and when you switch CD/DVDs the first command times out, so the scsi error handler runs. That is why the above code was modified. Before the patch the scsi layer would just retry the IO, and sr would not see the sense and so it would not detect the media changed. So the above code was meant to work like UA code in scsi_io_completion.
The SCSI standard requires all SCSI devices to return UA after a reset (that's the reason for the code). So, if I put this patch in, every removable device would be flagged as having changed media if we go into an error handling situation and reset it. I don't think that's a good idea to fix a single non-standard CD.
James
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

James Bottomley

maintainer

2011-08-09 15:22 UTC #7 lore raw

Quoted 10 lines · TARUISI Hiroaki

Hi James,
I posted a patch (as below) last year about this.
http://www.kerneltrap.org/mailarchive/linux-scsi/2010/12/22/6887895
I intended to distinguish UA caused by media change from UA caused by other reasons like resetting a device. First, it should be notified to upper layer driver, sr, but the other must be retried as scsi_error.c coded.
At least SCSI driver should not squash all UAs, I think.

Well ... the spec says the device should stack UAs in this event. However, I can totally believe that a broken CD would fail on this front.

If Mike and Rob can verify that it's realy asc/ascq 0x28/0x0 that the CD is emitting, I'd be happy to put the patch in (with a nice big comment explaining what's being done and why over the if() check rather than updating the global comment).

James

TARUISI Hiroaki

2011-08-11 11:25 UTC #8 lore raw

PATCH 1/1 SCSI: Prevent from retrying with expecting_cc_ua in case of disk change.

Hi James,

Thanks for your comment.
Actually, Rob and I are focusing to the same problem. And I have a dvd drive of that kind.

To confirm ASC,ASCQ in this case, I added some printk like this.
----

        * so that we can deal with it there.
        */
       if (scmd->device->expecting_cc_ua) {
               printk("scsi_check_sense: expecting_cc_ua=on"
                      " unit_attention asc=%02x ascq=%02x\n",
                      sshdr.asc, sshdr.ascq);
               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
               scmd->device->expecting_cc_ua = 0;
               return SCSI_MLQUEUE_DIS_DEV_RETRY;
       }
       /*
        * if the device is in the process of becoming ready, we
        * should retry.
        */

----
And I created boot dvd, booted the system with that in
rescue mode, changed the media after boot and read or write
that media, then, I could see these messages.
----
<6>Adding 4095992k swap on /dev/VolGroup00/LogVol01. Priority:-1 extents:1 across:4095996k <4>scsi_check_sense: expecting_cc_ua=on unit_attention asc=28 ascq=00 <4>end_request: I/O error, dev sr0, sector 7432832
----

As message says, my dvd drive reports media change sense and as the result end_request I/O error is caused.
Surely this drive may be naughty, but we should know what kind of UA and treat it.

---
 drivers/scsi/scsi_error.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

drivers/scsi/scsi_error.c +9 -2

index a4b9cdb..158ba46 100644
@@ -293,8 +293,15 @@ static int scsi_check_sense(struct scsi_cmnd *scmd)
                 * so that we can deal with it there.
                 */
                if (scmd->device->expecting_cc_ua) {
-                       scmd->device->expecting_cc_ua = 0;
-                       return NEEDS_RETRY;
+               /*
+                * Because some device does not queue unit attentions correctly,
+                * we carefully check additional sense code and qualifier
+                * so as not to squash media change unit attention.
+                */
+                       if (sshdr.asc != 0x28 || sshdr.ascq != 0x00) {
+                               scmd->device->expecting_cc_ua = 0;
+                               return NEEDS_RETRY;
+                       }
                }
                /*
                 * if the device is in the process of becoming ready, we

--
1.6.5

Quoted 31 lines · James Bottomley

Quoted 10 lines · TARUISI Hiroaki
Hi James,
I posted a patch (as below) last year about this.
http://www.kerneltrap.org/mailarchive/linux-scsi/2010/12/22/6887895
I intended to distinguish UA caused by media change from UA caused by other reasons like resetting a device. First, it should be notified to upper layer driver, sr, but the other must be retried as scsi_error.c coded.
At least SCSI driver should not squash all UAs, I think.
Well ... the spec says the device should stack UAs in this event. However, I can totally believe that a broken CD would fail on this front.
If Mike and Rob can verify that it's realy asc/ascq 0x28/0x0 that the CD is emitting, I'd be happy to put the patch in (with a nice big comment explaining what's being done and why over the if() check rather than updating the global comment).
James
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 1 line

taruisi