[PATCH] Another fix for suspend i/o - validate dev state before transition

[PATCH] Another fix for suspend i/o - validate dev state before transition

[James.Smart]

The block/unblock patch reports stack dumps and illegal transitions when the driver continues to call block or unblock after the device has failed (moved into offline state due to i/o error, etc). The LLDD/transport is unaware of the sdev transition, so it doesn't know better.

This patch has the internal device block/unblock routines validate the state transition before invoking it.

Hopefully this is it for this darn feature...

-- James S




--- scsi_lib.c.2.6.10-rc2	2004-11-16 09:03:58.000000000 -0500
+++ scsi_lib.c.NEW	2004-11-17 13:23:22.340667480 -0500
@@ -1813,6 +1813,12 @@
  *	state, all commands are deferred until the scsi lld reenables
  *	the device with scsi_device_unblock or device_block_tmo fires.
  *	This routine assumes the host_lock is held on entry.
+ *
+ *      As the LLDD/Transport that is calling this function doesn't
+ *	actually know what the device state is, the function may be
+ *	called at an inappropriate time. Therefore, before requesting
+ *	the state change, the function validates that the transition is
+ *	valid.
  **/
 int
 scsi_internal_device_block(struct scsi_device *sdev)
@@ -1821,6 +1827,10 @@
 	unsigned long flags;
 	int err = 0;
 
+	if ((sdev->sdev_state != SDEV_CREATED) &&
+	    (sdev->sdev_state != SDEV_RUNNING))
+		return 0;
+
 	err = scsi_device_set_state(sdev, SDEV_BLOCK);
 	if (err)
 		return err;
@@ -1853,6 +1863,12 @@
  *	(which must be a legal transition) allowing the midlayer to
  *	goose the queue for this device.  This routine assumes the 
  *	host_lock is held upon entry.
+ *
+ *      As the LLDD/Transport that is calling this function doesn't
+ *	actually know what the device state is, the function may be
+ *	called at an inappropriate time. Therefore, before requesting
+ *	the state change, the function validates that the transition is
+ *	valid.
  **/
 int
 scsi_internal_device_unblock(struct scsi_device *sdev)
@@ -1861,6 +1877,9 @@
 	int err;
 	unsigned long flags;
 	
+	if (sdev->sdev_state != SDEV_BLOCK)
+		return 0;
+	
 	/* 
 	 * Try to transition the scsi device to SDEV_RUNNING
 	 * and goose the device queue if successful.  

RE: [PATCH] Another fix for suspend i/o - validate dev state before transition

[James.Smart]

James,

I have yet to see this hit scsi-misc-2.6...  hope it didn't get dropped.

-- james

> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org
> [mailto:linux-scsi-owner@vger.kernel.org]On Behalf Of Smart, James
> Sent: Wednesday, November 17, 2004 1:40 PM
> To: linux-scsi@vger.kernel.org
> Subject: [PATCH] Another fix for suspend i/o - validate dev 
> state before
> transition
> 
> 
> The block/unblock patch reports stack dumps and illegal 
> transitions when the driver continues to call block or 
> unblock after the device has failed (moved into offline state 
> due to i/o error, etc). The LLDD/transport is unaware of the 
> sdev transition, so it doesn't know better.
> 
> This patch has the internal device block/unblock routines 
> validate the state transition before invoking it.
> 
> Hopefully this is it for this darn feature...
> 
> -- James S
> 
> 
> 
> 
> --- scsi_lib.c.2.6.10-rc2	2004-11-16 09:03:58.000000000 -0500
> +++ scsi_lib.c.NEW	2004-11-17 13:23:22.340667480 -0500
> @@ -1813,6 +1813,12 @@
>   *	state, all commands are deferred until the scsi lld reenables
>   *	the device with scsi_device_unblock or device_block_tmo fires.
>   *	This routine assumes the host_lock is held on entry.
> + *
> + *      As the LLDD/Transport that is calling this function doesn't
> + *	actually know what the device state is, the function may be
> + *	called at an inappropriate time. Therefore, before requesting
> + *	the state change, the function validates that the transition is
> + *	valid.
>   **/
>  int
>  scsi_internal_device_block(struct scsi_device *sdev)
> @@ -1821,6 +1827,10 @@
>  	unsigned long flags;
>  	int err = 0;
>  
> +	if ((sdev->sdev_state != SDEV_CREATED) &&
> +	    (sdev->sdev_state != SDEV_RUNNING))
> +		return 0;
> +
>  	err = scsi_device_set_state(sdev, SDEV_BLOCK);
>  	if (err)
>  		return err;
> @@ -1853,6 +1863,12 @@
>   *	(which must be a legal transition) allowing the midlayer to
>   *	goose the queue for this device.  This routine assumes the 
>   *	host_lock is held upon entry.
> + *
> + *      As the LLDD/Transport that is calling this function doesn't
> + *	actually know what the device state is, the function may be
> + *	called at an inappropriate time. Therefore, before requesting
> + *	the state change, the function validates that the transition is
> + *	valid.
>   **/
>  int
>  scsi_internal_device_unblock(struct scsi_device *sdev)
> @@ -1861,6 +1877,9 @@
>  	int err;
>  	unsigned long flags;
>  	
> +	if (sdev->sdev_state != SDEV_BLOCK)
> +		return 0;
> +	
>  	/* 
>  	 * Try to transition the scsi device to SDEV_RUNNING
>  	 * and goose the device queue if successful.  
> -
> 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] Another fix for suspend i/o - validate dev state before transition

[James Bottomley]

On Wed, 2004-11-17 at 13:39 -0500, James.Smart@Emulex.Com wrote:
> The block/unblock patch reports stack dumps and illegal transitions when the driver continues to call block or unblock after the device has failed (moved into offline state due to i/o error, etc). The LLDD/transport is unaware of the sdev transition, so it doesn't know better.

> This patch has the internal device block/unblock routines validate the state transition before invoking it.

Well, my basic problem with this is that the idea of the state model is
to consolidate the allowed transitions in a single place.  Having the
calling routines second guess the transitions rather defeats that
purpose.

If the only problem with the current scheme is the somewhat scary
verbosity then we can shut it up by making the message part of the
logging infrastructure.

How about the attached (ERROR_RECOVERY is the closest log function I can
find which seems to fit the bill).

James

===== scsi_lib.c 1.140 vs edited =====
--- 1.140/drivers/scsi/scsi_lib.c	2004-11-28 06:12:22 -06:00
+++ edited/scsi_lib.c	2004-12-10 22:44:53 -06:00
@@ -1710,11 +1710,12 @@
 	return 0;
 
  illegal:
-	dev_printk(KERN_ERR, &sdev->sdev_gendev,
-		   "Illegal state transition %s->%s\n",
-		   scsi_device_state_name(oldstate),
-		   scsi_device_state_name(state));
-	WARN_ON(1);
+	SCSI_LOG_ERROR_RECOVERY(1, 
+				dev_printk(KERN_ERR, &sdev->sdev_gendev,
+					   "Illegal state transition %s->%s\n",
+					   scsi_device_state_name(oldstate),
+					   scsi_device_state_name(state))
+				);
 	return -EINVAL;
 }
 EXPORT_SYMBOL(scsi_device_set_state);