* [PATCH 1/6] target: Fix VERIFY and WRITE VERIFY command parsing
[not found] <20170330171244.8346-1-bart.vanassche@sandisk.com>
@ 2017-03-30 17:12 ` Bart Van Assche
2017-04-02 22:43 ` Nicholas A. Bellinger
2017-03-30 17:12 ` [PATCH 2/6] target/iscsi: Call .iscsit_release_cmd() once Bart Van Assche
1 sibling, 1 reply; 13+ messages in thread
From: Bart Van Assche @ 2017-03-30 17:12 UTC (permalink / raw)
To: Nicholas Bellinger; +Cc: target-devel, Bart Van Assche, Max Lohrmann, stable
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, §ors, &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, §ors, &size);
+ if (ret)
+ return ret;
cmd->execute_cmd = sbc_emulate_noop;
goto check_lba;
case REZERO_UNIT:
--
2.12.0
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/6] target/iscsi: Call .iscsit_release_cmd() once
[not found] <20170330171244.8346-1-bart.vanassche@sandisk.com>
2017-03-30 17:12 ` [PATCH 1/6] target: Fix VERIFY and WRITE VERIFY command parsing Bart Van Assche
@ 2017-03-30 17:12 ` Bart Van Assche
2017-04-02 22:59 ` Nicholas A. Bellinger
1 sibling, 1 reply; 13+ messages in thread
From: Bart Van Assche @ 2017-03-30 17:12 UTC (permalink / raw)
To: Nicholas Bellinger; +Cc: target-devel, Bart Van Assche, Varun Prakash, stable
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/6] target: Fix VERIFY and WRITE VERIFY command parsing
2017-03-30 17:12 ` [PATCH 1/6] target: Fix VERIFY and WRITE VERIFY command parsing Bart Van Assche
@ 2017-04-02 22:43 ` Nicholas A. Bellinger
0 siblings, 0 replies; 13+ messages in thread
From: Nicholas A. Bellinger @ 2017-04-02 22:43 UTC (permalink / raw)
To: Bart Van Assche; +Cc: target-devel, Max Lohrmann, stable
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/6] target/iscsi: Call .iscsit_release_cmd() once
2017-03-30 17:12 ` [PATCH 2/6] target/iscsi: Call .iscsit_release_cmd() once Bart Van Assche
@ 2017-04-02 22:59 ` Nicholas A. Bellinger
2017-04-04 5:06 ` Varun Prakash
0 siblings, 1 reply; 13+ messages in thread
From: Nicholas A. Bellinger @ 2017-04-02 22:59 UTC (permalink / raw)
To: Bart Van Assche; +Cc: target-devel, Varun Prakash, stable
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/6] target/iscsi: Call .iscsit_release_cmd() once
2017-04-02 22:59 ` Nicholas A. Bellinger
@ 2017-04-04 5:06 ` Varun Prakash
2017-04-13 7:44 ` Varun Prakash
2017-11-01 0:07 ` Bart Van Assche
0 siblings, 2 replies; 13+ messages in thread
From: Varun Prakash @ 2017-04-04 5:06 UTC (permalink / raw)
To: Nicholas A. Bellinger; +Cc: Bart Van Assche, target-devel, stable
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/6] target/iscsi: Call .iscsit_release_cmd() once
2017-04-04 5:06 ` Varun Prakash
@ 2017-04-13 7:44 ` Varun Prakash
2017-05-02 4:33 ` Nicholas A. Bellinger
2017-11-01 0:07 ` Bart Van Assche
1 sibling, 1 reply; 13+ messages in thread
From: Varun Prakash @ 2017-04-13 7:44 UTC (permalink / raw)
To: Nicholas A. Bellinger; +Cc: Bart Van Assche, target-devel, stable
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/6] target/iscsi: Call .iscsit_release_cmd() once
2017-04-13 7:44 ` Varun Prakash
@ 2017-05-02 4:33 ` Nicholas A. Bellinger
2017-05-07 12:52 ` Varun Prakash
0 siblings, 1 reply; 13+ messages in thread
From: Nicholas A. Bellinger @ 2017-05-02 4:33 UTC (permalink / raw)
To: Varun Prakash; +Cc: Bart Van Assche, target-devel, stable
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..?
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/6] target/iscsi: Call .iscsit_release_cmd() once
2017-05-02 4:33 ` Nicholas A. Bellinger
@ 2017-05-07 12:52 ` Varun Prakash
2017-05-09 7:49 ` Nicholas A. Bellinger
0 siblings, 1 reply; 13+ messages in thread
From: Varun Prakash @ 2017-05-07 12:52 UTC (permalink / raw)
To: Nicholas A. Bellinger; +Cc: Bart Van Assche, target-devel, stable
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/6] target/iscsi: Call .iscsit_release_cmd() once
2017-05-07 12:52 ` Varun Prakash
@ 2017-05-09 7:49 ` Nicholas A. Bellinger
2017-05-10 16:03 ` Varun Prakash
0 siblings, 1 reply; 13+ messages in thread
From: Nicholas A. Bellinger @ 2017-05-09 7:49 UTC (permalink / raw)
To: Varun Prakash; +Cc: Bart Van Assche, target-devel, stable
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..?
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/6] target/iscsi: Call .iscsit_release_cmd() once
2017-05-09 7:49 ` Nicholas A. Bellinger
@ 2017-05-10 16:03 ` Varun Prakash
0 siblings, 0 replies; 13+ messages in thread
From: Varun Prakash @ 2017-05-10 16:03 UTC (permalink / raw)
To: Nicholas A. Bellinger; +Cc: Bart Van Assche, target-devel, stable
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/6] target/iscsi: Call .iscsit_release_cmd() once
2017-04-04 5:06 ` Varun Prakash
@ 2017-11-01 0:07 ` Bart Van Assche
2017-11-01 0:07 ` Bart Van Assche
1 sibling, 0 replies; 13+ messages in thread
From: Bart Van Assche @ 2017-11-01 0:07 UTC (permalink / raw)
To: varun, nab; +Cc: Bart Van Assche, target-devel, stable
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/6] target/iscsi: Call .iscsit_release_cmd() once
@ 2017-11-01 0:07 ` Bart Van Assche
0 siblings, 0 replies; 13+ messages in thread
From: Bart Van Assche @ 2017-11-01 0:07 UTC (permalink / raw)
To: varun, nab; +Cc: Bart Van Assche, target-devel, stable
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.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/6] target/iscsi: Call .iscsit_release_cmd() once
2017-11-01 0:07 ` Bart Van Assche
(?)
@ 2017-11-06 15:38 ` Varun Prakash
-1 siblings, 0 replies; 13+ messages in thread
From: Varun Prakash @ 2017-11-06 15:38 UTC (permalink / raw)
To: target-devel
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
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2017-11-06 15:38 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20170330171244.8346-1-bart.vanassche@sandisk.com>
2017-03-30 17:12 ` [PATCH 1/6] target: Fix VERIFY and WRITE VERIFY command parsing Bart Van Assche
2017-04-02 22:43 ` Nicholas A. Bellinger
2017-03-30 17:12 ` [PATCH 2/6] target/iscsi: Call .iscsit_release_cmd() once Bart Van Assche
2017-04-02 22:59 ` Nicholas A. Bellinger
2017-04-04 5:06 ` Varun Prakash
2017-04-13 7:44 ` Varun Prakash
2017-05-02 4:33 ` Nicholas A. Bellinger
2017-05-07 12:52 ` Varun Prakash
2017-05-09 7:49 ` Nicholas A. Bellinger
2017-05-10 16:03 ` Varun Prakash
2017-11-01 0:07 ` Bart Van Assche
2017-11-01 0:07 ` Bart Van Assche
2017-11-06 15:38 ` Varun Prakash
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.