Re: [PATCH 2/6] target/iscsi: Call .iscsit_release_cmd() once

[Varun Prakash <varun@chelsio.com>]

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 <bart.vanassche@sandisk.com>
> > > > > Cc: Varun Prakash <varun@chelsio.com>
> > > > > Cc: Nicholas Bellinger <nab@linux-iscsi.org>
> > > > > Cc: <stable@vger.kernel.org>
> > > > > ---
> > > > >  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.