From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:35019 "EHLO mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751882AbdEDXNi (ORCPT ); Thu, 4 May 2017 19:13:38 -0400 Received: from pps.filterd (m0098393.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.16.0.20/8.16.0.20) with SMTP id v44N4C69083119 for ; Thu, 4 May 2017 19:13:38 -0400 Received: from e38.co.us.ibm.com (e38.co.us.ibm.com [32.97.110.159]) by mx0a-001b2d01.pphosted.com with ESMTP id 2a8cm72f32-1 (version=TLSv1.2 cipher=AES256-SHA bits=256 verify=NOT) for ; Thu, 04 May 2017 19:13:37 -0400 Received: from localhost by e38.co.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 4 May 2017 17:13:37 -0600 Subject: Re: [PATCH v2] ibmvscsis: Do not send aborted task response To: "Nicholas A. Bellinger" References: <1493751261-81647-1-git-send-email-bryantly@linux.vnet.ibm.com> <1493783006.23202.87.camel@haakon3.risingtidesystems.com> <5c2536d3-7ff1-1e17-91c0-0be6a8befbfa@linux.vnet.ibm.com> <7d900f15-4590-5b09-0f48-8e8be862665e@linux.vnet.ibm.com> Cc: Bart.VanAssche@sandisk.com, martin.petersen@oracle.com, turtle.in.the.kernel@gmail.com, seroyer@linux.vnet.ibm.com, linux-scsi@vger.kernel.org, target-devel@vger.kernel.org, stable@vger.kernel.org From: "Bryant G. Ly" Date: Thu, 4 May 2017 18:13:30 -0500 MIME-Version: 1.0 In-Reply-To: <7d900f15-4590-5b09-0f48-8e8be862665e@linux.vnet.ibm.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Message-Id: <6ac46ce3-77b9-c138-a1d9-fb119a98a9ea@linux.vnet.ibm.com> Sender: stable-owner@vger.kernel.org List-ID: 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: # v4.8+ >>>> Signed-off-by: Bryant G. Ly >>>> Reviewed-by: Tyrel Datwyler >>>> --- >>>> 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