d/freebsd-dev
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Before issing the REMOVE_DEVICE command to the firmware, make sure that all

Browse Source 
commands have completed.

It's not OK to force complete any pending commands before we send the
REMOVE_DEVICE. Instead, make sure that all pending commands are complete before
sending that. By trying to second guess the firmware here, we run the risk of
completing commands twice, which leads to corruption.

This removes the forced completion of commands introduced in r218811. So it's a
partial backout of that commit, but replaces it with a more rebust
mechanism. Either these commands will complete due to the TARGET RESET, or they
will timeout and be aborted, but they will all complete.

Add assert that all commands are complete to REMOVE_DEVICE completion
routine. We attempt to assure this programatically, so we shouldn't have any
commands in the queue because we've waited for them all. Any commands that make
it into our action routine after we mark the target in removal will complete
immediately with an error.

When we're removing a target that's not a volume, advertise up the stack that
it's actually gone, as opposed to having a transient selection error we should
retry. Do this both in the action routine, and when we get a notification of an
aborted command. We don't do this for volumes because the driver tries hard not
to advertise to the OS a volume has disappeared.

Apply these changes to both mpr and mps since they are based on quite similar
designs.

Discussed with: scottl@
Differential Revision: https://reviews.freebsd.org/D23768
This commit is contained in:
Warner Losh 2020-02-25 04:27:23 +00:00
parent b9931c0786
commit 4c1cdd4a7c
Notes: svn2git 2020-12-20 02:59:44 +00:00 
svn path=/head/; revision=358308
4 changed files with 109 additions and 36 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

71
sys/dev/mpr/mpr_sas.c
View File

@ -559,7 +559,6 @@ mprsas_remove_device(struct mpr_softc *sc, struct mpr_command *tm)
MPI2_SCSI_TASK_MANAGE_REPLY *reply;
MPI2_SAS_IOUNIT_CONTROL_REQUEST *req;
struct mprsas_target *targ;
struct mpr_command *next_cm;
uint16_t handle;
MPR_FUNCTRACE(sc);
@ -609,7 +608,18 @@ mprsas_remove_device(struct mpr_softc *sc, struct mpr_command *tm)
tm->cm_complete = mprsas_remove_complete;
tm->cm_complete_data = (void *)(uintptr_t)handle;
mpr_map_command(sc, tm);
/*
* Wait to send the REMOVE_DEVICE until all the commands have cleared.
* They should be aborted or time out and we'll kick thus off there
* if so.
*/
if (TAILQ_FIRST(&targ->commands) == NULL) {
mpr_dprint(sc, MPR_INFO, "No pending commands: starting remove_device\n");
mpr_map_command(sc, tm);
targ->pending_remove_tm = NULL;
} else {
targ->pending_remove_tm = tm;
}
mpr_dprint(sc, MPR_INFO, "clearing target %u handle 0x%04x\n",
targ->tid, handle);
@ -618,15 +628,6 @@ mprsas_remove_device(struct mpr_softc *sc, struct mpr_command *tm)
"connector name (%4s)\n", targ->encl_level, targ->encl_slot,
targ->connector_name);
}
TAILQ_FOREACH_SAFE(tm, &targ->commands, cm_link, next_cm) {
union ccb *ccb;
mpr_dprint(sc, MPR_XINFO, "Completing missed command %p\n", tm);
ccb = tm->cm_complete_data;
mprsas_set_ccbstatus(ccb, CAM_DEV_NOT_THERE);
tm->cm_state = MPR_CM_STATE_BUSY;
mprsas_scsiio_complete(sc, tm);
}
}
static void
@ -642,6 +643,15 @@ mprsas_remove_complete(struct mpr_softc *sc, struct mpr_command *tm)
reply = (MPI2_SAS_IOUNIT_CONTROL_REPLY *)tm->cm_reply;
handle = (uint16_t)(uintptr_t)tm->cm_complete_data;
targ = tm->cm_targ;
/*
* At this point, we should have no pending commands for the target.
* The remove target has just completed.
*/
KASSERT(TAILQ_FIRST(&targ->commands) == NULL,
("%s: no commands should be pending\n", __func__));
/*
* Currently there should be no way we can hit this case. It only
* happens when we have a failure to allocate chain frames, and
@ -674,7 +684,6 @@ mprsas_remove_complete(struct mpr_softc *sc, struct mpr_command *tm)
*/
if ((le16toh(reply->IOCStatus) & MPI2_IOCSTATUS_MASK) ==
MPI2_IOCSTATUS_SUCCESS) {
targ = tm->cm_targ;
targ->handle = 0x0;
targ->encl_handle = 0x0;
targ->encl_level_valid = 0x0;
@ -1964,13 +1973,17 @@ mprsas_action_scsiio(struct mprsas_softc *sassc, union ccb *ccb)
/*
* If devinfo is 0 this will be a volume. In that case don't tell CAM
* that the volume has timed out. We want volumes to be enumerated
* until they are deleted/removed, not just failed.
* until they are deleted/removed, not just failed. In either event,
* we're removing the target due to a firmware event telling us
* the device is now gone (as opposed to some transient event). Since
* we're opting to remove failed devices from the OS's view, we need
* to propagate that status up the stack.
*/
if (targ->flags & MPRSAS_TARGET_INREMOVAL) {
if (targ->devinfo == 0)
mprsas_set_ccbstatus(ccb, CAM_REQ_CMP);
else
mprsas_set_ccbstatus(ccb, CAM_SEL_TIMEOUT);
mprsas_set_ccbstatus(ccb, CAM_DEV_NOT_THERE);
xpt_done(ccb);
return;
}
@ -2844,15 +2857,22 @@ mprsas_scsiio_complete(struct mpr_softc *sc, struct mpr_command *cm)
* potential risk of flagging false failures in the event
* of a topology-related error (e.g. a SAS expander problem
* causes a command addressed to a drive to fail), but
* avoiding getting into an infinite retry loop.
* avoiding getting into an infinite retry loop. However,
* if we get them while were moving a device, we should
* fail the request as 'not there' because the device
* is effectively gone.
*/
mprsas_set_ccbstatus(ccb, CAM_REQ_CMP_ERR);
if (cm->cm_targ->flags & MPRSAS_TARGET_INREMOVAL)
mprsas_set_ccbstatus(ccb, CAM_DEV_NOT_THERE);
else
mprsas_set_ccbstatus(ccb, CAM_REQ_CMP_ERR);
mpr_dprint(sc, MPR_INFO,
"Controller reported %s tgt %u SMID %u loginfo %x\n",
"Controller reported %s tgt %u SMID %u loginfo %x%s\n",
mpr_describe_table(mpr_iocstatus_string,
le16toh(rep->IOCStatus) & MPI2_IOCSTATUS_MASK),
target_id, cm->cm_desc.Default.SMID,
le32toh(rep->IOCLogInfo));
le32toh(rep->IOCLogInfo),
(cm->cm_targ->flags & MPRSAS_TARGET_INREMOVAL) ? " departing" : "");
mpr_dprint(sc, MPR_XINFO,
"SCSIStatus %x SCSIState %x xfercount %u\n",
rep->SCSIStatus, rep->SCSIState,
@ -2900,6 +2920,21 @@ mprsas_scsiio_complete(struct mpr_softc *sc, struct mpr_command *cm)
xpt_freeze_devq(ccb->ccb_h.path, /*count*/ 1);
}
/*
* Check to see if we're removing the device. If so, and this is the
* last command on the queue, proceed with the deferred removal of the
* device. Note, for removing a volume, this won't trigger because
* pending_remove_tm will be NULL.
*/
if (cm->cm_targ->flags & MPRSAS_TARGET_INREMOVAL) {
if (TAILQ_FIRST(&cm->cm_targ->commands) == NULL &&
cm->cm_targ->pending_remove_tm != NULL) {
mpr_dprint(sc, MPR_INFO, "Last pending command complete: starting remove_device\n");
mpr_map_command(sc, cm->cm_targ->pending_remove_tm);
cm->cm_targ->pending_remove_tm = NULL;
}
}
mpr_free_command(sc, cm);
xpt_done(ccb);
}

1
sys/dev/mpr/mpr_sas.h
View File

@ -64,6 +64,7 @@ struct mprsas_target {
SLIST_HEAD(, mprsas_lun) luns;
TAILQ_HEAD(, mpr_command) commands;
struct mpr_command *tm;
struct mpr_command *pending_remove_tm;
TAILQ_HEAD(, mpr_command) timedout_commands;
uint16_t exp_dev_handle;
uint16_t phy_num;

72
sys/dev/mps/mps_sas.c
View File

@ -556,7 +556,6 @@ mpssas_remove_device(struct mps_softc *sc, struct mps_command *tm)
MPI2_SCSI_TASK_MANAGE_REPLY *reply;
MPI2_SAS_IOUNIT_CONTROL_REQUEST *req;
struct mpssas_target *targ;
struct mps_command *next_cm;
uint16_t handle;
MPS_FUNCTRACE(sc);
@ -609,19 +608,22 @@ mpssas_remove_device(struct mps_softc *sc, struct mps_command *tm)
tm->cm_complete = mpssas_remove_complete;
tm->cm_complete_data = (void *)(uintptr_t)handle;
mps_map_command(sc, tm);
/*
* Wait to send the REMOVE_DEVICE until all the commands have cleared.
* They should be aborted or time out and we'll kick thus off there
* if so.
*/
if (TAILQ_FIRST(&targ->commands) == NULL) {
mps_dprint(sc, MPS_INFO, "No pending commands: starting remove_device\n");
mps_map_command(sc, tm);
targ->pending_remove_tm = NULL;
} else {
targ->pending_remove_tm = tm;
}
mps_dprint(sc, MPS_XINFO, "clearing target %u handle 0x%04x\n",
targ->tid, handle);
TAILQ_FOREACH_SAFE(tm, &targ->commands, cm_link, next_cm) {
union ccb *ccb;
mps_dprint(sc, MPS_XINFO, "Completing missed command %p\n", tm);
ccb = tm->cm_complete_data;
mpssas_set_ccbstatus(ccb, CAM_DEV_NOT_THERE);
tm->cm_state = MPS_CM_STATE_BUSY;
mpssas_scsiio_complete(sc, tm);
}
}
static void
@ -636,6 +638,15 @@ mpssas_remove_complete(struct mps_softc *sc, struct mps_command *tm)
reply = (MPI2_SAS_IOUNIT_CONTROL_REPLY *)tm->cm_reply;
handle = (uint16_t)(uintptr_t)tm->cm_complete_data;
targ = tm->cm_targ;
/*
* At this point, we should have no pending commands for the target.
* The remove target has just completed.
*/
KASSERT(TAILQ_FIRST(&targ->commands) == NULL,
("%s: no commands should be pending\n", __func__));
/*
* Currently there should be no way we can hit this case. It only
@ -671,7 +682,6 @@ mpssas_remove_complete(struct mps_softc *sc, struct mps_command *tm)
*/
if ((le16toh(reply->IOCStatus) & MPI2_IOCSTATUS_MASK) ==
MPI2_IOCSTATUS_SUCCESS) {
targ = tm->cm_targ;
targ->handle = 0x0;
targ->encl_handle = 0x0;
targ->encl_slot = 0x0;
@ -1713,13 +1723,17 @@ mpssas_action_scsiio(struct mpssas_softc *sassc, union ccb *ccb)
/*
* If devinfo is 0 this will be a volume. In that case don't tell CAM
* that the volume has timed out. We want volumes to be enumerated
* until they are deleted/removed, not just failed.
* until they are deleted/removed, not just failed. In either event,
* we're removing the target due to a firmware event telling us
* the device is now gone (as opposed to some transient event). Since
* we're opting to remove failed devices from the OS's view, we need
* to propagate that status up the stack.
*/
if (targ->flags & MPSSAS_TARGET_INREMOVAL) {
if (targ->devinfo == 0)
mpssas_set_ccbstatus(ccb, CAM_REQ_CMP);
else
mpssas_set_ccbstatus(ccb, CAM_SEL_TIMEOUT);
mpssas_set_ccbstatus(ccb, CAM_DEV_NOT_THERE);
xpt_done(ccb);
return;
}
@ -2352,15 +2366,22 @@ mpssas_scsiio_complete(struct mps_softc *sc, struct mps_command *cm)
* potential risk of flagging false failures in the event
* of a topology-related error (e.g. a SAS expander problem
* causes a command addressed to a drive to fail), but
* avoiding getting into an infinite retry loop.
* avoiding getting into an infinite retry loop. However,
* if we get them while were moving a device, we should
* fail the request as 'not there' because the device
* is effectively gone.
*/
mpssas_set_ccbstatus(ccb, CAM_REQ_CMP_ERR);
if (cm->cm_targ->flags & MPSSAS_TARGET_INREMOVAL)
mpssas_set_ccbstatus(ccb, CAM_DEV_NOT_THERE);
else
mpssas_set_ccbstatus(ccb, CAM_REQ_CMP_ERR);
mps_dprint(sc, MPS_INFO,
"Controller reported %s tgt %u SMID %u loginfo %x\n",
"Controller reported %s tgt %u SMID %u loginfo %x%s\n",
mps_describe_table(mps_iocstatus_string,
le16toh(rep->IOCStatus) & MPI2_IOCSTATUS_MASK),
target_id, cm->cm_desc.Default.SMID,
le32toh(rep->IOCLogInfo));
le32toh(rep->IOCLogInfo),
(cm->cm_targ->flags & MPSSAS_TARGET_INREMOVAL) ? " departing" : "");
mps_dprint(sc, MPS_XINFO,
"SCSIStatus %x SCSIState %x xfercount %u\n",
rep->SCSIStatus, rep->SCSIState,
@ -2401,6 +2422,21 @@ mpssas_scsiio_complete(struct mps_softc *sc, struct mps_command *cm)
xpt_freeze_devq(ccb->ccb_h.path, /*count*/ 1);
}
/*
* Check to see if we're removing the device. If so, and this is the
* last command on the queue, proceed with the deferred removal of the
* device. Note, for removing a volume, this won't trigger because
* pending_remove_tm will be NULL.
*/
if (cm->cm_targ->flags & MPSSAS_TARGET_INREMOVAL) {
if (TAILQ_FIRST(&cm->cm_targ->commands) == NULL &&
cm->cm_targ->pending_remove_tm != NULL) {
mps_dprint(sc, MPS_INFO, "Last pending command complete: starting remove_device\n");
mps_map_command(sc, cm->cm_targ->pending_remove_tm);
cm->cm_targ->pending_remove_tm = NULL;
}
}
mps_free_command(sc, cm);
xpt_done(ccb);
}

1
sys/dev/mps/mps_sas.h
View File

@ -62,6 +62,7 @@ struct mpssas_target {
SLIST_HEAD(, mpssas_lun) luns;
TAILQ_HEAD(, mps_command) commands;
struct mps_command *tm;
struct mps_command *pending_remove_tm;
TAILQ_HEAD(, mps_command) timedout_commands;
uint16_t exp_dev_handle;
uint16_t phy_num;