Hi Michael & Co, On Fri, 2016-06-10 at 11:19 -0500, Michael Cyr wrote: > On 6/10/16 8:56 AM, Bryant G. Ly wrote: > > On 2016-06-09 23:43, Nicholas A. Bellinger wrote: > >> Hey Bryant, > >> > >> On Wed, 2016-06-08 at 08:54 -0500, Bryant G. Ly wrote: > >>> If a Simple command is sent with an Unsupported SCSI Opcode (failure), > >>> target_setup_cmd_from_cdb returns with TCM_UNSUPPORTED_SCSI_OPCODE > >>> which then causes transport_generic_request_failure to decrement > >>> simple_cmds, due to call to transport_complete_task_attr. > >>> With this dev->simple_cmds or dev->dev_ordered_sync is now -1, not 0. > >>> So when a subsequent command with an Ordered Task is sent, it causes > >>> a hang, since dev->simple_cmds is at -1. Thus to prevent Unsupported > >>> SCSI Opcode Failure from decrementing simple_cmds I added a check > >>> to skip transport_complete_task_attr for sense_reaons that are > >>> TCM_UNSUPPORTED_SCSI_OPCODE. > >>> > >>> I dont know if this is a proper fix, but it worked for me, let me > >>> know what you think Nick. > >>> > >>> Signed-off-by: Bryant G. Ly <bryantly@xxxxxxxxxxxxxxxxxx> > >>> --- > >>> drivers/target/target_core_transport.c | 3 ++- > >>> 1 file changed, 2 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/drivers/target/target_core_transport.c > >>> b/drivers/target/target_core_transport.c > >>> index 6c089af..4578fcf 100644 > >>> --- a/drivers/target/target_core_transport.c > >>> +++ b/drivers/target/target_core_transport.c > >>> @@ -1684,7 +1684,8 @@ void transport_generic_request_failure(struct > >>> se_cmd *cmd, > >>> /* > >>> * For SAM Task Attribute emulation for failed struct se_cmd > >>> */ > >>> - transport_complete_task_attr(cmd); > >>> + if (sense_reason != TCM_UNSUPPORTED_SCSI_OPCODE) > >>> + transport_complete_task_attr(cmd); > >>> /* > >>> * Handle special case for COMPARE_AND_WRITE failure, where the > >>> * callback is expected to drop the per device ->caw_sem. > >> > >> Just to confirm, this is a regression with the earlier patch here: > >> > >> https://git.kernel.org/cgit/linux/kernel/git/nab/target-pending.git/commit/?id=15ce22fa491aa3bab7acd89ac40a9fc27aeed915 > >> > >> > >> Right..? > >> > > > > Yes it is a regression, so the change there should be: > > Bryant, I think you're a little confused. He's just referring to your > patch, not my later comment about an additional error case. > > This is not a regression, it, and the case I mentioned in that later > note, are simply additional cases where target_setup_cmd_from_cdb > returns an error, which means that we never get far enough to call > target_execute_cmd to increment simple_cmds. Got it. Thanks for the confirmation. > > > > > if (sense_reason != TCM_UNSUPPORTED_SCSI_OPCODE || > > sense_reason != TCM_INVALID_CDB_FIELD) > > transport_complete_task_attr(cmd); > > I don't think this works for a fix for the case I mentioned > (transport_check_alloc_task_attr failing). TCM_INVALID_CDB_FIELD can > get generated for a whole lot of cases, for most of which we want to > call transport_complete_task_attr. > > It seems like this might be more of a design flaw to which we're just > trying to apply band-aids. I'm not sure what the right fix is, but I > don't think this is it. Ok, how about something along the lines of the following..? Namely, to set a flag only once we've actually reached target_handle_task_attr(), otherwise in transport_complete_task_attr() avoid messing with se_device task attribute counters. So the assumption is until we reach target_execute_cmd() -> target_handle_task_attr(), all exceptions will not be touching se_device task attribute counters. WDYT..? diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c index bd77467..21dd65a 100644 --- a/drivers/target/target_core_transport.c +++ b/drivers/target/target_core_transport.c @@ -1827,6 +1827,8 @@ static bool target_handle_task_attr(struct se_cmd *cmd) if (dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH) return false; + cmd->se_cmd_flags |= SCF_TASK_ATTR_SET; + /* * Check for the existence of HEAD_OF_QUEUE, and if true return 1 * to allow the passed struct se_cmd list of tasks to the front of the list. @@ -1949,6 +1951,9 @@ static void transport_complete_task_attr(struct se_cmd *cmd) if (dev->transport->transport_flags & TRANSPORT_FLAG_PASSTHROUGH) return; + if (!(cmd->se_cmd_flags & SCF_TASK_ATTR_SET)) + goto restart; + if (cmd->sam_task_attr == TCM_SIMPLE_TAG) { atomic_dec_mb(&dev->simple_cmds); dev->dev_cur_ordered_id++; @@ -1965,7 +1970,7 @@ static void transport_complete_task_attr(struct se_cmd *cmd) pr_debug("Incremented dev_cur_ordered_id: %u for ORDERED\n", dev->dev_cur_ordered_id); } - +restart: target_restart_delayed_cmds(dev); } diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h index b316b44..fb8e3b6 100644 --- a/include/target/target_core_base.h +++ b/include/target/target_core_base.h @@ -142,6 +142,7 @@ enum se_cmd_flags_table { SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC = 0x00200000, SCF_ACK_KREF = 0x00400000, SCF_USE_CPUID = 0x00800000, + SCF_TASK_ATTR_SET = 0x01000000, }; /* -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe target-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
