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.rai@xxxxxxxxxxxxxx> > > 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-devel@xxxxxxxxxxxxxxx > > > > 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 <sumitrai96@xxxxxxxxx> > > 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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
