| Message ID | 1499510260d6358522b4023822022f0add88b687.1460204441.git.varun@chelsio.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
> 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 *); };
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 <varun@chelsio.com> --- drivers/target/iscsi/iscsi_target_util.c | 3 +++ include/target/iscsi/iscsi_transport.h | 1 + 2 files changed, 4 insertions(+)