Re: [PATCH v2 0/2] scsi: target: iscsi: fix stalled aborted WRITE commands

[Bart Van Assche <bvanassche@acm.org>]

[-- Attachment #1: Type: text/plain, Size: 190 bytes --]

On 10/25/26 7:43 AM, Laurence Oberman wrote:
> Gentle ping:
Please take a look at the attached review report. The report has been
generated by an LLM. As known LLMs may make mistakes.

Bart.

[-- Attachment #2: review.md --]
[-- Type: text/markdown, Size: 4538 bytes --]

Review https://lore.kernel.org/all/20261016175351.126373-1-loberman@redhat.com/

I've reviewed v2 of Laurence Oberman's two-patch series that fixes stalled aborted iSCSI WRITE commands, plus the Sashiko bot's review and the author's reply. I checked it against the local tree, which already contains the prerequisite commit d5869dae5080.

**Overall:** the diagnosis for patch 1 looks right, and the "claim the completion once" idea is reasonable. But the claim is only applied in some of the places that complete an aborted command. So the double-completion problem the series describes is only partly fixed, and part of that problem was introduced by d5869dae5080 itself.

## Main issues

**1. Not every completion site takes the claim, and one of the gaps is a recent regression.**
- After d5869dae5080, `__iscsit_check_dataout_hdr()` calls `target_complete_cmd()` on an aborted `TRANSPORT_WRITE_PENDING` command without taking the claim.
- When the abort is the asynchronous kind (`target_cmd_interrupted()` → `target_abort_work`), `t_state` is never changed. So if the connection drops before the command is freed, `iscsit_release_commands_from_conn()` completes it a second time.
- That second completion is the same `INIT_WORK()`/`queue_work()` on a still-queued `se_cmd->work` that the cover letter describes as the failure of the earlier attempt.
- d5869dae5080 is only in 7.3/scsi-staging so far. The fix belongs in this series, with `Fixes: d5869dae5080`, not in a later follow-up.
- `iscsit_check_dataout_payload()` → `DATAOUT_SEND_TO_TRANSPORT` → `target_execute_cmd()` has the same problem. It is older (it dates from ea87981a0ee8), but once the claim helper exists it is a two-line change. Please include it too.

**2. The author's answer to Sashiko's second point relies on the gap above.**
- He says that if `outstanding_r2ts` is read low, "the later completion in `__iscsit_check_dataout_hdr()` becomes a no-op". In v2 that path does not check the claim, so the statement is false as posted.
- The race itself is unlikely: both accesses happen on the same connection's RX thread. The only writers on other threads are the queue-full retry (`qf_work`) and the ERL2 task-reassign code.
- Once issue 1 is fixed, the reasoning holds. Until then, the reply should not be accepted as a justification.

**3. Consider a fix in target core instead.**
- The real cause is that `target_cmd_interrupted()` queues `target_abort_work` but records nothing under `t_state_lock`.
- A test-and-set flag in `transport_state` there (with a WARN on a second call) would cover every site in every fabric, including iSER and cxgbit, which also reach `iscsit_check_dataout_payload()`.
- That would be better than a private iSCSI flag that each call site has to remember to check. At the very least, the author should say why he chose not to do this.

## Smaller points

- **Missing `Fixes:` and `Cc: stable` tags** on patch 1. Candidates are ea87981a0ee8 ("scsi: target: iscsi: Handle abort for WRITE_PENDING cmds") and aaa00cc93c1d, which added the bare `return 0` in `iscsit_execute_cmd()`. Note that aaa00cc93c1d is your own 2018 TAS change.
- **Commit message for patch 2:** "Unlike patch 1 of this series…" and the paragraph about what was seen in the field should not end up in git history. Move them below the `---` line or into the cover letter.
- **Comment in `iscsi_target_core.h`:** it says `completion_claimed` is set "once the fabric has called `target_complete_cmd()`". It is actually set before the call. Reword it to "has claimed the completion".
- **Patch 2 is untested by the author's own account.** It looks correct to me, including the `iscsit_stop_dataout_timer()` call, which matters on this path. Ask for at least a fault-injection test, or a test with `MaxOutstandingR2T > 1`, before taking it.
- **Testing was on a 5.14-based distro backport.** Ask for a run on upstream (7.3-rc plus scsi-staging) as well.
- **Nit:** the unlocked `transport_state & CMD_T_ABORTED` reads in `iscsit_execute_cmd()` should use `READ_ONCE()`. This predates the series.

## Suggested verdict

Ask for a v3 that:
- sends every completion of an aborted WRITE_PENDING command through `iscsit_complete_aborted_cmd()`, including the d5869dae5080 site and `DATAOUT_SEND_TO_TRANSPORT`, or else moves the one-shot guard into target core;
- adds the `Fixes:`/stable tags;
- cleans up the commit message for patch 2.

Patch 1's core change (completing the command instead of returning when no R2T has been sent yet) is fine on its own merits.