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. > > > 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. > > > 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 have yet to test this code, have you tried running with a lot of > target instances? Yes, using 32 targets and 32 network portals. > In this case it's more likely that during the np reset stage the > initiator will retry to connect (CM connect requests to np in reset stage). > Agreed. --nab -- 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