With tape devices, the SCF_TREAT_READ_AS_NORMAL flag is used by the target
subsystem to mark commands which have both data to return as well as
sense data. But with pscsi, SCF_TREAT_READ_AS_NORMAL can be set even if
there is no data to return. The SCF_TREAT_READ_AS_NORMAL flag causes the
target core to call iscsit datain callbacks even if there is no data, which
iscsit does not support. This results in iscsit going into an error state
requiring recovery and being unable to complete the command to the
initiator.
This issue can be resolved by fixing pscsi to only set
SCF_TREAT_READ_AS_NORMAL if there is valid data to return along side the
sense data.
Fixes: bd81372065fa ("scsi: target: transport should handle st FM/EOM/ILI reads")
Signed-off-by: David Jeffery <[email protected]>
Tested-by: Laurence Oberman <[email protected]>
---
drivers/target/target_core_pscsi.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c
index ff292b75e23f..60dafe4c581b 100644
--- a/drivers/target/target_core_pscsi.c
+++ b/drivers/target/target_core_pscsi.c
@@ -588,7 +588,7 @@ static void pscsi_destroy_device(struct se_device *dev)
}
static void pscsi_complete_cmd(struct se_cmd *cmd, u8 scsi_status,
- unsigned char *req_sense)
+ unsigned char *req_sense, int valid_data)
{
struct pscsi_dev_virt *pdv = PSCSI_DEV(cmd->se_dev);
struct scsi_device *sd = pdv->pdv_sd;
@@ -681,7 +681,7 @@ static void pscsi_complete_cmd(struct se_cmd *cmd, u8 scsi_status,
* back despite framework assumption that a
* check condition means there is no data
*/
- if (sd->type == TYPE_TAPE &&
+ if (sd->type == TYPE_TAPE && valid_data &&
cmd->data_direction == DMA_FROM_DEVICE) {
/*
* is sense data valid, fixed format,
@@ -1032,6 +1032,7 @@ static void pscsi_req_done(struct request *req, blk_status_t status)
struct se_cmd *cmd = req->end_io_data;
struct scsi_cmnd *scmd = blk_mq_rq_to_pdu(req);
enum sam_status scsi_status = scmd->result & 0xff;
+ int valid_data = cmd->data_length - scmd->resid_len;
u8 *cdb = cmd->priv;
if (scsi_status != SAM_STAT_GOOD) {
@@ -1039,12 +1040,11 @@ static void pscsi_req_done(struct request *req, blk_status_t status)
" 0x%02x Result: 0x%08x\n", cmd, cdb[0], scmd->result);
}
- pscsi_complete_cmd(cmd, scsi_status, scmd->sense_buffer);
+ pscsi_complete_cmd(cmd, scsi_status, scmd->sense_buffer, valid_data);
switch (host_byte(scmd->result)) {
case DID_OK:
- target_complete_cmd_with_length(cmd, scsi_status,
- cmd->data_length - scmd->resid_len);
+ target_complete_cmd_with_length(cmd, scsi_status, valid_data);
break;
default:
pr_debug("PSCSI Host Byte exception at cmd: %p CDB:"
--
2.35.1
On Wed, 2022-04-27 at 14:32 -0400, David Jeffery wrote:
> With tape devices, the SCF_TREAT_READ_AS_NORMAL flag is used by the
> target
> subsystem to mark commands which have both data to return as well as
> sense data. But with pscsi, SCF_TREAT_READ_AS_NORMAL can be set even
> if
> there is no data to return. The SCF_TREAT_READ_AS_NORMAL flag causes
> the
> target core to call iscsit datain callbacks even if there is no data,
> which
> iscsit does not support. This results in iscsit going into an error
> state
> requiring recovery and being unable to complete the command to the
> initiator.
>
> This issue can be resolved by fixing pscsi to only set
> SCF_TREAT_READ_AS_NORMAL if there is valid data to return along side
> the
> sense data.
>
> Fixes: bd81372065fa ("scsi: target: transport should handle st
> FM/EOM/ILI reads")
> Signed-off-by: David Jeffery <[email protected]>
> Tested-by: Laurence Oberman <[email protected]>
> ---
> drivers/target/target_core_pscsi.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/target/target_core_pscsi.c
> b/drivers/target/target_core_pscsi.c
> index ff292b75e23f..60dafe4c581b 100644
> --- a/drivers/target/target_core_pscsi.c
> +++ b/drivers/target/target_core_pscsi.c
> @@ -588,7 +588,7 @@ static void pscsi_destroy_device(struct se_device
> *dev)
> }
>
> static void pscsi_complete_cmd(struct se_cmd *cmd, u8 scsi_status,
> - unsigned char *req_sense)
> + unsigned char *req_sense, int
> valid_data)
> {
> struct pscsi_dev_virt *pdv = PSCSI_DEV(cmd->se_dev);
> struct scsi_device *sd = pdv->pdv_sd;
> @@ -681,7 +681,7 @@ static void pscsi_complete_cmd(struct se_cmd
> *cmd, u8 scsi_status,
> * back despite framework assumption that a
> * check condition means there is no data
> */
> - if (sd->type == TYPE_TAPE &&
> + if (sd->type == TYPE_TAPE && valid_data &&
> cmd->data_direction == DMA_FROM_DEVICE) {
> /*
> * is sense data valid, fixed format,
> @@ -1032,6 +1032,7 @@ static void pscsi_req_done(struct request *req,
> blk_status_t status)
> struct se_cmd *cmd = req->end_io_data;
> struct scsi_cmnd *scmd = blk_mq_rq_to_pdu(req);
> enum sam_status scsi_status = scmd->result & 0xff;
> + int valid_data = cmd->data_length - scmd->resid_len;
> u8 *cdb = cmd->priv;
>
> if (scsi_status != SAM_STAT_GOOD) {
> @@ -1039,12 +1040,11 @@ static void pscsi_req_done(struct request
> *req, blk_status_t status)
> " 0x%02x Result: 0x%08x\n", cmd, cdb[0], scmd-
> >result);
> }
>
> - pscsi_complete_cmd(cmd, scsi_status, scmd->sense_buffer);
> + pscsi_complete_cmd(cmd, scsi_status, scmd->sense_buffer,
> valid_data);
>
> switch (host_byte(scmd->result)) {
> case DID_OK:
> - target_complete_cmd_with_length(cmd, scsi_status,
> - cmd->data_length - scmd->resid_len);
> + target_complete_cmd_with_length(cmd, scsi_status,
> valid_data);
> break;
> default:
> pr_debug("PSCSI Host Byte exception at cmd: %p CDB:"
We added a bucnh of debug to track this down, and the fix is solid and
makes perfect sense.
Reviewed-by: Laurence Oberman <[email protected]>
On Wed, 2022-04-27 at 15:26 -0400, Laurence Oberman wrote:
> On Wed, 2022-04-27 at 14:32 -0400, David Jeffery wrote:
> > With tape devices, the SCF_TREAT_READ_AS_NORMAL flag is used by the
> > target
> > subsystem to mark commands which have both data to return as well
> > as
> > sense data. But with pscsi, SCF_TREAT_READ_AS_NORMAL can be set
> > even
> > if
> > there is no data to return. The SCF_TREAT_READ_AS_NORMAL flag
> > causes
> > the
> > target core to call iscsit datain callbacks even if there is no
> > data,
> > which
> > iscsit does not support. This results in iscsit going into an error
> > state
> > requiring recovery and being unable to complete the command to the
> > initiator.
> >
> > This issue can be resolved by fixing pscsi to only set
> > SCF_TREAT_READ_AS_NORMAL if there is valid data to return along
> > side
> > the
> > sense data.
> >
> > Fixes: bd81372065fa ("scsi: target: transport should handle st
> > FM/EOM/ILI reads")
> > Signed-off-by: David Jeffery <[email protected]>
> > Tested-by: Laurence Oberman <[email protected]>
> > ---
> > drivers/target/target_core_pscsi.c | 10 +++++-----
> > 1 file changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/target/target_core_pscsi.c
> > b/drivers/target/target_core_pscsi.c
> > index ff292b75e23f..60dafe4c581b 100644
> > --- a/drivers/target/target_core_pscsi.c
> > +++ b/drivers/target/target_core_pscsi.c
> > @@ -588,7 +588,7 @@ static void pscsi_destroy_device(struct
> > se_device
> > *dev)
> > }
> >
> > static void pscsi_complete_cmd(struct se_cmd *cmd, u8 scsi_status,
> > - unsigned char *req_sense)
> > + unsigned char *req_sense, int
> > valid_data)
> > {
> > struct pscsi_dev_virt *pdv = PSCSI_DEV(cmd->se_dev);
> > struct scsi_device *sd = pdv->pdv_sd;
> > @@ -681,7 +681,7 @@ static void pscsi_complete_cmd(struct se_cmd
> > *cmd, u8 scsi_status,
> > * back despite framework assumption that a
> > * check condition means there is no data
> > */
> > - if (sd->type == TYPE_TAPE &&
> > + if (sd->type == TYPE_TAPE && valid_data &&
> > cmd->data_direction == DMA_FROM_DEVICE) {
> > /*
> > * is sense data valid, fixed format,
> > @@ -1032,6 +1032,7 @@ static void pscsi_req_done(struct request
> > *req,
> > blk_status_t status)
> > struct se_cmd *cmd = req->end_io_data;
> > struct scsi_cmnd *scmd = blk_mq_rq_to_pdu(req);
> > enum sam_status scsi_status = scmd->result & 0xff;
> > + int valid_data = cmd->data_length - scmd->resid_len;
> > u8 *cdb = cmd->priv;
> >
> > if (scsi_status != SAM_STAT_GOOD) {
> > @@ -1039,12 +1040,11 @@ static void pscsi_req_done(struct request
> > *req, blk_status_t status)
> > " 0x%02x Result: 0x%08x\n", cmd, cdb[0], scmd-
> > > result);
> >
> > }
> >
> > - pscsi_complete_cmd(cmd, scsi_status, scmd->sense_buffer);
> > + pscsi_complete_cmd(cmd, scsi_status, scmd->sense_buffer,
> > valid_data);
> >
> > switch (host_byte(scmd->result)) {
> > case DID_OK:
> > - target_complete_cmd_with_length(cmd, scsi_status,
> > - cmd->data_length - scmd->resid_len);
> > + target_complete_cmd_with_length(cmd, scsi_status,
> > valid_data);
> > break;
> > default:
> > pr_debug("PSCSI Host Byte exception at cmd: %p CDB:"
>
> We added a bucnh of debug to track this down, and the fix is solid
> and
> makes perfect sense.
>
> Reviewed-by: Laurence Oberman <[email protected]>
Martin, please add
Reported-by: Scott Hamilton <[email protected]>
On Wed, 27 Apr 2022 14:32:50 -0400, David Jeffery wrote:
> With tape devices, the SCF_TREAT_READ_AS_NORMAL flag is used by the target
> subsystem to mark commands which have both data to return as well as
> sense data. But with pscsi, SCF_TREAT_READ_AS_NORMAL can be set even if
> there is no data to return. The SCF_TREAT_READ_AS_NORMAL flag causes the
> target core to call iscsit datain callbacks even if there is no data, which
> iscsit does not support. This results in iscsit going into an error state
> requiring recovery and being unable to complete the command to the
> initiator.
>
> [...]
Applied to 5.18/scsi-fixes, thanks!
[1/1] target: pscsi: set SCF_TREAT_READ_AS_NORMAL flag only if there is valid data
https://git.kernel.org/mkp/scsi/c/8be70a842f70
--
Martin K. Petersen Oracle Linux Engineering