| Message ID | 419bcd22-b871-a812-fb5e-a889ecd85666@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Jiang, Apologies for the delayed follow-up. Comments below. On Mon, 2017-07-10 at 15:38 +0800, Jiang Yi wrote: > Hi Nic, > > I ran libiscsi test suite and find that some test cases failed. One of > them is "iSCSI.iSCSIdatasn": > > ./libiscsi/test-tool/iscsi-test-cu --test=iSCSI.iSCSIdatasn --verbose > --Verbose-scsi --dataloss iscsi://${IP}:3260/${IQN}/0 > > This is the output of the failed test: > > // start of test output > > Send READCAPACITY10 (Expecting SUCCESS) LBA:0 pmi:0 > [OK] READCAPACITY10 returned SUCCESS NO SENSE(0x00) (null)(0x0000) > Send READCAPACITY16 (Expecting SUCCESS) > [OK] READCAPACITY16 returned SUCCESS NO SENSE(0x00) (null)(0x0000) > Send INQUIRY (Expecting SUCCESS) evpd:0 page_code:00 alloc_len:64 > [OK] INQUIRY returned SUCCESS NO SENSE(0x00) (null)(0x0000) > Send INQUIRY (Expecting SUCCESS) evpd:1 page_code:b0 alloc_len:64 > [OK] INQUIRY returned SUCCESS NO SENSE(0x00) (null)(0x0000) > Send INQUIRY (Expecting SUCCESS) evpd:1 page_code:b1 alloc_len:255 > [OK] INQUIRY returned SUCCESS NO SENSE(0x00) (null)(0x0000) > Send INQUIRY (Expecting SUCCESS) evpd:1 page_code:b2 alloc_len:64 > [OK] INQUIRY returned SUCCESS NO SENSE(0x00) (null)(0x0000) > Send REPORT_SUPPORTED_OPCODE (Expecting SUCCESS) RCTD:1 OPTIONS:0 > OPCODE:0x00 SA:0 ALLOC_LEN:65535 > [SKIPPED] REPORT_SUPPORTED_OPCODES is not implemented. > Send MODESENSE6 (Expecting SUCCESS) > [OK] MODESENSE6 returned SUCCESS NO SENSE(0x00) (null)(0x0000) > Send PRIN/READ_KEYS > > > > CUnit - A unit testing framework for C - Version 2.1-2 > http://cunit.sourceforge.net/ > > > Suite: iSCSIdatasn > Test: iSCSIDataSnInvalid ... > Test sending invalid iSCSI DataSN > Send two Data-Out PDU's with DataSN==0. Should fail. > Send WRITE10 (Expecting SUCCESS) LBA:100 blocks:2 wrprotect:0 dpo:0 > fua:0 fua_nv:0 group:0 > [FAILED] WRITE10 command failed with sense. NO > SENSE(0x00)/(null)(0x0000) > Send Data-Out PDU with DataSN==27. Should fail > Send WRITE10 (Expecting SUCCESS) LBA:100 blocks:1 wrprotect:0 dpo:0 > fua:0 fua_nv:0 group:0 > [FAILED] WRITE10 command failed with sense. NO > SENSE(0x00)/(null)(0x0000) > Send Data-Out PDU with DataSN==-1. Should fail > Send WRITE10 (Expecting SUCCESS) LBA:100 blocks:1 wrprotect:0 dpo:0 > fua:0 fua_nv:0 group:0 > [OK] WRITE10 returned SUCCESS NO SENSE(0x00) (null)(0x0000) > Send Data-Out PDU's in reverse order (DataSN == 1,0). Should fail > Send WRITE10 (Expecting SUCCESS) LBA:100 blocks:2 wrprotect:0 dpo:0 > fua:0 fua_nv:0 group:0 > [FAILED] WRITE10 command failed with sense. NO > SENSE(0x00)/(null)(0x0000) > FAILED > 1. test_iscsi_datasn_invalid.c:142 - CU_ASSERT_NOT_EQUAL(ret,0) > > Run Summary: Type Total Ran Passed Failed Inactive > suites 1 1 n/a 0 0 > tests 1 1 0 1 0 > asserts 4 4 3 1 n/a > > Elapsed time = 2.808 seconds > Tests completed with return value: 0 > > // end of test output > > To fix it, I propose a patch: > > Signed-off-by: Jiang Yi <jiangyilism@gmail.com> > --- > drivers/target/iscsi/iscsi_target_erl0.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/drivers/target/iscsi/iscsi_target_erl0.c b/drivers/target/iscsi/iscsi_target_erl0.c > index 7fe2aa7..15fe495 100644 > --- a/drivers/target/iscsi/iscsi_target_erl0.c > +++ b/drivers/target/iscsi/iscsi_target_erl0.c > @@ -297,7 +297,10 @@ static int iscsit_dataout_check_sequence( > pr_err("Command ITT: 0x%08x set ISCSI_FLAG_CMD_FINAL" > " before end of DataOUT sequence, protocol" > " error.\n", cmd->init_task_tag); > - return DATAOUT_CANNOT_RECOVER; > + if (iscsit_dump_data_payload(conn, payload_length, 1) < 0) { > + return DATAOUT_CANNOT_RECOVER; > + } > + return DATAOUT_WITHIN_COMMAND_RECOVERY; > } > } else { > if (next_burst_len < seq->xfer_len) { > @@ -382,7 +385,10 @@ static int iscsit_dataout_check_datasn( > if (!conn->sess->sess_ops->ErrorRecoveryLevel) { > pr_err("Unable to perform within-command recovery" > " while ERL=0.\n"); > - return DATAOUT_CANNOT_RECOVER; > + if (iscsit_dump_data_payload(conn, payload_length, 1) < 0) { > + return DATAOUT_CANNOT_RECOVER; > + } > + return DATAOUT_WITHIN_COMMAND_RECOVERY; > } > dump: > if (iscsit_dump_data_payload(conn, payload_length, 1) < 0) So normally when a session is using ErrorRecoveryLevel=0, any case that requires within-command-recovery (eg: part of ErrorRecoveryLevel=1) normally translates to a session recovery event. ErrorRecoveryLevel=1 involves both sides being responsible for implicitly resending lost PDUs, and/or explicitly re-requesting the transfer of lost PDUs depending on the scenario. That said, here's a more for detail on the two cases in your patch: 1) When a FINAL_BIT is received in ErrorRecoveryLevel=0 for a solicited data-out sequence with DataSequenceInOrder=Yes, but not all of data-out sequence was received before the FINAL_BIT was received. This is considered a protocol error, because during ErrorRecoveryLevel=0 the target is not going to re-request lost solicited data-out sequences or PDUs using a recovery R2T. So silently dropping PDU and payload here is incorrect. 2) Likewise, when a solicited data-out PDU is received with a higher DataSN that currently expected in ErrorRecoveryLevel=0 mode, it means one of the earlier PDUs was dropped. During ErrorRecoveryLevel=1, the target will generate a recovery R2T but during ErrorRecoveryLevel=0 there is no within-command recovery logic enabled, so silently dropping it here is also incorrect. So wrt to libiscsi, any error that would normally involve within-command-recovery to kick in for ErrorRecoveryLevel=1, should translate to session recovery when ErrorRecoveryLeve=0 is used. I haven't looked at those specific tests, but I'm wondering if it's actually expecting a REJECT PDU during a protocol error like these, instead of the target forcing session recovery. -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/target/iscsi/iscsi_target_erl0.c b/drivers/target/iscsi/iscsi_target_erl0.c index 7fe2aa7..15fe495 100644 --- a/drivers/target/iscsi/iscsi_target_erl0.c +++ b/drivers/target/iscsi/iscsi_target_erl0.c @@ -297,7 +297,10 @@ static int iscsit_dataout_check_sequence( pr_err("Command ITT: 0x%08x set ISCSI_FLAG_CMD_FINAL" " before end of DataOUT sequence, protocol" " error.\n", cmd->init_task_tag); - return DATAOUT_CANNOT_RECOVER; + if (iscsit_dump_data_payload(conn, payload_length, 1) < 0) { + return DATAOUT_CANNOT_RECOVER; + } + return DATAOUT_WITHIN_COMMAND_RECOVERY; } } else { if (next_burst_len < seq->xfer_len) { @@ -382,7 +385,10 @@ static int iscsit_dataout_check_datasn( if (!conn->sess->sess_ops->ErrorRecoveryLevel) { pr_err("Unable to perform within-command recovery" " while ERL=0.\n"); - return DATAOUT_CANNOT_RECOVER; + if (iscsit_dump_data_payload(conn, payload_length, 1) < 0) { + return DATAOUT_CANNOT_RECOVER; + } + return DATAOUT_WITHIN_COMMAND_RECOVERY; } dump: if (iscsit_dump_data_payload(conn, payload_length, 1) < 0)
Hi Nic, I ran libiscsi test suite and find that some test cases failed. One of them is "iSCSI.iSCSIdatasn": ./libiscsi/test-tool/iscsi-test-cu --test=iSCSI.iSCSIdatasn --verbose --Verbose-scsi --dataloss iscsi://${IP}:3260/${IQN}/0 This is the output of the failed test: // start of test output Send READCAPACITY10 (Expecting SUCCESS) LBA:0 pmi:0 [OK] READCAPACITY10 returned SUCCESS NO SENSE(0x00) (null)(0x0000) Send READCAPACITY16 (Expecting SUCCESS) [OK] READCAPACITY16 returned SUCCESS NO SENSE(0x00) (null)(0x0000) Send INQUIRY (Expecting SUCCESS) evpd:0 page_code:00 alloc_len:64 [OK] INQUIRY returned SUCCESS NO SENSE(0x00) (null)(0x0000) Send INQUIRY (Expecting SUCCESS) evpd:1 page_code:b0 alloc_len:64 [OK] INQUIRY returned SUCCESS NO SENSE(0x00) (null)(0x0000) Send INQUIRY (Expecting SUCCESS) evpd:1 page_code:b1 alloc_len:255 [OK] INQUIRY returned SUCCESS NO SENSE(0x00) (null)(0x0000) Send INQUIRY (Expecting SUCCESS) evpd:1 page_code:b2 alloc_len:64 [OK] INQUIRY returned SUCCESS NO SENSE(0x00) (null)(0x0000) Send REPORT_SUPPORTED_OPCODE (Expecting SUCCESS) RCTD:1 OPTIONS:0 OPCODE:0x00 SA:0 ALLOC_LEN:65535 [SKIPPED] REPORT_SUPPORTED_OPCODES is not implemented. Send MODESENSE6 (Expecting SUCCESS) [OK] MODESENSE6 returned SUCCESS NO SENSE(0x00) (null)(0x0000) Send PRIN/READ_KEYS CUnit - A unit testing framework for C - Version 2.1-2 http://cunit.sourceforge.net/ Suite: iSCSIdatasn Test: iSCSIDataSnInvalid ... Test sending invalid iSCSI DataSN Send two Data-Out PDU's with DataSN==0. Should fail. Send WRITE10 (Expecting SUCCESS) LBA:100 blocks:2 wrprotect:0 dpo:0 fua:0 fua_nv:0 group:0 [FAILED] WRITE10 command failed with sense. NO SENSE(0x00)/(null)(0x0000) Send Data-Out PDU with DataSN==27. Should fail Send WRITE10 (Expecting SUCCESS) LBA:100 blocks:1 wrprotect:0 dpo:0 fua:0 fua_nv:0 group:0 [FAILED] WRITE10 command failed with sense. NO SENSE(0x00)/(null)(0x0000) Send Data-Out PDU with DataSN==-1. Should fail Send WRITE10 (Expecting SUCCESS) LBA:100 blocks:1 wrprotect:0 dpo:0 fua:0 fua_nv:0 group:0 [OK] WRITE10 returned SUCCESS NO SENSE(0x00) (null)(0x0000) Send Data-Out PDU's in reverse order (DataSN == 1,0). Should fail Send WRITE10 (Expecting SUCCESS) LBA:100 blocks:2 wrprotect:0 dpo:0 fua:0 fua_nv:0 group:0 [FAILED] WRITE10 command failed with sense. NO SENSE(0x00)/(null)(0x0000) FAILED 1. test_iscsi_datasn_invalid.c:142 - CU_ASSERT_NOT_EQUAL(ret,0) Run Summary: Type Total Ran Passed Failed Inactive suites 1 1 n/a 0 0 tests 1 1 0 1 0 asserts 4 4 3 1 n/a Elapsed time = 2.808 seconds Tests completed with return value: 0 // end of test output To fix it, I propose a patch: Signed-off-by: Jiang Yi <jiangyilism@gmail.com> --- drivers/target/iscsi/iscsi_target_erl0.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)