Re: [PATCH v4 0/3] scsi: target: iscsi: fix stalled and doubly completed aborted WRITEs

[Bart Van Assche]

On 10/26/26 9:12 AM, Laurence Oberman wrote:
> If the guard is one-shot across both, a second call that would have
> taken the CMD_T_STOP branch is suppressed too, and I am not sure that
> is safe -- a suppressed complete_all() could leave a waiter in
> transport_wait_for_tasks() stuck. Should the guard cover only the
> CMD_T_ABORTED branch, or do you see a reason it is safe across both?

Please limit the guard to the CMD_T_ABORTED branch. The only things in
target_cmd_interrupted() that must not run twice are in that branch:
INIT_WORK() + queue_work() on cmd->work, and the
transport_complete_callback() call. For COMPARE AND WRITE the latter
releases dev->caw_sem in compare_and_write_callback(), so calling it
twice would release that semaphore twice.

The CMD_T_STOP branch doesn't need a guard. complete_all() is
idempotent and t_transport_stop_comp is initialized only once, in
__target_init_cmd(). Suppressing a repeated complete_all() wouldn't
strand a waiter either, since the completion stays done. However, a
guard shared by both branches would be wrong for another reason: if
the first call takes the CMD_T_STOP branch and CMD_T_ABORTED is set
afterwards, a shared guard would suppress queueing target_abort_work
and we would be back to the target_put_cmd_and_wait() hang.

Something like this (untested):

	static bool target_cmd_interrupted(struct se_cmd *cmd)
	{
		bool queue_abort = false, stop = false;
		unsigned long flags;
		int post_ret;

		spin_lock_irqsave(&cmd->t_state_lock, flags);
		if (cmd->transport_state & CMD_T_ABORTED) {
			if (cmd->transport_state & CMD_T_ABORT_QUEUED) {
				spin_unlock_irqrestore(&cmd->t_state_lock,
						       flags);
				return true;
			}
			cmd->transport_state |= CMD_T_ABORT_QUEUED;
			queue_abort = true;
		} else if (cmd->transport_state & CMD_T_STOP) {
			stop = true;
		}
		spin_unlock_irqrestore(&cmd->t_state_lock, flags);

		if (!queue_abort && !stop)
			return false;
		if (cmd->transport_complete_callback)
			cmd->transport_complete_callback(cmd, false,
							 &post_ret);
		if (queue_abort) {
			INIT_WORK(&cmd->work, target_abort_work);
			queue_work(target_completion_wq, &cmd->work);
		} else {
			complete_all(&cmd->t_transport_stop_comp);
		}
		return true;
	}

Notes:
- Please use spin_lock_irqsave(): target_complete_cmd() may be called by
  backend drivers from interrupt context.
- Call the callback and queue_work() after unlocking t_state_lock,
  like the current code does.
- A repeated call in the aborted case must still return true so that
  callers don't proceed with normal completion.
- Please describe in the patch description that the second call is a
  no-op by design and that fabrics that complete once are unaffected.

Thanks,

Bart.