Re: [RFC][PATCH] limit state change to SDEV_BLOCK devices in scsi_internal_device_unblock

[Takahiro Yasui <tyasui@redhat.com>]

Takahiro Yasui wrote:
> Matthew Wilcox wrote:
>> On Mon, Apr 27, 2009 at 07:52:57PM -0400, Takahiro Yasui wrote:
>>> James Bottomley wrote:
>>>>> +	if (sdev->sdev_state != SDEV_BLOCK)
>>>> This isn't quite correct.  There are two blocked states in the model
>>>> currently:  SDEV_BLOCK and SDEV_CREATED_BLOCK ... you'd need to check
>>>> for both of them
>>>>
>>>>> +		return 0;
>>>> Traditionally the return for an attempted invalid state transition is
>>>> -EINVAL.
>>>>
>>>> I suppose for lower down, if you check the state, now we know we go 
>>>>
>>>> SDEV_CREATED_BLOCK -> SDEV_CREATED
>>>>
>>>> or
>>>>
>>>> SDEV_BLOCK -> SDEV_RUNNING
>>>>
>>>> so there's no need for the dual scsi_device_set_state.
>>> Thank you for the comments. If I understand your comments correctly, 
>>> the state transition is better to be defined in scsi_device_set_state(),
>>> and both transitions, "SDEV_CREATED_BLOCK -> SDEV_CREATED" and
>>> "SDEV_BLOCK -> SDEV_RUNNING" need to be covered. I updated the patch
>>> so that the transition of "SDEV_OFFLINE -> SDEV_RUNNING" is prohibited.
>> I don't think that's what he meant.  I think he meant something more like:

I have one question about the order of comparisons. The original code
tries to change to SDEV_BLOCK first and retries to SDEV_CREATED.
If they are transformed just that order, I think that comparisons will
be as follows.

	if (sdev->sdev_state == SDEV_BLOCK)
		sdev->sdev_state = SDEV_RUNNING;
	else if (sdev->sdev_state == SDEV_CREATED_BLOCK)
		sdev->sdev_state = SDEV_CREATED;
	else
		return -EINVAL;

Is this wrong order?

Regards,
---
Takahiro Yasui
Hitachi Computer Products (America), Inc.