Improve WriteResidual tests #345

nastya-nizharadze · 2020-10-20T17:58:05Z

Linux target implementation, TCM, doesn’t pass write residuals tests (Write10Residuals, Write12Residualds, Write16Residuals).

One of the reason why they fail is because TCM doesn’t complete writes that have residuals with GOOD status, instead it replies with CHECK CONDITION and INVALID FIELD IN CDB sense code. The sense code doesn’t seem to be a valid reply but it's legit for a target to reply with CHECK CONDITION as outlined in the discussion on T10, RFC 7143 (11.4.5.1. Field Semantics):

Targets may set the residual count, and initiators may use
   it when the response code is Command Completed at Target (even if the
   status returned is not GOOD)

and FCP-4 (9.4.2 FCP_DATA IUs for read and write operations):

If the command requested that data beyond the length specified by the FCP_DL field be transferred, then the device server shall set the FCP_RESID_OVER bit (see 9.5.8) to one in the FCP_RSP IU and:
a) process the command normally except that data beyond the FCP_DL count shall not be requested or transferred;
b) transfer no data and return CHECK CONDITION status with the sense key set to ILLEGAL REQUEST and the additional sense code set to INVALID FIELD IN COMMAND INFORMATION UNIT; or
c) may transfer data and return CHECK CONDITION status with the sense key set to ABORTED COMMAND and the additional sense code set to INVALID FIELD IN COMMAND INFORMATION UNIT.

To allow different replies from different targets, b) from FCP-4 was added as an alternative valid reply. With the PR, libiscsi supports both GOOD and CHECK CONDITION with INVALID FIELD IN COMMAND INFORMATION UNIT.

The tests have also been refactored to make sure that no transfer happens if target replies with INVALID FIELD IN COMMAND INFORMATION UNIT. As a side effect of the refactoring, most of the code duplication was removed.

After refactoring, it turned out that some test cases duplicate each other, so they were removed.

As soon as I send patches in TCM to fix sense key I will attach the link to it here.

roolebo

It was thoroughly reviewed internally and tested with the changes to be sent upstream by @nastya-nizharadze: https://github.com/roolebo/linux/commits/target-write-residuals-v1-feedback

bvanassche · 2020-10-24T03:52:02Z

test-tool/test_write10_residuals.c

@@ -75,11 +75,15 @@ test_write10_residuals(void)
                return;
        }        
        logging(LOG_VERBOSE, "Verify that the target returned SUCCESS");
-        if (task->status != SCSI_STATUS_GOOD) {
+        ok = task->status == SCSI_STATUS_GOOD ||


Please introduce a function or macro instead of repeating this comparison multiple times.

Hi @bvanassche,

It is a good practice to separate a refactoring and a changes for the more convenient review. All repetitions are removed in the next commit. Even if I introduce a function or a macro, it will be removed by the next commit. However, if you think that it is really better to remove the duplication in this commit, please tell me and I will do it.

Thanks!

It is a good practice not to introduce repetitive code. Please introduce a helper function in this commit that returns the result of the following expression:

task->status == SCSI_STATUS_GOOD ||
(task->status == SCSI_STATUS_CHECK_CONDITION &&
task->sense.key == SCSI_SENSE_ILLEGAL_REQUEST &&
task->sense.ascq == SCSI_SENSE_ASCQ_INVALID_FIELD_IN_INFORMATION_UNIT

nastya-nizharadze · 2021-01-27T09:46:44Z

Hi @bvanassche,

Our patches have been added to the 5.12/scsi-staging

Now TCM send ASCQ = 0x03, INVALID FIELD IN COMMAND INFORMATION UNIT for residual write case.

Could you please tell me whether it is really necessary to make the previously requested changes?
Also if there is something else I could do/improve as part of these changes?

Thanks!

bvanassche · 2021-01-31T04:40:49Z

test-tool/test_write_residuals.h

+  unsigned int residuals_kind;
+  size_t residuals_amount;
+  bool check_overwrite;
+  const char log_messages[100];


Please change the type of the log message from const char [100] into const char* to reduce the space occupied by this data structure.

bvanassche · 2021-01-31T04:42:00Z

test-tool/test_write_residuals.h

+  const char log_messages[100];
+};
+
+extern bool command_is_implemented;


What is the purpose of this global variable? Since it is set but not used, can it be removed?

This variable is needed because if the command is not implemented, testing should end. It is used in test_write10_residuals.c, test_write12_residuals.c and test_write12_residuals.c

if (!command_is_implemented) { CU_PASS("WRITE10 is not implemented."); return; }

bvanassche · 2021-01-31T04:43:12Z

test-tool/test_write_residuals.h

+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <stdbool.h>
+
+#include <CUnit/CUnit.h>
+
+#include "iscsi.h"
+#include "iscsi-private.h"
+#include "scsi-lowlevel.h"
+#include "iscsi-test-cu.h"


Please remove all include directives except #include <stddef.h> (for size_t).

bvanassche · 2021-01-31T04:43:52Z

test-tool/test_write10_residuals.c

-        int ok;
-        unsigned int i;
+        /* testing scenarios */
+        const struct residuals_test_data write10_residuals[] = {


Please change 'const' into 'static const'.

I am using block_size variable to initialize this structure. Therefore I can not make it static.

bvanassche · 2021-01-31T04:45:10Z

test-tool/test_write12_residuals.c

-        int ok;
-        unsigned int i;
+        /* testing scenarios */
+        const struct residuals_test_data write12_residuals[] = {


Same comment here and in the other source files: please change 'const' into 'static const'.

bvanassche · 2021-01-31T04:46:11Z

test-tool/test_write_residuals.c

+
+        if (scsi_status != SCSI_STATUS_GOOD) {
+                logging(LOG_VERBOSE, "Verify that blocks were NOT "
+                        "overwritten and still contains 'a'");


contains -> contain?

bvanassche · 2021-01-31T04:47:44Z

include/iscsi-private.h

@@ -40,6 +40,9 @@ extern "C" {
 #ifndef ARRAY_SIZE
 #define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
 #endif
+#ifndef DIV_ROUND_UP
+#define DIV_ROUND_UP(x, y) (((x) + ((y) - 1)) / (y))


There are more parentheses in this expression than necessary. I think that ((y) - 1) can be changed into (y) - 1 without changing the result of this expression.

bvanassche · 2021-01-31T04:50:52Z

test-tool/test_write10_residuals.c

@@ -75,11 +75,15 @@ test_write10_residuals(void)
                return;
        }        
        logging(LOG_VERBOSE, "Verify that the target returned SUCCESS");
-        if (task->status != SCSI_STATUS_GOOD) {
+        ok = task->status == SCSI_STATUS_GOOD ||


It is a good practice not to introduce repetitive code. Please introduce a helper function in this commit that returns the result of the following expression:

task->status == SCSI_STATUS_GOOD ||
(task->status == SCSI_STATUS_CHECK_CONDITION &&
task->sense.key == SCSI_SENSE_ILLEGAL_REQUEST &&
task->sense.ascq == SCSI_SENSE_ASCQ_INVALID_FIELD_IN_INFORMATION_UNIT

According to the RFC 7143 11.4.5.1: "Targets may set the residual count,and initiators may use it when the response code is Command Completed at Target (even if the status returned is not GOOD)." Therefore valid retuned status may be not only GOOD. Also this check: task->sense.ascq == SCSI_SENSE_ASCQ_INVALID_FIELD_IN_INFORMATION_UNIT would make Underflow/Overflow response universal for FC/ISCSI.

bvanassche · 2021-02-07T02:41:05Z

test-tool/test_write_residuals.c

-                memset(scratch, 'b', (tdata->xfer_len * tdata->buf_len));
+                memset(scratch, 'b', tdata->buf_len);
        } else {
-                memset(scratch, 0xa6, (tdata->xfer_len * tdata->buf_len));
+                memset(scratch, 0xa6, tdata->buf_len);


Are these changes perhaps a bug fix for the previous patch in this series? If so, please move these into the previous patch.

bvanassche · 2021-02-07T02:41:36Z

test-tool/test_write_residuals.h

+  size_t cdb_size;
+  unsigned int xfer_len;
+  unsigned int buf_len;
+  unsigned int residuals_kind;
+  size_t residuals_amount;
+  bool check_overwrite;
+  const char *log_messages;


Please document the meaning of each member of this data structure.

Looking at test_write10_residuals.c, test_write12_residuals.c and test_write16_residuals.c tests the similarity of the testing scenario can be found. There are several EDTL and SPDTL combinations which are the same for different length write command tests. They form the core of testing scenario of these commands. There aren't so much differences in the way of testing these combinations itself either. It is possible to move the main parameters describing the testing scenario into a separate structure and move the scenario itself into a separate function. It will increase the readability and reduce the duplicate code of these tests.

…FCP-4 According to FCP-4 there are several possibilities for target to react on incorrect data length field value in CDB: a) process the command normally except that data beyond the FCP_DL count shall not be requested or transferred; b) transfer no data and return CHECK CONDITION status with the sense key set to ILLEGAL REQUEST and the additional sense code set to INVALID FIELD IN COMMAND INFORMATION UNIT; Add check to support the second one and accept no data write.

Check if the residual data does not owerwrite existing data blocks has now been added for all testing data to improve the uniformity of test runs, increase test readability and remove the duplicate testing data records.

nastya-nizharadze · 2021-02-09T09:02:09Z

Thanks!

roolebo approved these changes Oct 20, 2020

View reviewed changes

bvanassche requested changes Oct 24, 2020

View reviewed changes

bvanassche force-pushed the master branch 2 times, most recently from 1112923 to a2fa59b Compare January 25, 2021 00:58

bvanassche requested changes Jan 31, 2021

View reviewed changes

bvanassche reviewed Feb 7, 2021

View reviewed changes

nastya-nizharadze added 3 commits February 8, 2021 15:23

test-tool: Add overwrite check for all test cases

1b7d174

Check if the residual data does not owerwrite existing data blocks has now been added for all testing data to improve the uniformity of test runs, increase test readability and remove the duplicate testing data records.

bvanassche merged commit 88a46a0 into sahlberg:master Feb 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve WriteResidual tests #345

Improve WriteResidual tests #345

nastya-nizharadze commented Oct 20, 2020

roolebo left a comment •

edited

bvanassche Oct 24, 2020 •

edited

nastya-nizharadze Oct 24, 2020

bvanassche Jan 31, 2021

nastya-nizharadze Feb 2, 2021

nastya-nizharadze commented Jan 27, 2021

bvanassche Jan 31, 2021

nastya-nizharadze Feb 2, 2021

bvanassche Jan 31, 2021

nastya-nizharadze Feb 2, 2021 •

edited

bvanassche Jan 31, 2021

nastya-nizharadze Feb 2, 2021

bvanassche Jan 31, 2021

nastya-nizharadze Feb 2, 2021

bvanassche Jan 31, 2021

bvanassche Jan 31, 2021

nastya-nizharadze Feb 2, 2021

bvanassche Jan 31, 2021

nastya-nizharadze Feb 2, 2021

bvanassche Jan 31, 2021

bvanassche Feb 7, 2021

nastya-nizharadze Feb 8, 2021

bvanassche Feb 7, 2021

nastya-nizharadze Feb 8, 2021

nastya-nizharadze commented Feb 9, 2021

Improve WriteResidual tests #345

Improve WriteResidual tests #345

Conversation

nastya-nizharadze commented Oct 20, 2020

roolebo left a comment • edited

Choose a reason for hiding this comment

bvanassche Oct 24, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nastya-nizharadze commented Jan 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nastya-nizharadze Feb 2, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nastya-nizharadze commented Feb 9, 2021

roolebo left a comment •

edited

bvanassche Oct 24, 2020 •

edited

nastya-nizharadze Feb 2, 2021 •

edited