[PATCH 0/3] scsi: target: Set correct residual data

[PATCH 0/3] scsi: target: Set correct residual data

[Anastasia Kovaleva]

Hi Martin,
Please apply the changes to 5.10/scsi-queue at your earliest
convenience.

Tne series changes the behavior of the target in regard to processing
commands with overflow/underflow in accordance with the standarts.

The target driver used to process the DMA_TO_DEVICE commands with
residuals (in particular, WRITE command) incorrectly. The target
response contained neither a residual count, nor an OVERFLOW/UNDERFLOW
bit. Such behavior did not comply with the RFC 7143. Also the
returned ASC and ASCQ were incorrect according to FCP-4,
and residuals were not set for the 4Kn devices.

This patches fix the major inconsistances in processing these kind of
commands.

This patch series has been tested with a modified libiscsi testing
library.
The link to the pull request:
https://github.com/sahlberg/libiscsi/pull/345

Write10Residuals, Write12Residuals, Write16Residuals tests have passed.

Thanks,
Anastasia

Anastasia Kovaleva (2):
  scsi: target: core: Signal WRITE residuals
  scsi: target: core: Change ASCQ for residual write

Roman Bolshakov (1):
  scsi: target: core: Set residuals for 4Kn devices

 drivers/target/target_core_transport.c | 53 +++++++++++++-------------
 include/target/target_core_base.h      |  1 +
 2 files changed, 28 insertions(+), 26 deletions(-)

-- 
2.24.3 (Apple Git-128)

[PATCH 1/3] scsi: target: core: Set residuals for 4Kn devices

[Anastasia Kovaleva]

From: Roman Bolshakov <r.bolshakov@yadro.com>

TCM always fails SBC commands with residuals for 4Kn devices when the
command is processed by sbc_parse_cdb(). That prevents residual
signalling to the transport driver because residual kind and residual
amount aren't set. It also makes residual handling different from
512-byte formatted devices - if there are residuals 512-byte LUN would
proceed with command execution while 4K-byte LUN would fail.

Based-on: https://patchwork.kernel.org/project/target-devel/patch/20170523234854.21452-31-bart.vanassche@sandisk.com/
Based-on-patch-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
Signed-off-by: Konstantin Vinogradov <k.vinogradov@yadro.com>
---
 drivers/target/target_core_transport.c | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 9fb0be0aa620..6d73b453c2cb 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1325,17 +1325,6 @@ target_cmd_size_check(struct se_cmd *cmd, unsigned int size)
 				return TCM_INVALID_CDB_FIELD;
 			}
 		}
-		/*
-		 * Reject READ_* or WRITE_* with overflow/underflow for
-		 * type SCF_SCSI_DATA_CDB.
-		 */
-		if (dev->dev_attrib.block_size != 512)  {
-			pr_err("Failing OVERFLOW/UNDERFLOW for LBA op"
-				" CDB on non 512-byte sector setup subsystem"
-				" plugin: %s\n", dev->transport->name);
-			/* Returns CHECK_CONDITION + INVALID_CDB_FIELD */
-			return TCM_INVALID_CDB_FIELD;
-		}
 		/*
 		 * For the overflow case keep the existing fabric provided
 		 * ->data_length.  Otherwise for the underflow case, reset
-- 
2.24.3 (Apple Git-128)

[PATCH 2/3] scsi: target: core: Signal WRITE residuals

[Anastasia Kovaleva]

According to RFC 7143 11.4.5.2.:

  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).

libiscsi has residual write tests that check residual kind and residual
amount and all of them (Write10Residuals, Write12Residuals,
Write16Residuals) currently fail.

One of the reasons why they fail is because target completes write
commands with INVALID FIELD IN CDB before setting the Overflow/Underflow
bit and residual amount.

Set the Overflow/Underflow bit and the residual amount before failing a
write to comply with RFC 7143.

Signed-off-by: Anastasia Kovaleva <a.kovaleva@yadro.com>
Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
---
 drivers/target/target_core_transport.c | 34 +++++++++++++++-----------
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 6d73b453c2cb..8cb3012721d8 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1306,6 +1306,26 @@ target_cmd_size_check(struct se_cmd *cmd, unsigned int size)
 			" %u does not match SCSI CDB Length: %u for SAM Opcode:"
 			" 0x%02x\n", cmd->se_tfo->fabric_name,
 				cmd->data_length, size, cmd->t_task_cdb[0]);
+		/*
+		 * For READ command for the overflow case keep the existing
+		 * fabric provided ->data_length. Otherwise for the underflow
+		 * case, reset ->data_length to the smaller SCSI expected data
+		 * transfer length.
+		 */
+		if (size > cmd->data_length) {
+			cmd->se_cmd_flags |= SCF_OVERFLOW_BIT;
+			cmd->residual_count = (size - cmd->data_length);
+		} else {
+			cmd->se_cmd_flags |= SCF_UNDERFLOW_BIT;
+			cmd->residual_count = (cmd->data_length - size);
+			/*
+			 * Do not truncate ->data_length for WRITE command to
+			 * dump all payload
+			 */
+			if (cmd->data_direction = DMA_FROM_DEVICE) {
+				cmd->data_length = size;
+			}
+		}
 
 		if (cmd->data_direction = DMA_TO_DEVICE) {
 			if (cmd->se_cmd_flags & SCF_SCSI_DATA_CDB) {
@@ -1325,20 +1345,6 @@ target_cmd_size_check(struct se_cmd *cmd, unsigned int size)
 				return TCM_INVALID_CDB_FIELD;
 			}
 		}
-		/*
-		 * For the overflow case keep the existing fabric provided
-		 * ->data_length.  Otherwise for the underflow case, reset
-		 * ->data_length to the smaller SCSI expected data transfer
-		 * length.
-		 */
-		if (size > cmd->data_length) {
-			cmd->se_cmd_flags |= SCF_OVERFLOW_BIT;
-			cmd->residual_count = (size - cmd->data_length);
-		} else {
-			cmd->se_cmd_flags |= SCF_UNDERFLOW_BIT;
-			cmd->residual_count = (cmd->data_length - size);
-			cmd->data_length = size;
-		}
 	}
 
 	return target_check_max_data_sg_nents(cmd, dev, size);
-- 
2.24.3 (Apple Git-128)

[PATCH 3/3] scsi: target: core: Change ASCQ for residual write

[Anastasia Kovaleva]

According to FCP-4 (9.4.2):

  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.

TCM follows b) and transfers no data for residual writes but returns
INVALID FIELD IN CDB instead of INVALID FIELD IN COMMAND INFORMATION
UNIT.

Change the ASCQ to INVALID FIELD IN COMMAND INFORMATION UNIT to meet the
standart.

Signed-off-by: Anastasia Kovaleva <a.kovaleva@yadro.com>
Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
---
 drivers/target/target_core_transport.c | 8 +++++++-
 include/target/target_core_base.h      | 1 +
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 8cb3012721d8..b225dab4deb9 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1331,7 +1331,7 @@ target_cmd_size_check(struct se_cmd *cmd, unsigned int size)
 			if (cmd->se_cmd_flags & SCF_SCSI_DATA_CDB) {
 				pr_err_ratelimited("Rejecting underflow/overflow"
 						   " for WRITE data CDB\n");
-				return TCM_INVALID_CDB_FIELD;
+				return TCM_INVALID_FIELD_IN_COMMAND_IU;
 			}
 			/*
 			 * Some fabric drivers like iscsi-target still expect to
@@ -1904,6 +1904,7 @@ void transport_generic_request_failure(struct se_cmd *cmd,
 	case TCM_UNSUPPORTED_TARGET_DESC_TYPE_CODE:
 	case TCM_TOO_MANY_SEGMENT_DESCS:
 	case TCM_UNSUPPORTED_SEGMENT_DESC_TYPE_CODE:
+	case TCM_INVALID_FIELD_IN_COMMAND_IU:
 		break;
 	case TCM_OUT_OF_RESOURCES:
 		cmd->scsi_status = SAM_STAT_TASK_SET_FULL;
@@ -3240,6 +3241,11 @@ static const struct sense_info sense_info_table[] = {
 		.asc = 0x55,
 		.ascq = 0x04, /* INSUFFICIENT REGISTRATION RESOURCES */
 	},
+	[TCM_INVALID_FIELD_IN_COMMAND_IU] = {
+		.key = ILLEGAL_REQUEST,
+		.asc = 0x0e,
+		.ascq = 0x03, /* INVALID FIELD IN COMMAND INFORMATION UNIT */
+	},
 };
 
 /**
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index 549947d407cf..01296d62ba5e 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -187,6 +187,7 @@ enum tcm_sense_reason_table {
 	TCM_UNSUPPORTED_SEGMENT_DESC_TYPE_CODE	= R(0x1c),
 	TCM_INSUFFICIENT_REGISTRATION_RESOURCES	= R(0x1d),
 	TCM_LUN_BUSY				= R(0x1e),
+	TCM_INVALID_FIELD_IN_COMMAND_IU         = R(0x1f),
 #undef R
 };
 
-- 
2.24.3 (Apple Git-128)

Re: [PATCH 3/3] scsi: target: core: Change ASCQ for residual write

[Bart Van Assche]

On 10/22/20 10:20 AM, Anastasia Kovaleva wrote:
> According to FCP-4 (9.4.2):
> 
>   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.
> 
> TCM follows b) and transfers no data for residual writes but returns
> INVALID FIELD IN CDB instead of INVALID FIELD IN COMMAND INFORMATION
> UNIT.
> 
> Change the ASCQ to INVALID FIELD IN COMMAND INFORMATION UNIT to meet the
> standart.

Is FCP the only standard that requires to report INVALID FIELD IN COMMAND
INFORMATION UNIT for residual overflow? I haven't found any similar
requirement in the iSCSI RFC nor in the SRP standard.

Additionally, what benefits does it provide to report a CHECK CONDITION
upon residual overflow? The SCST QLogic FC target driver doesn't do this
as far as I know, is more than ten years old, is widely used and so far
nobody complained about this.

Bart.

Re: [PATCH 3/3] scsi: target: core: Change ASCQ for residual write

[Roman Bolshakov]

On Fri, Oct 23, 2020 at 09:07:38PM -0700, Bart Van Assche wrote:
> On 10/22/20 10:20 AM, Anastasia Kovaleva wrote:
> > According to FCP-4 (9.4.2):
> > 
> >   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.
> > 
> > TCM follows b) and transfers no data for residual writes but returns
> > INVALID FIELD IN CDB instead of INVALID FIELD IN COMMAND INFORMATION
> > UNIT.
> > 
> > Change the ASCQ to INVALID FIELD IN COMMAND INFORMATION UNIT to meet the
> > standart.
> 
> Is FCP the only standard that requires to report INVALID FIELD IN COMMAND
> INFORMATION UNIT for residual overflow? I haven't found any similar
> requirement in the iSCSI RFC nor in the SRP standard.
> 

Hi Bart,

iSCSI doesn't specify a specific code but mentions a possibility of CHECK
CONDITION for residuals (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).

I have skimmed over SRP and haven't found if it's explicitly disallowed
to send CHECK CONDITION for READ/WRITEs with residuals.

> Additionally, what benefits does it provide to report a CHECK CONDITION
> upon residual overflow?

Typical use case for CHECK CONDITION in case of Underflow/Overflow is
extra robustness against buggy initiators [1][2]. Failing both READ and
WRITE is the most solid approach in that sense [3][4][5] as it prevents
data corruption at all costs.

Suppose an initiator wants to WRITE 8 LBA. For 512-byte formatted LUN,
8 LBAs need a buffer of 4K bytes. For 4096-byte formatted LUN the
command would need 32K data buffer.

An Overflow happens if initiator treats 4Kn device like 512n one but
provides a buffer of 4K. i.e. to complete the WRITE target needs to
consume 28K more data, otherwise only 1 LBA would be written and the
rest 7 LBAs would have indeterminate content.

An Underflow happens if initiator confuses 512n device with 4Kn one and
provides a buffer of 32K, i.e. target doesn't utilize all buffer for the
command.

> The SCST QLogic FC target driver doesn't do this as far as I know, is
> more than ten years old, is widely used and so far nobody complained
> about this.
> 

It might be true but there are no public tests for FC. We're planning to
extend libiscsi using SG_IO v4 to cover more corner cases, like FC
residuals and aborts/error recovery. Also, SLER (Sequence level error
recovery) is comming to FCP-5/FC-NVMe-2, it be useful to test it too.

1. https://www.t10.org/pipermail/t10/2014-June/017369.html
2. https://www.t10.org/pipermail/t10/2014-June/017370.html
3. https://www.t10.org/pipermail/t10/2012-September/016340.html
4. https://www.t10.org/pipermail/t10/2012-September/016350.html
5. https://mailarchive.ietf.org/arch/msg/ips/PpMCMQw-HzKQ5gwteFD54CN_0EY/

Regards,
Roman

Re: [PATCH 3/3] scsi: target: core: Change ASCQ for residual write

[Bart Van Assche]

On 10/24/20 5:13 AM, Roman Bolshakov wrote:
> iSCSI doesn't specify a specific code but mentions a possibility of CHECK
> CONDITION for residuals (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).

My interpretation of the above text is that it neither allows nor
requires to change the status GOOD into something else if there is a
residue.

>> Additionally, what benefits does it provide to report a CHECK CONDITION
>> upon residual overflow?
> 
> Typical use case for CHECK CONDITION in case of Underflow/Overflow is
> extra robustness against buggy initiators [1][2]. Failing both READ and
> WRITE is the most solid approach in that sense [3][4][5] as it prevents
> data corruption at all costs.
> 
> Suppose an initiator wants to WRITE 8 LBA. For 512-byte formatted LUN,
> 8 LBAs need a buffer of 4K bytes. For 4096-byte formatted LUN the
> command would need 32K data buffer.
> 
> An Overflow happens if initiator treats 4Kn device like 512n one but
> provides a buffer of 4K. i.e. to complete the WRITE target needs to
> consume 28K more data, otherwise only 1 LBA would be written and the
> rest 7 LBAs would have indeterminate content.
> 
> An Underflow happens if initiator confuses 512n device with 4Kn one and
> provides a buffer of 32K, i.e. target doesn't utilize all buffer for the
> command.

Thanks for the additional background information, this really helps. How
about only rejecting SCSI commands for which the data buffer size is not
a multiple of the block size? I'm concerned that flagging all SCSI
commands that have a residue as invalid will break SCSI tape software.

Thanks,

Bart.

Re: [PATCH 3/3] scsi: target: core: Change ASCQ for residual write

[Roman Bolshakov]

On Sat, Oct 24, 2020 at 05:25:17PM -0700, Bart Van Assche wrote:
> On 10/24/20 5:13 AM, Roman Bolshakov wrote:
> >> Additionally, what benefits does it provide to report a CHECK CONDITION
> >> upon residual overflow?
> > 
> > Typical use case for CHECK CONDITION in case of Underflow/Overflow is
> > extra robustness against buggy initiators [1][2]. Failing both READ and
> > WRITE is the most solid approach in that sense [3][4][5] as it prevents
> > data corruption at all costs.
> > 
> > Suppose an initiator wants to WRITE 8 LBA. For 512-byte formatted LUN,
> > 8 LBAs need a buffer of 4K bytes. For 4096-byte formatted LUN the
> > command would need 32K data buffer.
> > 
> > An Overflow happens if initiator treats 4Kn device like 512n one but
> > provides a buffer of 4K. i.e. to complete the WRITE target needs to
> > consume 28K more data, otherwise only 1 LBA would be written and the
> > rest 7 LBAs would have indeterminate content.
> > 
> > An Underflow happens if initiator confuses 512n device with 4Kn one and
> > provides a buffer of 32K, i.e. target doesn't utilize all buffer for the
> > command.
> 
> Thanks for the additional background information, this really helps. How
> about only rejecting SCSI commands for which the data buffer size is not
> a multiple of the block size? I'm concerned that flagging all SCSI
> commands that have a residue as invalid will break SCSI tape software.
> 

Hi Bart,

Could you please elaborate on how tape software will be broken?
I have no experience with tapes but I've looked into SSC-5 draft.

I haven't found anything concerning the writes but there are tape
variants of overflow/underflow for reads (G.3 General read rules) called
overlength and underlegth, respectively:

  If the read command requests fewer bytes than are available for
  transfer, then the read is an overlength read. If the read requests
  more bytes than are available, then the read is an underlength read.

  The amount of data returned is the smaller of the bytes available and
  the allocation length.

And the next paragraph defines cases where CHECK CONDITION should be
reported for such reads. However, GOOD status is also possible, the next
chapter of the annex (G.4 Examples from figure G.1 using variable-block
transfers and various SILI and BLOCK LENGTH settings) refines many cases
depending on SILI bit, whether block protection is enabled, if the
transfer is FIXED or variable-length and if BLOCK LENGTH is
zero/non-zero.

As far as I understand underlength and overlength are always suppressed
(status is GOOD) for devices where no "default" block size is defined
per SPC (7.5.7.1 General block descriptor format):

  For sequential access devices, a block length of zero indicates that the
  logical block size written to the medium is specified by the TRANSFER
  LENGTH field in the CDB (see SSC-4).

The cases are also summarized in annex D (D.3 Summary of length error
conditions on read type commands).

Note, that if we talk about SSC over FCP, then "9.4.2 FCP_DATA IUs for
read and write operations" does additionally apply. Perhaps a) from
"9.4.2 FCP_DATA IUs for read and write operations" works well for SSC:

  a) process the command normally except that data beyond the FCP_DL count
  shall not be requested or transferred;

The clause allows to accomodate variable-block tranfers from SSC.

So, what if we return CHECK CONDITION only for SBC WRITEs with
residuals?  Then it has no impact on SSC and other device types. In
future, we might also add a patch that would fail SBC READs with
residuals for sake of consistency. That behaviour would be beneficial
for SBC devices as no host could corrupt data or itself by forming,
requesting invalid data buffer.

Thanks,
Roman

Re: [PATCH 3/3] scsi: target: core: Change ASCQ for residual write

[Bart Van Assche]

On 10/26/20 6:12 AM, Roman Bolshakov wrote:
> Could you please elaborate on how tape software will be broken?
> I have no experience with tapes but I've looked into SSC-5 draft.
> 
> I haven't found anything concerning the writes but there are tape
> variants of overflow/underflow for reads (G.3 General read rules) called
> overlength and underlegth, respectively:
> 
>   If the read command requests fewer bytes than are available for
>   transfer, then the read is an overlength read. If the read requests
>   more bytes than are available, then the read is an underlength read.
> 
>   The amount of data returned is the smaller of the bytes available and
>   the allocation length.
> 
> And the next paragraph defines cases where CHECK CONDITION should be
> reported for such reads. However, GOOD status is also possible, the next
> chapter of the annex (G.4 Examples from figure G.1 using variable-block
> transfers and various SILI and BLOCK LENGTH settings) refines many cases
> depending on SILI bit, whether block protection is enabled, if the
> transfer is FIXED or variable-length and if BLOCK LENGTH is
> zero/non-zero.
> 
> As far as I understand underlength and overlength are always suppressed
> (status is GOOD) for devices where no "default" block size is defined
> per SPC (7.5.7.1 General block descriptor format):
> 
>   For sequential access devices, a block length of zero indicates that the
>   logical block size written to the medium is specified by the TRANSFER
>   LENGTH field in the CDB (see SSC-4).
> 
> The cases are also summarized in annex D (D.3 Summary of length error
> conditions on read type commands).
> 
> Note, that if we talk about SSC over FCP, then "9.4.2 FCP_DATA IUs for
> read and write operations" does additionally apply. Perhaps a) from
> "9.4.2 FCP_DATA IUs for read and write operations" works well for SSC:
> 
>   a) process the command normally except that data beyond the FCP_DL count
>   shall not be requested or transferred;
> 
> The clause allows to accomodate variable-block tranfers from SSC.
> 
> So, what if we return CHECK CONDITION only for SBC WRITEs with
> residuals?  Then it has no impact on SSC and other device types. In
> future, we might also add a patch that would fail SBC READs with
> residuals for sake of consistency. That behaviour would be beneficial
> for SBC devices as no host could corrupt data or itself by forming,
> requesting invalid data buffer.

Hi Roman,

Maybe I'm overly concerned. I do not know for sure which applications
rely on the current behavior of residual handling. All I know about
these applications is based on what others wrote about these
applications. An example from
https://www.t10.org/pipermail/t10/2003-November/009317.html: "We have
customers who also use overlength and underlength transfers as a normal
mode of operation."

An additional question is what behavior other operating systems than
Linux expect? There are probably setups in which another operating
system than Linux communicates with a LIO SCSI target?

Thanks,

Bart.

Re: [PATCH 3/3] scsi: target: core: Change ASCQ for residual write

[Roman Bolshakov]

On Mon, Oct 26, 2020 at 07:42:55PM -0700, Bart Van Assche wrote:
> On 10/26/20 6:12 AM, Roman Bolshakov wrote:
> > Note, that if we talk about SSC over FCP, then "9.4.2 FCP_DATA IUs for
> > read and write operations" does additionally apply. Perhaps a) from
> > "9.4.2 FCP_DATA IUs for read and write operations" works well for SSC:
> > 
> >   a) process the command normally except that data beyond the FCP_DL count
> >   shall not be requested or transferred;
> > 
> > The clause allows to accomodate variable-block tranfers from SSC.
> > 
> > So, what if we return CHECK CONDITION only for SBC WRITEs with
> > residuals?  Then it has no impact on SSC and other device types. In
> > future, we might also add a patch that would fail SBC READs with
> > residuals for sake of consistency. That behaviour would be beneficial
> > for SBC devices as no host could corrupt data or itself by forming,
> > requesting invalid data buffer.
> 
> Maybe I'm overly concerned. I do not know for sure which applications
> rely on the current behavior of residual handling. All I know about
> these applications is based on what others wrote about these
> applications. An example from
> https://www.t10.org/pipermail/t10/2003-November/009317.html: "We have
> customers who also use overlength and underlength transfers as a normal
> mode of operation."
> 

Hi Bart,

Thanks for raising the point about overlength/underlength. If you wish
we can add an extra check that fails DMA_TO_DEVICE && DATA with
residuals only for SBC devices but note that before the series,
underflow/overflow for WRITE didn't return GOOD status. The particular
patch only changes sense code to more meaningful from the former INVALID
FIELD IN CDB.

Theoretically, it could be good to have a configurable switch how LIO
handles overflows/underflows for a LUN. Then it'd be possible to
configure desired behaviour on a per-LUN basis. But there should be a
clear need & demand for the feature to avoid maintenance of dead code.

> An additional question is what behavior other operating systems than
> Linux expect? There are probably setups in which another operating
> system than Linux communicates with a LIO SCSI target?
> 

TBH I don't know any hosts that do SBC WRITE with residuals as normal
course of operation. They wouldn't be able to work with LIO because it
never returns GOOD status on WRITE with residuals. I can send an update
later if the series works fine with modern hosts (~1 month, after a few
cycles of system testing).

Fun fact, ~60 years ago WRITE overflows were used to achieve behaviour
similar to disk zeroing/WRITE SAME [1].

1. https://mailarchive.ietf.org/arch/msg/ips/135ycRlgwUg1sb3gRrUQ3-lSXg0/

Thanks,
Roman

Re: [PATCH 3/3] scsi: target: core: Change ASCQ for residual write

[Bart Van Assche]

On 10/27/20 4:46 PM, Roman Bolshakov wrote:
> Thanks for raising the point about overlength/underlength. If you wish
> we can add an extra check that fails DMA_TO_DEVICE && DATA with
> residuals only for SBC devices but note that before the series,
> underflow/overflow for WRITE didn't return GOOD status. The particular
> patch only changes sense code to more meaningful from the former INVALID
> FIELD IN CDB.
> 
> Theoretically, it could be good to have a configurable switch how LIO
> handles overflows/underflows for a LUN. Then it'd be possible to
> configure desired behaviour on a per-LUN basis. But there should be a
> clear need & demand for the feature to avoid maintenance of dead code.
>>> An additional question is what behavior other operating systems than
>> Linux expect? There are probably setups in which another operating
>> system than Linux communicates with a LIO SCSI target?
> 
> TBH I don't know any hosts that do SBC WRITE with residuals as normal
> course of operation. They wouldn't be able to work with LIO because it
> never returns GOOD status on WRITE with residuals. I can send an update
> later if the series works fine with modern hosts (~1 month, after a few
> cycles of system testing).

Hi Roman,

I'm not sure adding a new kernel switch is the best choice. That would
be an additional parameter users have to know about and have to learn
how to use.

Bodo seems to be in favor of this patch series. Are there other people
who want to share their opinion about this patch series?

Thanks,

Bart.

Re: [PATCH 3/3] scsi: target: core: Change ASCQ for residual write

[Anastasia Kovaleva]

On 28 Oct 2020, at 06:41, Bart Van Assche <bvanassche@acm.org> wrote:
> On 10/27/20 4:46 PM, Roman Bolshakov wrote:
>> Thanks for raising the point about overlength/underlength. If you wish
>> we can add an extra check that fails DMA_TO_DEVICE && DATA with
>> residuals only for SBC devices but note that before the series,
>> underflow/overflow for WRITE didn't return GOOD status. The particular
>> patch only changes sense code to more meaningful from the former INVALID
>> FIELD IN CDB.
>> 
>> Theoretically, it could be good to have a configurable switch how LIO
>> handles overflows/underflows for a LUN. Then it'd be possible to
>> configure desired behaviour on a per-LUN basis. But there should be a
>> clear need & demand for the feature to avoid maintenance of dead code.
>>>> An additional question is what behavior other operating systems than
>>> Linux expect? There are probably setups in which another operating
>>> system than Linux communicates with a LIO SCSI target?
>> 
>> TBH I don't know any hosts that do SBC WRITE with residuals as normal
>> course of operation. They wouldn't be able to work with LIO because it
>> never returns GOOD status on WRITE with residuals. I can send an update
>> later if the series works fine with modern hosts (~1 month, after a few
>> cycles of system testing).
> 
> Hi Roman,
> 
> I'm not sure adding a new kernel switch is the best choice. That would
> be an additional parameter users have to know about and have to learn
> how to use.
> 
> Bodo seems to be in favor of this patch series. Are there other people
> who want to share their opinion about this patch series?

Hi Bart,

Is this patch series good enough to be accepted in this form, without
the kernel switch? As far as i can see, no one has shared their opinion
about this changes. 

Thanks,

Anastasia

Re: [PATCH 3/3] scsi: target: core: Change ASCQ for residual write

[Bart Van Assche]

On 11/10/20 8:57 AM, Anastasia Kovaleva wrote:
> Is this patch series good enough to be accepted in this form, without
> the kernel switch? As far as i can see, no one has shared their opinion
> about this changes. 

Hi Anastasia,

I will leave it to others to review this patch series.

Thanks,

Bart.

Re: [PATCH 3/3] scsi: target: core: Change ASCQ for residual write

[Bodo Stroesser]

Am 25.10.20 um 02:25 schrieb Bart Van Assche:
> On 10/24/20 5:13 AM, Roman Bolshakov wrote:
>> iSCSI doesn't specify a specific code but mentions a possibility of CHECK
>> CONDITION for residuals (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).
> 
> My interpretation of the above text is that it neither allows nor
> requires to change the status GOOD into something else if there is a
> residue.
> 
>>> Additionally, what benefits does it provide to report a CHECK CONDITION
>>> upon residual overflow?
>>
>> Typical use case for CHECK CONDITION in case of Underflow/Overflow is
>> extra robustness against buggy initiators [1][2]. Failing both READ and
>> WRITE is the most solid approach in that sense [3][4][5] as it prevents
>> data corruption at all costs.
>>
>> Suppose an initiator wants to WRITE 8 LBA. For 512-byte formatted LUN,
>> 8 LBAs need a buffer of 4K bytes. For 4096-byte formatted LUN the
>> command would need 32K data buffer.
>>
>> An Overflow happens if initiator treats 4Kn device like 512n one but
>> provides a buffer of 4K. i.e. to complete the WRITE target needs to
>> consume 28K more data, otherwise only 1 LBA would be written and the
>> rest 7 LBAs would have indeterminate content.
>>
>> An Underflow happens if initiator confuses 512n device with 4Kn one and
>> provides a buffer of 32K, i.e. target doesn't utilize all buffer for the
>> command.
> 
> Thanks for the additional background information, this really helps. How
> about only rejecting SCSI commands for which the data buffer size is not
> a multiple of the block size? I'm concerned that flagging all SCSI
> commands that have a residue as invalid will break SCSI tape software.

AFAICS, there is no risk to break tape handling. target_cmd_size_check() is mainly used by sbc_parse_cdb(),
while passthrough_parse_cdb() optionally calls it for PERSISTENT_RESERVFE_IN/_OUT and RESERVE(_10)/RELEASE(_10) only.

sbc_parse_cdb is not usable for tape devices anyway, since CDB 'length' field in READ/WRITE for SSC devices needs special processing.
Depending on current state of the device, length 1 can have the meaning 1 byte or 1 times the optionally set fixed block size.
So the only way to set up a tape target LUN is to use tcmu or pscsi.

> 
> Thanks,
> 
> Bart.
> 

Re: [PATCH 0/3] scsi: target: Set correct residual data

[Bart Van Assche]

On 10/22/20 10:20 AM, Anastasia Kovaleva wrote:
> The link to the pull request:
> https://github.com/sahlberg/libiscsi/pull/345

I'm not going to merge that pull request without significant changes.
See also the comment that I added on the pull request on github.

Bart.