RE: CmdSN greather than MaxCmdSN protocol error in LIO Iser

Moussa Ba (moussaba)
2013-11-12T23:04:28Z

Your latest patch seems to have addressed the problem, I have not been able to reproduce it post patch and can readily reproduce it with the mainline 3.12 kernel. I will run a longer test tonight, but this seems to have done it.

Moussa

> -----Original Message-----
> From: Moussa Ba (moussaba)
> Sent: Tuesday, November 12, 2013 2:15 PM
> To: 'Nicholas A. Bellinger'
> 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
>
> That is what I am currently doing...Thanks. Will update shortly.
>
> Moussa
>
> > -----Original Message-----
> > From: Nicholas A. Bellinger [mailto:nab@linux-iscsi.org]
> > Sent: Tuesday, November 12, 2013 2:15 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 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
> >