| Message ID | 20170202005853.23456-19-bart.vanassche@sandisk.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, 2017-02-01 at 16:58 -0800, Bart Van Assche wrote: > The function transport_cmd_check_stop() has two callers. These callers > invoke this function as follows: > * transport_cmd_check_stop(cmd, true, false) > * transport_cmd_check_stop(cmd, false, true) > Hence inline this function into its callers. > > This patch does not change any functionality but improves source > code readability. Nice simplification here. One comment below. > > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com> > Reviewed-by: Hannes Reinecke <hare@suse.com> > Reviewed-by: Bryant G. Ly <bryantly@linux.vnet.ibm.com> > Cc: Christoph Hellwig <hch@lst.de> > Cc: Andy Grover <agrover@redhat.com> > Cc: David Disseldorp <ddiss@suse.de> > --- > drivers/target/target_core_transport.c | 68 +++++++++++++++++----------------- > include/target/target_core_fabric.h | 2 +- > 2 files changed, 34 insertions(+), 36 deletions(-) > > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c > index 25bc214a4eee..40dff4799984 100644 > --- a/drivers/target/target_core_transport.c > +++ b/drivers/target/target_core_transport.c > @@ -602,24 +602,18 @@ static void target_remove_from_state_list(struct se_cmd *cmd) > spin_unlock_irqrestore(&dev->execute_task_lock, flags); > } > > -static int transport_cmd_check_stop(struct se_cmd *cmd, bool remove_from_lists, > - bool write_pending) > +static int transport_cmd_check_stop_to_fabric(struct se_cmd *cmd) > { > unsigned long flags; > > - if (remove_from_lists) { > - target_remove_from_state_list(cmd); > + target_remove_from_state_list(cmd); > > - /* > - * Clear struct se_cmd->se_lun before the handoff to FE. > - */ > - cmd->se_lun = NULL; > - } > + /* > + * Clear struct se_cmd->se_lun before the handoff to FE. > + */ > + cmd->se_lun = NULL; > > spin_lock_irqsave(&cmd->t_state_lock, flags); > - if (write_pending) > - cmd->t_state = TRANSPORT_WRITE_PENDING; > - > /* > * Determine if frontend context caller is requesting the stopping of > * this command for frontend exceptions. > @@ -632,30 +626,17 @@ static int transport_cmd_check_stop(struct se_cmd *cmd, bool remove_from_lists, > } else { > cmd->transport_state &= ~CMD_T_ACTIVE; > } > - > - if (remove_from_lists) { > - /* > - * Some fabric modules like tcm_loop can release > - * their internally allocated I/O reference now and > - * struct se_cmd now. > - * > - * Fabric modules are expected to return '1' here if the > - * se_cmd being passed is released at this point, > - * or zero if not being released. > - */ > - if (cmd->se_tfo->check_stop_free != NULL) { > - spin_unlock_irqrestore(&cmd->t_state_lock, flags); > - return cmd->se_tfo->check_stop_free(cmd); > - } > - } > - > spin_unlock_irqrestore(&cmd->t_state_lock, flags); > - return 0; > -} > > -static int transport_cmd_check_stop_to_fabric(struct se_cmd *cmd) > -{ > - return transport_cmd_check_stop(cmd, true, false); > + /* > + * Some fabric modules like tcm_loop can release their internally > + * allocated I/O reference and struct se_cmd now. > + * > + * Fabric modules are expected to return '1' here if the se_cmd being > + * passed is released at this point, or zero if not being released. > + */ > + return cmd->se_tfo->check_stop_free ? cmd->se_tfo->check_stop_free(cmd) > + : 0; > } I think all of the in-tree fabric drivers provide a ->check_stop_free() callback. It would make sense to go ahead and enforce it's existence tree-wide at the target_fabric_tf_ops_check() level, and drop the extra conditional check here. -- 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
On Tue, 2017-02-07 at 06:38 -0800, Nicholas A. Bellinger wrote: > I think all of the in-tree fabric drivers provide a ->check_stop_free() > callback. > > It would make sense to go ahead and enforce it's existence tree-wide at > the target_fabric_tf_ops_check() level, and drop the extra conditional > check here. Let's keep that for later. This patch series is already long enough. 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/target_core_transport.c b/drivers/target/target_core_transport.c index 25bc214a4eee..40dff4799984 100644 --- a/drivers/target/target_core_transport.c +++ b/drivers/target/target_core_transport.c @@ -602,24 +602,18 @@ static void target_remove_from_state_list(struct se_cmd *cmd) spin_unlock_irqrestore(&dev->execute_task_lock, flags); } -static int transport_cmd_check_stop(struct se_cmd *cmd, bool remove_from_lists, - bool write_pending) +static int transport_cmd_check_stop_to_fabric(struct se_cmd *cmd) { unsigned long flags; - if (remove_from_lists) { - target_remove_from_state_list(cmd); + target_remove_from_state_list(cmd); - /* - * Clear struct se_cmd->se_lun before the handoff to FE. - */ - cmd->se_lun = NULL; - } + /* + * Clear struct se_cmd->se_lun before the handoff to FE. + */ + cmd->se_lun = NULL; spin_lock_irqsave(&cmd->t_state_lock, flags); - if (write_pending) - cmd->t_state = TRANSPORT_WRITE_PENDING; - /* * Determine if frontend context caller is requesting the stopping of * this command for frontend exceptions. @@ -632,30 +626,17 @@ static int transport_cmd_check_stop(struct se_cmd *cmd, bool remove_from_lists, } else { cmd->transport_state &= ~CMD_T_ACTIVE; } - - if (remove_from_lists) { - /* - * Some fabric modules like tcm_loop can release - * their internally allocated I/O reference now and - * struct se_cmd now. - * - * Fabric modules are expected to return '1' here if the - * se_cmd being passed is released at this point, - * or zero if not being released. - */ - if (cmd->se_tfo->check_stop_free != NULL) { - spin_unlock_irqrestore(&cmd->t_state_lock, flags); - return cmd->se_tfo->check_stop_free(cmd); - } - } - spin_unlock_irqrestore(&cmd->t_state_lock, flags); - return 0; -} -static int transport_cmd_check_stop_to_fabric(struct se_cmd *cmd) -{ - return transport_cmd_check_stop(cmd, true, false); + /* + * Some fabric modules like tcm_loop can release their internally + * allocated I/O reference and struct se_cmd now. + * + * Fabric modules are expected to return '1' here if the se_cmd being + * passed is released at this point, or zero if not being released. + */ + return cmd->se_tfo->check_stop_free ? cmd->se_tfo->check_stop_free(cmd) + : 0; } static void transport_lun_remove_cmd(struct se_cmd *cmd) @@ -2389,6 +2370,7 @@ EXPORT_SYMBOL(target_alloc_sgl); sense_reason_t transport_generic_new_cmd(struct se_cmd *cmd) { + unsigned long flags; int ret = 0; bool zero_flag = !(cmd->se_cmd_flags & SCF_SCSI_DATA_CDB); @@ -2454,8 +2436,24 @@ transport_generic_new_cmd(struct se_cmd *cmd) target_execute_cmd(cmd); return 0; } - if (transport_cmd_check_stop(cmd, false, true)) + + spin_lock_irqsave(&cmd->t_state_lock, flags); + cmd->t_state = TRANSPORT_WRITE_PENDING; + /* + * Determine if frontend context caller is requesting the stopping of + * this command for frontend exceptions. + */ + if (cmd->transport_state & CMD_T_STOP) { + pr_debug("%s:%d CMD_T_STOP for ITT: 0x%08llx\n", + __func__, __LINE__, cmd->tag); + + spin_unlock_irqrestore(&cmd->t_state_lock, flags); + + complete_all(&cmd->t_transport_stop_comp); return 0; + } + cmd->transport_state &= ~CMD_T_ACTIVE; + spin_unlock_irqrestore(&cmd->t_state_lock, flags); ret = cmd->se_tfo->write_pending(cmd); if (ret == -EAGAIN || ret == -ENOMEM) diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h index 358041bad1da..d7dd1427fe0d 100644 --- a/include/target/target_core_fabric.h +++ b/include/target/target_core_fabric.h @@ -47,7 +47,7 @@ struct target_core_fabric_ops { u32 (*tpg_get_inst_index)(struct se_portal_group *); /* * Optional to release struct se_cmd and fabric dependent allocated - * I/O descriptor in transport_cmd_check_stop(). + * I/O descriptor after command execution has finished. * * Returning 1 will signal a descriptor has been released. * Returning 0 will signal a descriptor has not been released.