On 6/14/16 1:13 AM, Nicholas A. Bellinger wrote:
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,
};
/*
This seems like it should work. I was thinking that a flag would work
well, but wasn't sure how resistant folks would be to a new flag.
--
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
