| Message ID | 20170208222507.25715-34-bart.vanassche@sandisk.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, 2017-02-08 at 14:25 -0800, Bart Van Assche wrote: > Remove the code that clears .se_lun from > transport_cmd_check_stop_to_fabric() such that the > transport_lun_remove_cmd() call can be moved into > target_release_cmd_kref(). Because this guarantees that > transport_lun_remove_cmd() will be called exactly once, it is > safe to change the cmpxchg() call into a test of > se_cmd.lun_ref_active. Inline transport_lun_remove_cmd() because > it is not worth to keep it as a separate function. > > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com> > Reviewed-by: 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/target_core_transport.c | 30 +++--------------------------- > 1 file changed, 3 insertions(+), 27 deletions(-) > > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c > index 8df03987bdb8..99531dc85f9c 100644 > --- a/drivers/target/target_core_transport.c > +++ b/drivers/target/target_core_transport.c > @@ -598,11 +598,6 @@ static int transport_cmd_check_stop_to_fabric(struct se_cmd *cmd) > > target_remove_from_state_list(cmd); > > - /* > - * Clear struct se_cmd->se_lun before the handoff to FE. > - */ > - cmd->se_lun = NULL; > - > spin_lock_irqsave(&cmd->t_state_lock, flags); > /* > * Determine if frontend context caller is requesting the stopping of > @@ -629,17 +624,6 @@ static int transport_cmd_check_stop_to_fabric(struct se_cmd *cmd) > : 0; > } > > -static void transport_lun_remove_cmd(struct se_cmd *cmd) > -{ > - struct se_lun *lun = cmd->se_lun; > - > - if (!lun) > - return; > - > - if (cmpxchg(&cmd->lun_ref_active, true, false)) > - percpu_ref_put(&lun->lun_ref); > -} > - > static void target_complete_failure_work(struct work_struct *work) > { > struct se_cmd *cmd = container_of(work, struct se_cmd, work); > @@ -675,8 +659,6 @@ static void transport_handle_abort(struct se_cmd *cmd) > { > bool ack_kref = cmd->se_cmd_flags & SCF_ACK_KREF; > > - transport_lun_remove_cmd(cmd); > - > if (cmd->send_abort_response) { > cmd->scsi_status = SAM_STAT_TASK_ABORTED; > pr_debug("Setting SAM_STAT_TASK_ABORTED status for CDB: 0x%02x, ITT: 0x%08llx\n", > @@ -1747,7 +1729,6 @@ void transport_generic_request_failure(struct se_cmd *cmd, > goto queue_full; > > check_stop: > - transport_lun_remove_cmd(cmd); > transport_cmd_check_stop_to_fabric(cmd); > return; > > @@ -2022,7 +2003,6 @@ static void transport_complete_qf(struct se_cmd *cmd) > transport_handle_queue_full(cmd, cmd->se_dev); > return; > } > - transport_lun_remove_cmd(cmd); > transport_cmd_check_stop_to_fabric(cmd); > } > > @@ -2096,7 +2076,6 @@ static void target_complete_ok_work(struct work_struct *work) > if (ret == -EAGAIN || ret == -ENOMEM) > goto queue_full; > > - transport_lun_remove_cmd(cmd); > transport_cmd_check_stop_to_fabric(cmd); > return; > } > @@ -2122,7 +2101,6 @@ static void target_complete_ok_work(struct work_struct *work) > if (ret == -EAGAIN || ret == -ENOMEM) > goto queue_full; > > - transport_lun_remove_cmd(cmd); > transport_cmd_check_stop_to_fabric(cmd); > return; > } > @@ -2147,7 +2125,6 @@ static void target_complete_ok_work(struct work_struct *work) > if (ret == -EAGAIN || ret == -ENOMEM) > goto queue_full; > > - transport_lun_remove_cmd(cmd); > transport_cmd_check_stop_to_fabric(cmd); > return; > } > @@ -2183,7 +2160,6 @@ static void target_complete_ok_work(struct work_struct *work) > break; > } > > - transport_lun_remove_cmd(cmd); > transport_cmd_check_stop_to_fabric(cmd); > return; > > @@ -2498,9 +2474,6 @@ int transport_generic_free_cmd(struct se_cmd *cmd, int wait_for_tasks) > */ > if (cmd->state_active) > target_remove_from_state_list(cmd); > - > - if (cmd->se_lun) > - transport_lun_remove_cmd(cmd); > } > /* > * Since the iSCSI and iSER targets driver assume that a SCSI command > @@ -2573,6 +2546,9 @@ static void target_release_cmd_kref(struct kref *kref) > > WARN_ON_ONCE(atomic_read(&se_cmd->tgt_ref) != 0); > > + if (se_cmd->lun_ref_active) > + percpu_ref_put(&se_cmd->se_lun->lun_ref); > + > if (se_sess) { > spin_lock_irqsave(&se_sess->sess_cmd_lock, flags); > if (likely(!list_empty(&se_cmd->se_cmd_list))) { As-is, this patch breaks active I/O configfs shutdown of /sys/kernel/config/target/$FABRIC/$WWN/$TPGT/lun/$LUN_ID/$DEV_SYMLINK. The reason why cmd->se_lun->lun_ref is dropped, and se_cmd->se_lun is cleared before the response hand-off back to the fabric driver is so that ../$WWN/$TPGT/lun/$LUN_ID/ shutdown can occur from configfs, without having to wait for se_cmd->cmd_kref to reach zero. Depending on initiator fabric ACKs coming back to drop the last se_cmd->cmd_kref in order to release se_lun->lun_ref once user has explicitly requested ../$WWN/$TPGT/lun/$LUN_ID doesn't work in practice for a scale-out distributed use-case. This must be able to happen asychronously from the fabric ACK. Dropping this patch. -- 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 Thu, 2017-02-09 at 03:28 -0800, Nicholas A. Bellinger wrote: > As-is, this patch breaks active I/O configfs shutdown of > /sys/kernel/config/target/$FABRIC/$WWN/$TPGT/lun/$LUN_ID/$DEV_SYMLINK. > > The reason why cmd->se_lun->lun_ref is dropped, and se_cmd->se_lun is > cleared before the response hand-off back to the fabric driver is so > that ../$WWN/$TPGT/lun/$LUN_ID/ shutdown can occur from configfs, > without having to wait for se_cmd->cmd_kref to reach zero. > > Depending on initiator fabric ACKs coming back to drop the last > se_cmd->cmd_kref in order to release se_lun->lun_ref once user has > explicitly requested ../$WWN/$TPGT/lun/$LUN_ID doesn't work in practice > for a scale-out distributed use-case. > > This must be able to happen asychronously from the fabric ACK. There can only be a delay between the last kref_put() call from the target driver and the final kref_put() if a task management function is being processed or a session is being shut down. The time needed to process any of these actions is finite so shutdown of a target driver while I/O is in progress still works. 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 8df03987bdb8..99531dc85f9c 100644 --- a/drivers/target/target_core_transport.c +++ b/drivers/target/target_core_transport.c @@ -598,11 +598,6 @@ static int transport_cmd_check_stop_to_fabric(struct se_cmd *cmd) target_remove_from_state_list(cmd); - /* - * Clear struct se_cmd->se_lun before the handoff to FE. - */ - cmd->se_lun = NULL; - spin_lock_irqsave(&cmd->t_state_lock, flags); /* * Determine if frontend context caller is requesting the stopping of @@ -629,17 +624,6 @@ static int transport_cmd_check_stop_to_fabric(struct se_cmd *cmd) : 0; } -static void transport_lun_remove_cmd(struct se_cmd *cmd) -{ - struct se_lun *lun = cmd->se_lun; - - if (!lun) - return; - - if (cmpxchg(&cmd->lun_ref_active, true, false)) - percpu_ref_put(&lun->lun_ref); -} - static void target_complete_failure_work(struct work_struct *work) { struct se_cmd *cmd = container_of(work, struct se_cmd, work); @@ -675,8 +659,6 @@ static void transport_handle_abort(struct se_cmd *cmd) { bool ack_kref = cmd->se_cmd_flags & SCF_ACK_KREF; - transport_lun_remove_cmd(cmd); - if (cmd->send_abort_response) { cmd->scsi_status = SAM_STAT_TASK_ABORTED; pr_debug("Setting SAM_STAT_TASK_ABORTED status for CDB: 0x%02x, ITT: 0x%08llx\n", @@ -1747,7 +1729,6 @@ void transport_generic_request_failure(struct se_cmd *cmd, goto queue_full; check_stop: - transport_lun_remove_cmd(cmd); transport_cmd_check_stop_to_fabric(cmd); return; @@ -2022,7 +2003,6 @@ static void transport_complete_qf(struct se_cmd *cmd) transport_handle_queue_full(cmd, cmd->se_dev); return; } - transport_lun_remove_cmd(cmd); transport_cmd_check_stop_to_fabric(cmd); } @@ -2096,7 +2076,6 @@ static void target_complete_ok_work(struct work_struct *work) if (ret == -EAGAIN || ret == -ENOMEM) goto queue_full; - transport_lun_remove_cmd(cmd); transport_cmd_check_stop_to_fabric(cmd); return; } @@ -2122,7 +2101,6 @@ static void target_complete_ok_work(struct work_struct *work) if (ret == -EAGAIN || ret == -ENOMEM) goto queue_full; - transport_lun_remove_cmd(cmd); transport_cmd_check_stop_to_fabric(cmd); return; } @@ -2147,7 +2125,6 @@ static void target_complete_ok_work(struct work_struct *work) if (ret == -EAGAIN || ret == -ENOMEM) goto queue_full; - transport_lun_remove_cmd(cmd); transport_cmd_check_stop_to_fabric(cmd); return; } @@ -2183,7 +2160,6 @@ static void target_complete_ok_work(struct work_struct *work) break; } - transport_lun_remove_cmd(cmd); transport_cmd_check_stop_to_fabric(cmd); return; @@ -2498,9 +2474,6 @@ int transport_generic_free_cmd(struct se_cmd *cmd, int wait_for_tasks) */ if (cmd->state_active) target_remove_from_state_list(cmd); - - if (cmd->se_lun) - transport_lun_remove_cmd(cmd); } /* * Since the iSCSI and iSER targets driver assume that a SCSI command @@ -2573,6 +2546,9 @@ static void target_release_cmd_kref(struct kref *kref) WARN_ON_ONCE(atomic_read(&se_cmd->tgt_ref) != 0); + if (se_cmd->lun_ref_active) + percpu_ref_put(&se_cmd->se_lun->lun_ref); + if (se_sess) { spin_lock_irqsave(&se_sess->sess_cmd_lock, flags); if (likely(!list_empty(&se_cmd->se_cmd_list))) {