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