Received-SPF: pass (google.com: domain of linux-kernel+bounces-68-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Thu, 14 Dec 2023 12:29:59 -0800
From: Chris Leech <cleech@redhat.com>
To: Lee Duncan <lduncan@suse.com>
Cc: target-devel@vger.kernel.org, linux-scsi@vger.kernel.org,
	linux-kernel@vger.kernel.org, dbond@suse.com, hare@suse.de,
	michael.christie@oracle.com
Subject: Re: [PATCH 1/2] scsi: target: iscsi: handle SCSI immediate commands
Message-ID: <ZXtlxzVtY3M_WrQ2@rhel-developer-toolbox-latest>
References: <cover.1701540918.git.lduncan@suse.com>
 <dc0006176e90cf3fb90e5b1c1917b54fe07c91cd.1701540918.git.lduncan@suse.com>
 <ZXoOtgVZW_QpkU11@rhel-developer-toolbox-latest>
 <CAPj3X_W5kOEOapG3F8NETBRzBmrQ1Lfudy7QGmCLXPT3UwUrkw@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAPj3X_W5kOEOapG3F8NETBRzBmrQ1Lfudy7QGmCLXPT3UwUrkw@mail.gmail.com>

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.

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

- Chris Leech