On 6/6/2014 2:08 AM, Nicholas A. Bellinger wrote:
On Thu, 2014-06-05 at 12:28 +0300, Sagi Grimberg wrote:
On 6/4/2014 4:17 AM, Nicholas A. Bellinger wrote:
From: Nicholas Bellinger <nab@xxxxxxxxxxxxxxx>
This patch fixes a iser-target specific regression introduced in
v3.15-rc6 with:
commit 14f4b54fe38f3a8f8392a50b951c8aa43b63687a
Author: Sagi Grimberg <sagig@xxxxxxxxxxxx>
Date: Tue Apr 29 13:13:47 2014 +0300
Target/iscsi,iser: Avoid accepting transport connections during stop stage
where the change to set iscsi_np->enabled = false within
iscsit_clear_tpg_np_login_thread() meant that a iscsi_np with
two iscsi_tpg_np exports would have it's parent iscsi_np set
to a disabled state, even if other iscsi_tpg_np exports still
existed.
This patch changes iscsit_clear_tpg_np_login_thread() to only
set iscsi_np->enabled = false when shutdown = true, and also
changes iscsit_del_np() to set iscsi_np->enabled = true when
iscsi_np->np_exports is non zero.
Hey Nic,
Can you please elaborate on what is the phenomenon and how you were able
to reproduce it?
I ran several long runs with unstable ports and target stack shutdowns
over 10 targets each exposing 4 NPs.
The code seemed to survive it. I would like to extend our QA test-suite
to hit this bug.
Easy. Create multiple targets that share the same network portal, then
remove one of the network portals from one of the targets.
From there, all logins on the targets with the network portal in
question will be rejected because np->enabled = false.
Of course...
Thanks, will add this to our test suite.
Cc: Sagi Grimberg <sagig@xxxxxxxxxxxxxxxxxx>
Cc: stable@xxxxxxxxxxxxxxx # 3.10+
Signed-off-by: Nicholas Bellinger <nab@xxxxxxxxxxxxxxx>
---
drivers/target/iscsi/iscsi_target.c | 1 +
drivers/target/iscsi/iscsi_target_tpg.c | 3 ++-
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 46588c8..9189bc0 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -460,6 +460,7 @@ int iscsit_del_np(struct iscsi_np *np)
spin_lock_bh(&np->np_thread_lock);
np->np_exports--;
if (np->np_exports) {
+ np->enabled = true;
Shouldn't this be already true?
No, that is the regression.
The original patch did not take into account that a single iscsi_np
instance can be shared across multiple iscsi_tpg_nps, so when one
iscsi_tpg_np was shutdown iscsi_np->enabled = false was set, and never
reset to iscsi_np->enabled = true, causing all other targets that where
still using the same iscsi_np to be unable to accept logins.
True, I only tested the shutdown = true case.
spin_unlock_bh(&np->np_thread_lock);
return 0;
}
diff --git a/drivers/target/iscsi/iscsi_target_tpg.c b/drivers/target/iscsi/iscsi_target_tpg.c
index ca18118..1431e84 100644
--- a/drivers/target/iscsi/iscsi_target_tpg.c
+++ b/drivers/target/iscsi/iscsi_target_tpg.c
@@ -184,7 +184,8 @@ static void iscsit_clear_tpg_np_login_thread(
return;
}
- tpg_np->tpg_np->enabled = false;
+ if (shutdown)
+ tpg_np->tpg_np->enabled = false;
iscsit_reset_np_thread(tpg_np->tpg_np, tpg_np, tpg, shutdown);
}
The enabled boolean was made for the transport (iser) to understand if
it can accept transport connection requests (cma).
In case the np_thread is during reset stage, shouldn't iser reject
connect requests?
There are two scenarios here.
The first is where iscsit_clear_tpg_np_login_thread() is called with
shutdown=false from iscsit_tpg_disable_portal_group() context, where the
TPG state is explicitly set to TPG_STATE_INACTIVE before hand, causing
any new logins to fail with error status in iscsi_target_locate_portal()
-> iscsit_get_tpg_from_np(). So not setting iscsi_np->enabled = false
here is OK because the network portal itself is not being shutdown, just
the target endpoint.
The second is where iscsit_clear_tpg_np_login_thread() with
shutdown=true is called from iscsit_tpg_release_np(), because the actual
iscsi_tpg_np group is being removed from configfs.
It's the latter case where disallowing logins to the parent iscsi_np
makes sense until the individual iscsi_tpg_np is released, but if other
iscsi_tpg_np are still using the iscsi_np, then ->enabled = true needs
to be reset in order for logins on other iscsi_tpg_nps to be processed.
I completely agree with you here.
Sagi.
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
