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

[Laurence Oberman <loberman@redhat.com>]

On Mon, 2026-10-26 at 09:47 -0700, Bart Van Assche wrote:
> 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.

Hello Bart, 
You always take the time to help me and I appreciate it very much.
I am following up shortly with a V5 that I think addresses your
concerns and also limits this to only two patches. 
I am back-porting to RHEL9 and will get it tested the same way there
too but I expect it will be fine.

Thank you
Laurence