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 <djeffery@xxxxxxxxxx>
> > Tested-by: Laurence Oberman <loberman@xxxxxxxxxx>
> > ---
> > 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 <loberman@xxxxxxxxxx>
Martin, please add
Reported-by: Scott Hamilton <scott.hamilton@xxxxxxxx>
