[PATCH 0/2] target: Fix v4.19-rc active I/O shutdown deadlock

[PATCH 0/2] target: Fix v4.19-rc active I/O shutdown deadlock

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

Hi MNC, MKP & Co,

While testing v4.19-rc recently with simple backend I/O error injection
(via delayed BIO completion), I was able to trigger an end-less loop
deadlock with recent changes in commit 00d909a107:

  Author: Bart Van Assche <bart.vanassche@wdc.com>
  Date:   Fri Jun 22 14:52:53 2018 -0700

      scsi: target: Make the session shutdown code also wait for commands that are being aborted

It comes down to an incorrect assumption wrt signals during session
shutdown plus active I/O quiesce, which triggers an endless loop
immediately during session shutdown as se_session->sess_list_wq
waits for outstanding backend I/O to complete.

The easiest reproduction is with iser-target or simulation with plain
old iscsi-target/TCP ports.  However, any fabric driver who triggers
session shutdown from user-space processes with signals pending can
easily trigger it and bring down the machine.

The fix is simple, but requires a new wait_event_lock_irq_timeout()
macro to allow TASK_UNINTERRUPTIBLE to be set in order to work as
expected for all fabric driver session shutdown cases.

So short of reverting commit 00d909a107 now for v4.19, this is going
to be the best option.

Please review for v4.19, or v4.20-rc1 with stable CC's for both.

Thank you.

Nicholas Bellinger (2):
  sched/wait: Add wait_event_lock_irq_timeout for TASK_UNINTERRUPTIBLE
    usage
  target: Fix target_wait_for_sess_cmds breakage with active signals

 drivers/target/target_core_transport.c |  4 ++--
 include/linux/wait.h                   | 20 +++++++++++++++-----
 2 files changed, 17 insertions(+), 7 deletions(-)

-- 
1.9.1

[PATCH 1/2] sched/wait: Add wait_event_lock_irq_timeout for TASK_UNINTERRUPTIBLE usage

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

Short of reverting commit 00d909a107 for v4.19, target-core needs a
wait_event_t marco can be executed using TASK_UNINTERRUPTIBLE to
function correctly with existing fabric drivers that expect to run
with signals pending during session shutdown and active se_cmd I/O
quiesce.

The most notable is iscsi-target/iser-target, while ibmvscsi_tgt invokes
session shutdown logic from userspace via configfs attribute that could
also potentially have signals pending.

So go ahead and introduce wait_event_lock_irq_timeout() to achieve this,
and update + rename __wait_event_lock_irq_timeout() to make it accept
'state' as a parameter.

Fixes: 00d909a107 ("scsi: target: Make the session shutdown code also wait for commands that are being aborted")
Cc: Bart Van Assche <bvanassche@acm.org>
Cc: Mike Christie <mchristi@redhat.com>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Sagi Grimberg <sagi@grimberg.me>
Cc: Bryant G. Ly <bryantly@linux.vnet.ibm.com>
Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
Tested-by: Nicholas Bellinger <nab@linux-iscsi.org>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 include/linux/wait.h | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/include/linux/wait.h b/include/linux/wait.h
index d9f131e..ed7c122 100644
--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -1052,10 +1052,9 @@ void __wake_up_locked_key_bookmark(struct wait_queue_head *wq_head,
 	__ret;									\
 })
 
-#define __wait_event_interruptible_lock_irq_timeout(wq_head, condition,		\
-						    lock, timeout)		\
+#define __wait_event_lock_irq_timeout(wq_head, condition, lock, timeout, state)	\
 	___wait_event(wq_head, ___wait_cond_timeout(condition),			\
-		      TASK_INTERRUPTIBLE, 0, timeout,				\
+		      state, 0, timeout,					\
 		      spin_unlock_irq(&lock);					\
 		      __ret = schedule_timeout(__ret);				\
 		      spin_lock_irq(&lock));
@@ -1089,8 +1088,19 @@ void __wake_up_locked_key_bookmark(struct wait_queue_head *wq_head,
 ({										\
 	long __ret = timeout;							\
 	if (!___wait_cond_timeout(condition))					\
-		__ret = __wait_event_interruptible_lock_irq_timeout(		\
-					wq_head, condition, lock, timeout);	\
+		__ret = __wait_event_lock_irq_timeout(				\
+					wq_head, condition, lock, timeout,	\
+					TASK_INTERRUPTIBLE);			\
+	__ret;									\
+})
+
+#define wait_event_lock_irq_timeout(wq_head, condition, lock, timeout)		\
+({										\
+	long __ret = timeout;							\
+	if (!___wait_cond_timeout(condition))					\
+		__ret = __wait_event_lock_irq_timeout(				\
+					wq_head, condition, lock, timeout,	\
+					TASK_UNINTERRUPTIBLE);			\
 	__ret;									\
 })
 
-- 
1.9.1

Re: [PATCH 1/2] sched/wait: Add wait_event_lock_irq_timeout for TASK_UNINTERRUPTIBLE usage

[Ly, Bryant]

Hi Nic, 

Good to see you back!

> On Oct 9, 2018, at 10:23 PM, Nicholas A. Bellinger <nab@linux-iscsi.org> wrote:
> 
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> Short of reverting commit 00d909a107 for v4.19, target-core needs a
> wait_event_t marco can be executed using TASK_UNINTERRUPTIBLE to
> function correctly with existing fabric drivers that expect to run
> with signals pending during session shutdown and active se_cmd I/O
> quiesce.
> 
> The most notable is iscsi-target/iser-target, while ibmvscsi_tgt invokes
> session shutdown logic from userspace via configfs attribute that could
> also potentially have signals pending.
> 
> So go ahead and introduce wait_event_lock_irq_timeout() to achieve this,
> and update + rename __wait_event_lock_irq_timeout() to make it accept
> 'state' as a parameter.
> 
> Fixes: 00d909a107 ("scsi: target: Make the session shutdown code also wait for commands that are being aborted")
> Cc: Bart Van Assche <bvanassche@acm.org>
> Cc: Mike Christie <mchristi@redhat.com>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Sagi Grimberg <sagi@grimberg.me>
> Cc: Bryant G. Ly <bryantly@linux.vnet.ibm.com>
> Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
> Tested-by: Nicholas Bellinger <nab@linux-iscsi.org>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> ---
> include/linux/wait.h | 20 +++++++++++++++-----
> 1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/wait.h b/include/linux/wait.h
> index d9f131e..ed7c122 100644
> --- a/include/linux/wait.h
> +++ b/include/linux/wait.h
> @@ -1052,10 +1052,9 @@ void __wake_up_locked_key_bookmark(struct wait_queue_head *wq_head,
> 	__ret;									\
> })
> 
>  

I do not technically work for IBM anymore as of 3 weeks ago, but my new company is a partner… Anyways,
IBM does invoke shutdown logic from userspace. Thanks for the patch!

Reviewed-by: Bryant G. Ly <bly@catalogicsoftware.com>

-Bryant Ly

Re: [PATCH 1/2] sched/wait: Add wait_event_lock_irq_timeout for TASK_UNINTERRUPTIBLE usage

[Peter Zijlstra]

On Wed, Oct 10, 2018 at 03:23:09AM +0000, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> Short of reverting commit 00d909a107 for v4.19, target-core needs a
> wait_event_t marco can be executed using TASK_UNINTERRUPTIBLE to
> function correctly with existing fabric drivers that expect to run
> with signals pending during session shutdown and active se_cmd I/O
> quiesce.
> 
> The most notable is iscsi-target/iser-target, while ibmvscsi_tgt invokes
> session shutdown logic from userspace via configfs attribute that could
> also potentially have signals pending.
> 
> So go ahead and introduce wait_event_lock_irq_timeout() to achieve this,
> and update + rename __wait_event_lock_irq_timeout() to make it accept
> 'state' as a parameter.
> 
> Fixes: 00d909a107 ("scsi: target: Make the session shutdown code also wait for commands that are being aborted")
> Cc: Bart Van Assche <bvanassche@acm.org>
> Cc: Mike Christie <mchristi@redhat.com>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Sagi Grimberg <sagi@grimberg.me>
> Cc: Bryant G. Ly <bryantly@linux.vnet.ibm.com>
> Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
> Tested-by: Nicholas Bellinger <nab@linux-iscsi.org>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>

Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>

Re: [PATCH 1/2] sched/wait: Add wait_event_lock_irq_timeout for TASK_UNINTERRUPTIBLE usage

[Bart Van Assche]

On 10/9/18 8:23 PM, Nicholas A. Bellinger wrote:
> Short of reverting commit 00d909a107 for v4.19, target-core needs a
> wait_event_t marco can be executed using TASK_UNINTERRUPTIBLE to
> function correctly with existing fabric drivers that expect to run
> with signals pending during session shutdown and active se_cmd I/O
> quiesce.
> 
> The most notable is iscsi-target/iser-target, while ibmvscsi_tgt invokes
> session shutdown logic from userspace via configfs attribute that could
> also potentially have signals pending.
> 
> So go ahead and introduce wait_event_lock_irq_timeout() to achieve this,
> and update + rename __wait_event_lock_irq_timeout() to make it accept
> 'state' as a parameter.
> 
> Fixes: 00d909a107 ("scsi: target: Make the session shutdown code also wait for commands that are being aborted")
> Cc: Bart Van Assche <bvanassche@acm.org>
> Cc: Mike Christie <mchristi@redhat.com>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Sagi Grimberg <sagi@grimberg.me>
> Cc: Bryant G. Ly <bryantly@linux.vnet.ibm.com>
> Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
> Tested-by: Nicholas Bellinger <nab@linux-iscsi.org>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

[PATCH 2/2] target: Fix target_wait_for_sess_cmds breakage with active signals

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

With the addition of commit 00d909a107 in v4.19-rc, it incorrectly assumes no
signals will be pending for task_struct executing the normal session shutdown
and I/O quiesce code-path.

For example, iscsi-target and iser-target issue SIGINT to all kthreads as
part of session shutdown.  This has been the behaviour since day one.

As-is when signals are pending with se_cmds active in se_sess->sess_cmd_list,
wait_event_interruptible_lock_irq_timeout() returns a negative number and
immediately kills the machine because of the do while (ret <= 0) loop that
was added in commit 00d909a107 to spin while backend I/O is taking any
amount of extended time (say 30 seconds) to complete.

Here's what it looks like in action with debug plus delayed backend I/O
completion:

[ 4951.909951] se_sess: 000000003e7e08fa before target_wait_for_sess_cmds
[ 4951.914600] target_wait_for_sess_cmds: signal_pending: 1
[ 4951.918015] wait_event_interruptible_lock_irq_timeout ret: -512 signal_pending: 1 loop count: 0
[ 4951.921639] wait_event_interruptible_lock_irq_timeout ret: -512 signal_pending: 1 loop count: 1
[ 4951.921944] wait_event_interruptible_lock_irq_timeout ret: -512 signal_pending: 1 loop count: 2
[ 4951.921944] wait_event_interruptible_lock_irq_timeout ret: -512 signal_pending: 1 loop count: 3
[ 4951.921944] wait_event_interruptible_lock_irq_timeout ret: -512 signal_pending: 1 loop count: 4
[ 4951.921944] wait_event_interruptible_lock_irq_timeout ret: -512 signal_pending: 1 loop count: 5
[ 4951.921944] wait_event_interruptible_lock_irq_timeout ret: -512 signal_pending: 1 loop count: 6
[ 4951.921944] wait_event_interruptible_lock_irq_timeout ret: -512 signal_pending: 1 loop count: 7
[ 4951.921944] wait_event_interruptible_lock_irq_timeout ret: -512 signal_pending: 1 loop count: 8
[ 4951.921944] wait_event_interruptible_lock_irq_timeout ret: -512 signal_pending: 1 loop count: 9

... followed by the usual RCU CPU stalls and deadlock.

There was never a case pre commit 00d909a107 where wait_for_complete(&se_cmd->cmd_wait_comp)
was able to be interruptted, so to address this for v4.19+ moving forward go ahead and
use wait_event_lock_irq_timeout() instead so new code works with all fabric drivers.

Also for commit 00d909a107, fix a minor regression in target_release_cmd_kref()
to only wake_up the new se_sess->cmd_list_wq only when shutdown has actually
been triggered via se_sess->sess_tearing_down.

Fixes: 00d909a107 ("scsi: target: Make the session shutdown code also wait for commands that are being aborted")
Cc: Bart Van Assche <bvanassche@acm.org>
Cc: Mike Christie <mchristi@redhat.com>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Sagi Grimberg <sagi@grimberg.me>
Cc: Bryant G. Ly <bryantly@linux.vnet.ibm.com>
Tested-by: Nicholas Bellinger <nab@linux-iscsi.org>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_transport.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 86c0156..fc3093d2 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -2754,7 +2754,7 @@ static void target_release_cmd_kref(struct kref *kref)
 	if (se_sess) {
 		spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
 		list_del_init(&se_cmd->se_cmd_list);
-		if (list_empty(&se_sess->sess_cmd_list))
+		if (se_sess->sess_tearing_down && list_empty(&se_sess->sess_cmd_list))
 			wake_up(&se_sess->cmd_list_wq);
 		spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
 	}
@@ -2907,7 +2907,7 @@ void target_wait_for_sess_cmds(struct se_session *se_sess)
 
 	spin_lock_irq(&se_sess->sess_cmd_lock);
 	do {
-		ret = wait_event_interruptible_lock_irq_timeout(
+		ret = wait_event_lock_irq_timeout(
 				se_sess->cmd_list_wq,
 				list_empty(&se_sess->sess_cmd_list),
 				se_sess->sess_cmd_lock, 180 * HZ);
-- 
1.9.1

Re: [PATCH 2/2] target: Fix target_wait_for_sess_cmds breakage with active signals

[Ly, Bryant]

> On Oct 9, 2018, at 10:23 PM, Nicholas A. Bellinger <nab@linux-iscsi.org> wrote:
> 
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> With the addition of commit 00d909a107 in v4.19-rc, it incorrectly assumes no
> signals will be pending for task_struct executing the normal session shutdown
> and I/O quiesce code-path.
> 
> For example, iscsi-target and iser-target issue SIGINT to all kthreads as
> part of session shutdown.  This has been the behaviour since day one.
> 
> As-is when signals are pending with se_cmds active in se_sess->sess_cmd_list,
> wait_event_interruptible_lock_irq_timeout() returns a negative number and
> immediately kills the machine because of the do while (ret <= 0) loop that
> was added in commit 00d909a107 to spin while backend I/O is taking any
> amount of extended time (say 30 seconds) to complete.
> 
> Here's what it looks like in action with debug plus delayed backend I/O
> completion:
> 
> [ 4951.909951] se_sess: 000000003e7e08fa before target_wait_for_sess_cmds
> [ 4951.914600] target_wait_for_sess_cmds: signal_pending: 1
> [ 4951.918015] wait_event_interruptible_lock_irq_timeout ret: -512 signal_pending: 1 loop count: 0
> [ 4951.921639] wait_event_interruptible_lock_irq_timeout ret: -512 signal_pending: 1 loop count: 1
> [ 4951.921944] wait_event_interruptible_lock_irq_timeout ret: -512 signal_pending: 1 loop count: 2
> [ 4951.921944] wait_event_interruptible_lock_irq_timeout ret: -512 signal_pending: 1 loop count: 3
> [ 4951.921944] wait_event_interruptible_lock_irq_timeout ret: -512 signal_pending: 1 loop count: 4
> [ 4951.921944] wait_event_interruptible_lock_irq_timeout ret: -512 signal_pending: 1 loop count: 5
> [ 4951.921944] wait_event_interruptible_lock_irq_timeout ret: -512 signal_pending: 1 loop count: 6
> [ 4951.921944] wait_event_interruptible_lock_irq_timeout ret: -512 signal_pending: 1 loop count: 7
> [ 4951.921944] wait_event_interruptible_lock_irq_timeout ret: -512 signal_pending: 1 loop count: 8
> [ 4951.921944] wait_event_interruptible_lock_irq_timeout ret: -512 signal_pending: 1 loop count: 9
> 
> ... followed by the usual RCU CPU stalls and deadlock.
> 
> There was never a case pre commit 00d909a107 where wait_for_complete(&se_cmd->cmd_wait_comp)
> was able to be interruptted, so to address this for v4.19+ moving forward go ahead and
> use wait_event_lock_irq_timeout() instead so new code works with all fabric drivers.
> 
> Also for commit 00d909a107, fix a minor regression in target_release_cmd_kref()
> to only wake_up the new se_sess->cmd_list_wq only when shutdown has actually
> been triggered via se_sess->sess_tearing_down.
> 
> Fixes: 00d909a107 ("scsi: target: Make the session shutdown code also wait for commands that are being aborted")
> Cc: Bart Van Assche <bvanassche@acm.org>
> Cc: Mike Christie <mchristi@redhat.com>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Sagi Grimberg <sagi@grimberg.me>
> Cc: Bryant G. Ly <bryantly@linux.vnet.ibm.com>
> Tested-by: Nicholas Bellinger <nab@linux-iscsi.org>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> ---
> drivers/target/target_core_transport.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
> 

Reviewed-by: Bryant G. Ly <bly@catalogicsoftware.com>

-Bryant

Re: [PATCH 2/2] target: Fix target_wait_for_sess_cmds breakage with active signals

[Bart Van Assche]

On 10/9/18 8:23 PM, Nicholas A. Bellinger wrote:
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 86c0156..fc3093d2 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -2754,7 +2754,7 @@ static void target_release_cmd_kref(struct kref *kref)
>   	if (se_sess) {
>   		spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
>   		list_del_init(&se_cmd->se_cmd_list);
> -		if (list_empty(&se_sess->sess_cmd_list))
> +		if (se_sess->sess_tearing_down && list_empty(&se_sess->sess_cmd_list))
>   			wake_up(&se_sess->cmd_list_wq);
>   		spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
>   	}
> @@ -2907,7 +2907,7 @@ void target_wait_for_sess_cmds(struct se_session *se_sess)
>   
>   	spin_lock_irq(&se_sess->sess_cmd_lock);
>   	do {
> -		ret = wait_event_interruptible_lock_irq_timeout(
> +		ret = wait_event_lock_irq_timeout(
>   				se_sess->cmd_list_wq,
>   				list_empty(&se_sess->sess_cmd_list),
>   				se_sess->sess_cmd_lock, 180 * HZ);

Since this patch addresses two different issues (performance improvement 
+ fix for a hang), I think it should be split in two patches.

Thanks,

Bart.

Re: [PATCH 2/2] target: Fix target_wait_for_sess_cmds breakage with active signals

[Peter Zijlstra]

On Wed, Oct 10, 2018 at 03:23:10AM +0000, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> With the addition of commit 00d909a107 in v4.19-rc, it incorrectly assumes no
> signals will be pending for task_struct executing the normal session shutdown
> and I/O quiesce code-path.
> 
> For example, iscsi-target and iser-target issue SIGINT to all kthreads as
> part of session shutdown.  This has been the behaviour since day one.

Not knowing much context here; but does it make sense for those
kthreads to handle signals, ever? Most kthreads should be fine with
ignore_signals().

Re: [PATCH 2/2] target: Fix target_wait_for_sess_cmds breakage with active signals

[Nicholas A. Bellinger]

Hey Peter & Co,

On Wed, 2018-10-10 at 10:43 +0200, Peter Zijlstra wrote:
> On Wed, Oct 10, 2018 at 03:23:10AM +0000, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@linux-iscsi.org>
> > 
> > With the addition of commit 00d909a107 in v4.19-rc, it incorrectly assumes no
> > signals will be pending for task_struct executing the normal session shutdown
> > and I/O quiesce code-path.
> > 
> > For example, iscsi-target and iser-target issue SIGINT to all kthreads as
> > part of session shutdown.  This has been the behaviour since day one.
> 
> Not knowing much context here; but does it make sense for those
> kthreads to handle signals, ever? Most kthreads should be fine with
> ignore_signals().
> 

iscsi-target + ib-isert uses SIGINT amongst dedicated rx/tx connection
kthreads to signal connection shutdown, requiring in-flight se_cmd I/O
descriptors to be quiesced before making forward progress to release
se_session.

By the point wait_event_lock_irq_timeout() is called in the example
here, one of the two rx/tx connection kthreads has been stopped, and the
other kthread is still processing shutdown.  So while historically the
pending SIGINTs where not cleared (or ignored) during shutdown at this
point, there is no reason why they could not be ignored for iscsi-target
+ ib-isert.

That said, pre commit 00d909a107 code always used wait_for_completion()
and ignored pending signals.  As-is target_wait_for_sess_cmds() is
called directly from fabric driver code and in one case also from
user-space via configfs_write_file(), so AFAICT it does need
TASK_UNINTERRUPTIBLE.

Re: [PATCH 2/2] target: Fix target_wait_for_sess_cmds breakage with active signals

[Peter Zijlstra]

On Wed, Oct 10, 2018 at 10:40:12PM -0700, Nicholas A. Bellinger wrote:
> Hey Peter & Co,
> 
> On Wed, 2018-10-10 at 10:43 +0200, Peter Zijlstra wrote:
> > On Wed, Oct 10, 2018 at 03:23:10AM +0000, Nicholas A. Bellinger wrote:
> > > From: Nicholas Bellinger <nab@linux-iscsi.org>
> > > 
> > > With the addition of commit 00d909a107 in v4.19-rc, it incorrectly assumes no
> > > signals will be pending for task_struct executing the normal session shutdown
> > > and I/O quiesce code-path.
> > > 
> > > For example, iscsi-target and iser-target issue SIGINT to all kthreads as
> > > part of session shutdown.  This has been the behaviour since day one.
> > 
> > Not knowing much context here; but does it make sense for those
> > kthreads to handle signals, ever? Most kthreads should be fine with
> > ignore_signals().
> > 
> 
> iscsi-target + ib-isert uses SIGINT amongst dedicated rx/tx connection
> kthreads to signal connection shutdown, requiring in-flight se_cmd I/O
> descriptors to be quiesced before making forward progress to release
> se_session.
> 
> By the point wait_event_lock_irq_timeout() is called in the example
> here, one of the two rx/tx connection kthreads has been stopped, and the
> other kthread is still processing shutdown.  So while historically the
> pending SIGINTs where not cleared (or ignored) during shutdown at this
> point, there is no reason why they could not be ignored for iscsi-target
> + ib-isert.
> 
> That said, pre commit 00d909a107 code always used wait_for_completion()
> and ignored pending signals.  As-is target_wait_for_sess_cmds() is
> called directly from fabric driver code and in one case also from
> user-space via configfs_write_file(), so AFAICT it does need
> TASK_UNINTERRUPTIBLE.
> 

Fair enough, thanks for the background. I'm always a bit wary when
kthreads need to deal with signals, but this seems OK.

Re: [PATCH 2/2] target: Fix target_wait_for_sess_cmds breakage with active signals

[Mike Christie]

On 10/09/2018 10:23 PM, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> With the addition of commit 00d909a107 in v4.19-rc, it incorrectly assumes no
> signals will be pending for task_struct executing the normal session shutdown
> and I/O quiesce code-path.
> 
> For example, iscsi-target and iser-target issue SIGINT to all kthreads as
> part of session shutdown.  This has been the behaviour since day one.
> 
> As-is when signals are pending with se_cmds active in se_sess->sess_cmd_list,
> wait_event_interruptible_lock_irq_timeout() returns a negative number and
> immediately kills the machine because of the do while (ret <= 0) loop that
> was added in commit 00d909a107 to spin while backend I/O is taking any
> amount of extended time (say 30 seconds) to complete.
> 
> Here's what it looks like in action with debug plus delayed backend I/O
> completion:
> 
> [ 4951.909951] se_sess: 000000003e7e08fa before target_wait_for_sess_cmds
> [ 4951.914600] target_wait_for_sess_cmds: signal_pending: 1
> [ 4951.918015] wait_event_interruptible_lock_irq_timeout ret: -512 signal_pending: 1 loop count: 0
> [ 4951.921639] wait_event_interruptible_lock_irq_timeout ret: -512 signal_pending: 1 loop count: 1
> [ 4951.921944] wait_event_interruptible_lock_irq_timeout ret: -512 signal_pending: 1 loop count: 2
> [ 4951.921944] wait_event_interruptible_lock_irq_timeout ret: -512 signal_pending: 1 loop count: 3
> [ 4951.921944] wait_event_interruptible_lock_irq_timeout ret: -512 signal_pending: 1 loop count: 4
> [ 4951.921944] wait_event_interruptible_lock_irq_timeout ret: -512 signal_pending: 1 loop count: 5
> [ 4951.921944] wait_event_interruptible_lock_irq_timeout ret: -512 signal_pending: 1 loop count: 6
> [ 4951.921944] wait_event_interruptible_lock_irq_timeout ret: -512 signal_pending: 1 loop count: 7
> [ 4951.921944] wait_event_interruptible_lock_irq_timeout ret: -512 signal_pending: 1 loop count: 8
> [ 4951.921944] wait_event_interruptible_lock_irq_timeout ret: -512 signal_pending: 1 loop count: 9
> 
> ... followed by the usual RCU CPU stalls and deadlock.
> 
> There was never a case pre commit 00d909a107 where wait_for_complete(&se_cmd->cmd_wait_comp)
> was able to be interruptted, so to address this for v4.19+ moving forward go ahead and
> use wait_event_lock_irq_timeout() instead so new code works with all fabric drivers.
> 
> Also for commit 00d909a107, fix a minor regression in target_release_cmd_kref()
> to only wake_up the new se_sess->cmd_list_wq only when shutdown has actually
> been triggered via se_sess->sess_tearing_down.
> 
> Fixes: 00d909a107 ("scsi: target: Make the session shutdown code also wait for commands that are being aborted")
> Cc: Bart Van Assche <bvanassche@acm.org>
> Cc: Mike Christie <mchristi@redhat.com>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Sagi Grimberg <sagi@grimberg.me>
> Cc: Bryant G. Ly <bryantly@linux.vnet.ibm.com>
> Tested-by: Nicholas Bellinger <nab@linux-iscsi.org>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> ---
>  drivers/target/target_core_transport.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 86c0156..fc3093d2 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -2754,7 +2754,7 @@ static void target_release_cmd_kref(struct kref *kref)
>  	if (se_sess) {
>  		spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
>  		list_del_init(&se_cmd->se_cmd_list);
> -		if (list_empty(&se_sess->sess_cmd_list))
> +		if (se_sess->sess_tearing_down && list_empty(&se_sess->sess_cmd_list))

I think there is another issue with 00d909a107 and ibmvscsi_tgt.

The problem is that ibmvscsi_tgt never called
target_sess_cmd_list_set_waiting. It only called
target_wait_for_sess_cmds. So before 00d909a107 there was a bug in that
driver and target_wait_for_sess_cmds never did what was intended because
sess_wait_list would always be empty.

With 00d909a107, we no longer need to call
target_sess_cmd_list_set_waiting to wait for outstanding commands, so
for ibmvscsi_tgt will now wait for commands like we wanted. However, the
commit added a WARN_ON that is hit if target_sess_cmd_list_set_waiting
is not called, so we could hit that.

So I think we need to add a target_sess_cmd_list_set_waiting call in
ibmvscsi_tgt to go along with your patch chunk above and make sure we do
not trigger the WARN_ON.

>  			wake_up(&se_sess->cmd_list_wq);
>  		spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
>  	}
> @@ -2907,7 +2907,7 @@ void target_wait_for_sess_cmds(struct se_session *se_sess)
>  
>  	spin_lock_irq(&se_sess->sess_cmd_lock);
>  	do {
> -		ret = wait_event_interruptible_lock_irq_timeout(
> +		ret = wait_event_lock_irq_timeout(
>  				se_sess->cmd_list_wq,
>  				list_empty(&se_sess->sess_cmd_list),
>  				se_sess->sess_cmd_lock, 180 * HZ);
> 

Re: [PATCH 2/2] target: Fix target_wait_for_sess_cmds breakage with active signals

[Nicholas A. Bellinger]

Hello MNC & Co,

On Wed, 2018-10-10 at 11:58 -0500, Mike Christie wrote:
> On 10/09/2018 10:23 PM, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@linux-iscsi.org>
> > 
> > With the addition of commit 00d909a107 in v4.19-rc, it incorrectly assumes no
> > signals will be pending for task_struct executing the normal session shutdown
> > and I/O quiesce code-path.
> > 

<SNIP>

> > diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> > index 86c0156..fc3093d2 100644
> > --- a/drivers/target/target_core_transport.c
> > +++ b/drivers/target/target_core_transport.c
> > @@ -2754,7 +2754,7 @@ static void target_release_cmd_kref(struct kref *kref)
> >  	if (se_sess) {
> >  		spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
> >  		list_del_init(&se_cmd->se_cmd_list);
> > -		if (list_empty(&se_sess->sess_cmd_list))
> > +		if (se_sess->sess_tearing_down && list_empty(&se_sess->sess_cmd_list))
> 
> I think there is another issue with 00d909a107 and ibmvscsi_tgt.
> 
> The problem is that ibmvscsi_tgt never called
> target_sess_cmd_list_set_waiting. It only called
> target_wait_for_sess_cmds. So before 00d909a107 there was a bug in that
> driver and target_wait_for_sess_cmds never did what was intended because
> sess_wait_list would always be empty.
> 
> With 00d909a107, we no longer need to call
> target_sess_cmd_list_set_waiting to wait for outstanding commands, so
> for ibmvscsi_tgt will now wait for commands like we wanted. However, the
> commit added a WARN_ON that is hit if target_sess_cmd_list_set_waiting
> is not called, so we could hit that.
> 
> So I think we need to add a target_sess_cmd_list_set_waiting call in
> ibmvscsi_tgt to go along with your patch chunk above and make sure we do
> not trigger the WARN_ON.
> 

Nice catch.  :)

With target_wait_for_sess_cmd() usage pre 00d909a107 doing a list-splice
in target_sess_cmd_list_set_waiting(), this particular usage in
ibmvscsi_tgt has always been list_empty(&sess->sess_wait_list) = true
(eg: no se_cmd I/O is quiesced, because no se_cmd in sess_wait_list)
since commit 712db3eb in 4.9.y code.

That said, ibmvscsi_tgt usage is very similar to vhost/scsi in the
respect individual /sys/kernel/config/target/$FABRIC/$WWN/$TPGT/
endpoints used by VMs do not remove their I_T nexus while the VM is
active.

So AFAICT, ibmvscsi_tgt doesn't strictly need target_sess_wait_for_cmd()
at all if this is true.

Re: [PATCH 2/2] target: Fix target_wait_for_sess_cmds breakage with active signals

[Ly, Bryant]

> On Oct 11, 2018, at 12:56 AM, Nicholas A. Bellinger <nab@linux-iscsi.org> wrote:
> 
> Hello MNC & Co,
> 
> On Wed, 2018-10-10 at 11:58 -0500, Mike Christie wrote:
>> On 10/09/2018 10:23 PM, Nicholas A. Bellinger wrote:
>>> From: Nicholas Bellinger <nab@linux-iscsi.org>
>>> 
>>> With the addition of commit 00d909a107 in v4.19-rc, it incorrectly assumes no
>>> signals will be pending for task_struct executing the normal session shutdown
>>> and I/O quiesce code-path.
>>> 
> 
> <SNIP>
> 
>>> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
>>> index 86c0156..fc3093d2 100644
>>> --- a/drivers/target/target_core_transport.c
>>> +++ b/drivers/target/target_core_transport.c
>>> @@ -2754,7 +2754,7 @@ static void target_release_cmd_kref(struct kref *kref)
>>> 	if (se_sess) {
>>> 		spin_lock_irqsave(&se_sess->sess_cmd_lock, flags);
>>> 		list_del_init(&se_cmd->se_cmd_list);
>>> -		if (list_empty(&se_sess->sess_cmd_list))
>>> +		if (se_sess->sess_tearing_down && list_empty(&se_sess->sess_cmd_list))
>> 
>> I think there is another issue with 00d909a107 and ibmvscsi_tgt.
>> 
>> The problem is that ibmvscsi_tgt never called
>> target_sess_cmd_list_set_waiting. It only called
>> target_wait_for_sess_cmds. So before 00d909a107 there was a bug in that
>> driver and target_wait_for_sess_cmds never did what was intended because
>> sess_wait_list would always be empty.
>> 
>> With 00d909a107, we no longer need to call
>> target_sess_cmd_list_set_waiting to wait for outstanding commands, so
>> for ibmvscsi_tgt will now wait for commands like we wanted. However, the
>> commit added a WARN_ON that is hit if target_sess_cmd_list_set_waiting
>> is not called, so we could hit that.
>> 
>> So I think we need to add a target_sess_cmd_list_set_waiting call in
>> ibmvscsi_tgt to go along with your patch chunk above and make sure we do
>> not trigger the WARN_ON.
>> 
> 
> Nice catch.  :)
> 
> With target_wait_for_sess_cmd() usage pre 00d909a107 doing a list-splice
> in target_sess_cmd_list_set_waiting(), this particular usage in
> ibmvscsi_tgt has always been list_empty(&sess->sess_wait_list) = true
> (eg: no se_cmd I/O is quiesced, because no se_cmd in sess_wait_list)
> since commit 712db3eb in 4.9.y code.
> 
> That said, ibmvscsi_tgt usage is very similar to vhost/scsi in the
> respect individual /sys/kernel/config/target/$FABRIC/$WWN/$TPGT/
> endpoints used by VMs do not remove their I_T nexus while the VM is
> active.
> 
> So AFAICT, ibmvscsi_tgt doesn't strictly need target_sess_wait_for_cmd()
> at all if this is true.
> 

VMs do not remove the I_T nexus while the VM is active, so we can remove
the target_wait_for_sess_cmd() call. 

-Bryant

Re: [PATCH 2/2] target: Fix target_wait_for_sess_cmds breakage with active signals

[Martin K. Petersen]

Bryant,

> VMs do not remove the I_T nexus while the VM is active, so we can remove
> the target_wait_for_sess_cmd() call. 

Patch forthcoming?

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 2/2] target: Fix target_wait_for_sess_cmds breakage with active signals

[Ly, Bryant]

Martin, 

> On Oct 15, 2018, at 11:13 PM, Martin K. Petersen <martin.petersen@oracle.com> wrote:
> 
> 
> Bryant,
> 
>> VMs do not remove the I_T nexus while the VM is active, so we can remove
>> the target_wait_for_sess_cmd() call. 
> 
> Patch forthcoming?

I can put out a patch, I’m trying to get someone at IBM to test it though.

-Bryant

Re: [PATCH 0/2] target: Fix v4.19-rc active I/O shutdown deadlock

[Nicholas A. Bellinger]

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

On Wed, 2018-10-10 at 03:23 +0000, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> Hi MNC, MKP & Co,
> 
> While testing v4.19-rc recently with simple backend I/O error injection
> (via delayed BIO completion), I was able to trigger an end-less loop
> deadlock with recent changes in commit 00d909a107:
> 
>   Author: Bart Van Assche <bart.vanassche@wdc.com>
>   Date:   Fri Jun 22 14:52:53 2018 -0700
> 
>       scsi: target: Make the session shutdown code also wait for commands that are being aborted
> 
> It comes down to an incorrect assumption wrt signals during session
> shutdown plus active I/O quiesce, which triggers an endless loop
> immediately during session shutdown as se_session->sess_list_wq
> waits for outstanding backend I/O to complete.
> 
> The easiest reproduction is with iser-target or simulation with plain
> old iscsi-target/TCP ports. 

For reference, attached are two debug patches and instructions to
trigger the end-less loop deadlock regression on v4.19-rc.

1) Simulate iscsi-target via iscsit_transport->iscsi_wait_conn()

This makes iscsi-target/TCP follow isert_wait_conn() code, and uses
iscsit_transport->iscsi_wait_conn() during active I/O shutdown to invoke
target_wait_for_sess_cmds() with signals pending per existing
iser-target session shutdown logic.

Useful to trigger in a VM, without a RDMA capable NIC.

2) Simulate IBLOCK WRITE delayed completion by 60 seconds

MNC likes to use scsi_debug for this, but I use BRD to add an arbitrary
completion delay.

-----------------------------------------------------------------------

So once an /sys/kernel/config/target/core/$IBLOCK_HBA/$IBLOCK_DEV/ has
been created + exported via /sys/kernel/config/target/iscsi/$IQN/$TPGT/,
issue a single block WRITE.

Once WRITE completion is delayed by IBLOCK, go ahead and send a 'kill
-SIGINT $PID' to iscsi_trx kthread to trigger usual iscsi/iser session
shutdown + reconnect for the connection with the outstanding delayed
I/O.

Once target_wait_for_sess_cmds() is called with signals pending, it will
immediately kill the machine.

[-- Attachment #2: 0001-iscsi-target-Add-iscsit_wait_conn-simulation-for-tes.patch --]
[-- Type: text/x-patch, Size: 2398 bytes --]

From 9b1d23148994edb0969a5efd3a131122ad25e39d Mon Sep 17 00:00:00 2001
From: Nicholas Bellinger <nab@linux-iscsi.org>
Date: Tue, 9 Oct 2018 01:57:53 -0700
Subject: [PATCH 1/2] iscsi-target: Add iscsit_wait_conn() simulation for
 testing

Simulate sess_tearing_down put in iscsit_queue_rsp following how
iser-target isert_put_cmd() works.

Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/iscsi/iscsi_target.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index cc756a1..b05d8af 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -485,6 +485,22 @@ int iscsit_del_np(struct iscsi_np *np)
 
 int iscsit_queue_rsp(struct iscsi_conn *conn, struct iscsi_cmd *cmd)
 {
+	if (conn && conn->sess && conn->sess->se_sess) {
+		struct se_session *se_sess = conn->sess->se_sess;
+
+		if (se_sess->sess_tearing_down) {
+			printk("Got iscsit_queue_rsp sess_tearing_down !!!!!!\n");
+
+			spin_lock_bh(&conn->cmd_lock);
+			if (!list_empty(&cmd->i_conn_node))
+				list_del_init(&cmd->i_conn_node);
+			spin_unlock_bh(&conn->cmd_lock);
+
+			transport_generic_free_cmd(&cmd->se_cmd, 0);
+			return 0;
+		}
+	}
+
 	return iscsit_add_cmd_to_response_queue(cmd, cmd->conn, cmd->i_state);
 }
 EXPORT_SYMBOL(iscsit_queue_rsp);
@@ -667,6 +683,16 @@ static enum target_prot_op iscsit_get_sup_prot_ops(struct iscsi_conn *conn)
 	return TARGET_PROT_NORMAL;
 }
 
+static void iscsit_wait_conn(struct iscsi_conn *conn)
+{
+        if (conn->sess) {
+                target_sess_cmd_list_set_waiting(conn->sess->se_sess);
+                printk("se_sess: %p before target_wait_for_sess_cmds\n", conn->sess->se_sess);
+                target_wait_for_sess_cmds(conn->sess->se_sess);
+                printk("se_sess: %p after target_wait_for_sess_cmds\n", conn->sess->se_sess);
+        }
+}
+
 static struct iscsit_transport iscsi_target_transport = {
 	.name			= "iSCSI/TCP",
 	.transport_type		= ISCSI_TCP,
@@ -686,6 +712,7 @@ static enum target_prot_op iscsit_get_sup_prot_ops(struct iscsi_conn *conn)
 	.iscsit_xmit_pdu	= iscsit_xmit_pdu,
 	.iscsit_get_rx_pdu	= iscsit_get_rx_pdu,
 	.iscsit_get_sup_prot_ops = iscsit_get_sup_prot_ops,
+	.iscsit_wait_conn	= iscsit_wait_conn,
 };
 
 static int __init iscsi_target_init_module(void)
-- 
1.9.1


[-- Attachment #3: 0002-target-iblock-Delayed-bios-for-active-I-O-shutdown-t.patch --]
[-- Type: text/x-patch, Size: 1956 bytes --]

From dc01432e1d561a4d20b20fe868fc8df22fcc4601 Mon Sep 17 00:00:00 2001
From: Nicholas Bellinger <nab@linux-iscsi.org>
Date: Thu, 9 Feb 2017 20:56:01 -0800
Subject: [PATCH 2/2] target/iblock: Delayed bios for active I/O shutdown
 testing

Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_iblock.c | 21 +++++++++++++++++++++
 drivers/target/target_core_iblock.h |  2 ++
 2 files changed, 23 insertions(+)

diff --git a/drivers/target/target_core_iblock.c b/drivers/target/target_core_iblock.c
index ce1321a..deef231 100644
--- a/drivers/target/target_core_iblock.c
+++ b/drivers/target/target_core_iblock.c
@@ -297,6 +297,19 @@ static void iblock_complete_cmd(struct se_cmd *cmd)
 	kfree(ibr);
 }
 
+static void iblock_delayed_work(struct work_struct *work)
+{
+	struct iblock_req *ibr = container_of(work,
+		struct iblock_req, delayed_work.work);
+	struct bio *bio = ibr->bio;
+	struct se_cmd *cmd = bio->bi_private;
+
+	printk("iblock_delayed_work: cmd: %p\n", cmd);
+
+	bio_put(bio);
+	iblock_complete_cmd(cmd);
+}
+
 static void iblock_bio_done(struct bio *bio)
 {
 	struct se_cmd *cmd = bio->bi_private;
@@ -311,6 +324,14 @@ static void iblock_bio_done(struct bio *bio)
 		smp_mb__after_atomic();
 	}
 
+	if (cmd->data_direction == DMA_TO_DEVICE) {
+		ibr->bio = bio;
+		INIT_DELAYED_WORK(&ibr->delayed_work, iblock_delayed_work);
+		schedule_delayed_work(&ibr->delayed_work, 60 * HZ);
+		printk("Queued ibr->delayed_work ! :-)\n");
+		return;
+	}
+
 	bio_put(bio);
 
 	iblock_complete_cmd(cmd);
diff --git a/drivers/target/target_core_iblock.h b/drivers/target/target_core_iblock.h
index 9cc3843..122b415 100644
--- a/drivers/target/target_core_iblock.h
+++ b/drivers/target/target_core_iblock.h
@@ -14,6 +14,8 @@
 struct iblock_req {
 	refcount_t pending;
 	atomic_t ib_bio_err_cnt;
+	struct delayed_work delayed_work;
+	struct bio *bio;
 } ____cacheline_aligned;
 
 #define IBDF_HAS_UDEV_PATH		0x01
-- 
1.9.1