Message-ID: <CAPj3X_VvpuDQGHt2xtBOJB2RNGvFfxQiX0GH0My19G3OP+76QQ@mail.gmail.com>
Date: Wed, 17 Jan 2024 13:09:03 -0800
From: Lee Duncan <lduncan@...e.com>
To: Chris Leech <cleech@...hat.com>
Cc: target-devel@...r.kernel.org, linux-scsi@...r.kernel.org, 
	linux-kernel@...r.kernel.org, dbond@...e.com, hare@...e.de, 
	michael.christie@...cle.com
Subject: Re: [PATCH 1/2] scsi: target: iscsi: handle SCSI immediate commands

Apologies for the delay in the reply, but over the
time to address Chris' two questions about this patch set. See below.

On Thu, Dec 14, 2023 at 12:30 PM Chris Leech <cleech@...hat.com> wrote:
>
> On Wed, Dec 13, 2023 at 05:24:54PM -0800, Lee Duncan wrote:
> > >
> > > > @@ -1255,14 +1248,15 @@ int iscsit_process_scsi_cmd(struct iscsit_conn *conn, struct iscsit_cmd *cmd,
> > > >       /*
> > > >        * Check the CmdSN against ExpCmdSN/MaxCmdSN here if
> > > >        * the Immediate Bit is not set, and no Immediate
> > > > -      * Data is attached.
> > > > +      * Data is attached. Also skip the check if this is
> > > > +      * an immediate command.
> > >
> > > This comment addition seems redundant, isn't that what the
> > > "Immediate Bit is not set" already means?
> >
> > The spec is confusing with respect to this. The "Immediate Bit" means
> > an immediate command. These commands are done "now", not queued, and
> > they do not increment the expected sequence number.
> >
> > Immediate data is different, and unfortunately named IMHO. It's when a
> > PDU supplies the data for the SCSI command in the current PDU instead
> > of the next PDU.
>
> I understand the protocol, just trying to make sense of the
> implementation and what the existing comment meant. And the existing
> comment already has two conditions in it, even if the code doesn't.
>
> I think I understand now why this is delaying CmdSN validation when
> there is immediate data, until after the DataCRC can be checked.
>
> This comment in iscsit_get_immediate_data, where the delayed processing
> occurs, also seems to read that "Immediate Bit" is in reference to an
> immediate command.
>
>   * A PDU/CmdSN carrying Immediate Data passed
>   * DataCRC, check against ExpCmdSN/MaxCmdSN if
>   * Immediate Bit is not set.
>
> but neither of these locations (before these changes) that mention the
> "Immediate Bit" in the comments actually check for cmd->immediate_cmd.
>

I talked to Chris a bit about this offline, for clarification. I believe I
understand his concern, and rather than try to assert the patch is
ok by inspection, I decided to just test it.

Turns out that normal PDU traffic for lots of writes generally
includes "immediate data", and so it was easy to test this.

Testing showed that Immediate Data still works correctly,
in SCSI Write PDUs. Test was:
* connect to an iSCSI target
* Write a bunch of data
* read back the data
* disconnect from target and compare data

In addition, I captured and analyzed the SCSI/iSCSI tcpdump trace,
and immediate data was present, as expected.

One co-worker ran a similar test (just the SCSI/iSCSI trace part),
and found the same results.

> > > >        *
> > > >        * A PDU/CmdSN carrying Immediate Data can only
> > > >        * be processed after the DataCRC has passed.
> > > >        * If the DataCRC fails, the CmdSN MUST NOT
> > > >        * be acknowledged. (See below)
> > > >        */
> > > > -     if (!cmd->immediate_data) {
> > > > +     if (!cmd->immediate_data && !cmd->immediate_cmd) {
> > > >               cmdsn_ret = iscsit_sequence_cmd(conn, cmd,
> > > >                                       (unsigned char *)hdr, hdr->cmdsn);
> > > >               if (cmdsn_ret == CMDSN_ERROR_CANNOT_RECOVER)
> > >
> > > Are you sure this needs to be checking both conditions here?  I'm
> > > struggling to understand why CmdSN checking would be bypassed for
> > > immediate data.  Is this a longstanding bug where the condition should
> > > have been on immediate_cmd (and only immediate_cmd) instead?
> >
> > The immediate data check was there already, and there haven't been any
> > bugs I know of, so I assumed that part of the code was ok.
> >
> > >
> > > Or is this because of the handling the immediate data with DataCRC case
> > > mentioned?  I do see iscsit_sequence_cmd also being called in
> > > iscsit_get_immediate_data.
> >
> > I will check that but I suspect you are correct.
>
> Is it correct to skip all of iscsit_sequence_cmd for an immediate
> command here? You are already skipping iscsit_check_received_cmdsn
> inside iscsit_sequence_cmd in this patch. If cmd->immediate_cmd is set,
> where does iscsit_execute_cmd now get called from?

I looked at the code and the SPEC in more detail, and I believe the answer
is "yes", it is correct.

That function checks the current PDU's sequence number with
the following tests (and side effects), but not in this order:
* check that seq# is not larger than maximum
* check that seq# is not larger than expected
* check that seq# is not smaller than expected
* else the seq# is correct, so *SIDE* *EFFECT* increment the
                                               expected seq# for next PDU

It turns out that the SPEC allow the sequence number to be
out of range for immediate commands! So none of the checks
in iscsit_sequence_check_received_cmndsn() are valid for
immediate commands, as far as I can see.

>
> - Chris Leech
>

Powered by blists - more mailing lists