limit state change to SDEV_BLOCK devices in scsi_internal_device_unblock
RFC PATCH 1/1 limit state change to SDEV_BLOCK devices in scsi_internal_device_unblock
Hi,
This is a patch to fix SCSI timeout error recovery issue which I posted about one week ago.
[RFC] SCSI timeout error recovery issue
http://marc.info/?l=linux-scsi&m=124042136915970&w=2
scsi timeout on two or more devices may cause extremely long execution time for user applications because SDEV_OFFLINE state is changed to SDEV_RUNNING state during scsi error recovery procedures triggered by a bus reset or a host reset of LLD, and scsi timeout can happens on the same devices many times.
This happens because scsi_internal_device_unblock() changes device's state to SDEV_RUNNING even if a device in other states than SDEV_BLOCK, but scsi_internal_device_block() is a pair with scsi_internal_device_unblock() and only devices in SDEV_BLOCK state are needed to be changed. This patch adds a check of the current device state so that state change and queue activation is not executed for devices in SDEV_BLOCK state.
This patch is based on the idea that devices in SDEV_OFFLINE state need to stay in SDEV_OFFLINE because SDEV_OFFLINE state is done by:
- All recovery procedures, bus reset and host reset, has already failed for devices in SDEV_OFFLINE state.
(There is no (or rare?) chance to be recovered.)
- A user has changed the device's state to SDEV_OFFLINE intentionally by an interface such as sysfs.
(A user needs to enable devices in SDEV_OFFLINE state before using
them again.)
I appreciate any comments and suggestions on this patch.
Regards,
---
Takahiro Yasui
Hitachi Computer Products (America), Inc.
--- drivers/scsi/scsi_lib.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
drivers/scsi/scsi_lib.c +5 -2
@@ -2633,9 +2633,12 @@ scsi_internal_device_unblock(struct scsiunsigned long flags;/* -* Try to transition the scsi device to SDEV_RUNNING -* and goose the device queue if successful. +* Try to transition the scsi device to SDEV_RUNNING if it is +* SDEV_BLOCK and goose the device queue if successful. */ +if (sdev->sdev_state !=SDEV_BLOCK ) +return 0 ; + err =scsi_device_set_state (sdev,SDEV_RUNNING );if (err) { err =scsi_device_set_state (sdev,SDEV_CREATED );
drivers/scsi/scsi_lib.c scsi_internal_device_unblock(struct scsi · line 2637
Quoted 8 lines · Takahiro Yasui
drivers/scsi/scsi_lib.c +2
-2@@ -2633,9 +2633,12 @@ scsi_internal_device_unblock(struct scsiunsigned long flags;/* -* Try to transition the scsi device to SDEV_RUNNING -* and goose the device queue if successful. +* Try to transition the scsi device to SDEV_RUNNING if it is +* SDEV_BLOCK and goose the device queue if successful.
I think the code looks good, but the edit to the comment dilutes its flavour somewhat. How about just moving the comment down below the check, and then you don't need to edit the comment at all?
Quoted 15 lines
drivers/scsi/scsi_lib.c +3
-1*/ +if (sdev->sdev_state !=SDEV_BLOCK ) +return 0 ; + err =scsi_device_set_state (sdev,SDEV_RUNNING );if (err) { err =scsi_device_set_state (sdev,SDEV_CREATED ); --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 · 4 lines
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."
Quoted 4 lines · Matthew Wilcox
Quoted 3 lines · Takahiro Yasui
(quoted diff) +0
-0@@ -2633,9 +2633,12 @@ scsi_internal_device_unblock(struct scsi unsigned long flags;
Quoted 9 lines
Review scsi_internal_device_unblock(struct scsi · line 2637
Quoted 5 lines
(quoted diff) +2
-2/* - * Try to transition the scsi device to SDEV_RUNNING - * and goose the device queue if successful. + * Try to transition the scsi device to SDEV_RUNNING if it is + * SDEV_BLOCK and goose the device queue if successful.I think the code looks good, but the edit to the comment dilutes its flavour somewhat. How about just moving the comment down below the check, and then you don't need to edit the comment at all?
Thank you for the suggestion. I updated the patch according to your comment.
Regards,
---
Takahiro Yasui
Hitachi Computer Products (America), Inc.
--- drivers/scsi/scsi_lib.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/scsi/scsi_lib.c +4 -1
@@ -2631,7 +2631,10 @@ scsi_internal_device_unblock(struct scsistruct request_queue *q = sdev->request_queue;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.
James Bottomley
maintainerQuoted 28 lines · Takahiro Yasui
Hi,
This is a patch to fix SCSI timeout error recovery issue which I posted about one week ago.
[RFC] SCSI timeout error recovery issue
http://marc.info/?l=linux-scsi&m=124042136915970&w=2scsi timeout on two or more devices may cause extremely long execution time for user applications because
SDEV_OFFLINEstate is changed toSDEV_RUNNINGstate during scsi error recovery procedures triggered by a bus reset or a host reset of LLD, and scsi timeout can happens on the same devices many times.This happens because
scsi_internal_device_unblock()changes device's state toSDEV_RUNNINGeven if a device in other states thanSDEV_BLOCK, butscsi_internal_device_block()is a pair withscsi_internal_device_unblock()and only devices inSDEV_BLOCKstate are needed to be changed. This patch adds a check of the current device state so that state change and queue activation is not executed for devices inSDEV_BLOCKstate.This patch is based on the idea that devices in
SDEV_OFFLINEstate need to stay inSDEV_OFFLINEbecauseSDEV_OFFLINEstate is done by:- All recovery procedures, bus reset and host reset, has already failed for devices in
SDEV_OFFLINEstate.
(There is no (or rare?) chance to be recovered.)
Quoted 6 lines
- A user has changed the device's state to
SDEV_OFFLINEintentionally by an interface such as sysfs.
(A user needs to enable devices inSDEV_OFFLINEstate before using
them again.)I appreciate any comments and suggestions on this patch.
The analysis seems good, and the idea of putting state guards to make sure we only transition from a correct state is fine.
The actual state transitions only check that going from in state to out is permitted by the state model ... rather than actually being correct for what the function is trying to do.
Quoted 16 lines
Regards,
---
Takahiro Yasui
Hitachi Computer Products (America), Inc.Signed-off-by Takahiro Yasui <[email protected]>--- drivers/scsi/scsi_lib.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)Index:
linux-2.6.29/drivers/scsi/scsi_lib.c
===================================================================
drivers/scsi/scsi_lib.c scsi_internal_device_unblock(struct scsi · line 2639
Quoted 9 lines
drivers/scsi/scsi_lib.c +3
-2@@ -2633,9 +2633,12 @@ scsi_internal_device_unblock(struct scsiunsigned long flags;/* -* Try to transition the scsi device to SDEV_RUNNING -* and goose the device queue if successful. +* Try to transition the scsi device to SDEV_RUNNING if it is +* SDEV_BLOCK and goose the device queue if successful. */ +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
drivers/scsi/scsi_lib.c scsi_internal_device_unblock(struct scsi · line 2640
Quoted 1 line
drivers/scsi/scsi_lib.c +1
-0+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.
James
Quoted 13 lines · James Bottomley
Review
Quoted 1 line
(quoted diff) +1
-0+ if (sdev->sdev_state != SDEV_BLOCK)This isn't quite correct. There are two blocked states in the model
currently:SDEV_BLOCKandSDEV_CREATED_BLOCK... you'd need to check for both of themReview
Quoted 1 line
(quoted diff) +1
-0+ 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
Quoted 7 lines
SDEV_CREATED_BLOCK->SDEV_CREATEDor
SDEV_BLOCK->SDEV_RUNNINGso 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.
A note in this change is all transitions of "SDEV_OFFLINE -> SDEV_RUNNING" is prohibited, and the following state change for devices in SDEV_OFFLINE state fails.
# echo running > /sys/block/sdX/device/state
Therefore, users need to delete the device once and then those devices need to be scanned as follows.
# echo yes > /sys/block/sdX/device/delete # echo "- - -" > /sys/class/scsi_host/hostX/scan
I appreciate your reviews on it.
Regards,
---
Takahiro Yasui
Hitachi Computer Products (America), Inc.
--- drivers/scsi/scsi_lib.c | 1 - 1 file changed, 1 deletion(-)
drivers/scsi/scsi_lib.c +0 -1
@@ -2258,7 +2258,6 @@ scsi_device_set_state(struct scsi_devicecase SDEV_RUNNING :switch (oldstate) {case SDEV_CREATED : -case SDEV_OFFLINE :case SDEV_QUIESCE :case SDEV_BLOCK :break ;
Quoted 20 lines · Takahiro Yasui
Quoted 19 lines · James Bottomley
Review
Quoted 1 line
(quoted diff) +1
-0+ if (sdev->sdev_state != SDEV_BLOCK)This isn't quite correct. There are two blocked states in the model
currently:SDEV_BLOCKandSDEV_CREATED_BLOCK... you'd need to check for both of themReview
Quoted 1 line
(quoted diff) +1
-0+ 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_CREATEDor
SDEV_BLOCK->SDEV_RUNNING
Quoted 7 lines
Quoted 1 line
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:
scsi_internal_device_unblock(struct scsi_device *sdev)
{
struct request_queue *q = sdev->request_queue;
- int err;
unsigned long flags;
/*
* Try to transition the scsi device to SDEV_RUNNING
* and goose the device queue if successful.
*/+ if (sdev->sdev_state == SDEV_CREATED_BLOCK)
+ sdev->sdev_state = SDEV_CREATED;
+ else if (sdev->sdev_state == SDEV_BLOCK)
+ sdev->sdev_state = SDEV_RUNNING;
+ else
+ return -EINVAL;
- err = scsi_device_set_state(sdev, SDEV_RUNNING);
- 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); spin_unlock_irqrestore(q->queue_lock, flags); return 0; }
Signature · 4 lines
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."
Quoted 19 lines · Matthew Wilcox
Quoted 18 lines · Takahiro Yasui
Quoted 17 lines · James Bottomley
Review
Quoted 1 line
(quoted diff) +1
-0+ if (sdev->sdev_state != SDEV_BLOCK)This isn't quite correct. There are two blocked states in the model
currently:SDEV_BLOCKandSDEV_CREATED_BLOCK... you'd need to check for both of themReview
Quoted 1 line
(quoted diff) +1
-0+ 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_CREATEDor
SDEV_BLOCK->SDEV_RUNNING
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
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:
Thanks a lot. I'll build a patch and send it.
Regards,
---
Takahiro Yasui
Hitachi Computer Products (America), Inc.
Quoted 20 lines · Takahiro Yasui
Quoted 19 lines · Matthew Wilcox
Quoted 18 lines · Takahiro Yasui
Quoted 17 lines · James Bottomley
Review
Quoted 1 line
(quoted diff) +1
-0+ if (sdev->sdev_state != SDEV_BLOCK)This isn't quite correct. There are two blocked states in the model
currently:SDEV_BLOCKandSDEV_CREATED_BLOCK... you'd need to check for both of themReview
Quoted 1 line
(quoted diff) +1
-0+ 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_CREATEDor
SDEV_BLOCK->SDEV_RUNNING
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
Quoted 1 line
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.