Hi Sumit,

Apologies for the delayed response.  Comments below..

On Sun, 2016-07-03 at 19:24 +0530, Sumit Rai wrote:
> > From: Sumit Rai <sumit....@calsoftinc.com>
> > Subject: LIO seems to use SCSI Allocation Length instead of SPDTL for 
> > calculating ResidualCount
> > Date: June 22, 2016 at 7:43:50 PM GMT+5:30
> > To: target-de...@vger.kernel.org
> > 
> > We are trying to test if LIO target sets ResidualCount correctly in case of 
> > ResidualOverflow for Data-In by following the below Steps:
> > 1. Send SCSI Inquiry command to iSCSI target, if there are no exceptions in 
> > the response (and no residual overflow) we are able to determine how many 
> > bytes the target SCSI layer on target side attempted to transfer to target 
> > iSCSI layer. On the initiator side when response is received: in the 
> > absence of any exceptions or residual overflow we assume all the data was 
> > successfully transferred and is equal to iSCSI DataSegmentLength. We will 
> > use this value as SPDTL for SCSI Inquiry command.
> > 
> > Note: SPDTL is detailed in RFC 7143 11.4.5.2 (and you may also refer to 
> > discussion on T10 mailing list: 
> > http://www.t10.org/pipermail/t10/2014-June/017367.html).
> > 
> > 2. Send out the same SCSI inquiry command as in Step 1. but set iSCSI EDTL 
> > to a value lower than SPDTL.
> > 
> > Assumption: Since we are not making any changes to iSCSI target LU 
> > configuration in our setup b/w Step 1. and 2, we assume amount of Inquiry 
> > Data target generates will be same of 1. and 2. since inquiry command is 
> > the same. During our testing we find this assumption to be true.
> > 
> > Expected Result:
> > Since in Step 2, SPDTL > EDTL, iSCSI target is expected to set 
> > ResidualOverflow flag and ResidualCount to SPDTL - EDTL.
> > In the attached PCAP text file, frame no. 11 (SCSI Inquiry Req.) and 12 
> > (Inquiry Response) are for Step 1. As explained, SPDTL = 0x24 or 36D.
> > Frame no. 14 is for (SCSI Inq. Req.) Step 2. EDTL = 0x08.
> > Hence, expected ResidualCount = 0x1C (28D) i.e. 0x24 (36D) - 0x08 (8D)
> > 
> > Observed Result:
> > iSCSI target sets the Residual Overflow flag correctly but value of 
> > ResidualCount doesn’t match expected value.
> > Target is setting, ResidualCount to 0x3f8 (1016D) as seen in frame no 16. 
> > In the frame no. 14, in the SCSI Inquiry CDB allocation length (SPC 5r01 
> > Section 4.2.5.6) was set to 0x0400 (1024D) and target seems to be using 
> > this value to calculate ResidualCount i.e. 0x0400 (1024D) - 0x08.
> > To further verify this we  changed the allocation length value in SCSI 
> > Inquiry CDB to 0x0200 (512D) bytes instead, we get ResidualCount of 0x1f8 
> > (504D).
> > 
> > Allocation Length is the maximum value of the SPDTL and not substitute for 
> > it, hence it shouldn’t be used to calculate ResidualCount except for cases 
> > where SPDTL > Allocation Length and Data is truncated (in that case both 
> > Alloc Len and SPDTL are same). (SPC 5r01 Section 4.2.5.6).
> > 
> > LIO Version: Datera Inc. iSCSI Target v4.1.0
> > Linux Kernel: 4.4.0-22-generic
> > 
> > RFC 7143:
> > 
> > 11.4.5.2.  Residuals Concepts Overview
> > 
> > 
> >   "SCSI-Presented Data Transfer Length (SPDTL)" is the term this
> >   document uses (see 
> > Section 2.2
> > for definition) to represent the
> >   aggregate data length that the target SCSI layer attempts to transfer
> >   using the local iSCSI layer for a task.  "Expected Data Transfer
> > 
> >   Length (EDTL)" is the iSCSI term that represents the length of data
> >   that the iSCSI layer expects to transfer for a task.  EDTL is
> >   specified in the SCSI Command PDU.
> > 
> >   When SPDTL = EDTL for a task, the target iSCSI layer completes the
> >   task with no residuals.  Whenever SPDTL differs from EDTL for a task,
> >   that task is said to have a residual.
> > 
> >   If SPDTL > EDTL for a task, iSCSI Overflow MUST be signaled in the
> >   SCSI Response PDU as specified in 
> > Section 11.4.5.1
> > .  The Residual
> >   Count MUST be set to the numerical value of (SPDTL - EDTL).
> > 
> >   If SPDTL < EDTL for a task, iSCSI Underflow MUST be signaled in the
> >   SCSI Response PDU as specified in 
> > Section 11.4.5.1
> > .  The Residual
> >   Count MUST be set to the numerical value of (EDTL - SPDTL).
> > 
> >   Note that the Overflow and Underflow scenarios are independent of
> >   Data-In and Data-Out.  Either scenario is logically possible in
> > 
> > 
> > SPC 5r01
> > 4.2.5.6 Allocation length
> > 
> > The ALLOCATION LENGTH field specifies the maximum number of bytes or blocks 
> > that an application client has allocated in the Data-In Buffer. The 
> > ALLOCATION LENGTH field specifies bytes unless a different requirement is 
> > stated in the command definition.
> > 
> > An allocation length of zero specifies that no data shall be transferred. 
> > This condition shall not be considered an error.
> > 
> > The device server shall terminate transfers to the Data-In Buffer when the 
> > number of bytes or blocks specified by the ALLOCATION LENGTH field have 
> > been transferred or when all available data have been transferred, 
> > whichever is less. The allocation length is used to limit the maximum 
> > amount of variable length data (e.g., mode data, log data, diagnostic data) 
> > returned to an application client. If the information being transferred to 
> > the Data-In Buffer includes fields containing counts of the number of bytes 
> > in some or all of the data (e.g., a PARAMETER DATA LENGTH field, a PAGE 
> > LENGTH field, a DESCRIPTOR LENGTH field, an AVAILABLE DATA field), then the 
> > contents of these fields shall not be altered to reflect the truncation, if 
> > any, that results from an insufficient ALLOCATION LENGTH value, unless the 
> > standard that describes the Data-In Buffer format states otherwise.
> > 
> > If the amount of information that is available to be transferred exceeds 
> > the maximum value that the ALLOCATION LENGTH field in combination with 
> > other fields in the CDB is capable of specifying, then no data shall be 
> > transferred and the command shall be terminated with CHECK CONDITION 
> > status, with the sense key set to ILLEGAL REQUEST, and the additional sense 
> > code set to INVALID FIELD IN CDB. 
> 
> I havn't received any comments on this issue, I guess the maintainer(s) are 
> occpied with
> other high priority issues, so I tried to fix the issue. I have successfully 
> tested
> the patch and it calculates the residual count as expected. I would 
> appreciate it if someone
> can do a review of the below patch:
> 
> --- linux/drivers/target/target_core_transport.c.orig 2016-07-03 
> 18:09:28.949281611 +0530
> +++ linux/drivers/target/target_core_transport.c      2016-07-03 
> 18:10:44.007293696 +0530
> @@ -754,7 +754,15 @@ EXPORT_SYMBOL(target_complete_cmd);
>  
>  void target_complete_cmd_with_length(struct se_cmd *cmd, u8 scsi_status, int 
> length)
>  {
> -     if (scsi_status == SAM_STAT_GOOD && length < cmd->data_length) {
> +     if (scsi_status != SAM_STAT_GOOD) {
> +             return;
> +     }
> +
> +     /*
> +      * Calculate new residual count based upon length of SCSI data
> +      * transferred.
> +      */
> +     if (length < cmd->data_length) {
>               if (cmd->se_cmd_flags & SCF_UNDERFLOW_BIT) {
>                       cmd->residual_count += cmd->data_length - length;
>               } else {
> @@ -763,6 +771,12 @@ void target_complete_cmd_with_length(str
>               }
>  
>               cmd->data_length = length;
> +     } else if (length > cmd->data_length) {
> +             cmd->se_cmd_flags |= SCF_OVERFLOW_BIT;
> +             cmd->residual_count = length - cmd->data_length;
> +     } else {
> +             cmd->se_cmd_flags &= ~(SCF_OVERFLOW_BIT | SCF_UNDERFLOW_BIT);
> +             cmd->residual_count = 0;
>       }
>  
>       target_complete_cmd(cmd, scsi_status);
> Signed-off-by: Sumit Rai <sumitra...@gmail.com>
> 
> Thanks to Ajay Nair in assisting with this patch.
> 

Applied to target-pending/for-next here.

https://git.kernel.org/cgit/linux/kernel/git/nab/target-pending.git/commit/?h=for-next&id=577d8f97b4f911881dfebe4c619b6eb9f087f042

Thanks Sumit + Ajay for tracking this down.

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to