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@xxxxxxxxxxx>
> > Cc: Hannes Reinecke <hare@xxxxxxxx>
> > Cc: Christoph Hellwig <hch@xxxxxx>
> > Cc: Andy Grover <agrover@xxxxxxxxxx>
> > Cc: David Disseldorp <ddiss@xxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
