Re: [PATCH v2 04/16] iscsi-target: add void (*iscsit_release_cmd)()

[Varun Prakash]

On Sun, Apr 10, 2016 at 08:42:44PM +0300, Sagi Grimberg wrote:
> 
> >Add void (*iscsit_release_cmd)() to
> >struct iscsit_transport, iscsi-target
> >uses this callback to release transport
> >driver resources associated with an iSCSI cmd.
> 
> I'd really like to see some reasoning on why you add
> abstraction callouts. It may have a valid reason but
> it needs to be documented in the change log...

This is for releasing DDP resource and sg page
in case of PASSTHROUGH_SG_TO_MEM_NOALLOC.
I will update commit log in -v3.

> 
> >diff --git a/drivers/target/iscsi/iscsi_target_util.c 
> >b/drivers/target/iscsi/iscsi_target_util.c
> >index 428b0d9..a533017 100644
> >--- a/drivers/target/iscsi/iscsi_target_util.c
> >+++ b/drivers/target/iscsi/iscsi_target_util.c
> >@@ -725,6 +725,9 @@ 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);
> >  }
> 
> Did you verify that you get here with conn = NULL (given that you test
> it)? If so, then can you please document why is it expected for this
> function to be called twice that we need to make it safe?
> 
> If not, then I'd move this check to be a WARN_ON/BUG_ON to hunt
> down when is this happening.

There is already a check for NULL conn in __iscsit_free_cmd()

if (conn && check_queues) {
iscsit_remove_cmd_from_immediate_queue(cmd, conn);
iscsit_remove_cmd_from_response_queue(cmd, conn);
}

cmd->conn can become NULL in ERL2 error recovery.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 04/16] iscsi-target: add void (*iscsit_release_cmd)()

[Sagi Grimberg]

Add void (*iscsit_release_cmd)() to
struct iscsit_transport, iscsi-target
uses this callback to release transport
driver resources associated with an iSCSI cmd.


I'd really like to see some reasoning on why you add
abstraction callouts. It may have a valid reason but
it needs to be documented in the change log...


diff --git a/drivers/target/iscsi/iscsi_target_util.c 
b/drivers/target/iscsi/iscsi_target_util.c
index 428b0d9..a533017 100644
--- a/drivers/target/iscsi/iscsi_target_util.c
+++ b/drivers/target/iscsi/iscsi_target_util.c
@@ -725,6 +725,9 @@ 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);
  }


Did you verify that you get here with conn = NULL (given that you test
it)? If so, then can you please document why is it expected for this
function to be called twice that we need to make it safe?

If not, then I'd move this check to be a WARN_ON/BUG_ON to hunt
down when is this happening.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 04/16] iscsi-target: add void (*iscsit_release_cmd)()

[Varun Prakash]

Add void (*iscsit_release_cmd)() to
struct iscsit_transport, iscsi-target
uses this callback to release transport
driver resources associated with an iSCSI cmd.

Signed-off-by: Varun Prakash 
---
 drivers/target/iscsi/iscsi_target_util.c | 3 +++
 include/target/iscsi/iscsi_transport.h   | 1 +
 2 files changed, 4 insertions(+)

diff --git a/drivers/target/iscsi/iscsi_target_util.c 
b/drivers/target/iscsi/iscsi_target_util.c
index 428b0d9..a533017 100644
--- a/drivers/target/iscsi/iscsi_target_util.c
+++ b/drivers/target/iscsi/iscsi_target_util.c
@@ -725,6 +725,9 @@ 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)
diff --git a/include/target/iscsi/iscsi_transport.h 
b/include/target/iscsi/iscsi_transport.h
index 77ae41e..19cef94 100644
--- a/include/target/iscsi/iscsi_transport.h
+++ b/include/target/iscsi/iscsi_transport.h
@@ -29,6 +29,7 @@ struct iscsit_transport {
int (*iscsit_xmit_datain_pdu)(struct iscsi_conn *, struct iscsi_cmd *,
  struct iscsi_datain_req *,
  struct iscsi_datain *);
+   void (*iscsit_release_cmd)(struct iscsi_conn *, struct iscsi_cmd *);
enum target_prot_op (*iscsit_get_sup_prot_ops)(struct iscsi_conn *);
 };
 
-- 
2.0.2

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html