On Wed, 2017-02-08 at 14:24 -0800, Bart Van Assche wrote:
> Due to the previous patch the write_pending_status() callback
> function is no longer called. Hence remove it.
>
> Signed-off-by: Bart Van Assche <bart.vanassche@xxxxxxxxxxx>
> Acked-by: Felipe Balbi <balbi@xxxxxx>
> Reviewed-by: Sagi Grimberg <sagig@xxxxxxxxxxxx>
> Reviewed-by: Andy Grover <agrover@xxxxxxxxxx>
> Reviewed-by: Christoph Hellwig <hch@xxxxxx>
> Reviewed-by: Hannes Reinecke <hare@xxxxxxxx>
> Reviewed-by: Bryant G. Ly <bryantly@xxxxxxxxxxxxxxxxxx>
> Cc: Himanshu Madhani <himanshu.madhani@xxxxxxxxxx>
> Cc: Quinn Tran <quinn.tran@xxxxxxxxxx>
> Cc: Saurav Kashyap <saurav.kashyap@xxxxxxxxxx>
> Cc: Michael S. Tsirkin <mst@xxxxxxxxxx>
> Cc: Juergen Gross <jgross@xxxxxxxx>
> ---
>
> diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> index 4d9180efb9aa..9a188b508973 100644
> --- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> +++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> @@ -413,26 +413,6 @@ static int tcm_qla2xxx_write_pending(struct se_cmd *se_cmd)
> return qlt_rdy_to_xfer(cmd);
> }
>
> -static int tcm_qla2xxx_write_pending_status(struct se_cmd *se_cmd)
> -{
> - unsigned long flags;
> - /*
> - * Check for WRITE_PENDING status to determine if we need to wait for
> - * CTIO aborts to be posted via hardware in tcm_qla2xxx_handle_data().
> - */
> - spin_lock_irqsave(&se_cmd->t_state_lock, flags);
> - if (se_cmd->t_state == TRANSPORT_WRITE_PENDING ||
> - se_cmd->t_state == TRANSPORT_COMPLETE_QF_WP) {
> - spin_unlock_irqrestore(&se_cmd->t_state_lock, flags);
> - wait_for_completion_timeout(&se_cmd->t_transport_stop_comp,
> - 50);
> - return 0;
> - }
> - spin_unlock_irqrestore(&se_cmd->t_state_lock, flags);
> -
> - return 0;
> -}
> -
Yeah, you can't randomly remove this code.
The whole reason this exists is to allow in-flight CTIOs for outstanding
WRITEs that have been aborted to complete, before going ahead and
completing sending status and releasing se_cmd memory.
This breaks with WRITEs + CMD_T_ABORTED in qla-target code.
> static void tcm_qla2xxx_set_default_node_attrs(struct se_node_acl *nacl)
> {
> return;
> @@ -522,14 +502,8 @@ static void tcm_qla2xxx_handle_data_work(struct work_struct *work)
>
> cmd->vha->tgt_counters.qla_core_ret_ctio++;
> if (!cmd->write_data_transferred) {
> - /*
> - * Check if se_cmd has already been aborted via LUN_RESET, and
> - * waiting upon completion in tcm_qla2xxx_write_pending_status()
> - */
> - if (cmd->se_cmd.transport_state & CMD_T_ABORTED) {
> - complete(&cmd->se_cmd.t_transport_stop_comp);
> + if (cmd->se_cmd.transport_state & CMD_T_ABORTED)
> return;
> - }
>
Likewise, the callback into tcm_qla2xxx_handle_data_work must wakeup the
se_tfo->write_pending_status() caller to tell target_core_tmr.c logic
the write transfer has completed, and it's safe to proceed with
CMD_T_ABORTED.
> diff --git a/drivers/target/iscsi/iscsi_target_configfs.c b/drivers/target/iscsi/iscsi_target_configfs.c
> index bf40f03755dd..7e112380d6d4 100644
> --- a/drivers/target/iscsi/iscsi_target_configfs.c
> +++ b/drivers/target/iscsi/iscsi_target_configfs.c
> @@ -1416,18 +1416,6 @@ static int lio_write_pending(struct se_cmd *se_cmd)
> return 0;
> }
>
> -static int lio_write_pending_status(struct se_cmd *se_cmd)
> -{
> - struct iscsi_cmd *cmd = container_of(se_cmd, struct iscsi_cmd, se_cmd);
> - int ret;
> -
> - spin_lock_bh(&cmd->istate_lock);
> - ret = !(cmd->cmd_flags & ICF_GOT_LAST_DATAOUT);
> - spin_unlock_bh(&cmd->istate_lock);
> -
> - return ret;
> -}
> -
This breaks solicited data-out WRITE handling with CMD_T_ABORTED in
iscsi-target.
iscsi-target must be allowed to complete it's solicited data-out WRITE
transfers before proceeding with CMD_T_ABORTED using delayed TAS logic.
open-iscsi expects solicited WRITE transfer to complete before getting a
SAM_STAT_TASK_ABORTED, and this will likely break other initiators as
well.
A big NACK on this.
--
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
