On Tue, 2015-05-19 at 16:30 +0200, Bart Van Assche wrote:
> Instead of invoking transport_lun_remove_cmd() before
> transport_cmd_check_stop_to_fabric(), inline that function in
> transport_release_cmd_kref(). This makes it easier to verify
> that LUN removal occurs before a command is released.
>
> Signed-off-by: Bart Van Assche <bart.vanassche@xxxxxxxxxxx>
> ---
This breaks active I/O LUN shutdown in a couple of different ways.
It's expected the se_lun reference is dropped from the descriptor before
it's handed back to fabric, because there's no guarantee that the
descriptor will be released in a reasonable about of time to drop the
se_lun reference after-the-fact, in order to allow LUN shutdown to
complete.
Also, se_cmd can fail early which means transport_generic_free_cmd()
still needs to drop the outstanding reference.
That said, I don't think this patch makes sense.
--nab
> drivers/target/target_core_device.c | 3 +++
> drivers/target/target_core_transport.c | 39 +++++++---------------------------
> 2 files changed, 11 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
> index 88dad15..c803071 100644
> --- a/drivers/target/target_core_device.c
> +++ b/drivers/target/target_core_device.c
> @@ -63,6 +63,9 @@ transport_lookup_cmd_lun(struct se_cmd *se_cmd, u32 unpacked_lun)
> struct se_device *dev;
> struct se_dev_entry *deve;
>
> + BUG_ON(se_cmd->lun_ref_active);
> + BUG_ON(list_empty(&se_cmd->se_cmd_list));
> +
> if (unpacked_lun >= TRANSPORT_MAX_LUNS_PER_TPG)
> return TCM_NON_EXISTENT_LUN;
>
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 0de29d8..675123e 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -579,15 +579,9 @@ static int transport_cmd_check_stop(struct se_cmd *cmd, bool remove_from_lists,
> if (write_pending)
> cmd->t_state = TRANSPORT_WRITE_PENDING;
>
> - if (remove_from_lists) {
> + if (remove_from_lists)
> target_remove_from_state_list(cmd);
>
> - /*
> - * Clear struct se_cmd->se_lun before the handoff to FE.
> - */
> - cmd->se_lun = NULL;
> - }
> -
> /*
> * Determine if frontend context caller is requesting the stopping of
> * this command for frontend exceptions.
> @@ -628,21 +622,8 @@ static int transport_cmd_check_stop_to_fabric(struct se_cmd *cmd)
> return transport_cmd_check_stop(cmd, true, false);
> }
>
> -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);
> -}
> -
> void transport_cmd_finish_abort(struct se_cmd *cmd, int remove)
> {
> - if (cmd->se_cmd_flags & SCF_SE_LUN_CMD)
> - transport_lun_remove_cmd(cmd);
> /*
> * Allow the fabric driver to unmap any resources before
> * releasing the descriptor via TFO->release_cmd()
> @@ -1172,6 +1153,8 @@ void transport_init_se_cmd(
>
> cmd->se_tfo = tfo;
> cmd->se_sess = se_sess;
> + cmd->se_lun = NULL;
> + cmd->lun_ref_active = false;
> cmd->data_length = data_length;
> cmd->data_direction = data_direction;
> cmd->sam_task_attr = task_attr;
> @@ -1720,7 +1703,6 @@ void transport_generic_request_failure(struct se_cmd *cmd,
> goto queue_full;
>
> check_stop:
> - transport_lun_remove_cmd(cmd);
> if (!transport_cmd_check_stop_to_fabric(cmd))
> ;
> return;
> @@ -1972,7 +1954,6 @@ out:
> transport_handle_queue_full(cmd, cmd->se_dev);
> return;
> }
> - transport_lun_remove_cmd(cmd);
> transport_cmd_check_stop_to_fabric(cmd);
> }
>
> @@ -2047,7 +2028,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;
> }
> @@ -2071,7 +2051,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;
> }
> @@ -2097,7 +2076,6 @@ queue_rsp:
> if (ret == -EAGAIN || ret == -ENOMEM)
> goto queue_full;
>
> - transport_lun_remove_cmd(cmd);
> transport_cmd_check_stop_to_fabric(cmd);
> return;
> }
> @@ -2140,7 +2118,6 @@ queue_rsp:
> break;
> }
>
> - transport_lun_remove_cmd(cmd);
> transport_cmd_check_stop_to_fabric(cmd);
> return;
>
> @@ -2464,9 +2441,6 @@ int transport_generic_free_cmd(struct se_cmd *cmd, int wait_for_tasks)
> spin_unlock_irqrestore(&cmd->t_state_lock, flags);
> }
>
> - if (cmd->se_lun)
> - transport_lun_remove_cmd(cmd);
> -
> ret = transport_put_cmd(cmd);
> }
> return ret;
> @@ -2512,12 +2486,17 @@ static void target_release_cmd_kref(struct kref *kref)
> {
> struct se_cmd *se_cmd = container_of(kref, struct se_cmd, cmd_kref);
> struct se_session *se_sess = se_cmd->se_sess;
> + struct se_lun *lun = se_cmd->se_lun;
>
> if (list_empty(&se_cmd->se_cmd_list)) {
> spin_unlock(&se_sess->sess_cmd_lock);
> se_cmd->se_tfo->release_cmd(se_cmd);
> return;
> }
> +
> + if (lun && se_cmd->lun_ref_active)
> + percpu_ref_put(&lun->lun_ref);
> +
> if (se_sess->sess_tearing_down && se_cmd->cmd_wait_set) {
> spin_unlock(&se_sess->sess_cmd_lock);
> complete(&se_cmd->cmd_wait_comp);
> @@ -3010,8 +2989,6 @@ void transport_send_task_abort(struct se_cmd *cmd)
> }
> cmd->scsi_status = SAM_STAT_TASK_ABORTED;
>
> - transport_lun_remove_cmd(cmd);
> -
> pr_debug("Setting SAM_STAT_TASK_ABORTED status for CDB: 0x%02x, ITT: 0x%08llx\n",
> cmd->t_task_cdb[0], cmd->tag);
>
--
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
