From 70eb596f60dc865e9a378ec449fca85320ac4352 Mon Sep 17 00:00:00 2001 From: Vladislav Bolkhovitin Date: Mon, 9 May 2011 23:07:23 +0000 Subject: [PATCH] Rework iSCSI aborts to make them more correct and remove recent workarounds (one more post-IET cleanup) git-svn-id: http://svn.code.sf.net/p/scst/svn/trunk@3444 d57e44dd-8a1f-0410-8b47-8ef2f437770f --- iscsi-scst/kernel/conn.c | 29 +++--- iscsi-scst/kernel/iscsi.c | 148 +++++++++++++++++------------- iscsi-scst/kernel/iscsi.h | 2 +- iscsi-scst/kernel/nthread.c | 14 +-- scst/include/scst.h | 22 ++++- scst/src/dev_handlers/scst_user.c | 3 +- scst/src/scst_targ.c | 33 ++++--- 7 files changed, 144 insertions(+), 107 deletions(-) diff --git a/iscsi-scst/kernel/conn.c b/iscsi-scst/kernel/conn.c index 3e112b270..b80f20d60 100644 --- a/iscsi-scst/kernel/conn.c +++ b/iscsi-scst/kernel/conn.c @@ -620,9 +620,9 @@ void iscsi_check_tm_data_wait_timeouts(struct iscsi_conn *conn, bool force) TRACE_ENTRY(); - TRACE_DBG_FLAG(force ? TRACE_CONN_OC_DBG : TRACE_MGMT_DEBUG, - "conn %p, read_cmnd %p, read_state %d, j %ld (TIMEOUT %d, " - "force %d)", conn, conn->read_cmnd, conn->read_state, j, + TRACE_DBG_FLAG(TRACE_MGMT_DEBUG, "conn %p, read_cmnd %p, read_state " + "%d, j %ld (TIMEOUT %d, force %d)", conn, conn->read_cmnd, + conn->read_state, j, ISCSI_TM_DATA_WAIT_TIMEOUT + ISCSI_ADD_SCHED_TIME, force); iscsi_extracheck_is_rd_thread(conn); @@ -635,23 +635,19 @@ again: list_for_each_entry(cmnd, &conn->write_timeout_list, write_timeout_list_entry) { if (test_bit(ISCSI_CMD_ABORTED, &cmnd->prelim_compl_flags)) { - TRACE_DBG_FLAG(force ? TRACE_CONN_OC_DBG : TRACE_MGMT_DEBUG, - "Checking aborted cmnd %p (scst_state %d, " - "on_write_timeout_list %d, write_start %ld, " - "r2t_len_to_receive %d)", cmnd, cmnd->scst_state, + TRACE_DBG_FLAG(TRACE_MGMT_DEBUG, "Checking aborted " + "cmnd %p (scst_state %d, on_write_timeout_list " + "%d, write_start %ld, r2t_len_to_receive %d)", + cmnd, cmnd->scst_state, cmnd->on_write_timeout_list, cmnd->write_start, cmnd->r2t_len_to_receive); if (cmnd == conn->read_cmnd) { - TRACE_DBG_FLAG(force ? TRACE_CONN_OC_DBG : TRACE_MGMT_DEBUG, + TRACE_DBG_FLAG(TRACE_MGMT_DEBUG, "cmnd %p is read cmd", cmnd); sBUG_ON(force); - if ((conn->read_state == RX_INIT_BHS) || - (conn->read_state == RX_BHS)) { - TRACE_MGMT_DBG("Unabort not yet received cmnd %p", - cmnd); - clear_bit(ISCSI_CMD_ABORTED, &cmnd->prelim_compl_flags); - continue; - } else if (cmnd->scst_state == ISCSI_CMD_STATE_RX_CMD) { + sBUG_ON((conn->read_state == RX_INIT_BHS) || + (conn->read_state == RX_BHS)); + if (cmnd->scst_state == ISCSI_CMD_STATE_RX_CMD) { TRACE_MGMT_DBG("Aborted cmnd %p is RX_CMD, " "keep waiting", cmnd); goto cont; @@ -661,7 +657,8 @@ again: (time_after_eq(j, cmnd->write_start + ISCSI_TM_DATA_WAIT_TIMEOUT) || force)) { if (cmnd == conn->read_cmnd) { - TRACE_MGMT_DBG("Clearing read_cmnd for conn %p", conn); + TRACE_MGMT_DBG("Clearing read_cmnd for " + "conn %p", conn); conn->read_cmnd = NULL; conn->read_state = RX_INIT_BHS; } diff --git a/iscsi-scst/kernel/iscsi.c b/iscsi-scst/kernel/iscsi.c index e57bfab0c..3e3731de6 100644 --- a/iscsi-scst/kernel/iscsi.c +++ b/iscsi-scst/kernel/iscsi.c @@ -61,7 +61,6 @@ static void iscsi_send_task_mgmt_resp(struct iscsi_cmnd *req, int status); static void iscsi_check_send_delayed_tm_resp(struct iscsi_session *sess); static void req_cmnd_release(struct iscsi_cmnd *req); static int cmnd_insert_data_wait_hash(struct iscsi_cmnd *cmnd); -static void __cmnd_abort(struct iscsi_cmnd *cmnd); static void iscsi_cmnd_init_write(struct iscsi_cmnd *rsp, int flags); static void iscsi_set_resid_no_scst_cmd(struct iscsi_cmnd *rsp); static void iscsi_set_resid(struct iscsi_cmnd *rsp); @@ -2250,7 +2249,7 @@ static void __cmnd_abort(struct iscsi_cmnd *cmnd) } /* Must be called from the read or conn close thread */ -static int cmnd_abort(struct iscsi_cmnd *req, int *status) +static int cmnd_abort_pre_checks(struct iscsi_cmnd *req, int *status) { struct iscsi_task_mgt_hdr *req_hdr = (struct iscsi_task_mgt_hdr *)&req->pdu.bhs; @@ -2268,7 +2267,6 @@ static int cmnd_abort(struct iscsi_cmnd *req, int *status) cmnd = cmnd_find_itt_get(req->conn, req_hdr->rtt); if (cmnd) { - struct iscsi_conn *conn = cmnd->conn; struct iscsi_scsi_cmd_hdr *hdr = cmnd_hdr(cmnd); if (req_hdr->lun != hdr->lun) { @@ -2310,10 +2308,6 @@ static int cmnd_abort(struct iscsi_cmnd *req, int *status) goto out_put; } - spin_lock_bh(&conn->cmd_list_lock); - __cmnd_abort(cmnd); - spin_unlock_bh(&conn->cmd_list_lock); - cmnd_put(cmnd); res = 0; } else { @@ -2351,69 +2345,86 @@ out_put: goto out; } -/* Must be called from the read or conn close thread */ -static int target_abort(struct iscsi_cmnd *req, int all) +struct iscsi_cmnd_abort_params { + struct work_struct iscsi_cmnd_abort_work; + struct scst_cmd *scst_cmd; +}; + +static mempool_t *iscsi_cmnd_abort_mempool; + +static void iscsi_cmnd_abort_fn(struct work_struct *work) { - struct iscsi_target *target = req->conn->session->target; - struct iscsi_task_mgt_hdr *req_hdr = - (struct iscsi_task_mgt_hdr *)&req->pdu.bhs; - struct iscsi_session *session; + struct iscsi_cmnd_abort_params *params = container_of(work, + struct iscsi_cmnd_abort_params, iscsi_cmnd_abort_work); + struct scst_cmd *scst_cmd = params->scst_cmd; + struct iscsi_session *session = scst_sess_get_tgt_priv(scst_cmd->sess); struct iscsi_conn *conn; - struct iscsi_cmnd *cmnd; + struct iscsi_cmnd *cmnd = scst_cmd_get_tgt_priv(scst_cmd); + bool done = false; - mutex_lock(&target->target_mutex); + TRACE_ENTRY(); - list_for_each_entry(session, &target->session_list, - session_list_entry) { - list_for_each_entry(conn, &session->conn_list, - conn_list_entry) { - spin_lock_bh(&conn->cmd_list_lock); - list_for_each_entry(cmnd, &conn->cmd_list, - cmd_list_entry) { - if (cmnd == req) - continue; - if (all) - __cmnd_abort(cmnd); - else if (req_hdr->lun == cmnd_hdr(cmnd)->lun) - __cmnd_abort(cmnd); - } - spin_unlock_bh(&conn->cmd_list_lock); - } - } + TRACE_MGMT_DBG("Checking aborted scst_cmd %p (cmnd %p)", scst_cmd, cmnd); - mutex_unlock(&target->target_mutex); - return 0; -} - -/* Must be called from the read or conn close thread */ -static void task_set_abort(struct iscsi_cmnd *req) -{ - struct iscsi_session *session = req->conn->session; - struct iscsi_task_mgt_hdr *req_hdr = - (struct iscsi_task_mgt_hdr *)&req->pdu.bhs; - struct iscsi_target *target = session->target; - struct iscsi_conn *conn; - struct iscsi_cmnd *cmnd; - - mutex_lock(&target->target_mutex); + mutex_lock(&session->target->target_mutex); + /* + * cmnd pointer is valid only under cmd_list_lock, but we can't know the + * corresponding conn without dereferencing cmnd at first, so let's + * check all conns and cmnds to find out if our cmnd is still valid + * under lock. + */ list_for_each_entry(conn, &session->conn_list, conn_list_entry) { + struct iscsi_cmnd *c; spin_lock_bh(&conn->cmd_list_lock); - list_for_each_entry(cmnd, &conn->cmd_list, cmd_list_entry) { - struct iscsi_scsi_cmd_hdr *hdr = cmnd_hdr(cmnd); - if (cmnd == req) - continue; - if (req_hdr->lun != hdr->lun) - continue; - if (before(req_hdr->cmd_sn, hdr->cmd_sn) || - req_hdr->cmd_sn == hdr->cmd_sn) - continue; - __cmnd_abort(cmnd); + list_for_each_entry(c, &conn->cmd_list, cmd_list_entry) { + if (c == cmnd) { + __cmnd_abort(cmnd); + done = true; + break; + } } spin_unlock_bh(&conn->cmd_list_lock); + if (done) + break; } - mutex_unlock(&target->target_mutex); + mutex_unlock(&session->target->target_mutex); + + scst_cmd_put(scst_cmd); + + mempool_free(params, iscsi_cmnd_abort_mempool); + + TRACE_EXIT(); + return; +} + +static void iscsi_on_abort_cmd(struct scst_cmd *scst_cmd) +{ + struct iscsi_cmnd_abort_params *params; + + TRACE_ENTRY(); + + params = mempool_alloc(iscsi_cmnd_abort_mempool, GFP_ATOMIC); + if (params == NULL) { + PRINT_CRIT_ERROR("Unable to create iscsi_cmnd_abort_params, " + "iSCSI cmnd for scst_cmd %p may not be aborted", + scst_cmd); + goto out; + } + + memset(params, 0, sizeof(*params)); + INIT_WORK(¶ms->iscsi_cmnd_abort_work, iscsi_cmnd_abort_fn); + params->scst_cmd = scst_cmd; + + scst_cmd_get(scst_cmd); + + TRACE_MGMT_DBG("Scheduling abort check for scst_cmd %p", scst_cmd); + + schedule_work(¶ms->iscsi_cmnd_abort_work); + +out: + TRACE_EXIT(); return; } @@ -2516,7 +2527,7 @@ static void execute_task_management(struct iscsi_cmnd *req) switch (function) { case ISCSI_FUNCTION_ABORT_TASK: - rc = cmnd_abort(req, &status); + rc = cmnd_abort_pre_checks(req, &status); if (rc == 0) { params.fn = SCST_ABORT_TASK; params.tag = (__force u32)req_hdr->rtt; @@ -2532,7 +2543,6 @@ static void execute_task_management(struct iscsi_cmnd *req) } break; case ISCSI_FUNCTION_ABORT_TASK_SET: - task_set_abort(req); params.fn = SCST_ABORT_TASK_SET; params.lun = (uint8_t *)&req_hdr->lun; params.lun_len = sizeof(req_hdr->lun); @@ -2544,7 +2554,6 @@ static void execute_task_management(struct iscsi_cmnd *req) status = ISCSI_RESPONSE_FUNCTION_REJECTED; break; case ISCSI_FUNCTION_CLEAR_TASK_SET: - task_set_abort(req); params.fn = SCST_CLEAR_TASK_SET; params.lun = (uint8_t *)&req_hdr->lun; params.lun_len = sizeof(req_hdr->lun); @@ -2568,7 +2577,6 @@ static void execute_task_management(struct iscsi_cmnd *req) break; case ISCSI_FUNCTION_TARGET_COLD_RESET: case ISCSI_FUNCTION_TARGET_WARM_RESET: - target_abort(req, 1); params.fn = SCST_TARGET_RESET; params.cmd_sn = req_hdr->cmd_sn; params.cmd_sn_set = 1; @@ -2577,7 +2585,6 @@ static void execute_task_management(struct iscsi_cmnd *req) status = ISCSI_RESPONSE_FUNCTION_REJECTED; break; case ISCSI_FUNCTION_LOGICAL_UNIT_RESET: - target_abort(req, 0); params.fn = SCST_LUN_RESET; params.lun = (uint8_t *)&req_hdr->lun; params.lun_len = sizeof(req_hdr->lun); @@ -3827,6 +3834,7 @@ struct scst_tgt_template iscsi_template = { .pre_exec = iscsi_pre_exec, .task_mgmt_affected_cmds_done = iscsi_task_mgmt_affected_cmds_done, .task_mgmt_fn_done = iscsi_task_mgmt_fn_done, + .on_abort_cmd = iscsi_on_abort_cmd, .report_aen = iscsi_report_aen, .get_initiator_port_transport_id = iscsi_get_initiator_port_transport_id, .get_scsi_transport_version = iscsi_get_scsi_transport_version, @@ -4016,12 +4024,19 @@ static int __init iscsi_init(void) sg_init_table(&dummy_sg, 1); sg_set_page(&dummy_sg, dummy_page, PAGE_SIZE, 0); + iscsi_cmnd_abort_mempool = mempool_create_kmalloc_pool(2500, + sizeof(struct iscsi_cmnd_abort_params)); + if (iscsi_cmnd_abort_mempool == NULL) { + err = -ENOMEM; + goto out_free_dummy; + } + #if defined(CONFIG_TCP_ZERO_COPY_TRANSFER_COMPLETION_NOTIFICATION) err = net_set_get_put_page_callbacks(iscsi_get_page_callback, iscsi_put_page_callback); if (err != 0) { PRINT_INFO("Unable to set page callbackes: %d", err); - goto out_free_dummy; + goto out_destroy_mempool; } #else #ifndef GENERATING_UPSTREAM_PATCH @@ -4092,6 +4107,9 @@ out_callb: #if defined(CONFIG_TCP_ZERO_COPY_TRANSFER_COMPLETION_NOTIFICATION) net_set_get_put_page_callbacks(NULL, NULL); +out_destroy_mempool: + mempool_destroy(iscsi_cmnd_abort_mempool); + out_free_dummy: #endif __free_pages(dummy_page, 0); @@ -4119,6 +4137,8 @@ static void __exit iscsi_exit(void) net_set_get_put_page_callbacks(NULL, NULL); #endif + mempool_destroy(iscsi_cmnd_abort_mempool); + __free_pages(dummy_page, 0); return; } diff --git a/iscsi-scst/kernel/iscsi.h b/iscsi-scst/kernel/iscsi.h index 6f05f7b37..4d9644a7f 100644 --- a/iscsi-scst/kernel/iscsi.h +++ b/iscsi-scst/kernel/iscsi.h @@ -672,7 +672,7 @@ static inline void iscsi_cmnd_set_length(struct iscsi_pdu *pdu) extern struct scst_tgt_template iscsi_template; /* - * Skip this command if result is not 0. Must be called under + * Skip this command if result is true. Must be called under * corresponding lock. */ static inline bool cmnd_get_check(struct iscsi_cmnd *cmnd) diff --git a/iscsi-scst/kernel/nthread.c b/iscsi-scst/kernel/nthread.c index bfc920bbe..c0935ad9a 100644 --- a/iscsi-scst/kernel/nthread.c +++ b/iscsi-scst/kernel/nthread.c @@ -19,7 +19,6 @@ #include #include #include -#include #include #include @@ -839,17 +838,10 @@ static int process_read_io(struct iscsi_conn *conn, int *closed) res = do_recv(conn); if (res == 0) { /* - * Clear aborted status if this command was - * accidentally aborted with other commands of - * this connection. This command not yet - * received on the aborted time, so shouldn't be - * affected by the abort. + * This command not yet received on the aborted + * time, so shouldn't be affected by any abort. */ - if (cmnd->prelim_compl_flags != 0) - TRACE_MGMT_DBG("Unabort not yet " - "received cmnd %p (flags %lx)", - cmnd, cmnd->prelim_compl_flags); - cmnd->prelim_compl_flags = 0; + EXTRACHECKS_BUG_ON(cmnd->prelim_compl_flags != 0); iscsi_cmnd_get_length(&cmnd->pdu); diff --git a/scst/include/scst.h b/scst/include/scst.h index 02af6a7de..434252192 100644 --- a/scst/include/scst.h +++ b/scst/include/scst.h @@ -810,6 +810,15 @@ struct scst_tgt_template { */ void (*task_mgmt_fn_done) (struct scst_mgmt_cmd *mgmt_cmd); + /* + * Called to notify target driver that the command is being aborted. + * If target driver wants to redirect processing to some outside + * processing, it should get it using scst_cmd_get(). + * + * OPTIONAL + */ + void (*on_abort_cmd) (struct scst_cmd *cmd); + /* * This function should detect the target adapters that * are present in the system. The function should return a value @@ -1232,7 +1241,13 @@ struct scst_dev_type { * - SCST_DEV_TM_NOT_COMPLETED - regular standard actions for the * command should be done * - * Called without any locks held from a thread context. + * Can be called under many internal SCST locks, including under + * disabled IRQs, so dev handler should be careful with locking and, + * if necessary, pass processing somewhere outside (in a work, e.g.) + * + * But at the moment it's called under disabled IRQs only for + * SCST_ABORT_TASK, however dev handler using it should add a BUG_ON + * trap to catch if it's changed in future. * * OPTIONAL */ @@ -1862,7 +1877,10 @@ struct scst_cmd { /* Set if cmd is done */ unsigned int done:1; - /* Set if cmd is finished */ + /* + * Set if cmd is finished. Used under sess_list_lock to sync + * between scst_finish_cmd() and scst_abort_cmd() + */ unsigned int finished:1; #ifdef CONFIG_SCST_DEBUG_TM diff --git a/scst/src/dev_handlers/scst_user.c b/scst/src/dev_handlers/scst_user.c index af952dd12..fe674c840 100644 --- a/scst/src/dev_handlers/scst_user.c +++ b/scst/src/dev_handlers/scst_user.c @@ -2408,6 +2408,7 @@ again: return; } +/* Can be called under some spinlock and IRQs off */ static int dev_user_task_mgmt_fn(struct scst_mgmt_cmd *mcmd, struct scst_tgt_dev *tgt_dev) { @@ -2458,7 +2459,7 @@ static int dev_user_task_mgmt_fn(struct scst_mgmt_cmd *mcmd, dev_user_abort_ready_commands(dev); /* We can't afford missing TM command due to memory shortage */ - ucmd = dev_user_alloc_ucmd(dev, GFP_KERNEL|__GFP_NOFAIL); + ucmd = dev_user_alloc_ucmd(dev, GFP_ATOMIC|__GFP_NOFAIL); ucmd->user_cmd_payload_len = offsetof(struct scst_user_get_cmd, tm_cmd) + diff --git a/scst/src/scst_targ.c b/scst/src/scst_targ.c index f5ab19f98..67c8080dd 100644 --- a/scst/src/scst_targ.c +++ b/scst/src/scst_targ.c @@ -508,7 +508,7 @@ int scst_pre_parse(struct scst_cmd *cmd) TRACE_DBG("op_name <%s> (cmd %p), direction=%d " "(expected %d, set %s), bufflen=%d, out_bufflen=%d (expected " - "len %d, out expected len %d), flags=%d", cmd->op_name, cmd, + "len %d, out expected len %d), flags=0x%x", cmd->op_name, cmd, cmd->data_direction, cmd->expected_data_direction, scst_cmd_is_expected_set(cmd) ? "yes" : "no", cmd->bufflen, cmd->out_bufflen, cmd->expected_transfer_len, @@ -3641,10 +3641,13 @@ static int scst_finish_cmd(struct scst_cmd *cmd) list_del(&cmd->sess_cmd_list_entry); - spin_unlock_irq(&sess->sess_list_lock); - + /* + * Done under sess_list_lock to sync with scst_abort_cmd() without + * using extra barrier. + */ cmd->finished = 1; - smp_mb(); /* to sync with scst_abort_cmd() */ + + spin_unlock_irq(&sess->sess_list_lock); if (unlikely(test_bit(SCST_CMD_ABORTED, &cmd->cmd_flags))) { TRACE_MGMT_DBG("Aborted cmd %p finished (cmd_ref %d)", @@ -4613,7 +4616,6 @@ static int scst_call_dev_task_mgmt_fn(struct scst_mgmt_cmd *mcmd, if (h->task_mgmt_fn) { TRACE_MGMT_DBG("Calling dev handler %s task_mgmt_fn(fn=%d)", h->name, mcmd->fn); - EXTRACHECKS_BUG_ON(in_irq() || irqs_disabled()); res = h->task_mgmt_fn(mcmd, tgt_dev); TRACE_MGMT_DBG("Dev handler %s task_mgmt_fn() returned %d", h->name, res); @@ -4639,7 +4641,10 @@ static inline int scst_is_strict_mgmt_fn(int mgmt_fn) } } -/* Might be called under sess_list_lock and IRQ off + BHs also off */ +/* + * Must be called under sess_list_lock to sync with finished flag assignment in + * scst_finish_cmd() + */ void scst_abort_cmd(struct scst_cmd *cmd, struct scst_mgmt_cmd *mcmd, bool other_ini, bool call_dev_task_mgmt_fn) { @@ -4684,9 +4689,9 @@ void scst_abort_cmd(struct scst_cmd *cmd, struct scst_mgmt_cmd *mcmd, spin_unlock_irqrestore(&other_ini_lock, flags); /* - * To sync with cmd->finished/done set in - * scst_finish_cmd()/scst_pre_xmit_response() and with setting UA for - * aborted cmd in scst_set_pending_UA(). + * To sync with setting cmd->done in scst_pre_xmit_response() (with + * scst_finish_cmd() we synced by using sess_list_lock) and with + * setting UA for aborted cmd in scst_set_pending_UA(). */ smp_mb__after_set_bit(); @@ -4697,10 +4702,8 @@ void scst_abort_cmd(struct scst_cmd *cmd, struct scst_mgmt_cmd *mcmd, wake_up(&scst_init_cmd_list_waitQ); } - if (call_dev_task_mgmt_fn && (cmd->tgt_dev != NULL)) { - EXTRACHECKS_BUG_ON(irqs_disabled()); + if (!cmd->finished && call_dev_task_mgmt_fn && (cmd->tgt_dev != NULL)) scst_call_dev_task_mgmt_fn(mcmd, cmd->tgt_dev, 1); - } spin_lock_irqsave(&scst_mcmd_lock, flags); if ((mcmd != NULL) && !cmd->finished) { @@ -4766,6 +4769,9 @@ void scst_abort_cmd(struct scst_cmd *cmd, struct scst_mgmt_cmd *mcmd, /* We don't need to wait for this cmd */ mempool_free(mstb, scst_mgmt_stub_mempool); } + + if (cmd->tgtt->on_abort_cmd) + cmd->tgtt->on_abort_cmd(cmd); } unlock: @@ -5495,7 +5501,10 @@ static int scst_abort_task(struct scst_mgmt_cmd *mcmd) cmd->tgt_sn, (long long unsigned int)mcmd->tag); mcmd->status = SCST_MGMT_STATUS_REJECTED; } else { + spin_lock_irq(&cmd->sess->sess_list_lock); scst_abort_cmd(cmd, mcmd, 0, 1); + spin_unlock_irq(&cmd->sess->sess_list_lock); + scst_unblock_aborted_cmds(0); }