Target-devel archive on lore.kernel.org
 help / color / Atom feed
From: Michael Christie <michael.christie@oracle.com>
To: Maurizio Lombardi <mlombard@redhat.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>,
	linux-scsi@vger.kernel.org, target-devel@vger.kernel.org,
	bvanassche@acm.org, m.lombardi85@gmail.com
Subject: Re: [PATCH 2/2] target: iscsi: fix a race condition when aborting a task
Date: Wed, 11 Nov 2020 15:37:51 +0000
Message-ID: <5D26782D-1249-4A2C-8BF9-7176D5B85F55@oracle.com> (raw)
In-Reply-To: <5111dcb0-ef0d-fc11-ee1a-ae2a9b30150a@redhat.com>



> On Nov 11, 2020, at 8:58 AM, Maurizio Lombardi <mlombard@redhat.com> wrote:
> 
> 
> 
> Dne 11. 11. 20 v 3:16 Mike Christie napsal(a):
>> Hey, I tested this out and I do not think this will happen. We will get stuck waiting on the TMF completion for the affected cmd/cmds.
>> 
>> In conn_cmd_list we would have [CMD1 -> ABORT TMF]. Those cmds get moved to the tmp list. It might happen where CMD1's CMD_T_ABORTED bit is set, and iscsit_release_commands_from_conn will would put it back onto the conn_cmd_list. But then it will see the ABORT on the list. We will then wait on the ABORT in:
>> 
>> iscsit_release_commands_from_conn -> iscsit_free_cmd -> transport_generic_free_cmd.
> 
> Hi Mike,
> 
> I'm not sure if I understood this part.
> 
> The commands are moved to the tmp_list;
> we check for CMD_T_ABORTED and eventually move the commands from tmp_list back to conn_cmd_list
> because it's the abort task the one that should do the cleanup.

I’m not sure what you mean here. Are you saying both CMD1’s se_cmd and the ABORT’s se_cmd will have the CMD_T_ABORTED bit set and will both go through the aborted_task callout?


> 
> iscsit_release_commands_from_conn() then scans the tmp_list and calls iscsit_free_cmd()... but not against
> those commands with CMD_T_ABORTED flag set because we just moved them back to conn_cmd_list
> and aren't linked to tmp_list anymore.
> 
> Am I missing something?


If you have a SCSI READ/WRITE se_cmd (CMD1 in my example) and a ABORT se_cmd (ABORT TMF in my example) on the conn_cmd_list, then the ABORT’s se_cmd would not have the CMD_T_ABORTED bit set, right? If so, what sets it?

If the SCSI R/W has the CMD_T_ABORTED bit set, we move it it back to the conn_cmd_list and the abort code path cleans it up. But then we still have the ABORT’s se_cmd on the tmp_list. We will then call 

transport_generic_free_cmd(wait_for_tasks=true) -> __transport_wait_for_tasks(fabric_stop=true)

And wait for the ABORT to complete, and the ABORT does not complete until the last ref on the command it’s aborting completes.

If you have a LUN RESET in the mix like:

[CMD1 -> ABORT TMF -> LUN RESET TMF]

Then CMD1 and the ABORT could have their CMD_T_ABORTED bit set. core_tmr_drain_tmr_list would call __target_check_io_state during the RESET processing. However, in this case, the LUN RESET’s se_cmd would not have the bit set, so we would end up waiting like I described for that to complete. In that case though the RESET waits for the cmds and tmfs it is cleaning up.

  reply index

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-07 14:53 [PATCH 0/2] fix race conditions with task aborts Maurizio Lombardi
2020-10-07 14:53 ` [PATCH 1/2] target: iscsi: prevent a race condition in iscsit_unmap_cmd() Maurizio Lombardi
2020-10-08  2:15   ` Bart Van Assche
2020-10-08  9:42     ` Maurizio Lombardi
2020-10-07 14:53 ` [PATCH 2/2] target: iscsi: fix a race condition when aborting a task Maurizio Lombardi
2020-10-22  2:42   ` Mike Christie
2020-10-27 13:49     ` Maurizio Lombardi
2020-10-27 17:54       ` Mike Christie
2020-10-27 20:03         ` Michael Christie
2020-10-28 17:09           ` Maurizio Lombardi
2020-10-28 20:37             ` Mike Christie
2020-11-10 21:29               ` Maurizio Lombardi
2020-11-10 23:08                 ` Mike Christie
2020-11-11  2:16             ` Mike Christie
2020-11-11 14:58               ` Maurizio Lombardi
2020-11-11 15:37                 ` Michael Christie [this message]
2020-11-11 15:48                   ` Maurizio Lombardi

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5D26782D-1249-4A2C-8BF9-7176D5B85F55@oracle.com \
    --to=michael.christie@oracle.com \
    --cc=bvanassche@acm.org \
    --cc=linux-scsi@vger.kernel.org \
    --cc=m.lombardi85@gmail.com \
    --cc=martin.petersen@oracle.com \
    --cc=mlombard@redhat.com \
    --cc=target-devel@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Target-devel archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/target-devel/0 target-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 target-devel target-devel/ https://lore.kernel.org/target-devel \
		target-devel@vger.kernel.org
	public-inbox-index target-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.target-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git