Date: Thu, 14 Jun 2018 17:08:44 +0000 (UTC) From: "Kenneth D. Merry" <ken@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r335154 - head/sys/cam/scsi Message-ID: <201806141708.w5EH8iWM087636@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: ken Date: Thu Jun 14 17:08:44 2018 New Revision: 335154 URL: https://svnweb.freebsd.org/changeset/base/335154 Log: Fix da(4) locking when probing SMR drives. 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 Modified: head/sys/cam/scsi/scsi_da.c Modified: head/sys/cam/scsi/scsi_da.c ============================================================================== --- head/sys/cam/scsi/scsi_da.c Thu Jun 14 17:06:19 2018 (r335153) +++ head/sys/cam/scsi/scsi_da.c Thu Jun 14 17:08:44 2018 (r335154) @@ -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 *d 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 *d 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 * 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 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 * 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 * 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 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 } } } - cam_periph_unlock(periph); free(csio->data_ptr, M_SCSIDA); @@ -5305,7 +5318,8 @@ dadone_probeataiddir(struct cam_periph *periph, union 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 } } } - cam_periph_unlock(periph); free(csio->data_ptr, M_SCSIDA); @@ -5396,6 +5409,8 @@ dadone_probeatasup(struct cam_periph *periph, union cc 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 cc * 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 c 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 c 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 c } } + free(csio->data_ptr, M_SCSIDA); daprobedone(periph, done_ccb); @@ -5602,6 +5616,8 @@ dadone_probezone(struct cam_periph *periph, union 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_ 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,
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201806141708.w5EH8iWM087636>