target: transport should allow st FM/EOM/ILI reads
PATCH v3 1/1 target: transport should allow st FM/EOM/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.
This is a long-standing transport issue, since it assumes 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 ILI, EOM, or FM bits set, with a sense code of NO_SENSE, treating such cases as if the read succeeded. The layered tape driver then "does the right thing" when it gets such a response.
Changes from v2:
- Cleaned up subject line and bug text formatting
- Removed unneeded inner braces
- Removed ugly goto
- Also updated the "queue full" path to handle this case
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
--- drivers/target/target_core_pscsi.c | 23 +++++++++++++++++++- drivers/target/target_core_transport.c | 39 +++++++++++++++++++++++++++++----- include/target/target_core_base.h | 1 + 3 files changed, 57 insertions(+), 6 deletions(-)
drivers/target/target_core_pscsi.c +22 -1
@@ -689,8 +689,29 @@ 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 */ +pr_debug ("Tape FM/EOM/ILI status detected. Treat as normal read.\n" ); + cmd->se_cmd_flags |=SCF_TREAT_READ_AS_NORMAL ; + } + } + } }enum {
drivers/target/target_core_transport.c +34 -5
@@ -2084,12 +2084,24 @@ static void transport_complete_qf(struct se_cmd *cmd) goto queue_status; } -if (cmd->se_cmd_flags &SCF_TRANSPORT_TASK_SENSE ) +/* +* Check if we need to send a sense buffer from +* the struct se_cmd in question. We do NOT want +* to take this path of the IO has been marked as +* needing to be treated like a "normal read". This +* is the case if it's a tape read, and either the +* FM, EOM, or ILI bits are set, but there is no +* sense data. +*/ +if (!(cmd->se_cmd_flags &SCF_TREAT_READ_AS_NORMAL ) && + cmd->se_cmd_flags &SCF_TRANSPORT_TASK_SENSE ) goto queue_status;switch (cmd->data_direction) {case DMA_FROM_DEVICE : -if (cmd->scsi_status) +/* queue status if not treating this as a normal read */ +if (cmd->scsi_status && + !(cmd->se_cmd_flags &SCF_TREAT_READ_AS_NORMAL )) goto queue_status;trace_target_cmd_complete (cmd);
@@ -2194,9 +2206,15 @@ static void target_complete_ok_work(struct work_struct *work)/* * Check if we need to send a sense buffer from -* the struct se_cmd in question. +* the struct se_cmd in question. We do NOT want +* to take this path of the IO has been marked as +* needing to be treated like a "normal read". This +* is the case if it's a tape read, and either the +* FM, EOM, or ILI bits are set, but there is no +* sense data. */ -if (cmd->se_cmd_flags &SCF_TRANSPORT_TASK_SENSE ) { +if (!(cmd->se_cmd_flags &SCF_TREAT_READ_AS_NORMAL ) && + cmd->se_cmd_flags &SCF_TRANSPORT_TASK_SENSE ) {WARN_ON (!cmd->scsi_status); ret =transport_send_check_condition_and_sense ( cmd,0 ,1 );
@@ -2238,7 +2256,18 @@ static void target_complete_ok_work(struct work_struct *work) queue_rsp:switch (cmd->data_direction) {case DMA_FROM_DEVICE : -if (cmd->scsi_status) +/* +* if this is a READ-type IO, but SCSI status +* is set, then skip returning data and just +* return the status -- unless this IO is marked +* as needing to be treated as a normal read, +* in which case we want to go ahead and return +* the data. This happens, for example, for tape +* reads with the FM, EOM, or ILI bits set, with +* no sense data. +*/ +if (cmd->scsi_status && + !(cmd->se_cmd_flags &SCF_TREAT_READ_AS_NORMAL )) goto queue_status; atomic_long_add(cmd->data_length,
include/target/target_core_base.h +1 -0
@@ -143,6 +143,7 @@ enum se_cmd_flags_table {SCF_ACK_KREF =0x00400000 ,SCF_USE_CPUID =0x00800000 ,SCF_TASK_ATTR_SET =0x01000000 , + SCF_TREAT_READ_AS_NORMAL = 0x02000000, };/*
Signature · 1 line
2.16.3
Quoted 149 lines · On Fri, 11 May 2018 10:56:24 -0700 Lee Duncan <[email protected]> wrote:
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 assumes 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 ILI, EOM, or FM bits set, with a sense code of
NO_SENSE, treating such cases as if the read succeeded. The layered tape driver then "does the right thing" when it gets such a response.Changes from v2:
- Cleaned up subject line and bug text formatting
- Removed unneeded inner braces
- Removed ugly goto
- Also updated the "queue full" path to handle this caseChanges 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 bitSigned-off-by Lee Duncan <[email protected]>--- drivers/target/target_core_pscsi.c | 23 +++++++++++++++++++- drivers/target/target_core_transport.c | 39+++++++++++++++++++++++++++++-----
include/target/target_core_base.h| 1 + 3 files changed, 57
insertions(+), 6 deletions(-)(quoted diff) +0
-0
b/drivers/target/target_core_pscsi.cindex0d99b242e82e..a9656368a96d100644 ---a/drivers/target/target_core_pscsi.c
+++b/drivers/target/target_core_pscsi.cdrivers/target/target_core_pscsi.c +15
-1@@ -689,8 +689,29 @@ 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 */
(quoted diff) +1
-0+ req_sense[2] & 0xe0 && /* FM,EOM, or ILI */
(quoted diff) +1
-0+ (req_sense[2] & 0xf) = 0) { /*key=NO_SENSE */
(quoted diff) +1
-0+ pr_debug("Tape FM/EOM/ILI statusdetected. Treat as normal read.\n");
drivers/target/target_core_pscsi.c +4
-0+ cmd->se_cmd_flags |> SCF_TREAT_READ_AS_NORMAL; + } + } + } }enum {(quoted diff) +0
-0
b/drivers/target/target_core_transport.cindex74b646f165d4..a15a9e3dce11100644 ---a/drivers/target/target_core_transport.c+++b/drivers/target/target_core_transport.c@@ -2084,12 +2084,24 @@ static voidtransport_complete_qf(struct se_cmd *cmd)gotoqueue_status; }drivers/target/target_core_transport.c +12
-2-if (cmd->se_cmd_flags &SCF_TRANSPORT_TASK_SENSE ) +/* +* Check if we need to send a sense buffer from +* the struct se_cmd in question. We do NOT want +* to take this path of the IO has been marked as +* needing to be treated like a "normal read". This +* is the case if it's a tape read, and either the +* FM, EOM, or ILI bits are set, but there is no +* sense data. +*/ +if (!(cmd->se_cmd_flags &SCF_TREAT_READ_AS_NORMAL ) && + cmd->se_cmd_flags &SCF_TRANSPORT_TASK_SENSE ) goto queue_status;switch (cmd->data_direction) {case DMA_FROM_DEVICE : -if (cmd->scsi_status) +/* queue status if not treating this as a normal read */
drivers/target/target_core_transport.c +10
-2+if (cmd->scsi_status && + !(cmd->se_cmd_flags &SCF_TREAT_READ_AS_NORMAL )) goto queue_status;trace_target_cmd_complete (cmd);@@ -2194,9 +2206,15 @@ static void target_complete_ok_work(struct work_struct *work)/* * Check if we need to send a sense buffer from -* the struct se_cmd in question. +* the struct se_cmd in question. We do NOT want +* to take this path of the IO has been marked as +* needing to be treated like a "normal read". This +* is the case if it's a tape read, and either the +* FM, EOM, or ILI bits are set, but there is no +* sense data. */ -if (cmd->se_cmd_flags &SCF_TRANSPORT_TASK_SENSE ) { +if (!(cmd->se_cmd_flags &SCF_TREAT_READ_AS_NORMAL ) && + cmd->se_cmd_flags &SCF_TRANSPORT_TASK_SENSE ) {WARN_ON (!cmd->scsi_status); ret =transport_send_check_condition_and_sense ( cmd,0 ,1 );@@ -2238,7 +2256,18 @@ static void target_complete_ok_work(struct
work_struct*work)queue_rsp:drivers/target/target_core_transport.c +12
-1switch (cmd->data_direction) {case DMA_FROM_DEVICE : -if (cmd->scsi_status) +/* +* if this is a READ-type IO, but SCSI status +* is set, then skip returning data and just +* return the status -- unless this IO is marked +* as needing to be treated as a normal read, +* in which case we want to go ahead and return +* the data. This happens, for example, for tape +* reads with the FM, EOM, or ILI bits set, with +* no sense data. +*/ +if (cmd->scsi_status && + !(cmd->se_cmd_flags &SCF_TREAT_READ_AS_NORMAL )) goto queue_status; atomic_long_add(cmd->data_length,(quoted diff) +0
-0
b/include/target/target_core_base.hindex9f9f5902af38..922a39f45abc100644 ---a/include/target/target_core_base.h
+++b/include/target/target_core_base.h
include/target/target_core_base.h enum se_cmd_flags_table { · line 149
Quoted 7 lines
include/target/target_core_base.h +1
-0@@ -143,6 +143,7 @@ enum se_cmd_flags_table {SCF_ACK_KREF =0x00400000 ,SCF_USE_CPUID =0x00800000 ,SCF_TASK_ATTR_SET =0x01000000 , + SCF_TREAT_READ_AS_NORMAL = 0x02000000, };/*
I would remove the 'hack to' in the first comment, but otherwise:
Cheers,
Hannes
Quoted 6 lines
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.
Quoted 8 lines
This is a long-standing transport issue, since it assumes 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 ILI, EOM, or FM bits set, with a sense code of
NO_SENSE, treating such cases as if the read succeeded. The layered tape driver then "does the right thing" when it gets such a response.
I tested the patch. Using the st driver to handle the exported tape drive on the initiator side, everything works fine.
But the st driver masks a problem that still persists. A real tape drive, if the READ is longer than the data block to read, will transfer only the amount of data from the tape block.
The exported tape drive with this patch transfers the full amount of data as requested by the READ. The over scoring part of the transmitted data is padded with NUL bytes.
Sending READ commands via sg with sg_raw I was able to test this behavior. I also verified it using a FibreChannel analyzer.
I'd suggest to call target_complete_cmd_with_length() instead of target_complete_cmd() in the descibed situation. That should fix the length of the transmitted data, I think.
BR, Bodo
Quoted 140 lines
Changes from v2:
- Cleaned up subject line and bug text formatting
- Removed unneeded inner braces
- Removed ugly goto
- Also updated the "queue full" path to handle this caseChanges 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 bitSigned-off-by Lee Duncan <[email protected]>--- drivers/target/target_core_pscsi.c | 23 +++++++++++++++++++- drivers/target/target_core_transport.c | 39 +++++++++++++++++++++++++++++----- include/target/target_core_base.h | 1 + 3 files changed, 57 insertions(+), 6 deletions(-)drivers/target/target_core_pscsi.c +22
-1@@ -689,8 +689,29 @@ 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 */ +pr_debug ("Tape FM/EOM/ILI status detected. Treat as normal read.\n" ); + cmd->se_cmd_flags |=SCF_TREAT_READ_AS_NORMAL ; + } + } + } }enum {drivers/target/target_core_transport.c +34
-5@@ -2084,12 +2084,24 @@ static void transport_complete_qf(struct se_cmd *cmd) goto queue_status; } -if (cmd->se_cmd_flags &SCF_TRANSPORT_TASK_SENSE ) +/* +* Check if we need to send a sense buffer from +* the struct se_cmd in question. We do NOT want +* to take this path of the IO has been marked as +* needing to be treated like a "normal read". This +* is the case if it's a tape read, and either the +* FM, EOM, or ILI bits are set, but there is no +* sense data. +*/ +if (!(cmd->se_cmd_flags &SCF_TREAT_READ_AS_NORMAL ) && + cmd->se_cmd_flags &SCF_TRANSPORT_TASK_SENSE ) goto queue_status;switch (cmd->data_direction) {case DMA_FROM_DEVICE : -if (cmd->scsi_status) +/* queue status if not treating this as a normal read */ +if (cmd->scsi_status && + !(cmd->se_cmd_flags &SCF_TREAT_READ_AS_NORMAL )) goto queue_status;trace_target_cmd_complete (cmd);@@ -2194,9 +2206,15 @@ static void target_complete_ok_work(struct work_struct *work)/* * Check if we need to send a sense buffer from -* the struct se_cmd in question. +* the struct se_cmd in question. We do NOT want +* to take this path of the IO has been marked as +* needing to be treated like a "normal read". This +* is the case if it's a tape read, and either the +* FM, EOM, or ILI bits are set, but there is no +* sense data. */ -if (cmd->se_cmd_flags &SCF_TRANSPORT_TASK_SENSE ) { +if (!(cmd->se_cmd_flags &SCF_TREAT_READ_AS_NORMAL ) && + cmd->se_cmd_flags &SCF_TRANSPORT_TASK_SENSE ) {WARN_ON (!cmd->scsi_status); ret =transport_send_check_condition_and_sense ( cmd,0 ,1 );@@ -2238,7 +2256,18 @@ static void target_complete_ok_work(struct work_struct *work) queue_rsp:switch (cmd->data_direction) {case DMA_FROM_DEVICE : -if (cmd->scsi_status) +/* +* if this is a READ-type IO, but SCSI status +* is set, then skip returning data and just +* return the status -- unless this IO is marked +* as needing to be treated as a normal read, +* in which case we want to go ahead and return +* the data. This happens, for example, for tape +* reads with the FM, EOM, or ILI bits set, with +* no sense data. +*/ +if (cmd->scsi_status && + !(cmd->se_cmd_flags &SCF_TREAT_READ_AS_NORMAL )) goto queue_status; atomic_long_add(cmd->data_length,include/target/target_core_base.h +1
-0@@ -143,6 +143,7 @@ enum se_cmd_flags_table {SCF_ACK_KREF =0x00400000 ,SCF_USE_CPUID =0x00800000 ,SCF_TASK_ATTR_SET =0x01000000 , + SCF_TREAT_READ_AS_NORMAL = 0x02000000, };/* Signature · 3 lines
2.16.3 --