| Message ID | 20170504225102.8931-16-bart.vanassche@sandisk.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05/05/2017 12:50 AM, Bart Van Assche wrote: > Since .se_tfo is only set if a command has been submitted to > the LIO core, check .se_tfo instead of .iscsi_opcode. Since > __iscsit_free_cmd() only affects SCSI commands but not TMFs, > calling that function for TMFs does not change behavior. This > patch does not change the behavior of iscsit_free_cmd(). > > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com> > Cc: Hannes Reinecke <hare@suse.com> > Cc: Christoph Hellwig <hch@lst.de> > Cc: Andy Grover <agrover@redhat.com> > Cc: David Disseldorp <ddiss@suse.de> > --- > drivers/target/iscsi/iscsi_target_util.c | 39 ++++---------------------------- > 1 file changed, 4 insertions(+), 35 deletions(-) > > diff --git a/drivers/target/iscsi/iscsi_target_util.c b/drivers/target/iscsi/iscsi_target_util.c > index 41b9e7cc08b8..1e36f83b5961 100644 > --- a/drivers/target/iscsi/iscsi_target_util.c > +++ b/drivers/target/iscsi/iscsi_target_util.c > @@ -734,49 +734,18 @@ void __iscsit_free_cmd(struct iscsi_cmd *cmd, bool check_queues) > > void iscsit_free_cmd(struct iscsi_cmd *cmd, bool shutdown) > { > - struct se_cmd *se_cmd = NULL; > + struct se_cmd *se_cmd = cmd->se_cmd.se_tfo ? &cmd->se_cmd : NULL; > int rc; > > - /* > - * Determine if a struct se_cmd is associated with > - * this struct iscsi_cmd. > - */ > - switch (cmd->iscsi_opcode) { > - case ISCSI_OP_SCSI_CMD: > - /* > - * Fallthrough > - */ > - case ISCSI_OP_SCSI_TMFUNC: > - se_cmd = &cmd->se_cmd; > - __iscsit_free_cmd(cmd, shutdown); > + __iscsit_free_cmd(cmd, shutdown); > + if (se_cmd) { > rc = transport_generic_free_cmd(se_cmd, shutdown); > if (!rc && shutdown && se_cmd->se_sess) { > __iscsit_free_cmd(cmd, shutdown); > target_put_sess_cmd(se_cmd); > } > - break; > - case ISCSI_OP_REJECT: > - /* > - * Handle special case for REJECT when iscsi_add_reject*() has > - * overwritten the original iscsi_opcode assignment, and the > - * associated cmd->se_cmd needs to be released. > - */ > - if (cmd->se_cmd.se_tfo != NULL) { > - se_cmd = &cmd->se_cmd; > - __iscsit_free_cmd(cmd, shutdown); > - > - rc = transport_generic_free_cmd(&cmd->se_cmd, shutdown); > - if (!rc && shutdown && se_cmd->se_sess) { > - __iscsit_free_cmd(cmd, shutdown); > - target_put_sess_cmd(se_cmd); > - } > - break; > - } > - /* Fall-through */ > - default: > - __iscsit_free_cmd(cmd, shutdown); > + } else { > iscsit_release_cmd(cmd); > - break; > } > } > EXPORT_SYMBOL(iscsit_free_cmd); > Maybe it's easier just to add if (!se_cmd) { iscsit_release_cmd(cmd); return; } to the beginning of the function; that would reduce code churn by quite a bit ... Cheers, Hannes
On Fri, 2017-05-05 at 13:22 +0200, Hannes Reinecke wrote: > On 05/05/2017 12:50 AM, Bart Van Assche wrote: > > Since .se_tfo is only set if a command has been submitted to > > the LIO core, check .se_tfo instead of .iscsi_opcode. Since > > __iscsit_free_cmd() only affects SCSI commands but not TMFs, > > calling that function for TMFs does not change behavior. This > > patch does not change the behavior of iscsit_free_cmd(). > > > > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com> > > Cc: Hannes Reinecke <hare@suse.com> > > Cc: Christoph Hellwig <hch@lst.de> > > Cc: Andy Grover <agrover@redhat.com> > > Cc: David Disseldorp <ddiss@suse.de> > > --- > > drivers/target/iscsi/iscsi_target_util.c | 39 ++++---------------------------- > > 1 file changed, 4 insertions(+), 35 deletions(-) > > > > diff --git a/drivers/target/iscsi/iscsi_target_util.c b/drivers/target/iscsi/iscsi_target_util.c > > index 41b9e7cc08b8..1e36f83b5961 100644 > > --- a/drivers/target/iscsi/iscsi_target_util.c > > +++ b/drivers/target/iscsi/iscsi_target_util.c > > @@ -734,49 +734,18 @@ void __iscsit_free_cmd(struct iscsi_cmd *cmd, bool check_queues) > > > > void iscsit_free_cmd(struct iscsi_cmd *cmd, bool shutdown) > > { > > - struct se_cmd *se_cmd = NULL; > > + struct se_cmd *se_cmd = cmd->se_cmd.se_tfo ? &cmd->se_cmd : NULL; > > int rc; > > > > - /* > > - * Determine if a struct se_cmd is associated with > > - * this struct iscsi_cmd. > > - */ > > - switch (cmd->iscsi_opcode) { > > - case ISCSI_OP_SCSI_CMD: > > - /* > > - * Fallthrough > > - */ > > - case ISCSI_OP_SCSI_TMFUNC: > > - se_cmd = &cmd->se_cmd; > > - __iscsit_free_cmd(cmd, shutdown); > > + __iscsit_free_cmd(cmd, shutdown); > > + if (se_cmd) { > > rc = transport_generic_free_cmd(se_cmd, shutdown); > > if (!rc && shutdown && se_cmd->se_sess) { > > __iscsit_free_cmd(cmd, shutdown); > > target_put_sess_cmd(se_cmd); > > } > > - break; > > - case ISCSI_OP_REJECT: > > - /* > > - * Handle special case for REJECT when iscsi_add_reject*() has > > - * overwritten the original iscsi_opcode assignment, and the > > - * associated cmd->se_cmd needs to be released. > > - */ > > - if (cmd->se_cmd.se_tfo != NULL) { > > - se_cmd = &cmd->se_cmd; > > - __iscsit_free_cmd(cmd, shutdown); > > - > > - rc = transport_generic_free_cmd(&cmd->se_cmd, shutdown); > > - if (!rc && shutdown && se_cmd->se_sess) { > > - __iscsit_free_cmd(cmd, shutdown); > > - target_put_sess_cmd(se_cmd); > > - } > > - break; > > - } > > - /* Fall-through */ > > - default: > > - __iscsit_free_cmd(cmd, shutdown); > > + } else { > > iscsit_release_cmd(cmd); > > - break; > > } > > } > > EXPORT_SYMBOL(iscsit_free_cmd); > > > > Maybe it's easier just to add > > if (!se_cmd) { > iscsit_release_cmd(cmd); > return; > } > > to the beginning of the function; that would reduce code churn by quite > a bit ... Hello Hannes, This patch mostly deletes code so I'm not sure how your suggestion would improve this patch? Bart.-- To unsubscribe from this list: send the line "unsubscribe target-devel" 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 41b9e7cc08b8..1e36f83b5961 100644 --- a/drivers/target/iscsi/iscsi_target_util.c +++ b/drivers/target/iscsi/iscsi_target_util.c @@ -734,49 +734,18 @@ void __iscsit_free_cmd(struct iscsi_cmd *cmd, bool check_queues) void iscsit_free_cmd(struct iscsi_cmd *cmd, bool shutdown) { - struct se_cmd *se_cmd = NULL; + struct se_cmd *se_cmd = cmd->se_cmd.se_tfo ? &cmd->se_cmd : NULL; int rc; - /* - * Determine if a struct se_cmd is associated with - * this struct iscsi_cmd. - */ - switch (cmd->iscsi_opcode) { - case ISCSI_OP_SCSI_CMD: - /* - * Fallthrough - */ - case ISCSI_OP_SCSI_TMFUNC: - se_cmd = &cmd->se_cmd; - __iscsit_free_cmd(cmd, shutdown); + __iscsit_free_cmd(cmd, shutdown); + if (se_cmd) { rc = transport_generic_free_cmd(se_cmd, shutdown); if (!rc && shutdown && se_cmd->se_sess) { __iscsit_free_cmd(cmd, shutdown); target_put_sess_cmd(se_cmd); } - break; - case ISCSI_OP_REJECT: - /* - * Handle special case for REJECT when iscsi_add_reject*() has - * overwritten the original iscsi_opcode assignment, and the - * associated cmd->se_cmd needs to be released. - */ - if (cmd->se_cmd.se_tfo != NULL) { - se_cmd = &cmd->se_cmd; - __iscsit_free_cmd(cmd, shutdown); - - rc = transport_generic_free_cmd(&cmd->se_cmd, shutdown); - if (!rc && shutdown && se_cmd->se_sess) { - __iscsit_free_cmd(cmd, shutdown); - target_put_sess_cmd(se_cmd); - } - break; - } - /* Fall-through */ - default: - __iscsit_free_cmd(cmd, shutdown); + } else { iscsit_release_cmd(cmd); - break; } } EXPORT_SYMBOL(iscsit_free_cmd);
Since .se_tfo is only set if a command has been submitted to the LIO core, check .se_tfo instead of .iscsi_opcode. Since __iscsit_free_cmd() only affects SCSI commands but not TMFs, calling that function for TMFs does not change behavior. This patch does not change the behavior of iscsit_free_cmd(). Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com> Cc: Hannes Reinecke <hare@suse.com> Cc: Christoph Hellwig <hch@lst.de> Cc: Andy Grover <agrover@redhat.com> Cc: David Disseldorp <ddiss@suse.de> --- drivers/target/iscsi/iscsi_target_util.c | 39 ++++---------------------------- 1 file changed, 4 insertions(+), 35 deletions(-)