From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from stargate.chelsio.com ([12.32.117.8]:37366 "EHLO stargate.chelsio.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756394AbdEGWDK (ORCPT ); Sun, 7 May 2017 18:03:10 -0400 Date: Sun, 7 May 2017 18:22:22 +0530 From: Varun Prakash To: "Nicholas A. Bellinger" Cc: Bart Van Assche , target-devel@vger.kernel.org, stable@vger.kernel.org Subject: Re: [PATCH 2/6] target/iscsi: Call .iscsit_release_cmd() once Message-ID: <20170507125220.GA1706@chelsio.com> References: <20170330171244.8346-1-bart.vanassche@sandisk.com> <20170330171244.8346-3-bart.vanassche@sandisk.com> <1491173945.8846.48.camel@haakon3.risingtidesystems.com> <20170404050649.GA1662@chelsio.com> <20170413074454.GA1724@chelsio.com> <1493699611.23202.30.camel@haakon3.risingtidesystems.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1493699611.23202.30.camel@haakon3.risingtidesystems.com> Sender: stable-owner@vger.kernel.org List-ID: On Mon, May 01, 2017 at 09:33:31PM -0700, Nicholas A. Bellinger wrote: > On Thu, 2017-04-13 at 13:14 +0530, Varun Prakash wrote: > > On Tue, Apr 04, 2017 at 10:36:50AM +0530, Varun Prakash wrote: > > > On Sun, Apr 02, 2017 at 03:59:05PM -0700, Nicholas A. Bellinger wrote: > > > > On Thu, 2017-03-30 at 10:12 -0700, Bart Van Assche wrote: > > > > > While releasing a command __iscsit_free_cmd() can be called multiple > > > > > times but .iscsit_release_cmd() must be called only once. Hence move > > > > > the .iscsit_release_cmd() call into iscsit_release_cmd(). The latter > > > > > function is only called once per command. The only driver that defines > > > > > the .iscsit_release_cmd() callback is the cxgbit driver so this change > > > > > only affects the cxgbit driver. > > > > > > > > > > Fixes: 7ec811a8e9c3 ("iscsi-target: add void (*iscsit_release_cmd)()") > > > > > Signed-off-by: Bart Van Assche > > > > > Cc: Varun Prakash > > > > > Cc: Nicholas Bellinger > > > > > Cc: > > > > > --- > > > > > drivers/target/iscsi/iscsi_target_util.c | 15 +++++++++------ > > > > > 1 file changed, 9 insertions(+), 6 deletions(-) > > > > > > > > > > > > > Applied to target-pending/for-next, but dropping the stable CC' because > > > > the single caller in cxgbit_release_cmd() is already checking to ensure > > > > resources are only released on the first invocation. > > > > > > > > So it's not a bug-fix. > > > > > > In case of DDP cxgbit driver assigns cmd->se_cmd.t_data_sg to ttinfo->sgl > > > and calls dma_map_sg(), cxgbit_release_cmd() calls dma_unmap_sg(), it needs > > > a valid sg(ttinfo->sgl), before calling iscsit_release_cmd() > > > cmd->se_cmd.t_data_sg gets freed so ttinfo->sgl will not be valid if we move > > > ->iscsit_release_cmd() to iscsit_release_cmd(). > > > > Please drop this patch, it will regress cxgbit driver, with this patch > > scatterlist is freed before calling ->iscsit_release_cmd(), cxgbit > > calls dma_unmap_sg() so scatterlist should not be freed before calling > > ->iscst_release_cmd(). > > > > Thanks for the heads up. Dropping this patch for now. > > To address this, AFAICT the cleanest approach is to simply do the > unmapping of DDP SGLs currently done by cxgbit_release_cmd(), directly > from a cxgbit specific iscsit_transport->iscsit_queue_rsp() callback. > > That way, we can unmap the DDP SGLs immediately before invoking > iscsit_queue_rsp() from a cxgbit specific handler, and drop the > iscsi_transport->iscsit_release_cmd() callback alltogether. > > WDYT..? This approach will work in success case, but in failure cases(digest errors, ...) ->iscsit_queue_status() is not called so dma umap will not happen.