RE: CmdSN greather than MaxCmdSN protocol error in LIO Iser

Moussa Ba (moussaba)
2013-11-12T22:12:34Z

The patch does not apply to 3.12, are you patching against 3.11? The MaxCmdSN error shows up on 3.12, the 3.11 error is a different one I am still chasing.

Moussa

> -----Original Message-----
> From: target-devel-owner@vger.kernel.org [mailto:target-devel-
> owner@vger.kernel.org] On Behalf Of Nicholas A. Bellinger
> Sent: Tuesday, November 12, 2013 1:16 PM
> To: Moussa Ba (moussaba)
> Cc: Mike Christie; target-devel@vger.kernel.org; Nicholas Bellinger; Or
> Gerlitz; linux-scsi
> Subject: Re: CmdSN greather than MaxCmdSN protocol error in LIO Iser
>
> On Tue, 2013-11-12 at 18:18 +0000, Moussa Ba (moussaba) wrote:
>
> <SNIP>
>
> > > >
> > > > Or to put it another way: what is preventing iscsi_data_xmit()
> from
> > > > completely draining both conn->cmdqueue + conn->requeue, even
> when
> > > the
> > > > CmdSN window has potentially been closed again..?
> > > >
> > >
> > > If the window was not big enough we would have not accepted the cmd
> > > from
> > > scsi-ml in iscsi_queuecommand. We would have never put it on the
> list
> > > above then.
> > >
> > > In the initiator we have:
> > >
> > > session->queued_cmdsn - It will always be less than or equal to the
> > > max_cmdsn, but greater than or equal to session->cmdsn. Think of it
> > > like
> > > preallocating the sn we are going to give the cmd. Depending on
> > > ordering
> > > it might be different (3 instead of 4), but the final value it gets
> > > will
> > > be less than the max_cmdsn.
> > >
> > > session->cmdsn - the sn we give the cmd when we actually put it on
> the
> > > wire. It is always less than or equal to queued_cmdsn.
> > >
> > > So we have cmdsn <= queued_cmdsn <= max_cmdsn.
> > >
> > > The iscsi_queuecommand window check makes sure we will only have
> put a
> > > cmd on the cmdqueue list if queued_cmdsn was less than or equal to
> the
> > > max_cmdsn. And cmdsn is less than or equal to queued_cmdsn, so we
> can
> > > run iscsi_data_xmit without checking the window values again,
> because
> > > it
> > > is not possible for something like max_cmdsn to decrease on us.
> >
> >
> >
> > I am attaching error codes on the inititator that seem to coincide
> with the timeout errors.
> >
> > [ 261.855254] connection6:0: pdu (op 0x65 itt 0x1) rejected. Reason
> code 0x4
> > [ 261.855266] connection6:0: pdu (op 0xa itt 0x1) rejected. Reason
> code 0x4
> > [ 261.855268] connection6:0: pdu (op 0x51 itt 0x1) rejected. Reason
> code 0x4
> >
> >
>
> Hi Moussa,
>
> After a bit more review, there may be a scenario introduced with v3.10
> code that could cause something like the reported CmdSN > MaxCmdSN
> error
> to occur..
>
> Care to attempt to reproduce with the following target patch..?
>
> Thanks,
>
> --nab
>
> diff --git a/drivers/infiniband/ulp/isert/ib_isert.c
> b/drivers/infiniband/ulp/isert/ib_isert.c
> index bbd86e8..5661075 100644
> --- a/drivers/infiniband/ulp/isert/ib_isert.c
> +++ b/drivers/infiniband/ulp/isert/ib_isert.c
> @@ -2029,8 +2029,6 @@ isert_map_rdma(struct iscsi_conn *conn, struct
> iscsi_cmd *cmd,
>
> if (wr->iser_ib_op == ISER_IB_RDMA_WRITE) {
> data_left = se_cmd->data_length;
> - iscsit_increment_maxcmdsn(cmd, conn->sess);
> - cmd->stat_sn = conn->stat_sn++;
> } else {
> sg_off = cmd->write_data_done / PAGE_SIZE;
> data_left = se_cmd->data_length - cmd->write_data_done;
> @@ -2242,8 +2240,6 @@ isert_reg_rdma_frwr(struct iscsi_conn *conn,
> struct iscsi_cmd *cmd,
>
> if (wr->iser_ib_op == ISER_IB_RDMA_WRITE) {
> data_left = se_cmd->data_length;
> - iscsit_increment_maxcmdsn(cmd, conn->sess);
> - cmd->stat_sn = conn->stat_sn++;
> } else {
> sg_off = cmd->write_data_done / PAGE_SIZE;
> data_left = se_cmd->data_length - cmd->write_data_done;
> @@ -2352,7 +2348,7 @@ isert_put_datain(struct iscsi_conn *conn, struct
> iscsi_cmd *cmd)
> * Build isert_conn->tx_desc for iSCSI response PDU and attach
> */
> isert_create_send_desc(isert_conn, isert_cmd, &isert_cmd-
> >tx_desc);
> - iscsit_build_rsp_pdu(cmd, conn, false, (struct iscsi_scsi_rsp
> *)
> + iscsit_build_rsp_pdu(cmd, conn, true, (struct iscsi_scsi_rsp *)
> &isert_cmd->tx_desc.iscsi_header);
> isert_init_tx_hdrs(isert_conn, &isert_cmd->tx_desc);
> isert_init_send_wr(isert_conn, isert_cmd,
> diff --git a/drivers/target/iscsi/iscsi_target_configfs.c
> b/drivers/target/iscsi/iscsi_target_config
> index 1eec37c..fde3624 100644
> --- a/drivers/target/iscsi/iscsi_target_configfs.c
> +++ b/drivers/target/iscsi/iscsi_target_configfs.c
> @@ -1790,6 +1790,11 @@ static int lio_queue_status(struct se_cmd
> *se_cmd)
> struct iscsi_cmd *cmd = container_of(se_cmd, struct iscsi_cmd,
> se_cmd);
>
> cmd->i_state = ISTATE_SEND_STATUS;
> +
> + if (cmd->se_cmd.scsi_status || cmd->sense_reason) {
> + iscsit_add_cmd_to_response_queue(cmd, cmd->conn, cmd-
> >i_state);
> + return 0;
> + }
> cmd->conn->conn_transport->iscsit_queue_status(cmd->conn, cmd);
>
> return 0;
> diff --git a/drivers/target/iscsi/iscsi_target_device.c
> b/drivers/target/iscsi/iscsi_target_device.c
> index 6c7a510..7087c73 100644
> --- a/drivers/target/iscsi/iscsi_target_device.c
> +++ b/drivers/target/iscsi/iscsi_target_device.c
> @@ -58,11 +58,7 @@ void iscsit_increment_maxcmdsn(struct iscsi_cmd
> *cmd, struct iscsi_session *sess
>
> cmd->maxcmdsn_inc = 1;
>
> - if (!mutex_trylock(&sess->cmdsn_mutex)) {
> - sess->max_cmd_sn += 1;
> - pr_debug("Updated MaxCmdSN to 0x%08x\n", sess-
> >max_cmd_sn);
> - return;
> - }
> + mutex_lock(&sess->cmdsn_mutex);
> sess->max_cmd_sn += 1;
> pr_debug("Updated MaxCmdSN to 0x%08x\n", sess->max_cmd_sn);
> mutex_unlock(&sess->cmdsn_mutex);
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe target-devel"
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html