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

[Laurence Oberman <loberman@redhat.com>]

These three patches apply on top of commit d5869dae5080 ("scsi: target: iscsi: Fix hang for aborted WRITE_PENDING commands").

A WRITE command that is still in TRANSPORT_WRITE_PENDING has never been
passed to the backend, so when a TMR aborts it the iSCSI fabric is the one
that has to complete it. That commit fixed the case where the final
DataOut PDU for an already-issued R2T arrives after the abort. Two more
paths can leave such a command with nothing to complete it, or let it be
completed twice:

 - Patch 1 is the hang this work started from. A WRITE that arrives with
   immediate data, aborted after that data has been received but before
   the first R2T is built, is never completed by anything: no R2T has been
   sent so the initiator cannot send more data, and no DataOut timer is
   armed so the ERL=0 timeout cannot fire either. core_tmr_lun_reset()
   then blocks for as long as the connection stays up.

 - Patches 2 and 3 fix two existing sites that complete an aborted
   WRITE_PENDING command without recording that they have done so, and
   which connection teardown can therefore complete a second time.

Patch 1 also adds a one-shot completion claim. target_complete_cmd() on a
command that already has CMD_T_ABORTED set goes into
target_cmd_interrupted(), which queues target_abort_work and returns
without setting CMD_T_COMPLETE and without changing t_state, so target
core records nothing that would stop a second completion. The command
stays in TRANSPORT_WRITE_PENDING and on conn_cmd_list until
target_abort_work reaches iscsit_aborted_task(), and
iscsit_release_commands_from_conn() completes anything it finds in
TRANSPORT_WRITE_PENDING. The claim is what makes exactly one site
complete the command.

Changes since v3:
 - Dropped the patch that completed an aborted WRITE in the
   DATAOUT_SEND_R2T branch of iscsit_check_dataout_payload(). The review
   of v3 pointed out that the adjacent DATAOUT_SEND_TO_TRANSPORT branch
   deliberately checks ICF_OOO_CMDSN and defers, and the dropped patch did
   not: it could complete and free a command still reachable by raw
   pointer from sess_ooo_cmdsn_list, which iscsit_execute_ooo_cmdsns()
   dereferences later. Simply mirroring the neighbour is not enough
   either, because a command with ICF_NON_IMMEDIATE_UNSOLICITED_DATA set
   falls through iscsit_execute_cmd() without being completed when the
   CmdSN gap is filled. That window needs more thought than this series
   should carry, and it was never observed in the field, so it will come
   back on its own.
 - Patches 1 to 3 are unchanged from v3 patches 1, 3 and 4 respectively.

v3: https://lore.kernel.org/linux-scsi/20261026124344.745124-1-loberman@redhat.com/
v2: https://lore.kernel.org/linux-scsi/20261016175351.126373-1-loberman@redhat.com/
v1: https://lore.kernel.org/linux-scsi/20261014143932.39453-1-loberman@redhat.com/

Two other findings against v3 are pre-existing rather than introduced
here, and I am reporting them separately rather than widening this series
again:

iscsit_get_immediate_data() reads cmd->sense_reason and
cmd->unsolicited_data after iscsit_sequence_cmd() returns, holding no
reference, and target_handle_abort() drops the final reference for an
aborted command. Patch 1 adds a path where that can happen, but so does
the branch directly above it: iscsit_handle_immediate_data() sets
ICF_GOT_LAST_DATAOUT when all of the command's data arrives as immediate
data, and iscsit_execute_cmd() then calls target_execute_cmd() from the
same caller with the same abort states. Patch 1 adds a third path with a
property two existing paths already have.

In patch 3, CMD_T_ABORTED can be set between the check and
target_cmd_interrupted() inside target_execute_cmd(), leaving the
completion unclaimed. That is also what happens today for every aborted
command on that path; the patch narrows the window rather than closing it.

A correction to something I said during the v2 review, in case it is
relied on: I wrote that if the outstanding_r2ts read in the dropped patch
was stale low, "the later completion in __iscsit_check_dataout_hdr()
becomes a no-op". That was wrong -- before patch 2 of this series that
site called target_complete_cmd() directly and never consulted the claim.

On moving the one-shot guard into target core:

This will be explored later on but is not in scope for this series; it
changes the path every fabric completes through and I have no way to test
easily in iSER, FC or SRP right now.

How this was found and tested:

The hang was reported against an Oracle RAC cluster on two-session iSCSI
LIO targets, where one node's LUN_RESET aborts WRITEs belonging to the
other node's session. It was not reproducible in a lab, so the diagnosis
came from instrumented kernels in the reporter's environment, logging
every site that completes or waits on an aborted command.

Across two hosts every stall had the identical signature given in patch 1,
and the stall only ever ended when the initiator gave up and dropped the
connection. 217 of 222 LUN_RESET drains completed in under a second; the
rest took 46 to 54 seconds, which is where the hung-task panics came from.
One 53.7 second stall recovered on its own and the host ran for a further
19 hours.

An earlier attempt at this fix completed the command from
core_tmr_drain_state_list() instead. That is not safe: at that point the
initiator may still be sending DataOut for the command, and the result was
a double completion -- re-queued se_cmd->work, list_del corruption and a
refcount underflow. Mentioning it in case the same idea comes up in
review.

What is and is not tested:

Patch 1, backported to a 5.14-based distro kernel, has run 300 LUN_RESET
cycles with no hang, in two independent runs: one instrumented and one
with the tracing removed. The same reproducer previously failed at 95 and
196 cycles. The instrumented kernels also carried the dropped
DATAOUT_SEND_R2T patch, and recorded that its site fired zero times in
either run, so removing it does not change anything those runs exercised.

Patches 2 and 3 are closed by inspection. Patch 2's site did run during
those tests, but by way of the unclaimed target_complete_cmd() it
replaces; when the claim succeeds the two are the same two calls in the
same order, so the tested behaviour is preserved. Patch 3 leaves the
non-aborted path through target_execute_cmd() untouched.

The reporter's environment is a production cluster and is the only place
this has ever reproduced, so I cannot offer an upstream -rc run from
there. A build of this series is going to them for a further 300-cycle run
and I will report back. If that is a blocker for patches 2 and 3, I would
rather you took patch 1 on its own than held the series.

Laurence Oberman (3):
  scsi: target: iscsi: Complete an aborted WRITE stuck before the first
    R2T
  scsi: target: iscsi: Claim the completion in
    __iscsit_check_dataout_hdr()
  scsi: target: iscsi: Claim the completion on the DataOut submit path

 drivers/target/iscsi/iscsi_target.c      | 28 +++++++++++----
 drivers/target/iscsi/iscsi_target_erl1.c | 10 +++++-
 drivers/target/iscsi/iscsi_target_util.c | 46 ++++++++++++++++++++++++
 drivers/target/iscsi/iscsi_target_util.h |  2 ++
 include/target/iscsi/iscsi_target_core.h |  6 ++++
 5 files changed, 85 insertions(+), 7 deletions(-)

-- 
2.43.0