| Message ID | 20180509205921.12348-1-lduncan@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, May 09, 2018 at 01:59:21PM -0700, Lee Duncan 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. Please use the full line length available for commit messages.. > 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 probably wants and uptdate of the subject line as well. > + * 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)) { No need for the inner braces. > + 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; Same here. > enum { > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c > index 74b646f165d4..56661a824266 100644 > --- a/drivers/target/target_core_transport.c > +++ b/drivers/target/target_core_transport.c > @@ -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. > @@ -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); > /* The goto looks at least a little odd as it skips many things that don't look realted. As far as I can tell what you want to skip is mostly the SCF_TRANSPORT_TASK_SENSE check, which can be done with an additional conditional. Do we also need to skip the scsi_status check above? If yes it needs another conditional, and a good comment. -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/09/2018 03:59 PM, Lee Duncan 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 > 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 <lduncan@suse.com> > --- > 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(-) > > diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c > index 0d99b242e82e..b237104af81c 100644 > --- a/drivers/target/target_core_pscsi.c > +++ b/drivers/target/target_core_pscsi.c > @@ -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 { > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c > index 74b646f165d4..56661a824266 100644 > --- a/drivers/target/target_core_transport.c > +++ b/drivers/target/target_core_transport.c > @@ -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. > @@ -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. -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/10/2018 01:51 PM, Mike Christie wrote: > On 05/09/2018 03:59 PM, Lee Duncan 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 >> 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 <lduncan@suse.com> >> --- >> 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(-) >> >> diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c >> index 0d99b242e82e..b237104af81c 100644 >> --- a/drivers/target/target_core_pscsi.c >> +++ b/drivers/target/target_core_pscsi.c >> @@ -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 { >> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c >> index 74b646f165d4..56661a824266 100644 >> --- a/drivers/target/target_core_transport.c >> +++ b/drivers/target/target_core_transport.c >> @@ -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. >> @@ -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. > 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? -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/10/2018 12:19 PM, Mike Christie wrote: > On 05/10/2018 01:51 PM, Mike Christie wrote: >> On 05/09/2018 03:59 PM, Lee Duncan 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 >>> 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 <lduncan@suse.com> >>> --- >>> 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(-) >>> >>> ... >> >> >> 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. Yes, both the sense data and the IO data get returned with this patch. >> >> 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. Good point. I will update the patch. >> > > 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? > Good question. I believe it will not be an issue. I believe that if the IO completes on multiple paths it will be treated the same on each path, which means that the SCF_SENT_CHECK_CONDITION will not be set on either path. Which is how it is handled now for a normal IO.
diff --git a/drivers/target/target_core_pscsi.c b/drivers/target/target_core_pscsi.c index 0d99b242e82e..b237104af81c 100644 --- a/drivers/target/target_core_pscsi.c +++ b/drivers/target/target_core_pscsi.c @@ -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 { diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c index 74b646f165d4..56661a824266 100644 --- a/drivers/target/target_core_transport.c +++ b/drivers/target/target_core_transport.c @@ -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. @@ -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); /* 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, }; /*
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 <lduncan@suse.com> --- 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(-)