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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
