iSCSI target driver regression

Commit Message

Nicholas A. Bellinger June 3, 2017, 12:49 p.m. UTC

On Fri, 2017-06-02 at 22:01 -0700, Nicholas A. Bellinger wrote:
> On Fri, 2017-06-02 at 18:14 +0000, Bart Van Assche wrote:
> > Hello Nic,
> > 
> > When I reran the libiscsi test suite against your for-next branch a kernel oops
> > appeared in the system log that I hadn't seen before. There are no iSCSI patches
> > from me on that branch so this crash was likely introduced by one of the iSCSI
> > target driver patches that were added to your for-next branch after kernel v4.11
> > was released. The topmost commit in the kernel tree that triggered this oops is
> > commit acdd4716bc86 ("target: reject COMPARE_AND_WRITE if emulate_caw is not set").
> > 
> 
> Yep, nothing immediate comes to mind in the explicit logout path that
> has changed recently.
> 
> > [  321.546438] iscsi_target_mod:lio_release_cmd: Entering lio_release_cmd for se_cmd: ffff880063134890
> > [  323.013563] 1 connection(s) still exist for iSCSI session to iqn.2007-10.com.github:sahlberg:libiscsi:iscsi-test-2
> > [  323.014358] ------------[ cut here ]------------
> > [  323.014864] kernel BUG at drivers/target/iscsi/iscsi_target.c:4346!

It turns out the only way to trigger this is to block tx thread context
from reaching iscsit_logout_post_handler_closesession() for longer than
SECONDS_FOR_LOGOUT_COMP (15 seconds), which causes sleep = 0 to be
processed while iscsit_close_connection() from rx thread context has
already cleared conn->tx_thread_active.

As it was, I have no idea what you did in your VM to cause a 15+ second
delay to reach iscsit_logout_post_handler_closesession(), but whatever
it was it's certainly not a regression in upstream.  ;)

In any event, here's how to simulate the issue, and the proper fix to
just let existing iscsit_close_connection() logic clean up the failed
logout as if iscsit_logout_post_handler_closesession() was never
reached.

Enjoy.


--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Bart Van Assche June 6, 2017, 6:31 p.m. UTC | #1

On Sat, 2017-06-03 at 05:49 -0700, Nicholas A. Bellinger wrote:
> In any event, here's how to simulate the issue, and the proper fix to
> just let existing iscsit_close_connection() logic clean up the failed
> logout as if iscsit_logout_post_handler_closesession() was never
> reached.

Hello Nic,

With the quick tests I ran so far this patch seems to be sufficient to avoid that
"kernel BUG at drivers/target/iscsi/iscsi_target.c:4346" gets reported.

Bart.--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Nicholas A. Bellinger June 9, 2017, 5:27 a.m. UTC | #2

On Tue, 2017-06-06 at 18:31 +0000, Bart Van Assche wrote:
> On Sat, 2017-06-03 at 05:49 -0700, Nicholas A. Bellinger wrote:
> > In any event, here's how to simulate the issue, and the proper fix to
> > just let existing iscsit_close_connection() logic clean up the failed
> > logout as if iscsit_logout_post_handler_closesession() was never
> > reached.
> 
> Hello Nic,
> 
> With the quick tests I ran so far this patch seems to be sufficient to avoid that
> "kernel BUG at drivers/target/iscsi/iscsi_target.c:4346" gets reported.

Also, thanks for confirming.

Added your 'Tested-by' to the patch as well.

Likewise, this patch has been run through the paces by DATERA's Q/A team
as well. 

--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 0d8f815..03a0224 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -4414,6 +4414,11 @@  static void iscsit_logout_post_handler_closesession(
 {
        struct iscsi_session *sess = conn->sess;
        int sleep = 1;
+
+       printk("Simulating broken out-of-tree codebase.\n");
+       ssleep(SECONDS_FOR_LOGOUT_COMP + 2);
+       printk("Simulation complete\n");
+
        /*
         * Traditional iscsi/tcp will invoke this logic from TX thread
         * context during session logout, so clear tx_thread_active and
@@ -4423,8 +4428,11 @@  static void iscsit_logout_post_handler_closesession(
         * always sleep waiting for RX/TX thread shutdown to complete
         * within iscsit_close_connection().
         */
-       if (!conn->conn_transport->rdma_shutdown)
+       if (!conn->conn_transport->rdma_shutdown) {
                sleep = cmpxchg(&conn->tx_thread_active, true, false);
+               if (!sleep)
+                       return;
+       }
 
        atomic_set(&conn->conn_logout_remove, 0);
        complete(&conn->conn_logout_comp);
@@ -4440,8 +4448,11 @@  static void iscsit_logout_post_handler_samecid(
 {
        int sleep = 1;
 
-       if (!conn->conn_transport->rdma_shutdown)
+       if (!conn->conn_transport->rdma_shutdown) {
                sleep = cmpxchg(&conn->tx_thread_active, true, false);
+               if (!sleep)
+                       return;
+       }
 
        atomic_set(&conn->conn_logout_remove, 0);
        complete(&conn->conn_logout_comp);