Re: [PATCH 1/1] scsi: Add added_to_sysfs sdev flag to fix device scanning oops

[Brian King <brking@linux.vnet.ibm.com>]

James Bottomley wrote:
> What this seems to be exposing is a bug in the state model around the
> blocked state.
> 
> The transition:
> 
> CREATED -> BLOCK -> RUNNING
> 
> shouldn't be legal.  My initial reaction is just to forbid the CREATED
> -> BLOCK transition, but it looks like the fc transport code never
> checks return values from scsi_target_block() (sigh!)
> 
> So an alternate fix should be to correct the state model rather than try
> and work around the deficiencies with additional flags.
> 
> It looks like, to allow the CREATED -> BLOCK -> CREATED transition we
> need an extra state (CREATED_BLOCK) and we forbid the CREATED -> BLOCK
> in favour of it.
> 
> The model also now allows an online transition to do CREATED_BLOCK ->
> BLOCK
> 
> This is a rough code of that, does it work for you?

I loaded up your patch and this fixes my issue as well. Do we need to also
add a check for SDEV_CREATED_BLOCK in scsi_dispatch_cmd?

Thanks,

Brian

> 
> James
> 
> ---
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index ff5d56b..42b33a3 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1250,6 +1250,7 @@ int scsi_prep_state_check(struct scsi_device *sdev, struct request *req)
>  			break;
>  		case SDEV_QUIESCE:
>  		case SDEV_BLOCK:
> +		case SDEV_CREATED_BLOCK:
>  			/*
>  			 * If the devices is blocked we defer normal commands.
>  			 */
> @@ -2063,10 +2064,13 @@ scsi_device_set_state(struct scsi_device *sdev, enum scsi_device_state state)
> 
>  	switch (state) {
>  	case SDEV_CREATED:
> -		/* There are no legal states that come back to
> -		 * created.  This is the manually initialised start
> -		 * state */
> -		goto illegal;
> +		switch (oldstate) {
> +		case SDEV_CREATED_BLOCK:
> +			break;
> +		default:
> +			goto illegal;
> +		}
> +		break;
>  			
>  	case SDEV_RUNNING:
>  		switch (oldstate) {
> @@ -2104,8 +2108,17 @@ scsi_device_set_state(struct scsi_device *sdev, enum scsi_device_state state)
> 
>  	case SDEV_BLOCK:
>  		switch (oldstate) {
> -		case SDEV_CREATED:
>  		case SDEV_RUNNING:
> +		case SDEV_CREATED_BLOCK:
> +			break;
> +		default:
> +			goto illegal;
> +		}
> +		break;
> +
> +	case SDEV_CREATED_BLOCK:
> +		switch (oldstate) {
> +		case SDEV_CREATED:
>  			break;
>  		default:
>  			goto illegal;
> @@ -2393,8 +2406,12 @@ scsi_internal_device_block(struct scsi_device *sdev)
>  	int err = 0;
> 
>  	err = scsi_device_set_state(sdev, SDEV_BLOCK);
> -	if (err)
> -		return err;
> +	if (err) {
> +		err = scsi_device_set_state(sdev, SDEV_CREATED_BLOCK);
> +
> +		if (err)
> +			return err;
> +	}
> 
>  	/* 
>  	 * The device has transitioned to SDEV_BLOCK.  Stop the
> @@ -2437,8 +2454,12 @@ scsi_internal_device_unblock(struct scsi_device *sdev)
>  	 * and goose the device queue if successful.  
>  	 */
>  	err = scsi_device_set_state(sdev, SDEV_RUNNING);
> -	if (err)
> -		return err;
> +	if (err) {
> +		err = scsi_device_set_state(sdev, SDEV_CREATED);
> +
> +		if (err)
> +			return err;
> +	}
> 
>  	spin_lock_irqsave(q->queue_lock, flags);
>  	blk_start_queue(q);
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index 84b4879..c6791a7 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -730,6 +730,8 @@ static int scsi_probe_lun(struct scsi_device *sdev, unsigned char *inq_result,
>  static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
>  		int *bflags, int async)
>  {
> +	int ret;
> +
>  	/*
>  	 * XXX do not save the inquiry, since it can change underneath us,
>  	 * save just vendor/model/rev.
> @@ -885,7 +887,17 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
> 
>  	/* set the device running here so that slave configure
>  	 * may do I/O */
> -	scsi_device_set_state(sdev, SDEV_RUNNING);
> +	ret = scsi_device_set_state(sdev, SDEV_RUNNING);
> +	if (ret) {
> +		ret = scsi_device_set_state(sdev, SDEV_BLOCK);
> +
> +		if (ret) {
> +			sdev_printk(KERN_ERR, sdev,
> +				    "in wrong state %s to complete scan\n",
> +				    scsi_device_state_name(sdev->sdev_state));
> +			return SCSI_SCAN_NO_RESPONSE;
> +		}
> +	}
> 
>  	if (*bflags & BLIST_MS_192_BYTES_FOR_3F)
>  		sdev->use_192_bytes_for_3f = 1;
> @@ -899,7 +911,7 @@ static int scsi_add_lun(struct scsi_device *sdev, unsigned char *inq_result,
>  	transport_configure_device(&sdev->sdev_gendev);
> 
>  	if (sdev->host->hostt->slave_configure) {
> -		int ret = sdev->host->hostt->slave_configure(sdev);
> +		ret = sdev->host->hostt->slave_configure(sdev);
>  		if (ret) {
>  			/*
>  			 * if LLDD reports slave not present, don't clutter
> @@ -994,7 +1006,8 @@ static int scsi_probe_and_add_lun(struct scsi_target *starget,
>  	 */
>  	sdev = scsi_device_lookup_by_target(starget, lun);
>  	if (sdev) {
> -		if (rescan || sdev->sdev_state != SDEV_CREATED) {
> +		if (rescan || (sdev->sdev_state != SDEV_CREATED &&
> +			       sdev->sdev_state != SDEV_CREATED_BLOCK)) {
>  			SCSI_LOG_SCAN_BUS(3, printk(KERN_INFO
>  				"scsi scan: device exists on %s\n",
>  				sdev->sdev_gendev.bus_id));
> @@ -1466,7 +1479,8 @@ static int scsi_report_lun_scan(struct scsi_target *starget, int bflags,
>  	kfree(lun_data);
>   out:
>  	scsi_device_put(sdev);
> -	if (sdev->sdev_state == SDEV_CREATED)
> +	if (sdev->sdev_state == SDEV_CREATED ||
> +	    sdev->sdev_state == SDEV_CREATED_BLOCK)
>  		/*
>  		 * the sdev we used didn't appear in the report luns scan
>  		 */
> diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> index ab3c718..09d311d 100644
> --- a/drivers/scsi/scsi_sysfs.c
> +++ b/drivers/scsi/scsi_sysfs.c
> @@ -34,6 +34,7 @@ static const struct {
>  	{ SDEV_QUIESCE, "quiesce" },
>  	{ SDEV_OFFLINE,	"offline" },
>  	{ SDEV_BLOCK,	"blocked" },
> +	{ SDEV_CREATED_BLOCK, "created-blocked" },
>  };
> 
>  const char *scsi_device_state_name(enum scsi_device_state state)
> diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
> index 80b2e93..ad3d42d 100644
> --- a/include/scsi/scsi_device.h
> +++ b/include/scsi/scsi_device.h
> @@ -42,9 +42,11 @@ enum scsi_device_state {
>  				 * originate in the mid-layer) */
>  	SDEV_OFFLINE,		/* Device offlined (by error handling or
>  				 * user request */
> -	SDEV_BLOCK,		/* Device blocked by scsi lld.  No scsi 
> -				 * commands from user or midlayer should be issued
> -				 * to the scsi lld. */
> +	SDEV_BLOCK,		/* Device blocked by scsi lld.  No
> +				 * scsi commands from user or midlayer
> +				 * should be issued to the scsi
> +				 * lld. */
> +	SDEV_CREATED_BLOCK,	/* same as above but for created devices */
>  };
> 
>  enum scsi_device_event {
> 
> 
> --
> 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


-- 
Brian King
Linux on Power Virtualization
IBM Linux Technology Center