[PATCH] target/iblock: Use -EAGAIN/-ENOMEM to propigate SAM BUSY/TASK_SET_FULL
From: Nicholas Bellinger
This patch updates target/iblock backend driver code to
check for bio completion status of -EAGAIN / -ENOMEM
provided by raw block drivers and scsi devices, in order
to generate the following SCSI status to initiators:
- SAM_STAT_BUSY
- SAM_STAT_TASK_SET_FULL
Note these two SAM status codes are useful to signal to
Linux SCSI host side that se_cmd should be retried
again, or with TASK_SET_FULL case to attempt to lower
our internal host LLD queue_depth and scsi_cmnd retry.
It also updates target_complete_cmd() to parse status
when signalling success to target_completion_wq.
Cc: Christoph Hellwig
Cc: Hannes Reinecke
Cc: Sagi Grimberg
Cc: Andy Grover
Cc: Mike Christie
Signed-off-by: Nicholas Bellinger
---
drivers/target/target_core_iblock.c| 38 +++---
drivers/target/target_core_iblock.h| 1 +
drivers/target/target_core_transport.c | 13 ++--
3 files changed, 43 insertions(+), 9 deletions(-)
diff --git a/drivers/target/target_core_iblock.c
b/drivers/target/target_core_iblock.c
index 5a2899f..77d0381 100644
--- a/drivers/target/target_core_iblock.c
+++ b/drivers/target/target_core_iblock.c
@@ -300,11 +300,28 @@ static void iblock_complete_cmd(struct se_cmd *cmd)
if (!atomic_dec_and_test(&ibr->pending))
return;
-
- if (atomic_read(&ibr->ib_bio_err_cnt))
- status = SAM_STAT_CHECK_CONDITION;
- else
+ /*
+* Propigate use these two bio completion values from raw block
+* drivers to signal up BUSY and TASK_SET_FULL status to the
+* host side initiator. The latter for Linux/iSCSI initiators
+* means the Linux/SCSI LLD will begin to reduce it's internal
+* per session queue_depth.
+*/
+ if (atomic_read(&ibr->ib_bio_err_cnt)) {
+ switch (ibr->ib_bio_retry) {
+ case -EAGAIN:
+ status = SAM_STAT_BUSY;
+ break;
+ case -ENOMEM:
+ status = SAM_STAT_TASK_SET_FULL;
+ break;
+ default:
+ status = SAM_STAT_CHECK_CONDITION;
+ break;
+ }
+ } else {
status = SAM_STAT_GOOD;
+ }
target_complete_cmd(cmd, status);
kfree(ibr);
@@ -316,7 +333,15 @@ static void iblock_bio_done(struct bio *bio)
struct iblock_req *ibr = cmd->priv;
if (bio->bi_error) {
- pr_err("bio error: %p, err: %d\n", bio, bio->bi_error);
+ pr_debug_ratelimited("test_bit(BIO_UPTODATE) failed for bio:
%p,"
+ " err: %d\n", bio, bio->bi_error);
+ /*
+* Save the retryable status provided and translate into
+* SAM status in iblock_complete_cmd().
+*/
+ if (bio->bi_error == -EAGAIN || bio->bi_error == -ENOMEM) {
+ ibr->ib_bio_retry = bio->bi_error;
+ }
/*
* Bump the ib_bio_err_cnt and release bio.
*/
@@ -655,8 +680,7 @@ iblock_execute_rw(struct se_cmd *cmd, struct scatterlist
*sgl, u32 sgl_nents,
u32 sg_num = sgl_nents;
sector_t block_lba;
unsigned bio_cnt;
- int rw = 0;
- int i;
+ int i, rw = 0;
if (data_direction == DMA_TO_DEVICE) {
struct iblock_dev *ib_dev = IBLOCK_DEV(dev);
diff --git a/drivers/target/target_core_iblock.h
b/drivers/target/target_core_iblock.h
index 01c2afd..ff3cfdd 100644
--- a/drivers/target/target_core_iblock.h
+++ b/drivers/target/target_core_iblock.h
@@ -9,6 +9,7 @@
struct iblock_req {
atomic_t pending;
atomic_t ib_bio_err_cnt;
+ int ib_bio_retry;
} cacheline_aligned;
#define IBDF_HAS_UDEV_PATH 0x01
diff --git a/drivers/target/target_core_transport.c
b/drivers/target/target_core_transport.c
index 6becc94..eb12ae2 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -732,11 +732,20 @@ static unsigned char *transport_get_sense_buffer(struct
se_cmd *cmd)
void target_complete_cmd(struct se_cmd *cmd, u8 scsi_status)
{
struct se_device *dev = cmd->se_dev;
- int success = scsi_status == GOOD;
+ int success;
unsigned long flags;
cmd->scsi_status = scsi_status;
-
+ switch (cmd->scsi_status) {
+ case SAM_STAT_GOOD:
+ case SAM_STAT_BUSY:
+ case SAM_STAT_TASK_SET_FULL:
+ success = 1;
+ break;
+ default:
+ success = 0;
+ break;
+ }
spin_lock_irqsave(&cmd->t_state_lock, flags);
cmd->transport_state &= ~CMD_T_BUSY;
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kerne
Re: [PATCH-v4 0/5] Fix LUN_RESET active I/O + TMR handling
On Sat, 2016-02-06 at 20:19 -0800, Bart Van Assche wrote: > On 02/06/16 19:17, Nicholas A. Bellinger wrote: > > Here is -v4 series to address the set of of LUN_RESET > > active I/O + TMR se_cmd->cmd_kref < 0 bugs as reported > > recently by Quinn & Co. This can occur during active > > I/O remote port TMR LUN_RESET with multi-port LIO > > configurations. > > Hi Nic, > > If I understood the purpose of this patch series correctly then this > patch series is a brave attempt to fix what is also fixed by my patch > called "target: Make ABORT and LUN RESET handling synchronous". Wouldn't > it be better to focus on that patch instead of trying to fix the current > approach in which TMR handling happens from the another context than the > command processing context ? > This statement is a gross oversimplification of the issues involved. If you'll recall, this was already highlighted in the context of your patch here: http://www.spinics.net/lists/target-devel/msg11057.html There are a number of comments on why the bug-fix was incorrect and broken, the basics of what needed to be done and in what order it should happen. But instead of replying to the comments, this was your response: http://www.spinics.net/lists/target-devel/msg11542.html If you are authentically interested in understanding the issues involved, you'll probably need to go back and comment on those topics individually, instead of ignoring them. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] qla2xxx: Add DebugFS node for target sess list.
On Sat, 2016-02-06 at 20:40 -0800, Nicholas A. Bellinger wrote: > Hi Himanshu & Quinn, > > On Thu, 2016-02-04 at 11:45 -0500, Himanshu Madhani wrote: > > From: Quinn Tran > > > > #cat /sys/kernel/debug/qla2xxx/qla2xxx_31/tgt_sess > > qla2xxx_31 > > Port ID Port NameHandle > > ff:fc:01 21:fd:00:05:33:c7:ec:16 0 > > 01:0e:00 21:00:00:24:ff:7b:8a:e4 1 > > 01:0f:00 21:00:00:24:ff:7b:8a:e5 2 > > > > > > Signed-off-by: Quinn Tran > > Signed-off-by: Himanshu Madhani > > --- > > drivers/scsi/qla2xxx/qla_def.h|1 + > > drivers/scsi/qla2xxx/qla_dfs.c| 55 > > > > drivers/scsi/qla2xxx/qla_target.c | 56 > > > > 3 files changed, 93 insertions(+), 19 deletions(-) > > > > So looking at this patch beyond the debugfs part, it does change where > ->check_initiator_node_acl() gets call during qlt_create_sess(). > > I assume this is related to new debugfs attribute, and these changes > (plus others in qlt_del_sess_work_fn) are not bug-fixes on their own, > correct..? > > Aside from that, I don't have an objection to merge as v4.6 for-next > code if QLogic finds it useful for debugging. > Btw, this patch has a conflict with target-pending/queue-next wrt to removal of be_sid + loopid from tgt_ops->check_initiator_node_acl(), which is part of the target_alloc_session() conversion for v4.6. Here's the updated version that's been applied to queue-next: https://git.kernel.org/cgit/linux/kernel/git/nab/target-pending.git/commit/?h=queue-next&id=cfd527d9db57ceac8a1e211b77c3357259df48cc Please verify that it looks + works as expected. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] qla2xxx: Add DebugFS node for target sess list.
On Thu, 2016-02-04 at 10:16 -0800, Bart Van Assche wrote: > On 02/04/2016 08:45 AM, Himanshu Madhani wrote: > > From: Quinn Tran > > > > #cat /sys/kernel/debug/qla2xxx/qla2xxx_31/tgt_sess > > qla2xxx_31 > > Port ID Port NameHandle > > ff:fc:01 21:fd:00:05:33:c7:ec:16 0 > > 01:0e:00 21:00:00:24:ff:7b:8a:e4 1 > > 01:0f:00 21:00:00:24:ff:7b:8a:e5 2 > > > > Hello Quinn and Himanshu, > > The above information is not only useful to people who are debugging the > QLogic target driver but also to end users who want to check which > initiator ports have already logged in to a target port. Hence my > proposal to move this information from debugfs to another location (e.g. > configfs or sysfs). Users of other target drivers are probably also > interested in seeing which sessions are active. We already have that. For explicit NodeACLS, see: /sys/kernel/config/target/$FABRIC/$T_WWPN/$TPGT/acls/$I_WWPN/info and for demo-mode dynamically generated se_node_acl, see: /sys/kernel/config/target/$FABRIC/$T_WWPN/$TPGT/dynamic_sessions -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] qla2xxx: Add DebugFS node for target sess list.
Hi Himanshu & Quinn,
On Thu, 2016-02-04 at 11:45 -0500, Himanshu Madhani wrote:
> From: Quinn Tran
>
> #cat /sys/kernel/debug/qla2xxx/qla2xxx_31/tgt_sess
> qla2xxx_31
> Port ID Port NameHandle
> ff:fc:01 21:fd:00:05:33:c7:ec:16 0
> 01:0e:00 21:00:00:24:ff:7b:8a:e4 1
> 01:0f:00 21:00:00:24:ff:7b:8a:e5 2
>
>
> Signed-off-by: Quinn Tran
> Signed-off-by: Himanshu Madhani
> ---
> drivers/scsi/qla2xxx/qla_def.h|1 +
> drivers/scsi/qla2xxx/qla_dfs.c| 55
> drivers/scsi/qla2xxx/qla_target.c | 56
> 3 files changed, 93 insertions(+), 19 deletions(-)
>
So looking at this patch beyond the debugfs part, it does change where
->check_initiator_node_acl() gets call during qlt_create_sess().
I assume this is related to new debugfs attribute, and these changes
(plus others in qlt_del_sess_work_fn) are not bug-fixes on their own,
correct..?
Aside from that, I don't have an objection to merge as v4.6 for-next
code if QLogic finds it useful for debugging.
> diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
> index 9872f34..e6c5bcf 100644
> --- a/drivers/scsi/qla2xxx/qla_def.h
> +++ b/drivers/scsi/qla2xxx/qla_def.h
> @@ -2929,6 +2929,7 @@ struct qlt_hw_data {
>
> uint8_t tgt_node_name[WWN_SIZE];
>
> + struct dentry *dfs_tgt_sess;
> struct list_head q_full_list;
> uint32_t num_pend_cmds;
> uint32_t num_qfull_cmds_alloc;
> diff --git a/drivers/scsi/qla2xxx/qla_dfs.c b/drivers/scsi/qla2xxx/qla_dfs.c
> index cd8b96a..34272fd 100644
> --- a/drivers/scsi/qla2xxx/qla_dfs.c
> +++ b/drivers/scsi/qla2xxx/qla_dfs.c
> @@ -13,6 +13,47 @@ static struct dentry *qla2x00_dfs_root;
> static atomic_t qla2x00_dfs_root_count;
>
> static int
> +qla2x00_dfs_tgt_sess_show(struct seq_file *s, void *unused)
> +{
> + scsi_qla_host_t *vha = s->private;
> + struct qla_hw_data *ha = vha->hw;
> + unsigned long flags;
> + struct qla_tgt_sess *sess = NULL;
> + struct qla_tgt *tgt= vha->vha_tgt.qla_tgt;
> +
> + seq_printf(s, "%s\n",vha->host_str);
> + if (tgt) {
> + seq_printf(s, "Port ID Port NameHandle\n");
> +
> + spin_lock_irqsave(&ha->tgt.sess_lock, flags);
> + list_for_each_entry(sess, &tgt->sess_list, sess_list_entry) {
> + seq_printf(s, "%02x:%02x:%02x %8phC %d\n",
> +
> sess->s_id.b.domain,sess->s_id.b.area,
> +sess->s_id.b.al_pa, sess->port_name,
> +sess->loop_id);
> + }
> + spin_unlock_irqrestore(&ha->tgt.sess_lock, flags);
> + }
> +
> + return 0;
> +}
> +
> +static int
> +qla2x00_dfs_tgt_sess_open(struct inode *inode, struct file *file)
> +{
> + scsi_qla_host_t *vha = inode->i_private;
> + return single_open(file, qla2x00_dfs_tgt_sess_show, vha);
> +}
> +
> +
> +static const struct file_operations dfs_tgt_sess_ops = {
> + .open = qla2x00_dfs_tgt_sess_open,
> + .read = seq_read,
> + .llseek = seq_lseek,
> + .release= single_release,
> +};
> +
> +static int
> qla_dfs_fw_resource_cnt_show(struct seq_file *s, void *unused)
> {
> struct scsi_qla_host *vha = s->private;
> @@ -248,6 +289,15 @@ create_nodes:
> "Unable to create debugfs fce node.\n");
> goto out;
> }
> +
> + ha->tgt.dfs_tgt_sess = debugfs_create_file("tgt_sess",
> + S_IRUSR, ha->dfs_dir, vha, &dfs_tgt_sess_ops);
> + if (!ha->tgt.dfs_tgt_sess) {
> + ql_log(ql_log_warn, vha, 0x,
> + "Unable to create debugFS tgt_sess node.\n");
> + goto out;
> + }
> +
> out:
> return 0;
> }
> @@ -257,6 +307,11 @@ qla2x00_dfs_remove(scsi_qla_host_t *vha)
> {
> struct qla_hw_data *ha = vha->hw;
>
> + if (ha->tgt.dfs_tgt_sess) {
> + debugfs_remove(ha->tgt.dfs_tgt_sess);
> + ha->tgt.dfs_tgt_sess = NULL;
> + }
> +
> if (ha->dfs_fw_resource_cnt) {
> debugfs_remove(ha->dfs_fw_resource_cnt);
> ha->dfs_fw_resource_cnt = NULL;
> diff --git a/drivers/scsi/qla2xxx/qla_target.c
> b/drivers/scsi/qla2xxx/qla_target.c
> index 46c6679..a754aa4 100644
> --- a/drivers/scsi/qla2xxx/qla_target.c
> +++ b/drivers/scsi/qla2xxx/qla_target.c
> @@ -641,7 +641,8 @@ void qlt_unreg_sess(struct qla_tgt_sess *sess)
> {
> struct scsi_qla_host *vha = sess->vha;
>
> - vha->hw->tgt.tgt_ops->clear_nacl_from_fcport_map(sess);
> + if (sess->se_sess)
> + vha->hw->tgt.tgt_ops->clear_nacl_from_fcport_map(sess);
>
> if (!list_empty(&sess->del_list_entry))
> list_del_init(&sess->del_list_entry);
> @@ -856,8 +857,12 @@ static void qlt_del_sess_work_fn(struct delayed_work
> *work)
>
Re: [PATCH 0/4] qla2xxx: Patches for target-pending branch
Hi Himanshu & Co, On Thu, 2016-02-04 at 11:45 -0500, Himanshu Madhani wrote: > Hi Nic, > > Please apply following patches to target-pending branch at your earliest > convenience. > > Thanks, > Himanshu > > Quinn Tran (3): > qla2xxx: Fix stale pointer access. > qla2xxx: Add DebugFS node for target sess list. > qla2xxx: Add DebugFS node to show irq vector's cpuid > > Swapnil Nagle (1): > qla2xxx: Use ATIO type to send correct tmr response > > drivers/scsi/qla2xxx/qla_def.h|3 + > drivers/scsi/qla2xxx/qla_dfs.c| 104 > + > drivers/scsi/qla2xxx/qla_init.c | 10 ++-- > drivers/scsi/qla2xxx/qla_isr.c| 20 ++- > drivers/scsi/qla2xxx/qla_mid.c|4 +- > drivers/scsi/qla2xxx/qla_os.c |6 ++ > drivers/scsi/qla2xxx/qla_target.c | 58 +--- > drivers/scsi/qla2xxx/qla_tmpl.c | 16 ++ > 8 files changed, 192 insertions(+), 29 deletions(-) > Applied #1 + #2 to target-pending/master. Commenting on the others inline. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH-v4 0/5] Fix LUN_RESET active I/O + TMR handling
On 02/06/16 19:17, Nicholas A. Bellinger wrote: Here is -v4 series to address the set of of LUN_RESET active I/O + TMR se_cmd->cmd_kref < 0 bugs as reported recently by Quinn & Co. This can occur during active I/O remote port TMR LUN_RESET with multi-port LIO configurations. Hi Nic, If I understood the purpose of this patch series correctly then this patch series is a brave attempt to fix what is also fixed by my patch called "target: Make ABORT and LUN RESET handling synchronous". Wouldn't it be better to focus on that patch instead of trying to fix the current approach in which TMR handling happens from the another context than the command processing context ? Thanks, Bart. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH-v4 2/5] target: Fix LUN_RESET active TMR descriptor handling
From: Nicholas Bellinger
This patch fixes a NULL pointer se_cmd->cmd_kref < 0
refcount bug during TMR LUN_RESET with active TMRs,
triggered during se_cmd + se_tmr_req descriptor
shutdown + release via core_tmr_drain_tmr_list().
To address this bug, go ahead and obtain a local
kref_get_unless_zero(&se_cmd->cmd_kref) for active I/O
to set CMD_T_ABORTED, and transport_wait_for_tasks()
followed by the final target_put_sess_cmd() to drop
the local ->cmd_kref.
Also add two new checks within target_tmr_work() to
avoid CMD_T_ABORTED -> TFO->queue_tm_rsp() callbacks
ahead of invoking the backend -> fabric put in
transport_cmd_check_stop_to_fabric().
For good measure, also change core_tmr_release_req()
to use list_del_init() ahead of se_tmr_req memory
free.
Reviewed-by: Quinn Tran
Cc: Himanshu Madhani
Cc: Sagi Grimberg
Cc: Christoph Hellwig
Cc: Hannes Reinecke
Cc: Andy Grover
Cc: Mike Christie
Cc: sta...@vger.kernel.org # 3.10+
Signed-off-by: Nicholas Bellinger
---
drivers/target/target_core_tmr.c | 22 +-
drivers/target/target_core_transport.c | 17 +
2 files changed, 38 insertions(+), 1 deletion(-)
diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
index fb3decc..072af07 100644
--- a/drivers/target/target_core_tmr.c
+++ b/drivers/target/target_core_tmr.c
@@ -68,7 +68,7 @@ void core_tmr_release_req(struct se_tmr_req *tmr)
if (dev) {
spin_lock_irqsave(&dev->se_tmr_lock, flags);
- list_del(&tmr->tmr_list);
+ list_del_init(&tmr->tmr_list);
spin_unlock_irqrestore(&dev->se_tmr_lock, flags);
}
@@ -194,9 +194,11 @@ static void core_tmr_drain_tmr_list(
struct list_head *preempt_and_abort_list)
{
LIST_HEAD(drain_tmr_list);
+ struct se_session *sess;
struct se_tmr_req *tmr_p, *tmr_pp;
struct se_cmd *cmd;
unsigned long flags;
+ bool rc;
/*
* Release all pending and outgoing TMRs aside from the received
* LUN_RESET tmr..
@@ -222,17 +224,31 @@ static void core_tmr_drain_tmr_list(
if (target_check_cdb_and_preempt(preempt_and_abort_list, cmd))
continue;
+ sess = cmd->se_sess;
+ if (WARN_ON_ONCE(!sess))
+ continue;
+
+ spin_lock(&sess->sess_cmd_lock);
spin_lock(&cmd->t_state_lock);
if (!(cmd->transport_state & CMD_T_ACTIVE)) {
spin_unlock(&cmd->t_state_lock);
+ spin_unlock(&sess->sess_cmd_lock);
continue;
}
if (cmd->t_state == TRANSPORT_ISTATE_PROCESSING) {
spin_unlock(&cmd->t_state_lock);
+ spin_unlock(&sess->sess_cmd_lock);
continue;
}
+ cmd->transport_state |= CMD_T_ABORTED;
spin_unlock(&cmd->t_state_lock);
+ rc = kref_get_unless_zero(&cmd->cmd_kref);
+ spin_unlock(&sess->sess_cmd_lock);
+ if (!rc) {
+ printk("LUN_RESET TMR: non-zero
kref_get_unless_zero\n");
+ continue;
+ }
list_move_tail(&tmr_p->tmr_list, &drain_tmr_list);
}
spin_unlock_irqrestore(&dev->se_tmr_lock, flags);
@@ -246,7 +262,11 @@ static void core_tmr_drain_tmr_list(
(preempt_and_abort_list) ? "Preempt" : "", tmr_p,
tmr_p->function, tmr_p->response, cmd->t_state);
+ cancel_work_sync(&cmd->work);
+ transport_wait_for_tasks(cmd);
+
transport_cmd_finish_abort(cmd, 1);
+ target_put_sess_cmd(cmd);
}
}
diff --git a/drivers/target/target_core_transport.c
b/drivers/target/target_core_transport.c
index af52f8b..94e372a 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -2900,8 +2900,17 @@ static void target_tmr_work(struct work_struct *work)
struct se_cmd *cmd = container_of(work, struct se_cmd, work);
struct se_device *dev = cmd->se_dev;
struct se_tmr_req *tmr = cmd->se_tmr_req;
+ unsigned long flags;
int ret;
+ spin_lock_irqsave(&cmd->t_state_lock, flags);
+ if (cmd->transport_state & CMD_T_ABORTED) {
+ tmr->response = TMR_FUNCTION_REJECTED;
+ spin_unlock_irqrestore(&cmd->t_state_lock, flags);
+ goto check_stop;
+ }
+ spin_unlock_irqrestore(&cmd->t_state_lock, flags);
+
switch (tmr->function) {
case TMR_ABORT_TASK:
core_tmr_abort_task(dev, tmr, cmd->se_sess);
@@ -2934,9 +2943,17 @@ static void target_tmr_work(struct work_struct *work)
break;
}
+ spin_lock_irqsave(&cmd->t_state_lock, flags);
+ if (cmd->trans
[PATCH-v4 1/5] target: Fix LUN_RESET active I/O handling for ACK_KREF
From: Nicholas Bellinger
This patch fixes a NULL pointer se_cmd->cmd_kref < 0
refcount bug during TMR LUN_RESET with active se_cmd
I/O, that can be triggered during se_cmd descriptor
shutdown + release via core_tmr_drain_state_list() code.
To address this bug, add common __target_check_io_state()
helper for ABORT_TASK + LUN_RESET w/ CMD_T_COMPLETE
checking, and set CMD_T_ABORTED + obtain ->cmd_kref for
both cases ahead of last target_put_sess_cmd() after
TFO->aborted_task() -> transport_cmd_finish_abort()
callback has completed.
It also introduces SCF_ACK_KREF to determine when
transport_cmd_finish_abort() needs to drop the second
extra reference, ahead of calling target_put_sess_cmd()
for the final kref_put(&se_cmd->cmd_kref).
It also updates transport_cmd_check_stop() to avoid
holding se_cmd->t_state_lock while dropping se_cmd
device state via target_remove_from_state_list(), now
that core_tmr_drain_state_list() is holding the
se_device lock while checking se_cmd state from
within TMR logic.
Finally, move transport_put_cmd() release of SGL +
TMR + extended CDB memory into target_free_cmd_mem()
in order to avoid potential resource leaks in TMR
ABORT_TASK + LUN_RESET code-paths. Also update
target_release_cmd_kref() accordingly.
Reviewed-by: Quinn Tran
Cc: Himanshu Madhani
Cc: Sagi Grimberg
Cc: Christoph Hellwig
Cc: Hannes Reinecke
Cc: Andy Grover
Cc: Mike Christie
Cc: sta...@vger.kernel.org # 3.10+
Signed-off-by: Nicholas Bellinger
---
drivers/target/target_core_tmr.c | 69 ++
drivers/target/target_core_transport.c | 67 ++---
include/target/target_core_base.h | 1 +
3 files changed, 76 insertions(+), 61 deletions(-)
diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
index fcdcb11..fb3decc 100644
--- a/drivers/target/target_core_tmr.c
+++ b/drivers/target/target_core_tmr.c
@@ -107,6 +107,34 @@ static int target_check_cdb_and_preempt(struct list_head
*list,
return 1;
}
+static bool __target_check_io_state(struct se_cmd *se_cmd)
+{
+ struct se_session *sess = se_cmd->se_sess;
+
+ assert_spin_locked(&sess->sess_cmd_lock);
+ WARN_ON_ONCE(!irqs_disabled());
+ /*
+* If command already reached CMD_T_COMPLETE state within
+* target_complete_cmd(), this se_cmd has been passed to
+* fabric driver and will not be aborted.
+*
+* Otherwise, obtain a local se_cmd->cmd_kref now for TMR
+* ABORT_TASK + LUN_RESET for CMD_T_ABORTED processing as
+* long as se_cmd->cmd_kref is still active unless zero.
+*/
+ spin_lock(&se_cmd->t_state_lock);
+ if (se_cmd->transport_state & CMD_T_COMPLETE) {
+ pr_debug("Attempted to abort io tag: %llu already complete,"
+ " skipping\n", se_cmd->tag);
+ spin_unlock(&se_cmd->t_state_lock);
+ return false;
+ }
+ se_cmd->transport_state |= CMD_T_ABORTED;
+ spin_unlock(&se_cmd->t_state_lock);
+
+ return kref_get_unless_zero(&se_cmd->cmd_kref);
+}
+
void core_tmr_abort_task(
struct se_device *dev,
struct se_tmr_req *tmr,
@@ -130,34 +158,22 @@ void core_tmr_abort_task(
if (tmr->ref_task_tag != ref_tag)
continue;
- if (!kref_get_unless_zero(&se_cmd->cmd_kref))
- continue;
-
printk("ABORT_TASK: Found referenced %s task_tag: %llu\n",
se_cmd->se_tfo->get_fabric_name(), ref_tag);
- spin_lock(&se_cmd->t_state_lock);
- if (se_cmd->transport_state & CMD_T_COMPLETE) {
- printk("ABORT_TASK: ref_tag: %llu already complete,"
- " skipping\n", ref_tag);
- spin_unlock(&se_cmd->t_state_lock);
+ if (!__target_check_io_state(se_cmd)) {
spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
-
target_put_sess_cmd(se_cmd);
-
goto out;
}
- se_cmd->transport_state |= CMD_T_ABORTED;
- spin_unlock(&se_cmd->t_state_lock);
-
list_del_init(&se_cmd->se_cmd_list);
spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
cancel_work_sync(&se_cmd->work);
transport_wait_for_tasks(se_cmd);
- target_put_sess_cmd(se_cmd);
transport_cmd_finish_abort(se_cmd, true);
+ target_put_sess_cmd(se_cmd);
printk("ABORT_TASK: Sending TMR_FUNCTION_COMPLETE for"
" ref_tag: %llu\n", ref_tag);
@@ -242,8 +258,10 @@ static void core_tmr_drain_state_list(
struct list_head *preempt_and_abort_list)
{
LIST_HEAD(drain_task_list);
+ struct se_session *sess;
struct se_cmd *cmd, *next;
un
[PATCH-v4 0/5] Fix LUN_RESET active I/O + TMR handling
From: Nicholas Bellinger Hi folks, Here is -v4 series to address the set of of LUN_RESET active I/O + TMR se_cmd->cmd_kref < 0 bugs as reported recently by Quinn & Co. This can occur during active I/O remote port TMR LUN_RESET with multi-port LIO configurations. To address this bug, we add a __target_check_io_state() common handler for ABORT_TASK + LUN_RESET I/O abort cases, and move the remaining se_cmd SGL page + release into target_free_cmd_mem() to now be called directly from final target_release_cmd_kref() callback. It also adds a target_wait_free_cmd() helper and makes transport_generic_free_cmd() aware of CMD_T_ABORTED status during concurrent session disconnects, and introduces CMD_T_FABRIC_STOP bit to signal this special case. Currently this series is running atop v4.5-rc1 + v3.14.y, and with iscsi-target ports is able to survive active I/O remote-port LUN resets, plus remote-port LUN_RESET with concurrent simulated session disconnects. At this point the changes are stable with iscsi-target ports, and as Himanshu + Co can verify with tcm_qla2xxx should be considered ready to merge for -rc4. Please review + test. --nab v4 changes: - Add explicit CMD_T_FABRIC_STOP check and drop cmd_wait_set bit set usage in __target_check_io_state(). - Set early CMD_T_TAS in __target_check_io_state to avoid potential race in transport_send_task_abort() with shutdown. - Add fabric_stop + aborted checks in __transport_wait_for_tasks() in order to let TMR CMD_T_ABORTED se_cmd shutdown complete during concurrent session disconnect. - Fix race with driver SCF_SEND_DELAYED_TAS handling when __transport_check_aborted_status() could happen before transport_send_task_abort() in TMR kthread context. Nicholas Bellinger (5): target: Fix LUN_RESET active I/O handling for ACK_KREF target: Fix LUN_RESET active TMR descriptor handling target: Fix TAS handling for multi-session se_node_acls target: Fix remote-port TMR ABORT + se_cmd fabric stop target: Fix race with SCF_SEND_DELAYED_TAS handling drivers/target/target_core_tmr.c | 139 - drivers/target/target_core_transport.c | 278 +++-- include/target/target_core_base.h | 3 + 3 files changed, 301 insertions(+), 119 deletions(-) -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH-v4 4/5] target: Fix remote-port TMR ABORT + se_cmd fabric stop
From: Nicholas Bellinger
To address the bug where fabric driver level shutdown
of se_cmd occurs at the same time when TMR CMD_T_ABORTED
is happening resulting in a -1 ->cmd_kref, this patch
adds a CMD_T_FABRIC_STOP bit that is used to determine
when TMR + driver I_T nexus shutdown is happening
concurrently.
It changes target_sess_cmd_list_set_waiting() to obtain
se_cmd->cmd_kref + set CMD_T_FABRIC_STOP, and drop local
reference in target_wait_for_sess_cmds() and invoke extra
target_put_sess_cmd() during Task Aborted Status (TAS)
when necessary.
Also, it adds a new target_wait_free_cmd() wrapper around
transport_wait_for_tasks() for the special case within
transport_generic_free_cmd() to set CMD_T_FABRIC_STOP,
and is now aware of CMD_T_ABORTED + CMD_T_TAS status
bits to know when an extra transport_put_cmd() during
TAS is required.
Note transport_generic_free_cmd() is expected to block on
cmd->cmd_wait_comp in order to follow what iscsi-target
expects during iscsi_conn context se_cmd shutdown.
Cc: Quinn Tran
Cc: Himanshu Madhani
Cc: Sagi Grimberg
Cc: Christoph Hellwig
Cc: Hannes Reinecke
Cc: Andy Grover
Cc: Mike Christie
Cc: sta...@vger.kernel.org # 3.10+
Signed-off-by: Nicholas Bellinger
---
drivers/target/target_core_tmr.c | 54
drivers/target/target_core_transport.c | 145 +
include/target/target_core_base.h | 2 +
3 files changed, 150 insertions(+), 51 deletions(-)
diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
index 3e0d77a..82a663b 100644
--- a/drivers/target/target_core_tmr.c
+++ b/drivers/target/target_core_tmr.c
@@ -75,16 +75,18 @@ void core_tmr_release_req(struct se_tmr_req *tmr)
kfree(tmr);
}
-static void core_tmr_handle_tas_abort(
- struct se_session *tmr_sess,
- struct se_cmd *cmd,
- int tas)
+static void core_tmr_handle_tas_abort(struct se_cmd *cmd, int tas)
{
- bool remove = true;
+ unsigned long flags;
+ bool remove = true, send_tas;
/*
* TASK ABORTED status (TAS) bit support
*/
- if (tmr_sess && tmr_sess != cmd->se_sess && tas) {
+ spin_lock_irqsave(&cmd->t_state_lock, flags);
+ send_tas = (cmd->transport_state & CMD_T_TAS);
+ spin_unlock_irqrestore(&cmd->t_state_lock, flags);
+
+ if (send_tas) {
remove = false;
transport_send_task_abort(cmd);
}
@@ -107,7 +109,8 @@ static int target_check_cdb_and_preempt(struct list_head
*list,
return 1;
}
-static bool __target_check_io_state(struct se_cmd *se_cmd)
+static bool __target_check_io_state(struct se_cmd *se_cmd,
+ struct se_session *tmr_sess, int tas)
{
struct se_session *sess = se_cmd->se_sess;
@@ -115,21 +118,32 @@ static bool __target_check_io_state(struct se_cmd *se_cmd)
WARN_ON_ONCE(!irqs_disabled());
/*
* If command already reached CMD_T_COMPLETE state within
-* target_complete_cmd(), this se_cmd has been passed to
-* fabric driver and will not be aborted.
+* target_complete_cmd() or CMD_T_FABRIC_STOP due to shutdown,
+* this se_cmd has been passed to fabric driver and will
+* not be aborted.
*
* Otherwise, obtain a local se_cmd->cmd_kref now for TMR
* ABORT_TASK + LUN_RESET for CMD_T_ABORTED processing as
* long as se_cmd->cmd_kref is still active unless zero.
*/
spin_lock(&se_cmd->t_state_lock);
- if (se_cmd->transport_state & CMD_T_COMPLETE) {
- pr_debug("Attempted to abort io tag: %llu already complete,"
+ if (se_cmd->transport_state & (CMD_T_COMPLETE | CMD_T_FABRIC_STOP)) {
+ pr_debug("Attempted to abort io tag: %llu already complete or"
+ " fabric stop, skipping\n", se_cmd->tag);
+ spin_unlock(&se_cmd->t_state_lock);
+ return false;
+ }
+ if (sess->sess_tearing_down || se_cmd->cmd_wait_set) {
+ pr_debug("Attempted to abort io tag: %llu already shutdown,"
" skipping\n", se_cmd->tag);
spin_unlock(&se_cmd->t_state_lock);
return false;
}
se_cmd->transport_state |= CMD_T_ABORTED;
+
+ if ((tmr_sess != se_cmd->se_sess) && tas)
+ se_cmd->transport_state |= CMD_T_TAS;
+
spin_unlock(&se_cmd->t_state_lock);
return kref_get_unless_zero(&se_cmd->cmd_kref);
@@ -161,7 +175,7 @@ void core_tmr_abort_task(
printk("ABORT_TASK: Found referenced %s task_tag: %llu\n",
se_cmd->se_tfo->get_fabric_name(), ref_tag);
- if (!__target_check_io_state(se_cmd)) {
+ if (!__target_check_io_state(se_cmd, se_sess, 0)) {
spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
target_put_sess_cmd(se_cmd);
[PATCH-v4 3/5] target: Fix TAS handling for multi-session se_node_acls
From: Nicholas Bellinger
This patch fixes a bug in TMR task aborted status (TAS)
handling when multiple sessions are connected to the
same target WWPN endpoint and se_node_acl descriptor,
resulting in TASK_ABORTED status to not be generated
for aborted se_cmds on the remote port.
This is due to core_tmr_handle_tas_abort() incorrectly
comparing se_node_acl instead of se_session, for which
the multi-session case is expected to be sharing the
same se_node_acl.
Instead, go ahead and update core_tmr_handle_tas_abort()
to compare tmr_sess + cmd->se_sess in order to determine
if the LUN_RESET was received on a different I_T nexus,
and TASK_ABORTED status response needs to be generated.
Reviewed-by: Christoph Hellwig
Cc: Quinn Tran
Cc: Himanshu Madhani
Cc: Sagi Grimberg
Cc: Hannes Reinecke
Cc: Andy Grover
Cc: Mike Christie
Cc: sta...@vger.kernel.org # 3.10+
Signed-off-by: Nicholas Bellinger
---
drivers/target/target_core_tmr.c | 16 +---
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
index 072af07..3e0d77a 100644
--- a/drivers/target/target_core_tmr.c
+++ b/drivers/target/target_core_tmr.c
@@ -76,7 +76,7 @@ void core_tmr_release_req(struct se_tmr_req *tmr)
}
static void core_tmr_handle_tas_abort(
- struct se_node_acl *tmr_nacl,
+ struct se_session *tmr_sess,
struct se_cmd *cmd,
int tas)
{
@@ -84,7 +84,7 @@ static void core_tmr_handle_tas_abort(
/*
* TASK ABORTED status (TAS) bit support
*/
- if ((tmr_nacl && (tmr_nacl != cmd->se_sess->se_node_acl)) && tas) {
+ if (tmr_sess && tmr_sess != cmd->se_sess && tas) {
remove = false;
transport_send_task_abort(cmd);
}
@@ -273,7 +273,7 @@ static void core_tmr_drain_tmr_list(
static void core_tmr_drain_state_list(
struct se_device *dev,
struct se_cmd *prout_cmd,
- struct se_node_acl *tmr_nacl,
+ struct se_session *tmr_sess,
int tas,
struct list_head *preempt_and_abort_list)
{
@@ -364,7 +364,7 @@ static void core_tmr_drain_state_list(
cancel_work_sync(&cmd->work);
transport_wait_for_tasks(cmd);
- core_tmr_handle_tas_abort(tmr_nacl, cmd, tas);
+ core_tmr_handle_tas_abort(tmr_sess, cmd, tas);
target_put_sess_cmd(cmd);
}
}
@@ -377,6 +377,7 @@ int core_tmr_lun_reset(
{
struct se_node_acl *tmr_nacl = NULL;
struct se_portal_group *tmr_tpg = NULL;
+ struct se_session *tmr_sess = NULL;
int tas;
/*
* TASK_ABORTED status bit, this is configurable via ConfigFS
@@ -395,8 +396,9 @@ int core_tmr_lun_reset(
* or struct se_device passthrough..
*/
if (tmr && tmr->task_cmd && tmr->task_cmd->se_sess) {
- tmr_nacl = tmr->task_cmd->se_sess->se_node_acl;
- tmr_tpg = tmr->task_cmd->se_sess->se_tpg;
+ tmr_sess = tmr->task_cmd->se_sess;
+ tmr_nacl = tmr_sess->se_node_acl;
+ tmr_tpg = tmr_sess->se_tpg;
if (tmr_nacl && tmr_tpg) {
pr_debug("LUN_RESET: TMR caller fabric: %s"
" initiator port %s\n",
@@ -409,7 +411,7 @@ int core_tmr_lun_reset(
dev->transport->name, tas);
core_tmr_drain_tmr_list(dev, tmr, preempt_and_abort_list);
- core_tmr_drain_state_list(dev, prout_cmd, tmr_nacl, tas,
+ core_tmr_drain_state_list(dev, prout_cmd, tmr_sess, tas,
preempt_and_abort_list);
/*
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH-v4 5/5] target: Fix race with SCF_SEND_DELAYED_TAS handling
From: Nicholas Bellinger
This patch fixes a race between setting of SCF_SEND_DELAYED_TAS
in transport_send_task_abort(), and check of the same bit in
transport_check_aborted_status().
It adds a __transport_check_aborted_status() version that is
used by target_execute_cmd() when se_cmd->t_state_lock is
held, and a transport_check_aborted_status() wrapper for
all other existing callers.
Also, it handles the case where the check happens before
transport_send_task_abort() gets called. For this, go
ahead and set SCF_SEND_DELAYED_TAS early when necessary,
and have transport_send_task_abort() send the abort.
Cc: Quinn Tran
Cc: Himanshu Madhani
Cc: Sagi Grimberg
Cc: Christoph Hellwig
Cc: Hannes Reinecke
Cc: Andy Grover
Cc: Mike Christie
Cc: sta...@vger.kernel.org # 3.10+
Signed-off-by: Nicholas Bellinger
---
drivers/target/target_core_transport.c | 53 ++
1 file changed, 41 insertions(+), 12 deletions(-)
diff --git a/drivers/target/target_core_transport.c
b/drivers/target/target_core_transport.c
index 3441b15..2e0b23a 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1858,19 +1858,21 @@ static bool target_handle_task_attr(struct se_cmd *cmd)
return true;
}
+static int __transport_check_aborted_status(struct se_cmd *, int);
+
void target_execute_cmd(struct se_cmd *cmd)
{
/*
-* If the received CDB has aleady been aborted stop processing it here.
-*/
- if (transport_check_aborted_status(cmd, 1))
- return;
-
- /*
* Determine if frontend context caller is requesting the stopping of
* this command for frontend exceptions.
+*
+* If the received CDB has aleady been aborted stop processing it here.
*/
spin_lock_irq(&cmd->t_state_lock);
+ if (__transport_check_aborted_status(cmd, 1)) {
+ spin_unlock_irq(&cmd->t_state_lock);
+ return;
+ }
if (cmd->transport_state & CMD_T_STOP) {
pr_debug("%s:%d CMD_T_STOP for ITT: 0x%08llx\n",
__func__, __LINE__, cmd->tag);
@@ -2911,28 +2913,49 @@ transport_send_check_condition_and_sense(struct se_cmd
*cmd,
}
EXPORT_SYMBOL(transport_send_check_condition_and_sense);
-int transport_check_aborted_status(struct se_cmd *cmd, int send_status)
+static int __transport_check_aborted_status(struct se_cmd *cmd, int
send_status)
+ __releases(&cmd->t_state_lock)
+ __acquires(&cmd->t_state_lock)
{
+ assert_spin_locked(&cmd->t_state_lock);
+ WARN_ON_ONCE(!irqs_disabled());
+
if (!(cmd->transport_state & CMD_T_ABORTED))
return 0;
-
/*
* If cmd has been aborted but either no status is to be sent or it has
* already been sent, just return
*/
- if (!send_status || !(cmd->se_cmd_flags & SCF_SEND_DELAYED_TAS))
+ if (!send_status || !(cmd->se_cmd_flags & SCF_SEND_DELAYED_TAS)) {
+ if (send_status)
+ cmd->se_cmd_flags |= SCF_SEND_DELAYED_TAS;
return 1;
+ }
- pr_debug("Sending delayed SAM_STAT_TASK_ABORTED status for CDB: 0x%02x
ITT: 0x%08llx\n",
-cmd->t_task_cdb[0], cmd->tag);
+ pr_debug("Sending delayed SAM_STAT_TASK_ABORTED status for CDB:"
+ " 0x%02x ITT: 0x%08llx\n", cmd->t_task_cdb[0], cmd->tag);
cmd->se_cmd_flags &= ~SCF_SEND_DELAYED_TAS;
cmd->scsi_status = SAM_STAT_TASK_ABORTED;
trace_target_cmd_complete(cmd);
+
+ spin_unlock_irq(&cmd->t_state_lock);
cmd->se_tfo->queue_status(cmd);
+ spin_lock_irq(&cmd->t_state_lock);
return 1;
}
+
+int transport_check_aborted_status(struct se_cmd *cmd, int send_status)
+{
+ int ret;
+
+ spin_lock_irq(&cmd->t_state_lock);
+ ret = __transport_check_aborted_status(cmd, send_status);
+ spin_unlock_irq(&cmd->t_state_lock);
+
+ return ret;
+}
EXPORT_SYMBOL(transport_check_aborted_status);
void transport_send_task_abort(struct se_cmd *cmd)
@@ -2954,11 +2977,17 @@ void transport_send_task_abort(struct se_cmd *cmd)
*/
if (cmd->data_direction == DMA_TO_DEVICE) {
if (cmd->se_tfo->write_pending_status(cmd) != 0) {
- cmd->transport_state |= CMD_T_ABORTED;
+ spin_lock_irqsave(&cmd->t_state_lock, flags);
+ if (cmd->se_cmd_flags & SCF_SEND_DELAYED_TAS) {
+ spin_unlock_irqrestore(&cmd->t_state_lock,
flags);
+ goto send_abort;
+ }
cmd->se_cmd_flags |= SCF_SEND_DELAYED_TAS;
+ spin_unlock_irqrestore(&cmd->t_state_lock, flags);
return;
}
}
+send_abort:
cmd->scsi_status = SAM_STAT_TASK_ABORTED;
transport_lun_remove_cmd(
complete boot failure in 4.5-rc1 caused by nvme: make SG_IO support optional
The reason is fairly obvious: the default for the new option BLK_DEV_NVME_SCSI is N and all the distribution kernels (and me when testing) take the default options (I checked in the OBS kernel builds and this is true). The net result is that scsi_id from udev no longer works on nvme disks and that means that the /dev/disk/by-id links are all gone in 4.5-rc1. If this happens to be how you name your disks in fstab or crypttab, you can't boot. If you're going to break an ABI in this way, you really have to plan for it in userspace. How are NVMe disk ids supposed to be exported now? Does udev need a nvme_id program to do this? Until there's an infrastructure ready to work in this way, we need to unconditionally enable BLK_DEV_NVME_SCSI. James -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
