[PATCH 1/6] target: Fix VERIFY and WRITE VERIFY command parsing

[PATCH 1/6] target: Fix VERIFY and WRITE VERIFY command parsing

[Bart Van Assche]

Use the value of the BYTCHK field to determine the size of the
Data-Out buffer. For VERIFY, honor the VRPROTECT, DPO and FUA
fields. This patch avoids that LIO complains about a mismatch
between the expected transfer length and the SCSI CDB length
if the value of the BYTCHK field is 0.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Max Lohrmann <post@wickenrode.com>
Cc: <stable@vger.kernel.org>
---
 drivers/target/target_core_sbc.c | 71 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 62 insertions(+), 9 deletions(-)

diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index c194063f169b..ee35c90e3b8d 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -828,6 +828,59 @@ sbc_check_dpofua(struct se_device *dev, struct se_cmd *cmd, unsigned char *cdb)
 	return 0;
 }
 
+/**
+ * sbc_parse_verify - parse VERIFY, VERIFY_16 and WRITE VERIFY commands
+ * @cmd:     (in)  structure that describes the SCSI command to be parsed.
+ * @sectors: (out) Number of logical blocks on the storage medium that will be
+ *           affected by the SCSI command.
+ * @bufflen: (out) Expected length of the SCSI Data-Out buffer.
+ */
+static sense_reason_t sbc_parse_verify(struct se_cmd *cmd, int *sectors,
+				       u32 *bufflen)
+{
+	struct se_device *dev = cmd->se_dev;
+	u8 *cdb = cmd->t_task_cdb;
+	u8 bytchk = (cdb[1] >> 1) & 3;
+	sense_reason_t ret;
+
+	switch (cdb[0]) {
+	case VERIFY:
+	case WRITE_VERIFY:
+		*sectors = transport_get_sectors_10(cdb);
+		cmd->t_task_lba = transport_lba_32(cdb);
+		break;
+	case VERIFY_16:
+		*sectors = transport_get_sectors_16(cdb);
+		cmd->t_task_lba = transport_lba_64(cdb);
+		break;
+	default:
+		WARN_ON_ONCE(true);
+		return TCM_UNSUPPORTED_SCSI_OPCODE;
+	}
+
+	if (sbc_check_dpofua(dev, cmd, cdb))
+		return TCM_INVALID_CDB_FIELD;
+
+	ret = sbc_check_prot(dev, cmd, cdb, *sectors, true);
+	if (ret)
+		return ret;
+
+	switch (bytchk) {
+	case 0:
+		*bufflen = 0;
+		break;
+	case 1:
+		*bufflen = sbc_get_size(cmd, *sectors);
+		cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
+		break;
+	default:
+		pr_err("Unsupported BYTCHK value %d for SCSI opcode %#x\n",
+		       bytchk, cdb[0]);
+		return TCM_INVALID_CDB_FIELD;
+	}
+	return TCM_NO_SENSE;
+}
+
 sense_reason_t
 sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 {
@@ -895,7 +948,6 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		cmd->execute_cmd = sbc_execute_rw;
 		break;
 	case WRITE_10:
-	case WRITE_VERIFY:
 		sectors = transport_get_sectors_10(cdb);
 		cmd->t_task_lba = transport_lba_32(cdb);
 
@@ -909,6 +961,12 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		cmd->se_cmd_flags |= SCF_SCSI_DATA_CDB;
 		cmd->execute_cmd = sbc_execute_rw;
 		break;
+	case WRITE_VERIFY:
+		ret = sbc_parse_verify(cmd, &sectors, &size);
+		if (ret)
+			return ret;
+		cmd->execute_cmd = sbc_execute_rw;
+		goto check_lba;
 	case WRITE_12:
 		sectors = transport_get_sectors_12(cdb);
 		cmd->t_task_lba = transport_lba_32(cdb);
@@ -1106,14 +1164,9 @@ sbc_parse_cdb(struct se_cmd *cmd, struct sbc_ops *ops)
 		break;
 	case VERIFY:
 	case VERIFY_16:
-		size = 0;
-		if (cdb[0] == VERIFY) {
-			sectors = transport_get_sectors_10(cdb);
-			cmd->t_task_lba = transport_lba_32(cdb);
-		} else {
-			sectors = transport_get_sectors_16(cdb);
-			cmd->t_task_lba = transport_lba_64(cdb);
-		}
+		ret = sbc_parse_verify(cmd, &sectors, &size);
+		if (ret)
+			return ret;
 		cmd->execute_cmd = sbc_emulate_noop;
 		goto check_lba;
 	case REZERO_UNIT:
-- 
2.12.0

Re: [PATCH 1/6] target: Fix VERIFY and WRITE VERIFY command parsing

[Nicholas A. Bellinger]

On Thu, 2017-03-30 at 10:12 -0700, Bart Van Assche wrote:
> Use the value of the BYTCHK field to determine the size of the
> Data-Out buffer. For VERIFY, honor the VRPROTECT, DPO and FUA
> fields. This patch avoids that LIO complains about a mismatch
> between the expected transfer length and the SCSI CDB length
> if the value of the BYTCHK field is 0.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
> Cc: Max Lohrmann <post@wickenrode.com>
> Cc: <stable@vger.kernel.org>
> ---
>  drivers/target/target_core_sbc.c | 71 +++++++++++++++++++++++++++++++++++-----
>  1 file changed, 62 insertions(+), 9 deletions(-)

Applied, but dropping the unnecessary stable CC'.

Since the code as-is generates a warning and doesn't fail the command,
it's not a candidate for stable.

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

[Bart Van Assche]

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(-)

diff --git a/drivers/target/iscsi/iscsi_target_util.c b/drivers/target/iscsi/iscsi_target_util.c
index 5041a9c8bdcb..8a022b5b2317 100644
--- a/drivers/target/iscsi/iscsi_target_util.c
+++ b/drivers/target/iscsi/iscsi_target_util.c
@@ -691,11 +691,17 @@ void iscsit_release_cmd(struct iscsi_cmd *cmd)
 {
 	struct iscsi_session *sess;
 	struct se_cmd *se_cmd = &cmd->se_cmd;
+	struct iscsi_conn *conn = cmd->conn;
+	void (*release)(struct iscsi_conn *, struct iscsi_cmd *);
 
-	if (cmd->conn)
-		sess = cmd->conn->sess;
-	else
+	if (conn) {
+		sess = conn->sess;
+		release = conn->conn_transport->iscsit_release_cmd;
+		if (release)
+			release(conn, cmd);
+	} else {
 		sess = cmd->sess;
+	}
 
 	BUG_ON(!sess || !sess->se_sess);
 
@@ -728,9 +734,6 @@ void __iscsit_free_cmd(struct iscsi_cmd *cmd, bool scsi_cmd,
 		iscsit_remove_cmd_from_immediate_queue(cmd, conn);
 		iscsit_remove_cmd_from_response_queue(cmd, conn);
 	}
-
-	if (conn && conn->conn_transport->iscsit_release_cmd)
-		conn->conn_transport->iscsit_release_cmd(conn, cmd);
 }
 
 void iscsit_free_cmd(struct iscsi_cmd *cmd, bool shutdown)
-- 
2.12.0

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

[Nicholas A. Bellinger]

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.

> diff --git a/drivers/target/iscsi/iscsi_target_util.c b/drivers/target/iscsi/iscsi_target_util.c
> index 5041a9c8bdcb..8a022b5b2317 100644
> --- a/drivers/target/iscsi/iscsi_target_util.c
> +++ b/drivers/target/iscsi/iscsi_target_util.c
> @@ -691,11 +691,17 @@ void iscsit_release_cmd(struct iscsi_cmd *cmd)
>  {
>  	struct iscsi_session *sess;
>  	struct se_cmd *se_cmd = &cmd->se_cmd;
> +	struct iscsi_conn *conn = cmd->conn;
> +	void (*release)(struct iscsi_conn *, struct iscsi_cmd *);
>  
> -	if (cmd->conn)
> -		sess = cmd->conn->sess;
> -	else
> +	if (conn) {
> +		sess = conn->sess;
> +		release = conn->conn_transport->iscsit_release_cmd;
> +		if (release)
> +			release(conn, cmd);
> +	} else {
>  		sess = cmd->sess;
> +	}
>  
>  	BUG_ON(!sess || !sess->se_sess);

Also, no need for a local (*release) function pointer and extra
assignment.

Dropping that part now, and squashing into the original patch.

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

[Varun Prakash]

Hi Nicholas and Bart,

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().

Thanks
Varun

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

[Varun Prakash]

Hi Nicholas,

On Tue, Apr 04, 2017 at 10:36:50AM +0530, Varun Prakash wrote:
> Hi Nicholas and Bart,
> 
> 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
Varun

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

[Nicholas A. Bellinger]

On Thu, 2017-04-13 at 13:14 +0530, Varun Prakash wrote:
> Hi Nicholas,
> 
> On Tue, Apr 04, 2017 at 10:36:50AM +0530, Varun Prakash wrote:
> > Hi Nicholas and Bart,
> > 
> > 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..?

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

[Varun Prakash]

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. 

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

[Nicholas A. Bellinger]

Hi Varun,

On Sun, 2017-05-07 at 18:22 +0530, Varun Prakash wrote:
> On Mon, May 01, 2017 at 09:33:31PM -0700, Nicholas A. Bellinger wrote:

<SNIP>

> > 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. 

Indeed.

Looking at other hw drivers like qla2xxx that have this same
requirement, they do *_unmap_sg() once a completion interrupt has
triggered, but before target_core_fabric_ops->release_cmd() is invoked
and se_cmd->t_task_sg has already been freed.

So I'm open to adding a target_core_fabric_ops->unmap_sg() to do this
ahead of target_core_transport.c:transport_free_pages().

That said, snother option is to perform *_unmap_sg() internally in
cxgbit once DDP completion for WRITEs has completed, but before it's
submitted into iscsi_target -> target_core.

AFAICT from a quick scan of cxgbit code, the two scenarios for this
would be:

*) For ISCSI_OP_SCSI_CMD with immediate data, once
cxgbit_get_immediate_data() is invoked.  Either for all cases when this
is invoked (eg: does the DDP SGLs both immediate, unsolicited and
solicited data when mapped..?), or only when iscsi_cmd->immediate_data =
1 && iscsi_cmd_flags & ICF_GOT_LAST_DATAOUT is true.

*) For ISCSI_OP_SCSI_DATA_OUT with FINAL_BIT set, before
iscsit_check_dataout_payload() is called to invoke target_execute_cmd()

WDYT..?

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

[Varun Prakash]

On Tue, May 09, 2017 at 12:49:26AM -0700, Nicholas A. Bellinger wrote:
> Hi Varun,
> 
> On Sun, 2017-05-07 at 18:22 +0530, Varun Prakash wrote:
> > On Mon, May 01, 2017 at 09:33:31PM -0700, Nicholas A. Bellinger wrote:
> 
> <SNIP>
> 
> Indeed.
> 
> Looking at other hw drivers like qla2xxx that have this same
> requirement, they do *_unmap_sg() once a completion interrupt has
> triggered, but before target_core_fabric_ops->release_cmd() is invoked
> and se_cmd->t_task_sg has already been freed.
> 
> So I'm open to adding a target_core_fabric_ops->unmap_sg() to do this
> ahead of target_core_transport.c:transport_free_pages().
> 
> That said, snother option is to perform *_unmap_sg() internally in
> cxgbit once DDP completion for WRITEs has completed, but before it's
> submitted into iscsi_target -> target_core.
> 
> AFAICT from a quick scan of cxgbit code, the two scenarios for this
> would be:
> 
> *) For ISCSI_OP_SCSI_CMD with immediate data, once
> cxgbit_get_immediate_data() is invoked.  Either for all cases when this
> is invoked (eg: does the DDP SGLs both immediate, unsolicited and
> solicited data when mapped..?), or only when iscsi_cmd->immediate_data =
> 1 && iscsi_cmd_flags & ICF_GOT_LAST_DATAOUT is true.
> 
> *) For ISCSI_OP_SCSI_DATA_OUT with FINAL_BIT set, before
> iscsit_check_dataout_payload() is called to invoke target_execute_cmd()
> 
> WDYT..?
>
 
Current cxgbit code does following in cxgbit_release_cmd() 
1. put_page() in case of PASSTHROUGH_SG_TO_MEM_NOALLOC.
2. dma_unmap_sg() DDP SGL.
3. free hw DDP resource.

If cxgbit does cleanup internally then lot of error handling code is
required in cxgbit driver, eg: - PDU with F bit set never arrives,
header digest errors etc.

Another approach to add target_core_fabrics_ops->unmap_sg() will not work
for ERL 2 case because for calling ->iscsit_unmap_sg() valid
cmd->conn pointer is required, in case of ERL 2 cmd->conn can be NULL,
but we can use this approach because in current cxgbit code I enable DDP
only for ERL 0 case, similiary I can add code
to enable PASSTHROUGH_SG_TO_MEM_NOALLOC only for ERL 0 case.

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

[Bart Van Assche]

T24gVHVlLCAyMDE3LTA0LTA0IGF0IDEwOjM2ICswNTMwLCBWYXJ1biBQcmFrYXNoIHdyb3RlOg0K
PiBPbiBTdW4sIEFwciAwMiwgMjAxNyBhdCAwMzo1OTowNVBNIC0wNzAwLCBOaWNob2xhcyBBLiBC
ZWxsaW5nZXIgd3JvdGU6DQo+ID4gT24gVGh1LCAyMDE3LTAzLTMwIGF0IDEwOjEyIC0wNzAwLCBC
YXJ0IFZhbiBBc3NjaGUgd3JvdGU6DQo+ID4gPiBXaGlsZSByZWxlYXNpbmcgYSBjb21tYW5kIF9f
aXNjc2l0X2ZyZWVfY21kKCkgY2FuIGJlIGNhbGxlZCBtdWx0aXBsZQ0KPiA+ID4gdGltZXMgYnV0
IC5pc2NzaXRfcmVsZWFzZV9jbWQoKSBtdXN0IGJlIGNhbGxlZCBvbmx5IG9uY2UuIEhlbmNlIG1v
dmUNCj4gPiA+IHRoZSAuaXNjc2l0X3JlbGVhc2VfY21kKCkgY2FsbCBpbnRvIGlzY3NpdF9yZWxl
YXNlX2NtZCgpLiBUaGUgbGF0dGVyDQo+ID4gPiBmdW5jdGlvbiBpcyBvbmx5IGNhbGxlZCBvbmNl
IHBlciBjb21tYW5kLiBUaGUgb25seSBkcml2ZXIgdGhhdCBkZWZpbmVzDQo+ID4gPiB0aGUgLmlz
Y3NpdF9yZWxlYXNlX2NtZCgpIGNhbGxiYWNrIGlzIHRoZSBjeGdiaXQgZHJpdmVyIHNvIHRoaXMg
Y2hhbmdlDQo+ID4gPiBvbmx5IGFmZmVjdHMgdGhlIGN4Z2JpdCBkcml2ZXIuDQo+ID4gPiANCj4g
PiA+IEZpeGVzOiA3ZWM4MTFhOGU5YzMgKCJpc2NzaS10YXJnZXQ6IGFkZCB2b2lkICgqaXNjc2l0
X3JlbGVhc2VfY21kKSgpIikNCj4gPiA+IFNpZ25lZC1vZmYtYnk6IEJhcnQgVmFuIEFzc2NoZSA8
YmFydC52YW5hc3NjaGVAc2FuZGlzay5jb20+DQo+ID4gPiBDYzogVmFydW4gUHJha2FzaCA8dmFy
dW5AY2hlbHNpby5jb20+DQo+ID4gPiBDYzogTmljaG9sYXMgQmVsbGluZ2VyIDxuYWJAbGludXgt
aXNjc2kub3JnPg0KPiA+ID4gQ2M6IDxzdGFibGVAdmdlci5rZXJuZWwub3JnPg0KPiA+ID4gLS0t
DQo+ID4gPiAgZHJpdmVycy90YXJnZXQvaXNjc2kvaXNjc2lfdGFyZ2V0X3V0aWwuYyB8IDE1ICsr
KysrKysrKy0tLS0tLQ0KPiA+ID4gIDEgZmlsZSBjaGFuZ2VkLCA5IGluc2VydGlvbnMoKyksIDYg
ZGVsZXRpb25zKC0pDQo+ID4gPiANCj4gPiANCj4gPiBBcHBsaWVkIHRvIHRhcmdldC1wZW5kaW5n
L2Zvci1uZXh0LCBidXQgZHJvcHBpbmcgdGhlIHN0YWJsZSBDQycgYmVjYXVzZQ0KPiA+IHRoZSBz
aW5nbGUgY2FsbGVyIGluIGN4Z2JpdF9yZWxlYXNlX2NtZCgpIGlzIGFscmVhZHkgY2hlY2tpbmcg
dG8gZW5zdXJlDQo+ID4gcmVzb3VyY2VzIGFyZSBvbmx5IHJlbGVhc2VkIG9uIHRoZSBmaXJzdCBp
bnZvY2F0aW9uLg0KPiA+IA0KPiA+IFNvIGl0J3Mgbm90IGEgYnVnLWZpeC4NCj4gDQo+IEluIGNh
c2Ugb2YgRERQIGN4Z2JpdCBkcml2ZXIgYXNzaWducyBjbWQtPnNlX2NtZC50X2RhdGFfc2cgdG8g
dHRpbmZvLT5zZ2wNCj4gYW5kIGNhbGxzIGRtYV9tYXBfc2coKSwgY3hnYml0X3JlbGVhc2VfY21k
KCkgY2FsbHMgZG1hX3VubWFwX3NnKCksIGl0IG5lZWRzDQo+IGEgdmFsaWQgc2codHRpbmZvLT5z
Z2wpLCBiZWZvcmUgY2FsbGluZyBpc2NzaXRfcmVsZWFzZV9jbWQoKQ0KPiBjbWQtPnNlX2NtZC50
X2RhdGFfc2cgZ2V0cyBmcmVlZCBzbyB0dGluZm8tPnNnbCB3aWxsIG5vdCBiZSB2YWxpZCBpZiB3
ZSBtb3ZlDQo+IC0+aXNjc2l0X3JlbGVhc2VfY21kKCkgdG8gaXNjc2l0X3JlbGVhc2VfY21kKCku
DQoNCihyZXBseWluZyB0byBhbiBlLW1haWwgb2Ygc2l4IG1vbnRocyBhZ28pDQoNCkhlbGxvIFZh
cnVuLA0KDQpIYXZlIHlvdSBub3RpY2VkIHRoYXQgY29tbWl0IGZlYmU1NjJjMjBkZiAodGFyZ2V0
OiBGaXggTFVOX1JFU0VUIGFjdGl2ZSBJL08NCmhhbmRsaW5nIGZvciBBQ0tfS1JFRjsgSmFudWFy
eSAyMDE2KSBtb3ZlZCB0aGUgdHJhbnNwb3J0X2ZyZWVfcGFnZXMoKSBjYWxsDQpmcm9tIHRyYW5z
cG9ydF9wdXRfY21kKCkgdG8gdGFyZ2V0X3JlbGVhc2VfY21kX2tyZWYoKT8gSSB0aGluayB0aGF0
IG1lYW5zDQp0aGF0IGl0IGlzIG5vdyBzYWZlIHRvIGNhbGwgLmlzY3NpdF9yZWxlYXNlX2NtZCgp
IGFmdGVyDQp0cmFuc3BvcnRfZ2VuZXJpY19mcmVlX2NtZCgpLg0KDQpUaGFua3MsDQoNCkJhcnQu

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

[Bart Van Assche]

On Tue, 2017-04-04 at 10:36 +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().

(replying to an e-mail of six months ago)

Hello Varun,

Have you noticed that commit febe562c20df (target: Fix LUN_RESET active I/O
handling for ACK_KREF; January 2016) moved the transport_free_pages() call
from transport_put_cmd() to target_release_cmd_kref()? I think that means
that it is now safe to call .iscsit_release_cmd() after
transport_generic_free_cmd().

Thanks,

Bart.

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

[Varun Prakash]

On Wed, Nov 01, 2017 at 12:07:52AM +0000, Bart Van Assche wrote:
> On Tue, 2017-04-04 at 10:36 +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(-)
> > > > 
> > >
 
> 
> (replying to an e-mail of six months ago)
> 
> Hello Varun,
> 
> Have you noticed that commit febe562c20df (target: Fix LUN_RESET active I/O
> handling for ACK_KREF; January 2016) moved the transport_free_pages() call
> from transport_put_cmd() to target_release_cmd_kref()? I think that means
> that it is now safe to call .iscsit_release_cmd() after
> transport_generic_free_cmd().
> 

Hello Bart,

The requirement here is to call .iscsit_release_cmd() before target free the
pages so that cxgbit driver can call dma_unmap_sg() and free the pages in case
of PASSTHROUGH_SG_TO_MEM_NOALLOC.

Currently .iscsit_release_cmd() is called from two functions -

iscsit_free_cmd() -> __iscsit_free_cmd() -> .iscsit_release_cmd()
iscsit_aborted_task() -> __iscsit_free_cmd() -> .iscsit_release_cmd()

If we move .iscsit_release_cmd() after transport_generic_free_cmd(), will it
handle all the error cases(abort etc)?

In case of abort currently it is called from iscsit_aborted_task(), if we
move then in case of abort .iscsit_release_cmd() will not be called.

If we can confirm that moving .iscsit_release_cmd() will not cause any
memory leak then we can move it.

Thanks
Varun