[PATCH 14/14] target: Fix handling of removed LUNs

[PATCH 14/14] target: Fix handling of removed LUNs

[Bart Van Assche]

Send a valid ASC / ASCQ combination back to the initiator if a SCSI
command is received after a LUN has been removed. This patch fixes
the following call trace:

WARNING: CPU: 0 PID: 4 at drivers/target/target_core_transport.c:3131 translate_sense_reason+0x164/0x190 [target_core_mod]
Workqueue: ib-comp-wq ib_cq_poll_work [ib_core]
RIP: 0010:translate_sense_reason+0x164/0x190 [target_core_mod]
Call Trace:
transport_send_check_condition_and_sense+0x95/0x1c0 [target_core_mod]
transport_generic_request_failure+0x102/0x270 [target_core_mod]
transport_generic_new_cmd+0x138/0x340 [target_core_mod]
transport_handle_cdb_direct+0x2f/0x80 [target_core_mod]
target_submit_cmd_map_sgls+0x212/0x2a0 [target_core_mod]
srpt_handle_new_iu+0x244/0x680 [ib_srpt]
__ib_process_cq+0x6d/0xc0 [ib_core]
ib_cq_poll_work+0x18/0x50 [ib_core]
process_one_work+0x20b/0x6a0
worker_thread+0x35/0x380
kthread+0x117/0x130
ret_from_fork+0x24/0x30

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Mike Christie <mchristi@redhat.com>
---
 drivers/target/target_core_ua.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/target/target_core_ua.c b/drivers/target/target_core_ua.c
index be25eb807a5f..08c1e550babd 100644
--- a/drivers/target/target_core_ua.c
+++ b/drivers/target/target_core_ua.c
@@ -218,17 +218,15 @@ void core_scsi3_ua_for_check_condition(
 		return;
 
 	nacl = sess->se_node_acl;
-	if (!nacl)
+	if (WARN_ON_ONCE(!nacl))
 		return;
 
 	rcu_read_lock();
 	deve = target_nacl_find_deve(nacl, cmd->orig_fe_lun);
-	if (!deve) {
-		rcu_read_unlock();
-		return;
-	}
-	if (!atomic_read(&deve->ua_count)) {
+	if (!deve || !atomic_read(&deve->ua_count)) {
 		rcu_read_unlock();
+		*asc = 0x25;
+		*ascq = 0;
 		return;
 	}
 	/*
-- 
2.16.2

Re: [PATCH 14/14] target: Fix handling of removed LUNs

[Mike Christie]

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

On 03/01/2018 04:26 PM, Bart Van Assche wrote:
> Send a valid ASC / ASCQ combination back to the initiator if a SCSI
> command is received after a LUN has been removed. This patch fixes
> the following call trace:
> 
> WARNING: CPU: 0 PID: 4 at drivers/target/target_core_transport.c:3131 translate_sense_reason+0x164/0x190 [target_core_mod]
> Workqueue: ib-comp-wq ib_cq_poll_work [ib_core]
> RIP: 0010:translate_sense_reason+0x164/0x190 [target_core_mod]
> Call Trace:
> transport_send_check_condition_and_sense+0x95/0x1c0 [target_core_mod]
> transport_generic_request_failure+0x102/0x270 [target_core_mod]
> transport_generic_new_cmd+0x138/0x340 [target_core_mod]
> transport_handle_cdb_direct+0x2f/0x80 [target_core_mod]
> target_submit_cmd_map_sgls+0x212/0x2a0 [target_core_mod]
> srpt_handle_new_iu+0x244/0x680 [ib_srpt]
> __ib_process_cq+0x6d/0xc0 [ib_core]
> ib_cq_poll_work+0x18/0x50 [ib_core]
> process_one_work+0x20b/0x6a0
> worker_thread+0x35/0x380
> kthread+0x117/0x130
> ret_from_fork+0x24/0x30
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Mike Christie <mchristi@redhat.com>
> ---
>  drivers/target/target_core_ua.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/target/target_core_ua.c b/drivers/target/target_core_ua.c
> index be25eb807a5f..08c1e550babd 100644
> --- a/drivers/target/target_core_ua.c
> +++ b/drivers/target/target_core_ua.c
> @@ -218,17 +218,15 @@ void core_scsi3_ua_for_check_condition(
>  		return;
>  
>  	nacl = sess->se_node_acl;
> -	if (!nacl)
> +	if (WARN_ON_ONCE(!nacl))
>  		return;
>  
>  	rcu_read_lock();
>  	deve = target_nacl_find_deve(nacl, cmd->orig_fe_lun);
> -	if (!deve) {
> -		rcu_read_unlock();
> -		return;
> -	}
> -	if (!atomic_read(&deve->ua_count)) {
> +	if (!deve || !atomic_read(&deve->ua_count)) {
>  		rcu_read_unlock();
> +		*asc = 0x25;
> +		*ascq = 0;
> 

Hey Bart, I figured out how to hit this. If you just queue up a UA doing
something like changing the ALUA state, then unmap the LUN while IO is
running you will hit it. You do need a little luck for the timing,
because you have to unmap the LUN right after target_scsi3_ua_check does
its target_nacl_find_deve call so it is still in the list there but not
when you call core_scsi3_ua_for_check_condition.

I was in the middle of fixing up the sense key value issue in the patch
like we discussed before (use illegal request instead of unit
attention), but it looks like there was another bug. If we have 2
commands going to the same device and they run target_scsi3_ua_check and
see ua_count=1 TCM_CHECK_CONDITION_UNIT_ATTENTION is returned for both.
They then call core_scsi3_ua_for_check_condition and one of them
deallocates a UA dropping ua_count=0. The second command then runs the
!atomic_read(&deve->ua_count) check and sees the other command has
decremented it. Or the second one might have passed the ua_count check
in core_scsi3_ua_for_check_condition and it runs the loop but nothing is
found since the first command has already removed it. We then return
bogus asc/ascq values.

In the attached patch I just return busy status for this race case. It
seemed easier than trying to add more locking and error handling.

Some notes/questions on some of the code the patch touched though.

If translate_sense_reason failed due to a short buffer it returned
-EINVAL. transport_send_check_condition_and_sense would then end up
calling transport_handle_queue_full/transport_complete_qf and that would
just end up failing the command with
TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE. Do you know if that long journey
intended or just an accident?

In this patch I fixed that so it just translated the error to
TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE immediately. If the patch is ok
and we meant to return that error code for the short buffer then I can
break that out into another patch.

[-- Attachment #2: ua-error-handling.patch --]
[-- Type: text/x-patch, Size: 5750 bytes --]

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 3f7ea71..7002247 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1822,7 +1822,10 @@ void transport_generic_request_failure(struct se_cmd *cmd,
 	}
 
 	ret = transport_send_check_condition_and_sense(cmd, sense_reason, 0);
-	if (ret)
+	if (ret == -EBUSY) {
+		cmd->scsi_status = SAM_STAT_BUSY;
+		goto queue_status;
+	} else if (ret)
 		goto queue_full;
 
 check_stop:
@@ -3149,10 +3152,13 @@ static int translate_sense_reason(struct se_cmd *cmd, sense_reason_t reason)
 {
 	const struct sense_info *si;
 	u8 *buffer = cmd->sense_buffer;
-	int r = (__force int)reason;
+	int r;
 	u8 asc, ascq;
 	bool desc_format = target_sense_desc_format(cmd->se_dev);
 
+lookup_reason:
+	r = (__force int)reason;
+
 	if (r < ARRAY_SIZE(sense_info_table) && sense_info_table[r].key)
 		si = &sense_info_table[r];
 	else
@@ -3160,7 +3166,13 @@ static int translate_sense_reason(struct se_cmd *cmd, sense_reason_t reason)
 				       TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE];
 
 	if (reason == TCM_CHECK_CONDITION_UNIT_ATTENTION) {
-		core_scsi3_ua_for_check_condition(cmd, &asc, &ascq);
+		reason = core_scsi3_ua_for_check_condition(cmd, &asc, &ascq);
+		if (reason == TCM_LUN_BUSY) {
+			return -EBUSY;
+		} else if (reason != TCM_CHECK_CONDITION_UNIT_ATTENTION) {
+			goto lookup_reason;
+		}
+
 		WARN_ON_ONCE(asc == 0);
 	} else if (si->asc == 0) {
 		WARN_ON_ONCE(cmd->scsi_asc == 0);
@@ -3172,11 +3184,13 @@ static int translate_sense_reason(struct se_cmd *cmd, sense_reason_t reason)
 	}
 
 	scsi_build_sense_buffer(desc_format, buffer, si->key, asc, ascq);
-	if (si->add_sector_info)
-		return scsi_set_sense_information(buffer,
-						  cmd->scsi_sense_length,
-						  cmd->bad_sector);
-
+	if (si->add_sector_info) {
+		if (scsi_set_sense_information(buffer, cmd->scsi_sense_length,
+					       cmd->bad_sector)) {
+			reason = TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
+			goto lookup_reason;
+		}
+	}
 	return 0;
 }
 
@@ -3197,12 +3211,16 @@ static int translate_sense_reason(struct se_cmd *cmd, sense_reason_t reason)
 	if (!from_transport) {
 		int rc;
 
+		rc = translate_sense_reason(cmd, reason);
+		if (rc) {
+			spin_lock_irqsave(&cmd->t_state_lock, flags);
+			cmd->se_cmd_flags &= ~SCF_SENT_CHECK_CONDITION;
+			spin_unlock_irqrestore(&cmd->t_state_lock, flags);
+			return rc;
+		}
 		cmd->se_cmd_flags |= SCF_EMULATED_TASK_SENSE;
 		cmd->scsi_status = SAM_STAT_CHECK_CONDITION;
 		cmd->scsi_sense_length  = TRANSPORT_SENSE_BUFFER;
-		rc = translate_sense_reason(cmd, reason);
-		if (rc)
-			return rc;
 	}
 
 	trace_target_cmd_complete(cmd);
diff --git a/drivers/target/target_core_ua.c b/drivers/target/target_core_ua.c
index be25eb8..2d43d9a 100644
--- a/drivers/target/target_core_ua.c
+++ b/drivers/target/target_core_ua.c
@@ -202,7 +202,7 @@ void core_scsi3_ua_release_all(
 	spin_unlock(&deve->ua_lock);
 }
 
-void core_scsi3_ua_for_check_condition(
+sense_reason_t core_scsi3_ua_for_check_condition(
 	struct se_cmd *cmd,
 	u8 *asc,
 	u8 *ascq)
@@ -213,24 +213,27 @@ void core_scsi3_ua_for_check_condition(
 	struct se_node_acl *nacl;
 	struct se_ua *ua = NULL, *ua_p;
 	int head = 1;
+	bool found = false;
 
 	if (!sess)
-		return;
+		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
 
 	nacl = sess->se_node_acl;
-	if (!nacl)
-		return;
+	if (WARN_ON_ONCE(!nacl))
+		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
 
 	rcu_read_lock();
 	deve = target_nacl_find_deve(nacl, cmd->orig_fe_lun);
 	if (!deve) {
 		rcu_read_unlock();
-		return;
+		return TCM_NON_EXISTENT_LUN;
 	}
+
 	if (!atomic_read(&deve->ua_count)) {
 		rcu_read_unlock();
-		return;
+		goto ua_not_found;
 	}
+
 	/*
 	 * The highest priority Unit Attentions are placed at the head of the
 	 * struct se_dev_entry->ua_list, and will be returned in CHECK_CONDITION +
@@ -246,6 +249,7 @@ void core_scsi3_ua_for_check_condition(
 		if (dev->dev_attrib.emulate_ua_intlck_ctrl != 0) {
 			*asc = ua->ua_asc;
 			*ascq = ua->ua_ascq;
+			found = true;
 			break;
 		}
 		/*
@@ -257,6 +261,7 @@ void core_scsi3_ua_for_check_condition(
 			*asc = ua->ua_asc;
 			*ascq = ua->ua_ascq;
 			head = 0;
+			found = true;
 		}
 		list_del(&ua->ua_nacl_list);
 		kmem_cache_free(se_ua_cache, ua);
@@ -266,6 +271,15 @@ void core_scsi3_ua_for_check_condition(
 	spin_unlock(&deve->ua_lock);
 	rcu_read_unlock();
 
+	if (!found) {
+ua_not_found:
+		/*
+		 * Multiple commands might have raced and hit the ua_count>0
+		 * check in target_scsi3_ua_check.
+		 */
+		return TCM_LUN_BUSY;
+	}
+
 	pr_debug("[%s]: %s UNIT ATTENTION condition with"
 		" INTLCK_CTRL: %d, mapped LUN: %llu, got CDB: 0x%02x"
 		" reported ASC: 0x%02x, ASCQ: 0x%02x\n",
@@ -273,6 +287,7 @@ void core_scsi3_ua_for_check_condition(
 		(dev->dev_attrib.emulate_ua_intlck_ctrl != 0) ? "Reporting" :
 		"Releasing", dev->dev_attrib.emulate_ua_intlck_ctrl,
 		cmd->orig_fe_lun, cmd->t_task_cdb[0], *asc, *ascq);
+	return TCM_CHECK_CONDITION_UNIT_ATTENTION;
 }
 
 int core_scsi3_ua_clear_for_request_sense(
diff --git a/drivers/target/target_core_ua.h b/drivers/target/target_core_ua.h
index 56bf34a..8a135e8 100644
--- a/drivers/target/target_core_ua.h
+++ b/drivers/target/target_core_ua.h
@@ -38,7 +38,8 @@
 extern int core_scsi3_ua_allocate(struct se_dev_entry *, u8, u8);
 extern void target_ua_allocate_lun(struct se_node_acl *, u32, u8, u8);
 extern void core_scsi3_ua_release_all(struct se_dev_entry *);
-extern void core_scsi3_ua_for_check_condition(struct se_cmd *, u8 *, u8 *);
+extern sense_reason_t core_scsi3_ua_for_check_condition(struct se_cmd *, u8 *,
+							u8 *);
 extern int core_scsi3_ua_clear_for_request_sense(struct se_cmd *,
 						u8 *, u8 *);
 

Re: [PATCH 14/14] target: Fix handling of removed LUNs

[Bart Van Assche]

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

On Wed, 2018-06-20 at 20:32 -0500, Mike Christie wrote:
> I was in the middle of fixing up the sense key value issue in the patch
> like we discussed before (use illegal request instead of unit
> attention), but it looks like there was another bug. If we have 2
> commands going to the same device and they run target_scsi3_ua_check and
> see ua_count=1 TCM_CHECK_CONDITION_UNIT_ATTENTION is returned for both.
> They then call core_scsi3_ua_for_check_condition and one of them
> deallocates a UA dropping ua_count=0. The second command then runs the
> !atomic_read(&deve->ua_count) check and sees the other command has
> decremented it. Or the second one might have passed the ua_count check
> in core_scsi3_ua_for_check_condition and it runs the loop but nothing is
> found since the first command has already removed it. We then return
> bogus asc/ascq values.
> 
> In the attached patch I just return busy status for this race case. It
> seemed easier than trying to add more locking and error handling.
> 
> Some notes/questions on some of the code the patch touched though.
> 
> If translate_sense_reason failed due to a short buffer it returned
> -EINVAL. transport_send_check_condition_and_sense would then end up
> calling transport_handle_queue_full/transport_complete_qf and that would
> just end up failing the command with
> TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE. Do you know if that long journey
> intended or just an accident?
> 
> In this patch I fixed that so it just translated the error to
> TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE immediately. If the patch is ok
> and we meant to return that error code for the short buffer then I can
> break that out into another patch.

Hello Mike,

That's an interesting observation. However, I think that the race described
above can be fixed with fewer code changes. Can you have a look at the three
attached (untested) patches?

Regarding translate_sense_reason() failing due to a short buffer: I don't
think that the current behavior in case of a short sense buffer is correct.
It's probably better to return a sense buffer that is incomplete and with
correct KEY / ASC / ASCQ values instead of modifying these values. How
about changing the code at the end of translate_sense_reason() as follows?

	if (si->add_sector_info)
		WARN_ON_ONCE(scsi_set_sense_information(buffer,
						  cmd->scsi_sense_length,
						  cmd->bad_sector) < 0);

	return 0;

Thanks,

Bart.



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-target-Do-not-duplicate-the-code-that-marks-that-a-c.patch --]
[-- Type: text/x-patch; name="0001-target-Do-not-duplicate-the-code-that-marks-that-a-c.patch", Size: 1939 bytes --]

From ab96515f4f53001462864dc6b71f0058ea6c99a8 Mon Sep 17 00:00:00 2001
From: Bart Van Assche <bart.vanassche@wdc.com>
Date: Thu, 21 Jun 2018 10:40:57 -0700
Subject: [PATCH 1/3] target: Do not duplicate the code that marks that a
 command has sense data

This patch does not change any functionality.

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Mike Christie <mchristi@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.com>
---
 drivers/target/target_core_transport.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 3f7ea7186d79..c7353f4218d1 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -2091,9 +2091,6 @@ static void transport_complete_qf(struct se_cmd *cmd)
 		if (cmd->scsi_status)
 			goto queue_status;
 
-		cmd->se_cmd_flags |= SCF_EMULATED_TASK_SENSE;
-		cmd->scsi_status = SAM_STAT_CHECK_CONDITION;
-		cmd->scsi_sense_length  = TRANSPORT_SENSE_BUFFER;
 		translate_sense_reason(cmd, TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE);
 		goto queue_status;
 	}
@@ -3171,6 +3168,9 @@ static int translate_sense_reason(struct se_cmd *cmd, sense_reason_t reason)
 		ascq = si->ascq;
 	}
 
+	cmd->se_cmd_flags |= SCF_EMULATED_TASK_SENSE;
+	cmd->scsi_status = SAM_STAT_CHECK_CONDITION;
+	cmd->scsi_sense_length  = TRANSPORT_SENSE_BUFFER;
 	scsi_build_sense_buffer(desc_format, buffer, si->key, asc, ascq);
 	if (si->add_sector_info)
 		return scsi_set_sense_information(buffer,
@@ -3197,9 +3197,6 @@ transport_send_check_condition_and_sense(struct se_cmd *cmd,
 	if (!from_transport) {
 		int rc;
 
-		cmd->se_cmd_flags |= SCF_EMULATED_TASK_SENSE;
-		cmd->scsi_status = SAM_STAT_CHECK_CONDITION;
-		cmd->scsi_sense_length  = TRANSPORT_SENSE_BUFFER;
 		rc = translate_sense_reason(cmd, reason);
 		if (rc)
 			return rc;
-- 
2.17.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-target-Fix-handling-of-removed-LUNs.patch --]
[-- Type: text/x-patch; name="0002-target-Fix-handling-of-removed-LUNs.patch", Size: 6474 bytes --]

From 4ac09224a732f862a6bae7a820373fbdf30716a0 Mon Sep 17 00:00:00 2001
From: Bart Van Assche <bart.vanassche@wdc.com>
Date: Fri, 23 Feb 2018 15:47:53 -0800
Subject: [PATCH 2/3] target: Fix handling of removed LUNs

Send a valid ASC / ASCQ combination back to the initiator if a SCSI
command is received after a LUN has been removed. This patch fixes
the following call trace:

WARNING: CPU: 0 PID: 4 at drivers/target/target_core_transport.c:3131 translate_sense_reason+0x164/0x190 [target_core_mod]
Workqueue: ib-comp-wq ib_cq_poll_work [ib_core]
RIP: 0010:translate_sense_reason+0x164/0x190 [target_core_mod]
Call Trace:
transport_send_check_condition_and_sense+0x95/0x1c0 [target_core_mod]
transport_generic_request_failure+0x102/0x270 [target_core_mod]
transport_generic_new_cmd+0x138/0x340 [target_core_mod]
transport_handle_cdb_direct+0x2f/0x80 [target_core_mod]
target_submit_cmd_map_sgls+0x212/0x2a0 [target_core_mod]
srpt_handle_new_iu+0x244/0x680 [ib_srpt]
__ib_process_cq+0x6d/0xc0 [ib_core]
ib_cq_poll_work+0x18/0x50 [ib_core]
process_one_work+0x20b/0x6a0
worker_thread+0x35/0x380
kthread+0x117/0x130
ret_from_fork+0x24/0x30

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Mike Christie <mchristi@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.com>
---
 drivers/target/target_core_transport.c | 23 +++++++++++++++----
 drivers/target/target_core_ua.c        | 31 +++++++++++++++-----------
 drivers/target/target_core_ua.h        |  3 ++-
 3 files changed, 39 insertions(+), 18 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index c7353f4218d1..0b4eb446becb 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -3142,12 +3142,23 @@ static const struct sense_info sense_info_table[] = {
 	},
 };
 
+/**
+ * translate_sense_reason - translate a sense reason into T10 key, asc and ascq
+ * @cmd: SCSI command in which the resulting sense buffer or SCSI status will
+ *   be stored.
+ * @reason: LIO sense reason code. If this argument has the value
+ *   TCM_CHECK_CONDITION_UNIT_ATTENTION, try to dequeue a unit attention. If
+ *   dequeuing a unit attention fails due to multiple commands being processed
+ *   concurrently, set the command status to BUSY.
+ *
+ * Return: 0 upon success or -EINVAL if the sense buffer is too small.
+ */
 static int translate_sense_reason(struct se_cmd *cmd, sense_reason_t reason)
 {
 	const struct sense_info *si;
 	u8 *buffer = cmd->sense_buffer;
 	int r = (__force int)reason;
-	u8 asc, ascq;
+	u8 key, asc, ascq;
 	bool desc_format = target_sense_desc_format(cmd->se_dev);
 
 	if (r < ARRAY_SIZE(sense_info_table) && sense_info_table[r].key)
@@ -3156,9 +3167,13 @@ static int translate_sense_reason(struct se_cmd *cmd, sense_reason_t reason)
 		si = &sense_info_table[(__force int)
 				       TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE];
 
+	key = si->key;
 	if (reason == TCM_CHECK_CONDITION_UNIT_ATTENTION) {
-		core_scsi3_ua_for_check_condition(cmd, &asc, &ascq);
-		WARN_ON_ONCE(asc == 0);
+		if (!core_scsi3_ua_for_check_condition(cmd, &key, &asc,
+						       &ascq)) {
+			cmd->scsi_status = SAM_STAT_BUSY;
+			return 0;
+		}
 	} else if (si->asc == 0) {
 		WARN_ON_ONCE(cmd->scsi_asc == 0);
 		asc = cmd->scsi_asc;
@@ -3171,7 +3186,7 @@ static int translate_sense_reason(struct se_cmd *cmd, sense_reason_t reason)
 	cmd->se_cmd_flags |= SCF_EMULATED_TASK_SENSE;
 	cmd->scsi_status = SAM_STAT_CHECK_CONDITION;
 	cmd->scsi_sense_length  = TRANSPORT_SENSE_BUFFER;
-	scsi_build_sense_buffer(desc_format, buffer, si->key, asc, ascq);
+	scsi_build_sense_buffer(desc_format, buffer, key, asc, ascq);
 	if (si->add_sector_info)
 		return scsi_set_sense_information(buffer,
 						  cmd->scsi_sense_length,
diff --git a/drivers/target/target_core_ua.c b/drivers/target/target_core_ua.c
index be25eb807a5f..d2fe3d98c335 100644
--- a/drivers/target/target_core_ua.c
+++ b/drivers/target/target_core_ua.c
@@ -202,10 +202,13 @@ void core_scsi3_ua_release_all(
 	spin_unlock(&deve->ua_lock);
 }
 
-void core_scsi3_ua_for_check_condition(
-	struct se_cmd *cmd,
-	u8 *asc,
-	u8 *ascq)
+/*
+ * Dequeue a unit attention from the unit attention list. This function
+ * returns true if the dequeuing succeeded and if *@key, *@asc and *@ascq have
+ * been set.
+ */
+bool core_scsi3_ua_for_check_condition(struct se_cmd *cmd, u8 *key, u8 *asc,
+				       u8 *ascq)
 {
 	struct se_device *dev = cmd->se_dev;
 	struct se_dev_entry *deve;
@@ -214,22 +217,21 @@ void core_scsi3_ua_for_check_condition(
 	struct se_ua *ua = NULL, *ua_p;
 	int head = 1;
 
-	if (!sess)
-		return;
+	if (WARN_ON_ONCE(!sess))
+		return false;
 
 	nacl = sess->se_node_acl;
-	if (!nacl)
-		return;
+	if (WARN_ON_ONCE(!nacl))
+		return false;
 
 	rcu_read_lock();
 	deve = target_nacl_find_deve(nacl, cmd->orig_fe_lun);
 	if (!deve) {
 		rcu_read_unlock();
-		return;
-	}
-	if (!atomic_read(&deve->ua_count)) {
-		rcu_read_unlock();
-		return;
+		*key = ILLEGAL_REQUEST;
+		*asc = 0x25; /* LOGICAL UNIT NOT SUPPORTED */
+		*ascq = 0;
+		return true;
 	}
 	/*
 	 * The highest priority Unit Attentions are placed at the head of the
@@ -244,6 +246,7 @@ void core_scsi3_ua_for_check_condition(
 		 * clearing it.
 		 */
 		if (dev->dev_attrib.emulate_ua_intlck_ctrl != 0) {
+			*key = UNIT_ATTENTION;
 			*asc = ua->ua_asc;
 			*ascq = ua->ua_ascq;
 			break;
@@ -273,6 +276,8 @@ void core_scsi3_ua_for_check_condition(
 		(dev->dev_attrib.emulate_ua_intlck_ctrl != 0) ? "Reporting" :
 		"Releasing", dev->dev_attrib.emulate_ua_intlck_ctrl,
 		cmd->orig_fe_lun, cmd->t_task_cdb[0], *asc, *ascq);
+
+	return head == 0;
 }
 
 int core_scsi3_ua_clear_for_request_sense(
diff --git a/drivers/target/target_core_ua.h b/drivers/target/target_core_ua.h
index b0f4205a96cd..76487c9be090 100644
--- a/drivers/target/target_core_ua.h
+++ b/drivers/target/target_core_ua.h
@@ -37,7 +37,8 @@ extern sense_reason_t target_scsi3_ua_check(struct se_cmd *);
 extern int core_scsi3_ua_allocate(struct se_dev_entry *, u8, u8);
 extern void target_ua_allocate_lun(struct se_node_acl *, u32, u8, u8);
 extern void core_scsi3_ua_release_all(struct se_dev_entry *);
-extern void core_scsi3_ua_for_check_condition(struct se_cmd *, u8 *, u8 *);
+extern bool core_scsi3_ua_for_check_condition(struct se_cmd *, u8 *, u8 *,
+					      u8 *);
 extern int core_scsi3_ua_clear_for_request_sense(struct se_cmd *,
 						u8 *, u8 *);
 
-- 
2.17.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-target-Remove-se_dev_entry.ua_count.patch --]
[-- Type: text/x-patch; name="0003-target-Remove-se_dev_entry.ua_count.patch", Size: 3585 bytes --]

From fac94854486b98dc04c89c21a4b2f24f895bbafe Mon Sep 17 00:00:00 2001
From: Bart Van Assche <bart.vanassche@wdc.com>
Date: Thu, 21 Jun 2018 10:19:05 -0700
Subject: [PATCH 3/3] target: Remove se_dev_entry.ua_count

se_dev_entry.ua_count is only used to check whether or not
se_dev_entry.ua_list is empty. Use list_empty_careful() instead.
Checking whether or not ua_list is empty without holding the
lock that protects that list is fine because the code that
dequeues from that list will check again whether or not that
list is empty.

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Mike Christie <mchristi@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.com>
---
 drivers/target/target_core_device.c |  1 -
 drivers/target/target_core_ua.c     | 12 ++----------
 include/target/target_core_base.h   |  1 -
 3 files changed, 2 insertions(+), 12 deletions(-)

diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
index e8d2a7f22382..d91cbff57680 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -336,7 +336,6 @@ int core_enable_device_list_for_node(
 		return -ENOMEM;
 	}
 
-	atomic_set(&new->ua_count, 0);
 	spin_lock_init(&new->ua_lock);
 	INIT_LIST_HEAD(&new->ua_list);
 	INIT_LIST_HEAD(&new->lun_link);
diff --git a/drivers/target/target_core_ua.c b/drivers/target/target_core_ua.c
index d2fe3d98c335..49ccac620fcf 100644
--- a/drivers/target/target_core_ua.c
+++ b/drivers/target/target_core_ua.c
@@ -55,7 +55,7 @@ target_scsi3_ua_check(struct se_cmd *cmd)
 		rcu_read_unlock();
 		return 0;
 	}
-	if (!atomic_read(&deve->ua_count)) {
+	if (list_empty_careful(&deve->ua_list)) {
 		rcu_read_unlock();
 		return 0;
 	}
@@ -154,7 +154,6 @@ int core_scsi3_ua_allocate(
 				&deve->ua_list);
 		spin_unlock(&deve->ua_lock);
 
-		atomic_inc_mb(&deve->ua_count);
 		return 0;
 	}
 	list_add_tail(&ua->ua_nacl_list, &deve->ua_list);
@@ -164,7 +163,6 @@ int core_scsi3_ua_allocate(
 		" 0x%02x, ASCQ: 0x%02x\n", deve->mapped_lun,
 		asc, ascq);
 
-	atomic_inc_mb(&deve->ua_count);
 	return 0;
 }
 
@@ -196,8 +194,6 @@ void core_scsi3_ua_release_all(
 	list_for_each_entry_safe(ua, ua_p, &deve->ua_list, ua_nacl_list) {
 		list_del(&ua->ua_nacl_list);
 		kmem_cache_free(se_ua_cache, ua);
-
-		atomic_dec_mb(&deve->ua_count);
 	}
 	spin_unlock(&deve->ua_lock);
 }
@@ -263,8 +259,6 @@ bool core_scsi3_ua_for_check_condition(struct se_cmd *cmd, u8 *key, u8 *asc,
 		}
 		list_del(&ua->ua_nacl_list);
 		kmem_cache_free(se_ua_cache, ua);
-
-		atomic_dec_mb(&deve->ua_count);
 	}
 	spin_unlock(&deve->ua_lock);
 	rcu_read_unlock();
@@ -304,7 +298,7 @@ int core_scsi3_ua_clear_for_request_sense(
 		rcu_read_unlock();
 		return -EINVAL;
 	}
-	if (!atomic_read(&deve->ua_count)) {
+	if (list_empty_careful(&deve->ua_list)) {
 		rcu_read_unlock();
 		return -EPERM;
 	}
@@ -327,8 +321,6 @@ int core_scsi3_ua_clear_for_request_sense(
 		}
 		list_del(&ua->ua_nacl_list);
 		kmem_cache_free(se_ua_cache, ua);
-
-		atomic_dec_mb(&deve->ua_count);
 	}
 	spin_unlock(&deve->ua_lock);
 	rcu_read_unlock();
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index ca59e065c1fd..7a4ee7852ca4 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -638,7 +638,6 @@ struct se_dev_entry {
 	atomic_long_t		total_cmds;
 	atomic_long_t		read_bytes;
 	atomic_long_t		write_bytes;
-	atomic_t		ua_count;
 	/* Used for PR SPEC_I_PT=1 and REGISTER_AND_MOVE */
 	struct kref		pr_kref;
 	struct completion	pr_comp;
-- 
2.17.1

Re: [PATCH 14/14] target: Fix handling of removed LUNs

[Mike Christie]

On 06/21/2018 12:53 PM, Bart Van Assche wrote:
> On Wed, 2018-06-20 at 20:32 -0500, Mike Christie wrote:
>> > I was in the middle of fixing up the sense key value issue in the patch
>> > like we discussed before (use illegal request instead of unit
>> > attention), but it looks like there was another bug. If we have 2
>> > commands going to the same device and they run target_scsi3_ua_check and
>> > see ua_count=1 TCM_CHECK_CONDITION_UNIT_ATTENTION is returned for both.
>> > They then call core_scsi3_ua_for_check_condition and one of them
>> > deallocates a UA dropping ua_count=0. The second command then runs the
>> > !atomic_read(&deve->ua_count) check and sees the other command has
>> > decremented it. Or the second one might have passed the ua_count check
>> > in core_scsi3_ua_for_check_condition and it runs the loop but nothing is
>> > found since the first command has already removed it. We then return
>> > bogus asc/ascq values.
>> > 
>> > In the attached patch I just return busy status for this race case. It
>> > seemed easier than trying to add more locking and error handling.
>> > 
>> > Some notes/questions on some of the code the patch touched though.
>> > 
>> > If translate_sense_reason failed due to a short buffer it returned
>> > -EINVAL. transport_send_check_condition_and_sense would then end up
>> > calling transport_handle_queue_full/transport_complete_qf and that would
>> > just end up failing the command with
>> > TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE. Do you know if that long journey
>> > intended or just an accident?
>> > 
>> > In this patch I fixed that so it just translated the error to
>> > TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE immediately. If the patch is ok
>> > and we meant to return that error code for the short buffer then I can
>> > break that out into another patch.
> Hello Mike,
> 
> That's an interesting observation. However, I think that the race described
> above can be fixed with fewer code changes. Can you have a look at the three
> attached (untested) patches?
> 
> Regarding translate_sense_reason() failing due to a short buffer: I don't
> think that the current behavior in case of a short sense buffer is correct.
> It's probably better to return a sense buffer that is incomplete and with
> correct KEY / ASC / ASCQ values instead of modifying these values. How
> about changing the code at the end of translate_sense_reason() as follows?
> 
> 	if (si->add_sector_info)
> 		WARN_ON_ONCE(scsi_set_sense_information(buffer,
> 						  cmd->scsi_sense_length,
> 						  cmd->bad_sector) < 0);
> 
> 	return 0;

Agree.

> 
> @@ -3156,9 +3167,13 @@ static int translate_sense_reason(struct se_cmd *cmd, sense_reason_t reason)
>  		si = &sense_info_table[(__force int)
>  				       TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE];
>  
> +	key = si->key;
>  	if (reason = TCM_CHECK_CONDITION_UNIT_ATTENTION) {
> -		core_scsi3_ua_for_check_condition(cmd, &asc, &ascq);
> -		WARN_ON_ONCE(asc = 0);
> +		if (!core_scsi3_ua_for_check_condition(cmd, &key, &asc,
> +						       &ascq)) {
> +			cmd->scsi_status = SAM_STAT_BUSY;

Here is another question I had about this code path.

If you hit this case do you need to undo the setting of the
SCF_SENT_CHECK_CONDITION bit done in
transport_send_check_condition_and_sense or does the check for that bit
need to be fixed up.

With the code addition above we have this path setting
SCF_SENT_CHECK_CONDITION so
iscsit_check_task_reassign_expdatasn/iscsit_task_reassign_complete_scsi_cmnd/
transport_send_task_abort would see the CC bit set and return.

If transport_generic_request_failure is passed TCM_LUN_BUSY which gets
translate to SAM_STAT_BUSY then
iscsit_check_task_reassign_expdatasn/iscsit_task_reassign_complete_scsi_cmnd/transport_send_task_abort
would not see the CC bit is set and those functions would not return
right away.

It seems like
iscsit_check_task_reassign_expdatasn/iscsit_task_reassign_complete_scsi_cmnd/
transport_send_task_abort want the same behavior for both check
conditions and commands that completed with non good status, so it
should be checking for the CC bit set and also if non good status has
been returned.


>  	 * The highest priority Unit Attentions are placed at the head of the
> @@ -244,6 +246,7 @@ void core_scsi3_ua_for_check_condition(
>  		 * clearing it.
>  		 */
>  		if (dev->dev_attrib.emulate_ua_intlck_ctrl != 0) {
> +			*key = UNIT_ATTENTION;


Is the key already set to UNIT_ATTENTION here?


Seems nice!

Re: [PATCH 14/14] target: Fix handling of removed LUNs

[Bart Van Assche]

T24gVGh1LCAyMDE4LTA2LTIxIGF0IDE0OjEwIC0wNTAwLCBNaWtlIENocmlzdGllIHdyb3RlOg0K
PiA+IA0KPiA+IEBAIC0zMTU2LDkgKzMxNjcsMTMgQEAgc3RhdGljIGludCB0cmFuc2xhdGVfc2Vu
c2VfcmVhc29uKHN0cnVjdCBzZV9jbWQgKmNtZCwgc2Vuc2VfcmVhc29uX3QgcmVhc29uKQ0KPiA+
ICAJCXNpID0gJnNlbnNlX2luZm9fdGFibGVbKF9fZm9yY2UgaW50KQ0KPiA+ICAJCQkJICAgICAg
IFRDTV9MT0dJQ0FMX1VOSVRfQ09NTVVOSUNBVElPTl9GQUlMVVJFXTsNCj4gPiAgDQo+ID4gKwlr
ZXkgPSBzaS0+a2V5Ow0KPiA+ICAJaWYgKHJlYXNvbiA9PSBUQ01fQ0hFQ0tfQ09ORElUSU9OX1VO
SVRfQVRURU5USU9OKSB7DQo+ID4gLQkJY29yZV9zY3NpM191YV9mb3JfY2hlY2tfY29uZGl0aW9u
KGNtZCwgJmFzYywgJmFzY3EpOw0KPiA+IC0JCVdBUk5fT05fT05DRShhc2MgPT0gMCk7DQo+ID4g
KwkJaWYgKCFjb3JlX3Njc2kzX3VhX2Zvcl9jaGVja19jb25kaXRpb24oY21kLCAma2V5LCAmYXNj
LA0KPiA+ICsJCQkJCQkgICAgICAgJmFzY3EpKSB7DQo+ID4gKwkJCWNtZC0+c2NzaV9zdGF0dXMg
PSBTQU1fU1RBVF9CVVNZOw0KPiANCj4gSGVyZSBpcyBhbm90aGVyIHF1ZXN0aW9uIEkgaGFkIGFi
b3V0IHRoaXMgY29kZSBwYXRoLg0KPiANCj4gSWYgeW91IGhpdCB0aGlzIGNhc2UgZG8geW91IG5l
ZWQgdG8gdW5kbyB0aGUgc2V0dGluZyBvZiB0aGUNCj4gU0NGX1NFTlRfQ0hFQ0tfQ09ORElUSU9O
IGJpdCBkb25lIGluDQo+IHRyYW5zcG9ydF9zZW5kX2NoZWNrX2NvbmRpdGlvbl9hbmRfc2Vuc2Ug
b3IgZG9lcyB0aGUgY2hlY2sgZm9yIHRoYXQgYml0DQo+IG5lZWQgdG8gYmUgZml4ZWQgdXAuDQo+
IA0KPiBXaXRoIHRoZSBjb2RlIGFkZGl0aW9uIGFib3ZlIHdlIGhhdmUgdGhpcyBwYXRoIHNldHRp
bmcNCj4gU0NGX1NFTlRfQ0hFQ0tfQ09ORElUSU9OIHNvDQo+IGlzY3NpdF9jaGVja190YXNrX3Jl
YXNzaWduX2V4cGRhdGFzbi9pc2NzaXRfdGFza19yZWFzc2lnbl9jb21wbGV0ZV9zY3NpX2NtbmQv
DQo+IHRyYW5zcG9ydF9zZW5kX3Rhc2tfYWJvcnQgd291bGQgc2VlIHRoZSBDQyBiaXQgc2V0IGFu
ZCByZXR1cm4uDQoNCkhlbGxvIE1pa2UsDQoNCkhhdmUgeW91IG5vdGljZWQgdGhhdCBwYXRjaCAx
LzMgdGhhdCB3YXMgYXR0YWNoZWQgdG8gbXkgcHJldmlvdXMgZS1tYWlsDQptb3ZlcyB0aGUgY29k
ZSB0aGF0IHNldHMgdGhlIFNDRl9TRU5UX0NIRUNLX0NPTkRJVElPTiBmbGFnIGluc2lkZQ0KdHJh
bnNsYXRlX3NlbnNlX3JlYXNvbigpIGFuZCBwYXN0IHRoZSBwb2ludCB3aGVyZSBTQU1fU1RBVF9C
VVNZIGlzIHJldHVybmVkPw0KDQo+ID4gIAkgKiBUaGUgaGlnaGVzdCBwcmlvcml0eSBVbml0IEF0
dGVudGlvbnMgYXJlIHBsYWNlZCBhdCB0aGUgaGVhZCBvZiB0aGUNCj4gPiBAQCAtMjQ0LDYgKzI0
Niw3IEBAIHZvaWQgY29yZV9zY3NpM191YV9mb3JfY2hlY2tfY29uZGl0aW9uKA0KPiA+ICAJCSAq
IGNsZWFyaW5nIGl0Lg0KPiA+ICAJCSAqLw0KPiA+ICAJCWlmIChkZXYtPmRldl9hdHRyaWIuZW11
bGF0ZV91YV9pbnRsY2tfY3RybCAhPSAwKSB7DQo+ID4gKwkJCSprZXkgPSBVTklUX0FUVEVOVElP
TjsNCj4gDQo+IA0KPiBJcyB0aGUga2V5IGFscmVhZHkgc2V0IHRvIFVOSVRfQVRURU5USU9OIGhl
cmU/DQoNCkdvb2QgY2F0Y2ggLSB0aGF0IGFzc2lnbm1lbnQgaXMgc3VwZXJmbHVvdXMuIEJ1dCBJ
IHdvdWxkIGxpa2UgdG8ga2VlcCBpdA0KYmVjYXVzZSBJIHRoaW5rIGl0IG1ha2VzIHRoZSBjb3Jl
X3Njc2kzX3VhX2Zvcl9jaGVja19jb25kaXRpb24oKSBlYXN5IHRvDQpyZWFkLiBIb3dldmVyLCB0
aGF0IGFzc2lnbm1lbnQgbWFrZXMgdGhlIGZvbGxvd2luZyBlbnRyeSBpbiBzZW5zZV9pbmZvX3Rh
YmxlW10NCnN1cGVyZmx1b3VzOg0KDQoJW1RDTV9DSEVDS19DT05ESVRJT05fVU5JVF9BVFRFTlRJ
T05dID0gew0KCQkua2V5ID0gVU5JVF9BVFRFTlRJT04sDQoJfSwNCg0KQmFydC4

Re: [PATCH 14/14] target: Fix handling of removed LUNs

[Mike Christie]

On 06/21/2018 03:25 PM, Bart Van Assche wrote:
> On Thu, 2018-06-21 at 14:10 -0500, Mike Christie wrote:
>>>
>>> @@ -3156,9 +3167,13 @@ static int translate_sense_reason(struct se_cmd *cmd, sense_reason_t reason)
>>>  		si = &sense_info_table[(__force int)
>>>  				       TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE];
>>>  
>>> +	key = si->key;
>>>  	if (reason = TCM_CHECK_CONDITION_UNIT_ATTENTION) {
>>> -		core_scsi3_ua_for_check_condition(cmd, &asc, &ascq);
>>> -		WARN_ON_ONCE(asc = 0);
>>> +		if (!core_scsi3_ua_for_check_condition(cmd, &key, &asc,
>>> +						       &ascq)) {
>>> +			cmd->scsi_status = SAM_STAT_BUSY;
>>
>> Here is another question I had about this code path.
>>
>> If you hit this case do you need to undo the setting of the
>> SCF_SENT_CHECK_CONDITION bit done in
>> transport_send_check_condition_and_sense or does the check for that bit
>> need to be fixed up.
>>
>> With the code addition above we have this path setting
>> SCF_SENT_CHECK_CONDITION so
>> iscsit_check_task_reassign_expdatasn/iscsit_task_reassign_complete_scsi_cmnd/
>> transport_send_task_abort would see the CC bit set and return.
> 
> Hello Mike,
> 
> Have you noticed that patch 1/3 that was attached to my previous e-mail
> moves the code that sets the SCF_SENT_CHECK_CONDITION flag inside

I am not seeing that. I saw SCF_EMULATED_TASK_SENSE was moved but not
SCF_SENT_CHECK_CONDITION. The latter is set a little earlier in
transport_send_check_condition_and_sense.

> translate_sense_reason() and past the point where SAM_STAT_BUSY is returned?
> 

Re: [PATCH 14/14] target: Fix handling of removed LUNs

[Mike Christie]

On 06/21/2018 03:25 PM, Bart Van Assche wrote:
>>>  		 * clearing it.
>>> > >  		 */
>>> > >  		if (dev->dev_attrib.emulate_ua_intlck_ctrl != 0) {
>>> > > +			*key = UNIT_ATTENTION;
>> > 
>> > 
>> > Is the key already set to UNIT_ATTENTION here?
> Good catch - that assignment is superfluous. But I would like to keep it
> because I think it makes the core_scsi3_ua_for_check_condition() easy to
> read. However, that assignment makes the following entry in sense_info_table[]
> superfluous:

If you keep it above then I think it would be good to add it below in
the emulate_ua_intlck_ctrl=0 case:

                if (head) {
                        *asc = ua->ua_asc;
                        *ascq = ua->ua_ascq;
                        head = 0;
                }

so it is consistent.

Re: [PATCH 14/14] target: Fix handling of removed LUNs

[Bart Van Assche]

T24gVGh1LCAyMDE4LTA2LTIxIGF0IDE1OjM4IC0wNTAwLCBNaWtlIENocmlzdGllIHdyb3RlOg0K
PiBPbiAwNi8yMS8yMDE4IDAzOjI1IFBNLCBCYXJ0IFZhbiBBc3NjaGUgd3JvdGU6DQo+ID4gPiA+
ICAJCSAqIGNsZWFyaW5nIGl0Lg0KPiA+ID4gPiA+ID4gIAkJICovDQo+ID4gPiA+ID4gPiAgCQlp
ZiAoZGV2LT5kZXZfYXR0cmliLmVtdWxhdGVfdWFfaW50bGNrX2N0cmwgIT0gMCkgew0KPiA+ID4g
PiA+ID4gKwkJCSprZXkgPSBVTklUX0FUVEVOVElPTjsNCj4gPiA+ID4gDQo+ID4gPiA+IA0KPiA+
ID4gPiBJcyB0aGUga2V5IGFscmVhZHkgc2V0IHRvIFVOSVRfQVRURU5USU9OIGhlcmU/DQo+ID4g
DQo+ID4gR29vZCBjYXRjaCAtIHRoYXQgYXNzaWdubWVudCBpcyBzdXBlcmZsdW91cy4gQnV0IEkg
d291bGQgbGlrZSB0byBrZWVwIGl0DQo+ID4gYmVjYXVzZSBJIHRoaW5rIGl0IG1ha2VzIHRoZSBj
b3JlX3Njc2kzX3VhX2Zvcl9jaGVja19jb25kaXRpb24oKSBlYXN5IHRvDQo+ID4gcmVhZC4gSG93
ZXZlciwgdGhhdCBhc3NpZ25tZW50IG1ha2VzIHRoZSBmb2xsb3dpbmcgZW50cnkgaW4gc2Vuc2Vf
aW5mb190YWJsZVtdDQo+ID4gc3VwZXJmbHVvdXM6DQo+IA0KPiBJZiB5b3Uga2VlcCBpdCBhYm92
ZSB0aGVuIEkgdGhpbmsgaXQgd291bGQgYmUgZ29vZCB0byBhZGQgaXQgYmVsb3cgaW4NCj4gdGhl
IGVtdWxhdGVfdWFfaW50bGNrX2N0cmw9MCBjYXNlOg0KPiANCj4gICAgICAgICAgICAgICAgIGlm
IChoZWFkKSB7DQo+ICAgICAgICAgICAgICAgICAgICAgICAgICphc2MgPSB1YS0+dWFfYXNjOw0K
PiAgICAgICAgICAgICAgICAgICAgICAgICAqYXNjcSA9IHVhLT51YV9hc2NxOw0KPiAgICAgICAg
ICAgICAgICAgICAgICAgICBoZWFkID0gMDsNCj4gICAgICAgICAgICAgICAgIH0NCj4gDQo+IHNv
IGl0IGlzIGNvbnNpc3RlbnQuDQoNCk9LLCB3aWxsIGRvLg0KDQpCYXJ0Lg0KDQoNCg0K

Re: [PATCH 14/14] target: Fix handling of removed LUNs

[Bart Van Assche]

T24gVGh1LCAyMDE4LTA2LTIxIGF0IDE1OjMyIC0wNTAwLCBNaWtlIENocmlzdGllIHdyb3RlOg0K
PiBJIGFtIG5vdCBzZWVpbmcgdGhhdC4gSSBzYXcgU0NGX0VNVUxBVEVEX1RBU0tfU0VOU0Ugd2Fz
IG1vdmVkIGJ1dCBub3QNCj4gU0NGX1NFTlRfQ0hFQ0tfQ09ORElUSU9OLiBUaGUgbGF0dGVyIGlz
IHNldCBhIGxpdHRsZSBlYXJsaWVyIGluDQo+IHRyYW5zcG9ydF9zZW5kX2NoZWNrX2NvbmRpdGlv
bl9hbmRfc2Vuc2UuDQoNCkFyZSB5b3Ugc3VyZSB3ZSBuZWVkIHRvIGNoYW5nZSB0aGUgY29kZSB0
aGF0IHNldHMgU0NGX1NFTlRfQ0hFQ0tfQ09ORElUSU9OPw0KSXQgc2VlbXMgdG8gbWUgbGlrZSB0
aGUgbmFtZSBvZiB0aGF0IGZsYWcgaXMgbWlzbGVhZGluZy4gSSB0aGluayB0aGF0IGZsYWcNCmlz
IHVzZWQgdG8ga2VlcCB0cmFjayBvZiB3aGV0aGVyIG9yIG5vdCBhIHJlc3BvbnNlIGhhcyBiZWVu
IHNlbnQgdG8gdGhlDQppbml0aWF0b3IuIFNvIGtlZXBpbmcgdGhlIGN1cnJlbnQgY29kZSBmb3Ig
dGhlIG1hbmlwdWxhdGlvbiBvZiB0aGF0IGZsYWcNCnNob3VsZCBiZSBmaW5lLg0KDQpCYXJ0Lg0K
DQoNCg=

Re: [PATCH 14/14] target: Fix handling of removed LUNs

[Mike Christie]

On 06/21/2018 03:58 PM, Bart Van Assche wrote:
> On Thu, 2018-06-21 at 15:32 -0500, Mike Christie wrote:
>> I am not seeing that. I saw SCF_EMULATED_TASK_SENSE was moved but not
>> SCF_SENT_CHECK_CONDITION. The latter is set a little earlier in
>> transport_send_check_condition_and_sense.
> 
> Are you sure we need to change the code that sets SCF_SENT_CHECK_CONDITION?
> It seems to me like the name of that flag is misleading. I think that flag
> is used to keep track of whether or not a response has been sent to the
> initiator. So keeping the current code for the manipulation of that flag
> should be fine.
> 

I think with your patch the behavior will be the same as before, so it
is safe.

I was more asking if in another future patch we need to set that bit for
the case where TCM_LUN_BUSY/TCM_OUT_OF_RESOURCES is passed to
transport_generic_request_failure so the behavior for that code path is
the same as when you now set cmd->scsi_status = TCM_LUN_BUSY in your patch.

Re: [PATCH 14/14] target: Fix handling of removed LUNs

[Bart Van Assche]

On 06/21/18 20:45, Mike Christie wrote:
> On 06/21/2018 03:58 PM, Bart Van Assche wrote:
>> On Thu, 2018-06-21 at 15:32 -0500, Mike Christie wrote:
>>> I am not seeing that. I saw SCF_EMULATED_TASK_SENSE was moved but not
>>> SCF_SENT_CHECK_CONDITION. The latter is set a little earlier in
>>> transport_send_check_condition_and_sense.
>>
>> Are you sure we need to change the code that sets SCF_SENT_CHECK_CONDITION?
>> It seems to me like the name of that flag is misleading. I think that flag
>> is used to keep track of whether or not a response has been sent to the
>> initiator. So keeping the current code for the manipulation of that flag
>> should be fine.
> 
> I think with your patch the behavior will be the same as before, so it
> is safe.
> 
> I was more asking if in another future patch we need to set that bit for
> the case where TCM_LUN_BUSY/TCM_OUT_OF_RESOURCES is passed to
> transport_generic_request_failure so the behavior for that code path is
> the same as when you now set cmd->scsi_status = TCM_LUN_BUSY in your patch.

Hello Mike,

As you know today the code that processes the abort task management 
function is executed concurrently with the regular command processing 
code. I think if we would process task aborts from inside the regular 
command processing path that not only that code would become simpler but 
also that we wouldn't need the SCF_SENT_CHECK_CONDITION flag anymore for 
most target drivers. I'm not sure though about the iSCSI target driver. 
It's possible that we still would need that flag for the iSCSI target 
driver. How about me resurrecting the patch that moves abort processing
into the same context as the regular SCSI command execution?

Bart.