On Fri, 2011-05-27 at 12:07 -0700, Andy Grover wrote:
> Turn iscsit_check_received_cmdsn into a similar function that just
> checks cmdsn (and increments it if it's the next expected sn). Add
> a new function iscsit_sequence_cmd that dispositions the cmd based on
> the result.
>
> Change callsites accordingly and simplify, based on sequence_cmd queuing
> lower-than-expected cmdsns for removal on callee's behalf.
>
> Reorder logic in handle_logout_cmd() a little.
>
> Signed-off-by: Andy Grover <agrover@xxxxxxxxxx>
> ---
This does need to be tested, but AFAICT the changes look OK to be merged
now..
Committed as 60ab5356375.
Thanks,
--nab
> drivers/target/iscsi/iscsi_target.c | 113 ++++++++-----------------
> drivers/target/iscsi/iscsi_target_util.c | 136 +++++++++++++-----------------
> drivers/target/iscsi/iscsi_target_util.h | 2 +-
> 3 files changed, 93 insertions(+), 158 deletions(-)
>
> diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
> index 2ef6436..b5b6981 100644
> --- a/drivers/target/iscsi/iscsi_target.c
> +++ b/drivers/target/iscsi/iscsi_target.c
> @@ -1211,22 +1211,13 @@ attach_cmd:
> * be acknowledged. (See below)
> */
> if (!cmd->immediate_data) {
> - cmdsn_ret = iscsit_check_received_cmdsn(conn,
> - cmd, hdr->cmdsn);
> - if ((cmdsn_ret == CMDSN_NORMAL_OPERATION) ||
> - (cmdsn_ret == CMDSN_HIGHER_THAN_EXP))
> - do {} while (0);
> - else if (cmdsn_ret == CMDSN_LOWER_THAN_EXP) {
> - cmd->i_state = ISTATE_REMOVE;
> - iscsit_add_cmd_to_immediate_queue(cmd,
> - conn, cmd->i_state);
> - return 0;
> - } else { /* (cmdsn_ret == CMDSN_ERROR_CANNOT_RECOVER) */
> + cmdsn_ret = iscsit_sequence_cmd(conn, cmd, hdr->cmdsn);
> + if (cmdsn_ret == CMDSN_ERROR_CANNOT_RECOVER)
> return iscsit_add_reject_from_cmd(
> - ISCSI_REASON_PROTOCOL_ERROR,
> - 1, 0, buf, cmd);
> - }
> + ISCSI_REASON_PROTOCOL_ERROR,
> + 1, 0, buf, cmd);
> }
> +
> iscsit_ack_from_expstatsn(conn, hdr->exp_statsn);
>
> /*
> @@ -1281,8 +1272,7 @@ after_immediate_data:
> * DataCRC, check against ExpCmdSN/MaxCmdSN if
> * Immediate Bit is not set.
> */
> - cmdsn_ret = iscsit_check_received_cmdsn(conn,
> - cmd, hdr->cmdsn);
> + cmdsn_ret = iscsit_sequence_cmd(conn, cmd, hdr->cmdsn);
> /*
> * Special case for Unsupported SAM WRITE Opcodes
> * and ImmediateData=Yes.
> @@ -1298,20 +1288,11 @@ after_immediate_data:
> spin_unlock_bh(&cmd->dataout_timeout_lock);
> }
>
> - if (cmdsn_ret == CMDSN_NORMAL_OPERATION)
> - return 0;
> - else if (cmdsn_ret == CMDSN_HIGHER_THAN_EXP)
> - return 0;
> - else if (cmdsn_ret == CMDSN_LOWER_THAN_EXP) {
> - cmd->i_state = ISTATE_REMOVE;
> - iscsit_add_cmd_to_immediate_queue(cmd,
> - conn, cmd->i_state);
> - return 0;
> - } else { /* (cmdsn_ret == CMDSN_ERROR_CANNOT_RECOVER) */
> + if (cmdsn_ret == CMDSN_ERROR_CANNOT_RECOVER)
> return iscsit_add_reject_from_cmd(
> - ISCSI_REASON_PROTOCOL_ERROR,
> - 1, 0, buf, cmd);
> - }
> + ISCSI_REASON_PROTOCOL_ERROR,
> + 1, 0, buf, cmd);
> +
> } else if (immed_ret == IMMEDIATE_DATA_ERL1_CRC_FAILURE) {
> /*
> * Immediate Data failed DataCRC and ERL>=1,
> @@ -1835,23 +1816,15 @@ static int iscsit_handle_nop_out(
> return 0;
> }
>
> - cmdsn_ret = iscsit_check_received_cmdsn(conn, cmd, hdr->cmdsn);
> - if ((cmdsn_ret == CMDSN_NORMAL_OPERATION) ||
> - (cmdsn_ret == CMDSN_HIGHER_THAN_EXP)) {
> - return 0;
> - } else if (cmdsn_ret == CMDSN_LOWER_THAN_EXP) {
> - cmd->i_state = ISTATE_REMOVE;
> - iscsit_add_cmd_to_immediate_queue(cmd, conn,
> - cmd->i_state);
> + cmdsn_ret = iscsit_sequence_cmd(conn, cmd, hdr->cmdsn);
> + if (cmdsn_ret == CMDSN_LOWER_THAN_EXP) {
> ret = 0;
> goto ping_out;
> - } else { /* (cmdsn_ret == CMDSN_ERROR_CANNOT_RECOVER) */
> + }
> + if (cmdsn_ret == CMDSN_ERROR_CANNOT_RECOVER)
> return iscsit_add_reject_from_cmd(
> ISCSI_REASON_PROTOCOL_ERROR,
> 1, 0, buf, cmd);
> - ret = -1;
> - goto ping_out;
> - }
>
> return 0;
> }
> @@ -1901,7 +1874,8 @@ static int iscsit_handle_task_mgt_cmd(
> struct iscsi_tmr_req *tmr_req;
> struct iscsi_tm *hdr;
> u32 payload_length;
> - int cmdsn_ret, out_of_order_cmdsn = 0, ret;
> + int out_of_order_cmdsn = 0;
> + int ret;
> u8 function;
>
> hdr = (struct iscsi_tm *) buf;
> @@ -2024,16 +1998,10 @@ attach:
> spin_unlock_bh(&conn->cmd_lock);
>
> if (!(hdr->opcode & ISCSI_OP_IMMEDIATE)) {
> - cmdsn_ret = iscsit_check_received_cmdsn(conn,
> - cmd, hdr->cmdsn);
> - if (cmdsn_ret == CMDSN_NORMAL_OPERATION)
> - do {} while (0);
> - else if (cmdsn_ret == CMDSN_HIGHER_THAN_EXP)
> + int cmdsn_ret = iscsit_sequence_cmd(conn, cmd, hdr->cmdsn);
> + if (cmdsn_ret == CMDSN_HIGHER_THAN_EXP)
> out_of_order_cmdsn = 1;
> else if (cmdsn_ret == CMDSN_LOWER_THAN_EXP) {
> - cmd->i_state = ISTATE_REMOVE;
> - iscsit_add_cmd_to_immediate_queue(cmd, conn,
> - cmd->i_state);
> return 0;
> } else { /* (cmdsn_ret == CMDSN_ERROR_CANNOT_RECOVER) */
> return iscsit_add_reject_from_cmd(
> @@ -2209,19 +2177,11 @@ static int iscsit_handle_text_cmd(
> iscsit_ack_from_expstatsn(conn, hdr->exp_statsn);
>
> if (!(hdr->opcode & ISCSI_OP_IMMEDIATE)) {
> - cmdsn_ret = iscsit_check_received_cmdsn(conn, cmd, hdr->cmdsn);
> - if ((cmdsn_ret == CMDSN_NORMAL_OPERATION) ||
> - (cmdsn_ret == CMDSN_HIGHER_THAN_EXP))
> - return 0;
> - else if (cmdsn_ret == CMDSN_LOWER_THAN_EXP) {
> - iscsit_add_cmd_to_immediate_queue(cmd, conn,
> - ISTATE_REMOVE);
> - return 0;
> - } else { /* (cmdsn_ret == CMDSN_ERROR_CANNOT_RECOVER) */
> + cmdsn_ret = iscsit_sequence_cmd(conn, cmd, hdr->cmdsn);
> + if (cmdsn_ret == CMDSN_ERROR_CANNOT_RECOVER)
> return iscsit_add_reject_from_cmd(
> ISCSI_REASON_PROTOCOL_ERROR,
> 1, 0, buf, cmd);
> - }
>
> return 0;
> }
> @@ -2406,29 +2366,24 @@ static int iscsit_handle_logout_cmd(
> iscsit_ack_from_expstatsn(conn, hdr->exp_statsn);
>
> /*
> - * Non-Immediate Logout Commands are executed in CmdSN order..
> + * Immediate commands are executed, well, immediately.
> + * Non-Immediate Logout Commands are executed in CmdSN order.
> */
> - if (!(hdr->opcode & ISCSI_OP_IMMEDIATE)) {
> - cmdsn_ret = iscsit_check_received_cmdsn(conn, cmd, hdr->cmdsn);
> - if ((cmdsn_ret == CMDSN_NORMAL_OPERATION) ||
> - (cmdsn_ret == CMDSN_HIGHER_THAN_EXP))
> - return logout_remove;
> - else if (cmdsn_ret == CMDSN_LOWER_THAN_EXP) {
> - cmd->i_state = ISTATE_REMOVE;
> - iscsit_add_cmd_to_immediate_queue(cmd, conn,
> - cmd->i_state);
> - return 0;
> - } else { /* (cmdsn_ret == CMDSN_ERROR_CANNOT_RECOVER) */
> + if (hdr->opcode & ISCSI_OP_IMMEDIATE) {
> + int ret = iscsit_execute_cmd(cmd, 0);
> +
> + if (ret < 0)
> + return ret;
> + } else {
> + cmdsn_ret = iscsit_sequence_cmd(conn, cmd, hdr->cmdsn);
> + if (cmdsn_ret == CMDSN_LOWER_THAN_EXP) {
> + logout_remove = 0;
> + } else if (cmdsn_ret == CMDSN_ERROR_CANNOT_RECOVER) {
> return iscsit_add_reject_from_cmd(
> - ISCSI_REASON_PROTOCOL_ERROR,
> - 1, 0, buf, cmd);
> + ISCSI_REASON_PROTOCOL_ERROR,
> + 1, 0, buf, cmd);
> }
> }
> - /*
> - * Immediate Logout Commands are executed, well, Immediately.
> - */
> - if (iscsit_execute_cmd(cmd, 0) < 0)
> - return -1;
>
> return logout_remove;
> }
> diff --git a/drivers/target/iscsi/iscsi_target_util.c b/drivers/target/iscsi/iscsi_target_util.c
> index 144f555..612658a 100644
> --- a/drivers/target/iscsi/iscsi_target_util.c
> +++ b/drivers/target/iscsi/iscsi_target_util.c
> @@ -385,100 +385,80 @@ struct iscsi_r2t *iscsit_get_holder_for_r2tsn(
> return NULL;
> }
>
> -inline int iscsit_check_received_cmdsn(
> - struct iscsi_conn *conn,
> - struct iscsi_cmd *cmd,
> - u32 cmdsn)
> +static inline int iscsit_check_received_cmdsn(struct iscsi_session *sess, u32 cmdsn)
> {
> int ret;
> +
> /*
> * This is the proper method of checking received CmdSN against
> * ExpCmdSN and MaxCmdSN values, as well as accounting for out
> * or order CmdSNs due to multiple connection sessions and/or
> * CRC failures.
> */
> - spin_lock(&conn->sess->cmdsn_lock);
> - if (iscsi_sna_gt(cmdsn, conn->sess->max_cmd_sn)) {
> + if (iscsi_sna_gt(cmdsn, sess->max_cmd_sn)) {
> printk(KERN_ERR "Received CmdSN: 0x%08x is greater than"
> - " MaxCmdSN: 0x%08x, protocol error.\n", cmdsn,
> - conn->sess->max_cmd_sn);
> - spin_unlock(&conn->sess->cmdsn_lock);
> - return CMDSN_ERROR_CANNOT_RECOVER;
> - }
> -
> - if (list_empty(&conn->sess->sess_ooo_cmdsn_list)) {
> - if (cmdsn == conn->sess->exp_cmd_sn) {
> - conn->sess->exp_cmd_sn++;
> - TRACE(TRACE_CMDSN, "Received CmdSN matches ExpCmdSN,"
> - " incremented ExpCmdSN to: 0x%08x\n",
> - conn->sess->exp_cmd_sn);
> - ret = iscsit_execute_cmd(cmd, 0);
> - spin_unlock(&conn->sess->cmdsn_lock);
> -
> - return (!ret) ? CMDSN_NORMAL_OPERATION :
> - CMDSN_ERROR_CANNOT_RECOVER;
> - } else if (iscsi_sna_gt(cmdsn, conn->sess->exp_cmd_sn)) {
> - TRACE(TRACE_CMDSN, "Received CmdSN: 0x%08x is greater"
> - " than ExpCmdSN: 0x%08x, not acknowledging.\n",
> - cmdsn, conn->sess->exp_cmd_sn);
> - goto ooo_cmdsn;
> - } else {
> - printk(KERN_ERR "Received CmdSN: 0x%08x is less than"
> - " ExpCmdSN: 0x%08x, ignoring.\n", cmdsn,
> - conn->sess->exp_cmd_sn);
> - spin_unlock(&conn->sess->cmdsn_lock);
> - return CMDSN_LOWER_THAN_EXP;
> - }
> + " MaxCmdSN: 0x%08x, protocol error.\n", cmdsn,
> + sess->max_cmd_sn);
> + ret = CMDSN_ERROR_CANNOT_RECOVER;
> +
> + } else if (cmdsn == sess->exp_cmd_sn) {
> + sess->exp_cmd_sn++;
> + TRACE(TRACE_CMDSN, "Received CmdSN matches ExpCmdSN,"
> + " incremented ExpCmdSN to: 0x%08x\n",
> + sess->exp_cmd_sn);
> + ret = CMDSN_NORMAL_OPERATION;
> +
> + } else if (iscsi_sna_gt(cmdsn, sess->exp_cmd_sn)) {
> + TRACE(TRACE_CMDSN, "Received CmdSN: 0x%08x is greater"
> + " than ExpCmdSN: 0x%08x, not acknowledging.\n",
> + cmdsn, sess->exp_cmd_sn);
> + ret = CMDSN_HIGHER_THAN_EXP;
> +
> } else {
> - int counter = 0;
> - u32 old_expcmdsn = 0;
> - if (cmdsn == conn->sess->exp_cmd_sn) {
> - old_expcmdsn = conn->sess->exp_cmd_sn++;
> - TRACE(TRACE_CMDSN, "Got missing CmdSN: 0x%08x matches"
> - " ExpCmdSN, incremented ExpCmdSN to 0x%08x.\n",
> - cmdsn, conn->sess->exp_cmd_sn);
> -
> - if (iscsit_execute_cmd(cmd, 0) < 0) {
> - spin_unlock(&conn->sess->cmdsn_lock);
> - return CMDSN_ERROR_CANNOT_RECOVER;
> - }
> - } else if (iscsi_sna_gt(cmdsn, conn->sess->exp_cmd_sn)) {
> - TRACE(TRACE_CMDSN, "CmdSN: 0x%08x greater than"
> - " ExpCmdSN: 0x%08x, not acknowledging.\n",
> - cmdsn, conn->sess->exp_cmd_sn);
> - goto ooo_cmdsn;
> - } else {
> - printk(KERN_ERR "CmdSN: 0x%08x less than ExpCmdSN:"
> - " 0x%08x, ignoring.\n", cmdsn,
> - conn->sess->exp_cmd_sn);
> - spin_unlock(&conn->sess->cmdsn_lock);
> - return CMDSN_LOWER_THAN_EXP;
> - }
> + printk(KERN_ERR "Received CmdSN: 0x%08x is less than"
> + " ExpCmdSN: 0x%08x, ignoring.\n", cmdsn,
> + sess->exp_cmd_sn);
> + ret = CMDSN_LOWER_THAN_EXP;
> + }
>
> - counter = iscsit_execute_ooo_cmdsns(conn->sess);
> - if (counter < 0) {
> - spin_unlock(&conn->sess->cmdsn_lock);
> - return CMDSN_ERROR_CANNOT_RECOVER;
> - }
> + return ret;
> +}
>
> -#ifdef CONFIG_ISCSI_TARGET_DEBUG
> - if (list_empty(&conn->sess->sess_ooo_cmdsn_list)) {
> - TRACE(TRACE_CMDSN, "Received final missing"
> - " CmdSNs: 0x%08x->0x%08x.\n",
> - old_expcmdsn, (conn->sess->exp_cmd_sn - 1));
> - } else {
> - TRACE(TRACE_CMDSN, "Still missing CmdSN(s),"
> - " continuing out of order operation.\n");
> - }
> -#endif
> +/*
> + * Commands may be received out of order if MC/S is in use.
> + * Ensure they are executed in CmdSN order.
> + */
> +int iscsit_sequence_cmd(
> + struct iscsi_conn *conn,
> + struct iscsi_cmd *cmd,
> + u32 cmdsn)
> +{
> + int ret;
> + int cmdsn_ret;
>
> - spin_unlock(&conn->sess->cmdsn_lock);
> - return CMDSN_NORMAL_OPERATION;
> - }
> + spin_lock(&conn->sess->cmdsn_lock);
>
> -ooo_cmdsn:
> - ret = iscsit_handle_ooo_cmdsn(conn->sess, cmd, cmdsn);
> + cmdsn_ret = iscsit_check_received_cmdsn(conn->sess, cmdsn);
> + switch (cmdsn_ret) {
> + case CMDSN_NORMAL_OPERATION:
> + ret = iscsit_execute_cmd(cmd, 0);
> + if ((ret >= 0) && !list_empty(&conn->sess->sess_ooo_cmdsn_list))
> + iscsit_execute_ooo_cmdsns(conn->sess);
> + break;
> + case CMDSN_HIGHER_THAN_EXP:
> + ret = iscsit_handle_ooo_cmdsn(conn->sess, cmd, cmdsn);
> + break;
> + case CMDSN_LOWER_THAN_EXP:
> + cmd->i_state = ISTATE_REMOVE;
> + iscsit_add_cmd_to_immediate_queue(cmd, conn, cmd->i_state);
> + ret = cmdsn_ret;
> + break;
> + default:
> + ret = cmdsn_ret;
> + break;
> + }
> spin_unlock(&conn->sess->cmdsn_lock);
> +
> return ret;
> }
>
> diff --git a/drivers/target/iscsi/iscsi_target_util.h b/drivers/target/iscsi/iscsi_target_util.h
> index 5015ceb..885ca6f 100644
> --- a/drivers/target/iscsi/iscsi_target_util.h
> +++ b/drivers/target/iscsi/iscsi_target_util.h
> @@ -53,7 +53,7 @@ extern int iscsit_decide_list_to_build(struct iscsi_cmd *, u32);
> extern struct iscsi_seq *iscsit_get_seq_holder_for_datain(struct iscsi_cmd *, u32);
> extern struct iscsi_seq *iscsit_get_seq_holder_for_r2t(struct iscsi_cmd *);
> extern struct iscsi_r2t *iscsit_get_holder_for_r2tsn(struct iscsi_cmd *, u32);
> -extern int iscsit_check_received_cmdsn(struct iscsi_conn *, struct iscsi_cmd *, u32);
> +int iscsit_sequence_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd, u32 cmdsn);
> extern int iscsit_check_unsolicited_dataout(struct iscsi_cmd *, unsigned char *);
> extern struct iscsi_cmd *iscsit_find_cmd_from_itt(struct iscsi_conn *, u32);
> extern struct iscsi_cmd *iscsit_find_cmd_from_itt_or_dump(struct iscsi_conn *,
> --
> 1.7.1
>
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
