Date: Thu, 19 Jun 2014 13:19:35 +0000 (UTC) From: Alexander Motin <mav@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r267643 - head/sys/cam/ctl Message-ID: <201406191319.s5JDJZZ9032127@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: mav Date: Thu Jun 19 13:19:35 2014 New Revision: 267643 URL: http://svnweb.freebsd.org/changeset/base/267643 Log: Execute task management request directly in ctl_queue() context. From one side it allows to remove CTL_FLAG_TASK_PENDING flag, handling of which significantly complicates fine-grained locking. From the other side it reduces task management requests latency even below then that flag could. As downside, it denies task management code to sleep, but that is not needed any way now. Discussed with: ken Modified: head/sys/cam/ctl/ctl.c head/sys/cam/ctl/ctl_private.h Modified: head/sys/cam/ctl/ctl.c ============================================================================== --- head/sys/cam/ctl/ctl.c Thu Jun 19 13:18:23 2014 (r267642) +++ head/sys/cam/ctl/ctl.c Thu Jun 19 13:19:35 2014 (r267643) @@ -414,7 +414,7 @@ static int ctl_target_reset(struct ctl_s static int ctl_lun_reset(struct ctl_lun *lun, union ctl_io *io, ctl_ua_type ua_type); static int ctl_abort_task(union ctl_io *io); -static void ctl_run_task_queue(struct ctl_softc *ctl_softc); +static void ctl_run_task(union ctl_io *io); #ifdef CTL_IO_DELAY static void ctl_datamove_timer_wakeup(void *arg); static void ctl_done_timer_wakeup(void *arg); @@ -866,10 +866,7 @@ ctl_isc_event_handler(ctl_ha_channel cha cs_prof_gettime(&taskio->io_hdr.start_ticks); #endif #endif /* CTL_TIME_IO */ - STAILQ_INSERT_TAIL(&ctl_softc->task_queue, - &taskio->io_hdr, links); - ctl_softc->flags |= CTL_FLAG_TASK_PENDING; - ctl_wakeup_thread(); + ctl_run_task((union ctl_io *)taskio); break; } /* Persistent Reserve action which needs attention */ @@ -1033,7 +1030,6 @@ ctl_init(void) softc->target.wwid[1] = 0x87654321; STAILQ_INIT(&softc->lun_list); STAILQ_INIT(&softc->pending_lun_queue); - STAILQ_INIT(&softc->task_queue); STAILQ_INIT(&softc->incoming_queue); STAILQ_INIT(&softc->rtr_queue); STAILQ_INIT(&softc->done_queue); @@ -3604,49 +3600,9 @@ ctl_free_io(union ctl_io *io) */ if (io->io_hdr.pool != NULL) { struct ctl_io_pool *pool; -#if 0 - struct ctl_softc *ctl_softc; - union ctl_io *tmp_io; - unsigned long xflags; - int i; - - ctl_softc = control_softc; -#endif pool = (struct ctl_io_pool *)io->io_hdr.pool; - mtx_lock(&pool->ctl_softc->pool_lock); -#if 0 - save_flags(xflags); - - for (i = 0, tmp_io = (union ctl_io *)STAILQ_FIRST( - &ctl_softc->task_queue); tmp_io != NULL; i++, - tmp_io = (union ctl_io *)STAILQ_NEXT(&tmp_io->io_hdr, - links)) { - if (tmp_io == io) { - printf("%s: %p is still on the task queue!\n", - __func__, tmp_io); - printf("%s: (%d): type %d " - "msg %d cdb %x iptl: " - "%d:%d:%d:%d tag 0x%04x " - "flg %#lx\n", - __func__, i, - tmp_io->io_hdr.io_type, - tmp_io->io_hdr.msg_type, - tmp_io->scsiio.cdb[0], - tmp_io->io_hdr.nexus.initid.id, - tmp_io->io_hdr.nexus.targ_port, - tmp_io->io_hdr.nexus.targ_target.id, - tmp_io->io_hdr.nexus.targ_lun, - (tmp_io->io_hdr.io_type == - CTL_IO_TASK) ? - tmp_io->taskio.tag_num : - tmp_io->scsiio.tag_num, - xflags); - panic("I/O still on the task queue!"); - } - } -#endif io->io_hdr.io_type = 0xff; STAILQ_INSERT_TAIL(&pool->free_queue, &io->io_hdr, links); pool->total_freed++; @@ -11570,156 +11526,125 @@ bailout: * handler as well as from the work thread. */ static void -ctl_run_task_queue(struct ctl_softc *ctl_softc) +ctl_run_task(union ctl_io *io) { - union ctl_io *io, *next_io; - - mtx_assert(&ctl_softc->ctl_lock, MA_OWNED); - - CTL_DEBUG_PRINT(("ctl_run_task_queue\n")); + struct ctl_softc *ctl_softc; + int retval; + const char *task_desc; - for (io = (union ctl_io *)STAILQ_FIRST(&ctl_softc->task_queue); - io != NULL; io = next_io) { - int retval; - const char *task_desc; + CTL_DEBUG_PRINT(("ctl_run_task\n")); - next_io = (union ctl_io *)STAILQ_NEXT(&io->io_hdr, links); + ctl_softc = control_softc; + retval = 0; - retval = 0; + KASSERT(io->io_hdr.io_type == CTL_IO_TASK, + ("ctl_run_task: Unextected io_type %d\n", + io->io_hdr.io_type)); - switch (io->io_hdr.io_type) { - case CTL_IO_TASK: { - task_desc = ctl_scsi_task_string(&io->taskio); - if (task_desc != NULL) { + task_desc = ctl_scsi_task_string(&io->taskio); + if (task_desc != NULL) { #ifdef NEEDTOPORT - csevent_log(CSC_CTL | CSC_SHELF_SW | - CTL_TASK_REPORT, - csevent_LogType_Trace, - csevent_Severity_Information, - csevent_AlertLevel_Green, - csevent_FRU_Firmware, - csevent_FRU_Unknown, - "CTL: received task: %s",task_desc); + csevent_log(CSC_CTL | CSC_SHELF_SW | + CTL_TASK_REPORT, + csevent_LogType_Trace, + csevent_Severity_Information, + csevent_AlertLevel_Green, + csevent_FRU_Firmware, + csevent_FRU_Unknown, + "CTL: received task: %s",task_desc); #endif - } else { + } else { #ifdef NEEDTOPORT - csevent_log(CSC_CTL | CSC_SHELF_SW | - CTL_TASK_REPORT, - csevent_LogType_Trace, - csevent_Severity_Information, - csevent_AlertLevel_Green, - csevent_FRU_Firmware, - csevent_FRU_Unknown, - "CTL: received unknown task " - "type: %d (%#x)", - io->taskio.task_action, - io->taskio.task_action); -#endif - } - switch (io->taskio.task_action) { - case CTL_TASK_ABORT_TASK: - retval = ctl_abort_task(io); - break; - case CTL_TASK_ABORT_TASK_SET: - break; - case CTL_TASK_CLEAR_ACA: - break; - case CTL_TASK_CLEAR_TASK_SET: - break; - case CTL_TASK_LUN_RESET: { - struct ctl_lun *lun; - uint32_t targ_lun; - int retval; - - targ_lun = io->io_hdr.nexus.targ_lun; - if (io->io_hdr.nexus.lun_map_fn != NULL) - targ_lun = io->io_hdr.nexus.lun_map_fn(io->io_hdr.nexus.lun_map_arg, targ_lun); - - if ((targ_lun < CTL_MAX_LUNS) - && (ctl_softc->ctl_luns[targ_lun] != NULL)) - lun = ctl_softc->ctl_luns[targ_lun]; - else { - retval = 1; - break; - } - - if (!(io->io_hdr.flags & - CTL_FLAG_FROM_OTHER_SC)) { - union ctl_ha_msg msg_info; - - io->io_hdr.flags |= - CTL_FLAG_SENT_2OTHER_SC; - msg_info.hdr.msg_type = - CTL_MSG_MANAGE_TASKS; - msg_info.hdr.nexus = io->io_hdr.nexus; - msg_info.task.task_action = - CTL_TASK_LUN_RESET; - msg_info.hdr.original_sc = NULL; - msg_info.hdr.serializing_sc = NULL; - if (CTL_HA_STATUS_SUCCESS != - ctl_ha_msg_send(CTL_HA_CHAN_CTL, - (void *)&msg_info, - sizeof(msg_info), 0)) { - } - } + csevent_log(CSC_CTL | CSC_SHELF_SW | + CTL_TASK_REPORT, + csevent_LogType_Trace, + csevent_Severity_Information, + csevent_AlertLevel_Green, + csevent_FRU_Firmware, + csevent_FRU_Unknown, + "CTL: received unknown task " + "type: %d (%#x)", + io->taskio.task_action, + io->taskio.task_action); +#endif + } + switch (io->taskio.task_action) { + case CTL_TASK_ABORT_TASK: + retval = ctl_abort_task(io); + break; + case CTL_TASK_ABORT_TASK_SET: + break; + case CTL_TASK_CLEAR_ACA: + break; + case CTL_TASK_CLEAR_TASK_SET: + break; + case CTL_TASK_LUN_RESET: { + struct ctl_lun *lun; + uint32_t targ_lun; + int retval; - retval = ctl_lun_reset(lun, io, - CTL_UA_LUN_RESET); - break; - } - case CTL_TASK_TARGET_RESET: - retval = ctl_target_reset(ctl_softc, io, - CTL_UA_TARG_RESET); - break; - case CTL_TASK_BUS_RESET: - retval = ctl_bus_reset(ctl_softc, io); - break; - case CTL_TASK_PORT_LOGIN: - break; - case CTL_TASK_PORT_LOGOUT: - break; - default: - printf("ctl_run_task_queue: got unknown task " - "management event %d\n", - io->taskio.task_action); - break; - } - if (retval == 0) - io->io_hdr.status = CTL_SUCCESS; - else - io->io_hdr.status = CTL_ERROR; + targ_lun = io->io_hdr.nexus.targ_lun; + if (io->io_hdr.nexus.lun_map_fn != NULL) + targ_lun = io->io_hdr.nexus.lun_map_fn(io->io_hdr.nexus.lun_map_arg, targ_lun); - STAILQ_REMOVE(&ctl_softc->task_queue, &io->io_hdr, - ctl_io_hdr, links); - /* - * This will queue this I/O to the done queue, but the - * work thread won't be able to process it until we - * return and the lock is released. - */ - ctl_done_lock(io, /*have_lock*/ 1); + if ((targ_lun < CTL_MAX_LUNS) + && (ctl_softc->ctl_luns[targ_lun] != NULL)) + lun = ctl_softc->ctl_luns[targ_lun]; + else { + retval = 1; break; } - default: { - printf("%s: invalid I/O type %d msg %d cdb %x" - " iptl: %ju:%d:%ju:%d tag 0x%04x\n", - __func__, io->io_hdr.io_type, - io->io_hdr.msg_type, io->scsiio.cdb[0], - (uintmax_t)io->io_hdr.nexus.initid.id, - io->io_hdr.nexus.targ_port, - (uintmax_t)io->io_hdr.nexus.targ_target.id, - io->io_hdr.nexus.targ_lun /* XXX */, - (io->io_hdr.io_type == CTL_IO_TASK) ? - io->taskio.tag_num : io->scsiio.tag_num); - STAILQ_REMOVE(&ctl_softc->task_queue, &io->io_hdr, - ctl_io_hdr, links); - ctl_free_io(io); - break; - } + if (!(io->io_hdr.flags & + CTL_FLAG_FROM_OTHER_SC)) { + union ctl_ha_msg msg_info; + + io->io_hdr.flags |= + CTL_FLAG_SENT_2OTHER_SC; + msg_info.hdr.msg_type = + CTL_MSG_MANAGE_TASKS; + msg_info.hdr.nexus = io->io_hdr.nexus; + msg_info.task.task_action = + CTL_TASK_LUN_RESET; + msg_info.hdr.original_sc = NULL; + msg_info.hdr.serializing_sc = NULL; + if (CTL_HA_STATUS_SUCCESS != + ctl_ha_msg_send(CTL_HA_CHAN_CTL, + (void *)&msg_info, + sizeof(msg_info), 0)) { + } } + + retval = ctl_lun_reset(lun, io, + CTL_UA_LUN_RESET); + break; } + case CTL_TASK_TARGET_RESET: + retval = ctl_target_reset(ctl_softc, io, CTL_UA_TARG_RESET); + break; + case CTL_TASK_BUS_RESET: + retval = ctl_bus_reset(ctl_softc, io); + break; + case CTL_TASK_PORT_LOGIN: + break; + case CTL_TASK_PORT_LOGOUT: + break; + default: + printf("ctl_run_task: got unknown task management event %d\n", + io->taskio.task_action); + break; + } + if (retval == 0) + io->io_hdr.status = CTL_SUCCESS; + else + io->io_hdr.status = CTL_ERROR; - ctl_softc->flags &= ~CTL_FLAG_TASK_PENDING; + /* + * This will queue this I/O to the done queue, but the + * work thread won't be able to process it until we + * return and the lock is released. + */ + ctl_done_lock(io, /*have_lock*/ 1); } /* @@ -11778,8 +11703,6 @@ ctl_handle_isc(union ctl_io *io) mtx_lock(&ctl_softc->ctl_lock); TAILQ_REMOVE(&lun->ooa_queue, &io->io_hdr, ooa_links); - STAILQ_REMOVE(&ctl_softc->task_queue, - &io->io_hdr, ctl_io_hdr, links); ctl_check_blocked(lun); mtx_unlock(&ctl_softc->ctl_lock); } @@ -12034,26 +11957,6 @@ ctl_datamove(union ctl_io *io) } } #endif - /* - * If we have any pending task management commands, process them - * first. This is necessary to eliminate a race condition with the - * FETD: - * - * - FETD submits a task management command, like an abort. - * - Back end calls fe_datamove() to move the data for the aborted - * command. The FETD can't really accept it, but if it did, it - * would end up transmitting data for a command that the initiator - * told us to abort. - * - * We close the race by processing all pending task management - * commands here (we can't block!), and then check this I/O to see - * if it has been aborted. If so, return it to the back end with - * bad status, so the back end can say return an error to the back end - * and then when the back end returns an error, we can return the - * aborted command to the FETD, so it can clean up its resources. - */ - if (control_softc->flags & CTL_FLAG_TASK_PENDING) - ctl_run_task_queue(control_softc); /* * This command has been aborted. Set the port status, so we fail @@ -13365,42 +13268,23 @@ ctl_queue(union ctl_io *io) getbintime(&io->io_hdr.start_bt); #endif /* CTL_TIME_IO */ - mtx_lock(&ctl_softc->ctl_lock); - switch (io->io_hdr.io_type) { case CTL_IO_SCSI: + mtx_lock(&ctl_softc->ctl_lock); STAILQ_INSERT_TAIL(&ctl_softc->incoming_queue, &io->io_hdr, links); + mtx_unlock(&ctl_softc->ctl_lock); + ctl_wakeup_thread(); break; case CTL_IO_TASK: - STAILQ_INSERT_TAIL(&ctl_softc->task_queue, &io->io_hdr, links); - /* - * Set the task pending flag. This is necessary to close a - * race condition with the FETD: - * - * - FETD submits a task management command, like an abort. - * - Back end calls fe_datamove() to move the data for the - * aborted command. The FETD can't really accept it, but - * if it did, it would end up transmitting data for a - * command that the initiator told us to abort. - * - * We close the race condition by setting the flag here, - * and checking it in ctl_datamove(), before calling the - * FETD's fe_datamove routine. If we've got a task - * pending, we run the task queue and then check to see - * whether our particular I/O has been aborted. - */ - ctl_softc->flags |= CTL_FLAG_TASK_PENDING; + mtx_lock(&ctl_softc->ctl_lock); + ctl_run_task(io); + mtx_unlock(&ctl_softc->ctl_lock); break; default: - mtx_unlock(&ctl_softc->ctl_lock); printf("ctl_queue: unknown I/O type %d\n", io->io_hdr.io_type); return (-EINVAL); - break; /* NOTREACHED */ } - mtx_unlock(&ctl_softc->ctl_lock); - - ctl_wakeup_thread(); return (CTL_RETVAL_COMPLETE); } @@ -13580,7 +13464,6 @@ ctl_work_thread(void *arg) /* * We handle the queues in this order: - * - task management * - ISC * - done queue (to free up resources, unblock other commands) * - RtR queue @@ -13589,11 +13472,6 @@ ctl_work_thread(void *arg) * If those queues are empty, we break out of the loop and * go to sleep. */ - io = (union ctl_io *)STAILQ_FIRST(&softc->task_queue); - if (io != NULL) { - ctl_run_task_queue(softc); - continue; - } io = (union ctl_io *)STAILQ_FIRST(&softc->isc_queue); if (io != NULL) { STAILQ_REMOVE_HEAD(&softc->isc_queue, links); Modified: head/sys/cam/ctl/ctl_private.h ============================================================================== --- head/sys/cam/ctl/ctl_private.h Thu Jun 19 13:18:23 2014 (r267642) +++ head/sys/cam/ctl/ctl_private.h Thu Jun 19 13:19:35 2014 (r267643) @@ -401,7 +401,6 @@ struct ctl_lun { }; typedef enum { - CTL_FLAG_TASK_PENDING = 0x01, CTL_FLAG_REAL_SYNC = 0x02, CTL_FLAG_MASTER_SHELF = 0x04 } ctl_gen_flags; @@ -438,7 +437,6 @@ struct ctl_softc { uint64_t aps_locked_lun; STAILQ_HEAD(, ctl_lun) lun_list; STAILQ_HEAD(, ctl_be_lun) pending_lun_queue; - STAILQ_HEAD(, ctl_io_hdr) task_queue; STAILQ_HEAD(, ctl_io_hdr) incoming_queue; STAILQ_HEAD(, ctl_io_hdr) rtr_queue; STAILQ_HEAD(, ctl_io_hdr) done_queue;
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201406191319.s5JDJZZ9032127>