Re: [PATCH v2] ibmvscsis: Do not send aborted task response

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 5/3/17 1:55 PM, Bryant G. Ly wrote:
On 5/3/17 11:38 AM, Bryant G. Ly wrote:

Hi Nick,

On 5/2/17 10:43 PM, Nicholas A. Bellinger wrote:

On Tue, 2017-05-02 at 13:54 -0500, Bryant G. Ly wrote:
The driver is sending a response to the actual scsi op that was
aborted by an abort task TM, while LIO is sending a response to
the abort task TM.

ibmvscsis_tgt does not send the response to the client until
release_cmd time. The reason for this was because if we did it
at queue_status time, then the client would be free to reuse the
tag for that command, but we're still using the tag until the
command is released at release_cmd time, so we chose to delay
sending the response until then. That then caused this issue, because
release_cmd is always called, even if queue_status is not.

SCSI spec says that the initiator that sends the abort task
TM NEVER gets a response to the aborted op and with the current
code it will send a response. Thus this fix will remove that response
if the TAS bit is set.

Cc: <stable@xxxxxxxxxxxxxxx> # v4.8+
Signed-off-by: Bryant G. Ly <bryantly@xxxxxxxxxxxxxxxxxx>
Reviewed-by: Tyrel Datwyler <tyreld@xxxxxxxxxxxxxxxxxx>
---
drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 66 ++++++++++++++++++++++----------
  1 file changed, 45 insertions(+), 21 deletions(-)
Applied, with a small update to the last sentence of the commit log wrt
to 'if ABORTED && !TAS bit is set'.

Thanks Bryant + Tyrel.

So I have been running tests on this patch extensively and it seems like after running 2 hrs of consecutive aborts it fails again with the same problem of driver sending a response to the actual scsi op.

It looks like when LIO processes the abort and if (!__target_check_io_state(se_cmd, se_sess, 0)) then rsp gets set as TMR_TASK_DOES_NOT_EXIST.

With that response the CMD_T_ABORTED state is not set, so then the ibmvscsis driver still sends that duplicate response.

I think the best solution would be in driver when queue_tm_rsp gets called and when the driver builds the response to check for the TMR_TASK_DOES_NOT_EXIST and set our own flag.
Then we would also check for that flag, so it would be:

if ((cmd->se_cmd.transport_state & CMD_T_ABORTED && !(cmd->se_cmd.transport_state & CMD_T_TAS)) || cmd->flags & CMD_ABORTED) {

This will ensure in the CMD_T_ABORTED w/o CMD_T_TAS scenarios are handled and the ABORT_TASK: Sending TMR_TASK_DOES_NOT_EXIST case.

-Bryant


Hi Nick,

You can ignore the email about the patch being broken. The script that was used for testing isn't right.

So the patch is working as intended now, where on an abort the client does not receive an extra response to the actual scsi op.

Thanks,

Bryant Ly

Sorry Nick, but can you hold off on this patch. We are still getting extra responses but in a small timing window.

The majority of the aborts work but if we get an abort between LIO calling queue_state and LIO calling release_cmd, we get an extra response.


Working on a solution, and will still have to do the tests where we basically send aborts over and over on the client btwn random timing widows.

-Bryant




[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]