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.
From ab96515f4f53001462864dc6b71f0058ea6c99a8 Mon Sep 17 00:00:00 2001
From: Bart Van Assche <bart.vanassche@xxxxxxx>
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@xxxxxxx>
Cc: Mike Christie <mchristi@xxxxxxxxxx>
Cc: Christoph Hellwig <hch@xxxxxx>
Cc: Hannes Reinecke <hare@xxxxxxxx>
---
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
From 4ac09224a732f862a6bae7a820373fbdf30716a0 Mon Sep 17 00:00:00 2001
From: Bart Van Assche <bart.vanassche@xxxxxxx>
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@xxxxxxx>
Cc: Mike Christie <mchristi@xxxxxxxxxx>
Cc: Christoph Hellwig <hch@xxxxxx>
Cc: Hannes Reinecke <hare@xxxxxxxx>
---
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
From fac94854486b98dc04c89c21a4b2f24f895bbafe Mon Sep 17 00:00:00 2001
From: Bart Van Assche <bart.vanassche@xxxxxxx>
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@xxxxxxx>
Cc: Mike Christie <mchristi@xxxxxxxxxx>
Cc: Christoph Hellwig <hch@xxxxxx>
Cc: Hannes Reinecke <hare@xxxxxxxx>
---
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
