target: transport should allow st ILI reads

When a tape drive is exported via LIO using the pscsi module, a read that requests more bytes per block than the tape can supply returns an empty buffer. This is because the pscsi pass-through target module sees the "ILI" illegal length bit set and thinks there is no reason to return the data.

and the read succeeded. The layered tape driver then "does the right thing" when it gets such a response.
- Moved ugly code from transport to pscsi module

enum {

drivers/target/target_core_transport.c +5 -0

index 74b646f165d4..56661a824266 100644
@@ -2192,6 +2192,11 @@ static void target_complete_ok_work(struct work_struct *work)
 	if (atomic_read(&cmd->se_dev->dev_qf_count) != 0)
 		schedule_work(&cmd->se_dev->qf_work_queue);
 
+	if (cmd->se_cmd_flags & SCF_TREAT_READ_AS_NORMAL) {
+		pr_debug("Tape FM/EOM/ILI status detected. Treating as OK\n");
+		goto treat_as_normal_read;
+	}
+
 	/*
 	 * Check if we need to send a sense buffer from
 	 * the struct se_cmd in question.

When a tape drive is exported via LIO using the pscsi module, a read that requests more bytes per block than the tape can supply returns an empty buffer. This is because the pscsi pass-through target module sees the "ILI" illegal length bit set and thinks there is no reason to return the data.

This is a long-standing transport issue, since it assume that no data need be returned under a check condition, which isn't always the case for tape.

Add in a check for tape reads with the the ILI, EOM, or FM bits set, with a sense code of NO_SENSE, treating such cases as if there is no sense data and the read succeeded. The layered tape driver then "does the right thing" when it gets such a response.

Changes from RFC:
- Moved ugly code from transport to pscsi module
- Added checking EOM and FM bits, as well as ILI
- fixed malformed patch
- Clarified description a bit

Signed-off-by Lee Duncan <[email protected]>

---
 drivers/target/target_core_pscsi.c     | 22 +++++++++++++++++++++-
 drivers/target/target_core_transport.c |  6 ++++++
 include/target/target_core_base.h      |  1 +
 3 files changed, 28 insertions(+), 1 deletion(-)

drivers/target/target_core_pscsi.c +21 -1

index 0d99b242e82e..b237104af81c 100644
@@ -689,8 +689,28 @@ static void pscsi_complete_cmd(struct se_cmd *cmd, u8 scsi_status,
 	}
 after_mode_select:
 
-	if (scsi_status = SAM_STAT_CHECK_CONDITION)
+	if (scsi_status = SAM_STAT_CHECK_CONDITION) {
 		transport_copy_sense_to_cmd(cmd, req_sense);
+
+		/*
+		 * hack to check for TAPE device reads with
+		 * FM/EOM/ILI set, so that we can get data
+		 * back despite framework assumption that a
+		 * check condition means there is no data
+		 */
+		if ((sd->type = TYPE_TAPE) &&
+		    (cmd->data_direction = DMA_FROM_DEVICE)) {
+			/*
+			 * is sense data valid, fixed format,
+			 * and have FM, EOM, or ILI set?
+			 */
+			if ((req_sense[0] = 0xf0) &&	/* valid, fixed format */
+			    (req_sense[2] & 0xe0) &&	/* FM, EOM, or ILI */
+			    ((req_sense[2] & 0xf) = 0)) { /* key=NO_SENSE */
+				cmd->se_cmd_flags |= SCF_TREAT_READ_AS_NORMAL;
+			}
+		}
+	}
 }
 
 enum {

drivers/target/target_core_transport.c +5 -0

index 74b646f165d4..56661a824266 100644
@@ -2192,6 +2192,11 @@ static void target_complete_ok_work(struct work_struct *work)
 	if (atomic_read(&cmd->se_dev->dev_qf_count) != 0)
 		schedule_work(&cmd->se_dev->qf_work_queue);
 
+	if (cmd->se_cmd_flags & SCF_TREAT_READ_AS_NORMAL) {
+		pr_debug("Tape FM/EOM/ILI status detected. Treating as OK\n");
+		goto treat_as_normal_read;
+	}
+
 	/*
 	 * Check if we need to send a sense buffer from
 	 * the struct se_cmd in question.

Quoted 81 lines · Lee Duncan

When a tape drive is exported via LIO using the pscsi module, a read that requests more bytes per block than the tape can supply returns an empty buffer. This is because the pscsi pass-through target module sees the "ILI" illegal length bit set and thinks there is no reason to return the data.

This is a long-standing transport issue, since it assume that no data need be returned under a check condition, which isn't always the case for tape.

Add in a check for tape reads with the the ILI, EOM, or FM bits set, with a sense code of NO_SENSE, treating such cases as if there is no sense data and the read succeeded. The layered tape driver then "does the right thing" when it gets such a response.

Changes from RFC:
- Moved ugly code from transport to pscsi module
- Added checking EOM and FM bits, as well as ILI
- fixed malformed patch
- Clarified description a bit

Signed-off-by Lee Duncan <[email protected]>

---
 drivers/target/target_core_pscsi.c     | 22 +++++++++++++++++++++-
 drivers/target/target_core_transport.c |  6 ++++++
 include/target/target_core_base.h      |  1 +
 3 files changed, 28 insertions(+), 1 deletion(-)

drivers/target/target_core_pscsi.c +21 -1

index 0d99b242e82e..b237104af81c 100644
@@ -689,8 +689,28 @@ static void pscsi_complete_cmd(struct se_cmd *cmd, u8 scsi_status,
 	}
 after_mode_select:
 
-	if (scsi_status = SAM_STAT_CHECK_CONDITION)
+	if (scsi_status = SAM_STAT_CHECK_CONDITION) {
 		transport_copy_sense_to_cmd(cmd, req_sense);
+
+		/*
+		 * hack to check for TAPE device reads with
+		 * FM/EOM/ILI set, so that we can get data
+		 * back despite framework assumption that a
+		 * check condition means there is no data
+		 */
+		if ((sd->type = TYPE_TAPE) &&
+		    (cmd->data_direction = DMA_FROM_DEVICE)) {
+			/*
+			 * is sense data valid, fixed format,
+			 * and have FM, EOM, or ILI set?
+			 */
+			if ((req_sense[0] = 0xf0) &&	/* valid, fixed format */
+			    (req_sense[2] & 0xe0) &&	/* FM, EOM, or ILI */
+			    ((req_sense[2] & 0xf) = 0)) { /* key=NO_SENSE */
+				cmd->se_cmd_flags |= SCF_TREAT_READ_AS_NORMAL;
+			}
+		}
+	}
 }
 
 enum {

drivers/target/target_core_transport.c +5 -0

index 74b646f165d4..56661a824266 100644
@@ -2192,6 +2192,11 @@ static void target_complete_ok_work(struct work_struct *work)
 	if (atomic_read(&cmd->se_dev->dev_qf_count) != 0)
 		schedule_work(&cmd->se_dev->qf_work_queue);
 
+	if (cmd->se_cmd_flags & SCF_TREAT_READ_AS_NORMAL) {
+		pr_debug("Tape FM/EOM/ILI status detected. Treating as OK\n");
+		goto treat_as_normal_read;
+	}
+
 	/*
 	 * Check if we need to send a sense buffer from
 	 * the struct se_cmd in question.

drivers/target/target_core_transport.c static void target_complete_ok_work(struct work_struct *work) · line 2251

Quoted 6 lines

drivers/target/target_core_transport.c +1 -0

@@ -2241,6 +2246,7 @@ static void target_complete_ok_work(struct work_struct *work)
 		if (cmd->scsi_status)
 			goto queue_status;
 
+treat_as_normal_read:
 		atomic_long_add(cmd->data_length,
 				&cmd->se_lun->lun_stats.tx_data_octets);

Do you want to return both the data and sense or just one or the other? Both right? In this code path we would return both the data and sense for drivers like iscsi.

If the queue_data_in call a little below this line returns -ENOMEM/-EAGAIN then I think the queue full handling is going to end up only returning the sense like in your original bug. You would need a similar change to transport_complete_qf so it returns the data.

Quoted 31 lines · Mike Christie

Quoted 30 lines · Lee Duncan

When a tape drive is exported via LIO using the pscsi module, a read that requests more bytes per block than the tape can supply returns an empty buffer. This is because the pscsi pass-through target module sees the "ILI" illegal length bit set and thinks there is no reason to return the data.

This is a long-standing transport issue, since it assume that no data need be returned under a check condition, which isn't always the case for tape.

Add in a check for tape reads with the the ILI, EOM, or FM bits set, with a sense code of NO_SENSE, treating such cases as if there is no sense data and the read succeeded. The layered tape driver then "does the right thing" when it gets such a response.

Changes from RFC:
- Moved ugly code from transport to pscsi module
- Added checking EOM and FM bits, as well as ILI
- fixed malformed patch
- Clarified description a bit

Signed-off-by Lee Duncan <[email protected]>

---
 drivers/target/target_core_pscsi.c     | 22 +++++++++++++++++++++-
 drivers/target/target_core_transport.c |  6 ++++++
 include/target/target_core_base.h      |  1 +
 3 files changed, 28 insertions(+), 1 deletion(-)

Quoted 6 lines

Quoted 1 line

...

Do you want to return both the data and sense or just one or the other? Both right? In this code path we would return both the data and sense for drivers like iscsi.

Quoted 5 lines

If the queue_data_in call a little below this line returns -ENOMEM/-EAGAIN then I think the queue full handling is going to end up only returning the sense like in your original bug. You would need a similar change to transport_complete_qf so it returns the data.

Quoted 1 line

Oh yeah, one other question/comment. The above code is bypassing the normal sense sending code so SCF_SENT_CHECK_CONDITION is not going to be set. For iscsi could you end up where 2 paths complete the same command because a reassign could race with one of these completions?