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

Fix da(4) locking when probing SMR drives.

Browse Source 
Probing host aware and host managed SMR drives got broken in revision
330796.

The added cam_periph_lock() calls were in areas in dadone() where
the peripheral lock was already held.

Since then, dadone() has been split into separate functions that are
dedicated to each probe state.

The result is that when probing a host aware drive, I ran into a recursive
lock acquisition in dadone_probeatalogdir(). I would have run into the
same problem in dadone_probeataiddir(), and in dadone_probeatasup() and
dadone_probeatazone() in the error paths had the probe continued.

The solution is to take out all of the extra cam_periph_lock() calls. I
also added cam_periph_assert(periph, MA_OWNED) near the top of each of
the dadone_* calls. These make it clear to anyone coming along in the
the future that the lock is held in the probe done functions.

Also add a locking assert in daprobedone(), to make it clear that it must
be called with the periph lock held.

Sponsored by:	Spectra Logic
Differential Revision:	https://reviews.freebsd.org/D15764
This commit is contained in:
ken 2018-06-14 17:08:44 +00:00
parent 6dd6964074
commit 896df23a52
1 changed files with 27 additions and 8 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

35
sys/cam/scsi/scsi_da.c
View File

@ -2428,6 +2428,8 @@ daprobedone(struct cam_periph *periph, union ccb *ccb)
softc = (struct da_softc *)periph->softc;
cam_periph_assert(periph, MA_OWNED);
dadeletemethodchoose(softc, DA_DELETE_NONE);
if (bootverbose && (softc->flags & DA_FLAG_ANNOUNCED) == 0) {
@ -4505,6 +4507,8 @@ dadone_probewp(struct cam_periph *periph, union ccb *done_ccb)
priority = done_ccb->ccb_h.pinfo.priority;
csio = &done_ccb->csio;
cam_periph_assert(periph, MA_OWNED);
if (softc->minimum_cmd_size > 6) {
mode_hdr10 = (struct scsi_mode_header_10 *)csio->data_ptr;
dev_spec = mode_hdr10->dev_spec;
@ -4578,6 +4582,8 @@ dadone_proberc(struct cam_periph *periph, union ccb *done_ccb)
rcaplong = (struct scsi_read_capacity_data_long *)
csio->data_ptr;
cam_periph_assert(periph, MA_OWNED);
if ((csio->ccb_h.status & CAM_STATUS_MASK) == CAM_REQ_CMP) {
struct disk_params *dp;
uint32_t block_size;
@ -4835,6 +4841,8 @@ dadone_probelbp(struct cam_periph *periph, union ccb *done_ccb)
csio = &done_ccb->csio;
lbp = (struct scsi_vpd_logical_block_prov *)csio->data_ptr;
cam_periph_assert(periph, MA_OWNED);
if ((csio->ccb_h.status & CAM_STATUS_MASK) == CAM_REQ_CMP) {
/*
* T10/1799-D Revision 31 states at least one of these
@ -4891,6 +4899,8 @@ dadone_probeblklimits(struct cam_periph *periph, union ccb *done_ccb)
csio = &done_ccb->csio;
block_limits = (struct scsi_vpd_block_limits *)csio->data_ptr;
cam_periph_assert(periph, MA_OWNED);
if ((csio->ccb_h.status & CAM_STATUS_MASK) == CAM_REQ_CMP) {
uint32_t max_txfer_len = scsi_4btoul(
block_limits->max_txfer_len);
@ -4983,6 +4993,8 @@ dadone_probebdc(struct cam_periph *periph, union ccb *done_ccb)
csio = &done_ccb->csio;
bdc = (struct scsi_vpd_block_device_characteristics *)csio->data_ptr;
cam_periph_assert(periph, MA_OWNED);
if ((csio->ccb_h.status & CAM_STATUS_MASK) == CAM_REQ_CMP) {
uint32_t valid_len;
@ -5088,6 +5100,8 @@ dadone_probeata(struct cam_periph *periph, union ccb *done_ccb)
continue_probe = 0;
error = 0;
cam_periph_assert(periph, MA_OWNED);
if ((csio->ccb_h.status & CAM_STATUS_MASK) == CAM_REQ_CMP) {
uint16_t old_rate;
@ -5223,7 +5237,7 @@ dadone_probeatalogdir(struct cam_periph *periph, union ccb *done_ccb)
priority = done_ccb->ccb_h.pinfo.priority;
csio = &done_ccb->csio;
cam_periph_lock(periph);
cam_periph_assert(periph, MA_OWNED);
if ((csio->ccb_h.status & CAM_STATUS_MASK) == CAM_REQ_CMP) {
error = 0;
softc->valid_logdir_len = 0;
@ -5276,7 +5290,6 @@ dadone_probeatalogdir(struct cam_periph *periph, union ccb *done_ccb)
}
}
}
cam_periph_unlock(periph);
free(csio->data_ptr, M_SCSIDA);
@ -5305,7 +5318,8 @@ dadone_probeataiddir(struct cam_periph *periph, union ccb *done_ccb)
priority = done_ccb->ccb_h.pinfo.priority;
csio = &done_ccb->csio;
cam_periph_lock(periph);
cam_periph_assert(periph, MA_OWNED);
if ((csio->ccb_h.status & CAM_STATUS_MASK) == CAM_REQ_CMP) {
off_t entries_offset, max_entries;
error = 0;
@ -5368,7 +5382,6 @@ dadone_probeataiddir(struct cam_periph *periph, union ccb *done_ccb)
}
}
}
cam_periph_unlock(periph);
free(csio->data_ptr, M_SCSIDA);
@ -5396,6 +5409,8 @@ dadone_probeatasup(struct cam_periph *periph, union ccb *done_ccb)
priority = done_ccb->ccb_h.pinfo.priority;
csio = &done_ccb->csio;
cam_periph_assert(periph, MA_OWNED);
if ((csio->ccb_h.status & CAM_STATUS_MASK) == CAM_REQ_CMP) {
uint32_t valid_len;
size_t needed_size;
@ -5466,9 +5481,7 @@ dadone_probeatasup(struct cam_periph *periph, union ccb *done_ccb)
* Supported Capabilities page, clear the
* flag...
*/
cam_periph_lock(periph);
softc->flags &= ~DA_FLAG_CAN_ATA_SUPCAP;
cam_periph_unlock(periph);
/*
* And clear zone capabilities.
*/
@ -5508,6 +5521,8 @@ dadone_probeatazone(struct cam_periph *periph, union ccb *done_ccb)
softc = (struct da_softc *)periph->softc;
csio = &done_ccb->csio;
cam_periph_assert(periph, MA_OWNED);
if ((csio->ccb_h.status & CAM_STATUS_MASK) == CAM_REQ_CMP) {
struct ata_zoned_info_log *zi_log;
uint32_t valid_len;
@ -5568,10 +5583,8 @@ dadone_probeatazone(struct cam_periph *periph, union ccb *done_ccb)
if (error == ERESTART)
return;
else if (error != 0) {
cam_periph_lock(periph);
softc->flags &= ~DA_FLAG_CAN_ATA_ZONE;
softc->flags &= ~DA_ZONE_FLAG_SET_MASK;
cam_periph_unlock(periph);
if ((done_ccb->ccb_h.status & CAM_DEV_QFRZN) != 0) {
/* Don't wedge this device's queue */
@ -5584,6 +5597,7 @@ dadone_probeatazone(struct cam_periph *periph, union ccb *done_ccb)
}
}
free(csio->data_ptr, M_SCSIDA);
daprobedone(periph, done_ccb);
@ -5602,6 +5616,8 @@ dadone_probezone(struct cam_periph *periph, union ccb *done_ccb)
softc = (struct da_softc *)periph->softc;
csio = &done_ccb->csio;
cam_periph_assert(periph, MA_OWNED);
if ((csio->ccb_h.status & CAM_STATUS_MASK) == CAM_REQ_CMP) {
uint32_t valid_len;
size_t needed_len;
@ -5672,6 +5688,9 @@ dadone_tur(struct cam_periph *periph, union ccb *done_ccb)
softc = (struct da_softc *)periph->softc;
csio = &done_ccb->csio;
cam_periph_assert(periph, MA_OWNED);
if ((done_ccb->ccb_h.status & CAM_STATUS_MASK) != CAM_REQ_CMP) {
if (daerror(done_ccb, CAM_RETRY_SELTO,