[PATCH] sd: sd should not modify read capacity, cache type or write protect flag on rescan when there is a transport error

[PATCH] sd: sd should not modify read capacity, cache type or write protect flag on rescan when there is a transport error

[Menny_Hamburger]

From: Menny Hamburger <Menny_Hamburger@Dell.com>

When sd scan fails in apprehending capacity, cache_type or write protect flag
property from the device, it automatically assigns a default value to the
failed property. When rescanning, in case of transport/host error, this default 
value is invalid since the problem is with the connection to the device and not in 
the device itself that may (in most cases) still be intact. Applying a default value
when failing may lead to problems when connection is re-established since the default
value persists unless an additional rescan is performed.

This problem was witnessed when running in a iSCSI environment under multipath
(with I/O on the active path). In this case we get a ping-ping effect where
multipathd switches between alternate paths forever (until rescan) because the
path checker states that the device is OK, and I/O fails immediately because of
the 0 capacity (assigned to the device when rescanning while the device was 
disconnected from the storage).

Reproduction over ISCSI environment:
1) dd if=/dev/dm-0 of=/dev/zero bs=64 count=10000
2) ifdown ethN, ethM, ethK, ... (where ethX is an interface from which the
   machine establishes connection to the storage array).
3) iscsiadm -m session -R
4) ifup ethN, ethM, ethK, ...

The following patch leaves the capacity, cache type and write protect flag unchanged on a host/transport problem. 
I want to thank  Mike Cristie (mchristi@redhat.com) for helping out with this one.

diff -r -U 2 a/drivers/scsi/sd.c b/drivers/scsi/sd.c
--- a/drivers/scsi/sd.c    2011-02-27 11:57:52.159333000 +0200
+++ b/drivers/scsi/sd.c    2011-02-27 11:57:52.222659000 +0200
@@ -1502,10 +1502,16 @@
          set_media_not_present(sdkp);

-    /*
-    * We used to set media_present to 0 here to indicate no media
-    * in the drive, but some drives fail read capacity even with
-    * media present, so we can't do that.
-    */
-    sdkp->capacity = 0; /* unknown mapped to zero - as usual */
+    if (host_byte(the_result) && !sdkp->first_scan)
+          sd_printk(KERN_NOTICE, sdkp, "host error while reading capacity\n");
+    else {
+          /*
+          * We used to set media_present to 0 here to indicate no media
+          * in the drive, but some drives fail read capacity even with
+          * media present, so we can't do that.
+          */
+          sdkp->capacity = 0; /* unknown mapped to zero - as usual */
+    }
+
+    sd_printk(KERN_NOTICE, sdkp, "assuming capacity %llu.\n", sdkp->capacity);
}

@@ -1878,6 +1884,11 @@

     if (!scsi_status_is_good(res)) {
-          sd_printk(KERN_WARNING, sdkp,
-                 "Test WP failed, assume Write Enabled\n");
+          if (host_byte(res) && !sdkp->first_scan) {
+               sd_printk(KERN_NOTICE, sdkp, "host error while reading write protect flag\n");
+               set_disk_ro(sdkp->disk, sdkp->write_prot);
+          }
+
+          sd_printk(KERN_WARNING, sdkp, 
+                 "Test WP failed, assume Write %s\n", (sdkp->write_prot) ? "Disabled" : "Enabled");
    } else {
          sdkp->write_prot = ((data.device_specific & 0x80) != 0);
@@ -2034,8 +2045,14 @@

 defaults:
-    sd_printk(KERN_ERR, sdkp, "Assuming drive cache: write through\n");
-    sdkp->WCE = 0;
-    sdkp->RCD = 0;
-    sdkp->DPOFUA = 0;
+    if (host_byte(res) && !sdkp->first_scan) 
+          sd_printk(KERN_NOTICE, sdkp, "host error while reading cache type\n");
+    else {
+          sdkp->WCE = 0;
+          sdkp->RCD = 0;
+          sdkp->DPOFUA = 0;
+    }
+
+    sd_printk(KERN_ERR, sdkp, "Assuming drive cache: %s\n", 
+            sd_cache_types[sdkp->RCD + 2*sdkp->WCE]);
}

Re: [PATCH] sd: sd should not modify read capacity, cache type or write protect flag on rescan when there is a transport error

[Matthew Wilcox]

On Sun, Feb 27, 2011 at 02:21:58PM +0000, Menny_Hamburger@Dell.com wrote:
> @@ -1502,10 +1502,16 @@
>           set_media_not_present(sdkp);
> 
> -    /*
> -    * We used to set media_present to 0 here to indicate no media
> -    * in the drive, but some drives fail read capacity even with
> -    * media present, so we can't do that.
> -    */
> -    sdkp->capacity = 0; /* unknown mapped to zero - as usual */
> +    if (host_byte(the_result) && !sdkp->first_scan)
> +          sd_printk(KERN_NOTICE, sdkp, "host error while reading capacity\n");

This patch is corrupted ... looks like Exchange has converted tabs
to spaces.

> +    else {
> +          /*
> +          * We used to set media_present to 0 here to indicate no media
> +          * in the drive, but some drives fail read capacity even with
> +          * media present, so we can't do that.
> +          */
> +          sdkp->capacity = 0; /* unknown mapped to zero - as usual */
> +    }
> +
> +    sd_printk(KERN_NOTICE, sdkp, "assuming capacity %llu.\n", sdkp->capacity);
> }
> 
> @@ -1878,6 +1884,11 @@
> 
>      if (!scsi_status_is_good(res)) {
> -          sd_printk(KERN_WARNING, sdkp,
> -                 "Test WP failed, assume Write Enabled\n");
> +          if (host_byte(res) && !sdkp->first_scan) {
> +               sd_printk(KERN_NOTICE, sdkp, "host error while reading write protect flag\n");
> +               set_disk_ro(sdkp->disk, sdkp->write_prot);
> +          }
> +
> +          sd_printk(KERN_WARNING, sdkp, 
> +                 "Test WP failed, assume Write %s\n", (sdkp->write_prot) ? "Disabled" : "Enabled");
>     } else {
>           sdkp->write_prot = ((data.device_specific & 0x80) != 0);
> @@ -2034,8 +2045,14 @@
> 
>  defaults:
> -    sd_printk(KERN_ERR, sdkp, "Assuming drive cache: write through\n");
> -    sdkp->WCE = 0;
> -    sdkp->RCD = 0;
> -    sdkp->DPOFUA = 0;
> +    if (host_byte(res) && !sdkp->first_scan) 
> +          sd_printk(KERN_NOTICE, sdkp, "host error while reading cache type\n");
> +    else {
> +          sdkp->WCE = 0;
> +          sdkp->RCD = 0;
> +          sdkp->DPOFUA = 0;
> +    }
> +
> +    sd_printk(KERN_ERR, sdkp, "Assuming drive cache: %s\n", 
> +            sd_cache_types[sdkp->RCD + 2*sdkp->WCE]);
> }
> 
> 
> --
> 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

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] sd: sd should not modify read capacity, cache type or write protect flag on rescan when there is a transport error

[James Bottomley]

On Sun, 2011-02-27 at 14:21 +0000, Menny_Hamburger@Dell.com wrote:
> From: Menny Hamburger <Menny_Hamburger@Dell.com>
> 
> When sd scan fails in apprehending capacity, cache_type or write protect flag
> property from the device, it automatically assigns a default value to the
> failed property. When rescanning, in case of transport/host error, this default 
> value is invalid since the problem is with the connection to the device and not in 
> the device itself that may (in most cases) still be intact. Applying a default value
> when failing may lead to problems when connection is re-established since the default
> value persists unless an additional rescan is performed.

That's correct.  Zero means we know there's something there but we
couldn't get the necessary information.  A zero size device can't be
read from or written to.

> This problem was witnessed when running in a iSCSI environment under multipath
> (with I/O on the active path). In this case we get a ping-ping effect where
> multipathd switches between alternate paths forever (until rescan) because the
> path checker states that the device is OK, and I/O fails immediately because of
> the 0 capacity (assigned to the device when rescanning while the device was 
> disconnected from the storage).
> 
> Reproduction over ISCSI environment:
> 1) dd if=/dev/dm-0 of=/dev/zero bs=64 count=10000
> 2) ifdown ethN, ethM, ethK, ... (where ethX is an interface from which the
>    machine establishes connection to the storage array).
> 3) iscsiadm -m session -R
> 4) ifup ethN, ethM, ethK, ...

This really doesn't look like a good idea.  It's a layering violation in
that the SCSI mid layer now has to try to determine if certain command
failures are the result of host disruption.

The idea of believing a prior value if a READ_CAPACITY fails also
doesn't look to be such a good one.  This could lead to volume
corruption if the disruption is part of an array configuration.

The correct fix looks to be to initiate a rescan when the host is active
via hotplug, and just teach the path checker about zero size devices?

James

RE: [PATCH] sd: sd should not modify read capacity, cache type or write protect flag on rescan when there is a transport error

[Menny_Hamburger]

I think the kernel should have some ability to handle this situation (or at least propagate the fact that there is a problem) without the userland being involved.

We could add this as a devinfo option - enable this functionality for specific devices.
Another totally different way may be to have the SCSI layer send some notification (unknown property, needs rescan) that could be picked up and handled by the transport layer.  

Menny

-----Original Message-----
From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-owner@vger.kernel.org] On Behalf Of James Bottomley
Sent: 28 February, 2011 17:35
To: Hamburger, Menny
Cc: linux-scsi@vger.kernel.org
Subject: Re: [PATCH] sd: sd should not modify read capacity, cache type or write protect flag on rescan when there is a transport error

On Sun, 2011-02-27 at 14:21 +0000, Menny_Hamburger@Dell.com wrote:
> From: Menny Hamburger <Menny_Hamburger@Dell.com>
> 
> When sd scan fails in apprehending capacity, cache_type or write protect flag
> property from the device, it automatically assigns a default value to the
> failed property. When rescanning, in case of transport/host error, this default 
> value is invalid since the problem is with the connection to the device and not in 
> the device itself that may (in most cases) still be intact. Applying a default value
> when failing may lead to problems when connection is re-established since the default
> value persists unless an additional rescan is performed.

That's correct.  Zero means we know there's something there but we
couldn't get the necessary information.  A zero size device can't be
read from or written to.

> This problem was witnessed when running in a iSCSI environment under multipath
> (with I/O on the active path). In this case we get a ping-ping effect where
> multipathd switches between alternate paths forever (until rescan) because the
> path checker states that the device is OK, and I/O fails immediately because of
> the 0 capacity (assigned to the device when rescanning while the device was 
> disconnected from the storage).
> 
> Reproduction over ISCSI environment:
> 1) dd if=/dev/dm-0 of=/dev/zero bs=64 count=10000
> 2) ifdown ethN, ethM, ethK, ... (where ethX is an interface from which the
>    machine establishes connection to the storage array).
> 3) iscsiadm -m session -R
> 4) ifup ethN, ethM, ethK, ...

This really doesn't look like a good idea.  It's a layering violation in
that the SCSI mid layer now has to try to determine if certain command
failures are the result of host disruption.

The idea of believing a prior value if a READ_CAPACITY fails also
doesn't look to be such a good one.  This could lead to volume
corruption if the disruption is part of an array configuration.

The correct fix looks to be to initiate a rescan when the host is active
via hotplug, and just teach the path checker about zero size devices?

James


--
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] sd: sd should not modify read capacity, cache type or write protect flag on rescan when there is a transport error

[James Bottomley]

Could you not top post: it makes it hard to follow what you're actually
replying to.

On Tue, 2011-03-08 at 09:30 +0000, Menny_Hamburger@Dell.com wrote:
> I think the kernel should have some ability to handle this situation
> (or at least propagate the fact that there is a problem) without the
> userland being involved.
> 
> We could add this as a devinfo option - enable this functionality for
> specific devices.
> Another totally different way may be to have the SCSI layer send some
> notification (unknown property, needs rescan) that could be picked up
> and handled by the transport layer.  

This is the hotplug I suggested, isn't it?

James

Re: [PATCH] sd: sd should not modify read capacity, cache type or write protect flag on rescan when there is a transport error

[Mike Christie]

On 03/08/2011 08:22 AM, James Bottomley wrote:
> On Tue, 2011-03-08 at 09:30 +0000, Menny_Hamburger@Dell.com wrote:
>> Another totally different way may be to have the SCSI layer send some
>> notification (unknown property, needs rescan) that could be picked up
>> and handled by the transport layer.
>
> This is the hotplug I suggested, isn't it?
>

Yeah.

Menny, I think you can just do the scsi/iscsi_rescan_target option I 
suggested in my pseudo patch where when the transport comes back online 
the iscsi class calls some helper scsi function that calls 
scsi_rescan_device for each device on the target, so proper values are 
picked up.

RE: [PATCH] sd: sd should not modify read capacity, cache type or write protect flag on rescan when there is a transport error

[Menny_Hamburger]

Mike, 

I think there are still two open issues still here:
1) Should we always rescan when transport comes back up? My own opinion to this question is that doing a full rescan may be an overkill 
   In this case. If the decision is not to rescan always (only when we have invalid capacity, cache type, write protect flag), we will need to maintain
   Additional state information in the device to indicate that the current values are invalid and a rescan is required.
   James stated that it is a layering violation to test the host byte of the result in order to decide whether or not a rescan is required, so we will 
   need setup this state information in any case of failure when getting these properties from the device.
2) Should we rescan from within the kernel, or issue a uevent (hotplug) that would be picked up by userland (that would initiate the rescan)?
   Rescanning from within the kernel seems a bit more "clean", however there may be other considerations here.

-----Original Message-----
From: Mike Christie [mailto:michaelc@cs.wisc.edu] 
Sent: 10 March, 2011 02:01
To: James Bottomley
Cc: Hamburger, Menny; linux-scsi@vger.kernel.org
Subject: Re: [PATCH] sd: sd should not modify read capacity, cache type or write protect flag on rescan when there is a transport error

Yeah.

Menny, I think you can just do the scsi/iscsi_rescan_target option I 
suggested in my pseudo patch where when the transport comes back online 
the iscsi class calls some helper scsi function that calls 
scsi_rescan_device for each device on the target, so proper values are 
picked up.

Re: [PATCH] sd: sd should not modify read capacity, cache type or write protect flag on rescan when there is a transport error

[Mike Christie]

On 03/10/2011 02:49 AM, Menny_Hamburger@Dell.com wrote:
> Mike,
>
> I think there are still two open issues still here:
> 1) Should we always rescan when transport comes back up? My own opinion to this question is that doing a full rescan may be an overkill

I think we should always rescan. The values could have changed on the 
target even if we have not actually sent a command and had it failed, so 
either way we need to pick up new values.

>     In this case. If the decision is not to rescan always (only when we have invalid capacity, cache type, write protect flag), we will need to maintain
>     Additional state information in the device to indicate that the current values are invalid and a rescan is required.
>     James stated that it is a layering violation to test the host byte of the result in order to decide whether or not a rescan is required, so we will
>     need setup this state information in any case of failure when getting these properties from the device.
> 2) Should we rescan from within the kernel, or issue a uevent (hotplug) that would be picked up by userland (that would initiate the rescan)?
>     Rescanning from within the kernel seems a bit more "clean", however there may be other considerations here.

Kernel. In the iscsi and fc class we already do a scan for devices when 
the transport comes back up from the kernel, so this rescan of devices 
values can just run from the same function.

RE: [PATCH] sd: sd should not modify read capacity, cache type or write protect flag on rescan when there is a transport error

[Menny_Hamburger]

Sorry for not responding earlier.
I think that a simple walkthrough rescan on existing channel/id/lun will be OK in 99% of the cases.
When the target was reconfigured (when one adds luns for example), a simple rescan will not pick up the changes (or worse). 
Seams that there is reason why there is no kernel based rescan function on a target today - only rescan scripts that walk through everything and call the
device rescan code. 
Anyway, I see that this conversation may have been picked up by another thread discussing uevents and unit attention.
http://www.spinics.net/lists/linux-scsi/msg50241.html


-----Original Message-----
From: Mike Christie [mailto:michaelc@cs.wisc.edu] 
Sent: 10 March, 2011 22:29
To: Hamburger, Menny
Cc: linux-scsi@vger.kernel.org; James.Bottomley@suse.de
Subject: Re: [PATCH] sd: sd should not modify read capacity, cache type or write protect flag on rescan when there is a transport error

On 03/10/2011 02:49 AM, Menny_Hamburger@Dell.com wrote:
> Mike,
>
> I think there are still two open issues still here:
> 1) Should we always rescan when transport comes back up? My own opinion to this question is that doing a full rescan may be an overkill

I think we should always rescan. The values could have changed on the 
target even if we have not actually sent a command and had it failed, so 
either way we need to pick up new values.

>     In this case. If the decision is not to rescan always (only when we have invalid capacity, cache type, write protect flag), we will need to maintain
>     Additional state information in the device to indicate that the current values are invalid and a rescan is required.
>     James stated that it is a layering violation to test the host byte of the result in order to decide whether or not a rescan is required, so we will
>     need setup this state information in any case of failure when getting these properties from the device.
> 2) Should we rescan from within the kernel, or issue a uevent (hotplug) that would be picked up by userland (that would initiate the rescan)?
>     Rescanning from within the kernel seems a bit more "clean", however there may be other considerations here.

Kernel. In the iscsi and fc class we already do a scan for devices when 
the transport comes back up from the kernel, so this rescan of devices 
values can just run from the same function.

[PATCH] sd: sd should not modify read capacity, cache type or write protect flag on rescan when there is a transport error

[Menny_Hamburger]

[-- Attachment #1: Type: text/plain, Size: 42 bytes --]

Attached is the original patch file.



[-- Attachment #2: scsi_host_error_during_propery_read.patch --]
[-- Type: application/octet-stream, Size: 3323 bytes --]

From: Menny Hamburger <Menny_Hamburger@Dell.com>

When sd scan fails in apprehending capacity, cache_type or write protect flag
property from the device, it automatically assigns a default value to the
failed property. When rescanning, in case of transport/host error this default 
value is invalid since the problem is with the connection to the device and not in 
the device itself that may (in most cases) still be intact. Applying a default value
when failing may lead to problems when connection is re-established since the default
value persists unless an additional rescan is performed.

This problem was witnessed when running in a iSCSI environment under multipath
(with I/O on the active path). In this case we get a ping-ping effect where
multipathd switches between alternate paths forever (until rescan) because the
path checker states that the device is OK, and I/O fails immediately because of
the 0 capacity.

Reproduction over ISCSI environment:
1) dd if=/dev/dm-0 of=/dev/zero bs=64 count=10000
2) ifdown ethN, ethM, ethK, ... (where ethX is an interface from which the
   machine establishes connection to the storage array).
3) iscsiadm -m session -R
4) ifup ethN, ethM, ethK, ...

The following patch leaves the capacity, cache type and write protect flag unchanged on a host/transport problem. 
I want to thank  Mike Cristie (mchristi@redhat.com) for helping out with this one.

diff -r -U 2 a/drivers/scsi/sd.c b/drivers/scsi/sd.c
--- a/drivers/scsi/sd.c	2011-02-27 11:57:52.159333000 +0200
+++ b/drivers/scsi/sd.c	2011-02-27 11:57:52.222659000 +0200
@@ -1502,10 +1502,16 @@
 		set_media_not_present(sdkp);
 
-	/*
-	 * We used to set media_present to 0 here to indicate no media
-	 * in the drive, but some drives fail read capacity even with
-	 * media present, so we can't do that.
-	 */
-	sdkp->capacity = 0; /* unknown mapped to zero - as usual */
+	if (host_byte(the_result) && !sdkp->first_scan)
+		sd_printk(KERN_NOTICE, sdkp, "host error while reading capacity\n");
+	else {
+		/*
+		 * We used to set media_present to 0 here to indicate no media
+		 * in the drive, but some drives fail read capacity even with
+		 * media present, so we can't do that.
+		 */
+		sdkp->capacity = 0; /* unknown mapped to zero - as usual */
+	}
+
+	sd_printk(KERN_NOTICE, sdkp, "assuming capacity %llu.\n", sdkp->capacity);
 }
 
@@ -1878,6 +1884,11 @@
 
 	if (!scsi_status_is_good(res)) {
-		sd_printk(KERN_WARNING, sdkp,
-			  "Test WP failed, assume Write Enabled\n");
+		if (host_byte(res) && !sdkp->first_scan) {
+			sd_printk(KERN_NOTICE, sdkp, "host error while reading write protect flag\n");
+			set_disk_ro(sdkp->disk, sdkp->write_prot);
+		}
+
+		sd_printk(KERN_WARNING, sdkp, 
+			  "Test WP failed, assume Write %s\n", (sdkp->write_prot) ? "DISABLED" : "ENABLED");
 	} else {
 		sdkp->write_prot = ((data.device_specific & 0x80) != 0);
@@ -2034,8 +2045,14 @@
 
 defaults:
-	sd_printk(KERN_ERR, sdkp, "Assuming drive cache: write through\n");
-	sdkp->WCE = 0;
-	sdkp->RCD = 0;
-	sdkp->DPOFUA = 0;
+	if (host_byte(res) && !sdkp->first_scan) 
+		sd_printk(KERN_NOTICE, sdkp, "host error while reading cache type\n");
+	else {
+		sdkp->WCE = 0;
+		sdkp->RCD = 0;
+		sdkp->DPOFUA = 0;
+	}
+
+	sd_printk(KERN_ERR, sdkp, "Assuming drive cache: %s\n", 
+		  sd_cache_types[sdkp->RCD + 2*sdkp->WCE]);
 }