Re: CmdSN greather than MaxCmdSN protocol error in LIO Iser

Nicholas A. Bellinger
2013-11-12T22:14:37Z

On Tue, 2013-11-12 at 22:11 +0000, Moussa Ba (moussaba) wrote:
> 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.

It's against the target-pending/for-next tree at v3.12-rc5..

Care to apply it manually..? The effort involved should be minimal.

--nab

>
> 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