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 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 > > Signed-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(-) > > diff --git a/drivers/target/target_core_pscsi.c > b/drivers/target/target_core_pscsi.c index 0d99b242e82e..a9656368a96d > 100644 --- a/drivers/target/target_core_pscsi.c > +++ b/drivers/target/target_core_pscsi.c > @@ -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 { > diff --git a/drivers/target/target_core_transport.c > b/drivers/target/target_core_transport.c index > 74b646f165d4..a15a9e3dce11 100644 --- > a/drivers/target/target_core_transport.c +++ > b/drivers/target/target_core_transport.c @@ -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, > diff --git a/include/target/target_core_base.h > b/include/target/target_core_base.h index 9f9f5902af38..922a39f45abc > 100644 --- a/include/target/target_core_base.h > +++ b/include/target/target_core_base.h > @@ -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: Reviewed-by: Hannes Reinecke <[email protected]> Cheers, Hannes
