* [PATCH 1/3] target/iscsi: Set np_thread_state to ACTIVE before adding to g_np_list
2014-01-25 0:18 [PATCH 0/3] target/iscsi networkportal race fixes Andy Grover
@ 2014-01-25 0:18 ` Andy Grover
2014-01-25 0:18 ` [PATCH 2/3] target/iscsi: Convert np_lock from a spinlock to a mutex Andy Grover
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Andy Grover @ 2014-01-25 0:18 UTC (permalink / raw)
To: target-devel; +Cc: linux-scsi
When creating network portals rapidly, such as when restoring a
configuration, LIO's code to reuse existing portals can return a false
negative if the thread hasn't run yet and set np_thread_state to
ISCSI_NP_THREAD_ACTIVE. This causes an error in the network stack
when attempting to bind to the same address/port.
This patch sets NP_THREAD_ACTIVE before the np is placed on g_np_list,
so even if the thread hasn't run yet, iscsit_get_np will return the
existing np.
Signed-off-by: Andy Grover <agrover@redhat.com>
---
drivers/target/iscsi/iscsi_target.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index d70e911..bfd4bf7 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -400,6 +400,7 @@ struct iscsi_np *iscsit_add_np(
* point because iscsi_np has not been added to g_np_list yet.
*/
np->np_exports = 1;
+ np->np_thread_state = ISCSI_NP_THREAD_ACTIVE;
spin_lock_bh(&np_lock);
list_add_tail(&np->np_list, &g_np_list);
--
1.7.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 2/3] target/iscsi: Convert np_lock from a spinlock to a mutex
2014-01-25 0:18 [PATCH 0/3] target/iscsi networkportal race fixes Andy Grover
2014-01-25 0:18 ` [PATCH 1/3] target/iscsi: Set np_thread_state to ACTIVE before adding to g_np_list Andy Grover
@ 2014-01-25 0:18 ` Andy Grover
2014-01-25 0:18 ` [PATCH 3/3] target/iscsi: hold np_lock mutex across adding new net portal to g_np_list Andy Grover
2014-01-28 19:15 ` [PATCH 0/3] target/iscsi networkportal race fixes Nicholas A. Bellinger
3 siblings, 0 replies; 6+ messages in thread
From: Andy Grover @ 2014-01-25 0:18 UTC (permalink / raw)
To: target-devel; +Cc: linux-scsi
All callers are in process context.
Convert spin_lock to spin_lock_bh in iscsit_get_np since outer spin_lock_bh
is now not there.
Signed-off-by: Andy Grover <agrover@redhat.com>
---
drivers/target/iscsi/iscsi_target.c | 22 +++++++++++-----------
1 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index bfd4bf7..45ec498 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -52,7 +52,7 @@
static LIST_HEAD(g_tiqn_list);
static LIST_HEAD(g_np_list);
static DEFINE_SPINLOCK(tiqn_lock);
-static DEFINE_SPINLOCK(np_lock);
+static DEFINE_MUTEX(np_lock);
static struct idr tiqn_idr;
struct idr sess_idr;
@@ -314,9 +314,9 @@ static struct iscsi_np *iscsit_get_np(
struct iscsi_np *np;
bool match;
- spin_lock_bh(&np_lock);
+ mutex_lock(&np_lock);
list_for_each_entry(np, &g_np_list, np_list) {
- spin_lock(&np->np_thread_lock);
+ spin_lock_bh(&np->np_thread_lock);
if (np->np_thread_state != ISCSI_NP_THREAD_ACTIVE) {
spin_unlock(&np->np_thread_lock);
continue;
@@ -330,13 +330,13 @@ static struct iscsi_np *iscsit_get_np(
* while iscsi_tpg_add_network_portal() is called.
*/
np->np_exports++;
- spin_unlock(&np->np_thread_lock);
- spin_unlock_bh(&np_lock);
+ spin_unlock_bh(&np->np_thread_lock);
+ mutex_unlock(&np_lock);
return np;
}
- spin_unlock(&np->np_thread_lock);
+ spin_unlock_bh(&np->np_thread_lock);
}
- spin_unlock_bh(&np_lock);
+ mutex_unlock(&np_lock);
return NULL;
}
@@ -402,9 +402,9 @@ struct iscsi_np *iscsit_add_np(
np->np_exports = 1;
np->np_thread_state = ISCSI_NP_THREAD_ACTIVE;
- spin_lock_bh(&np_lock);
+ mutex_lock(&np_lock);
list_add_tail(&np->np_list, &g_np_list);
- spin_unlock_bh(&np_lock);
+ mutex_unlock(&np_lock);
pr_debug("CORE[0] - Added Network Portal: %s:%hu on %s\n",
np->np_ip, np->np_port, np->np_transport->name);
@@ -470,9 +470,9 @@ int iscsit_del_np(struct iscsi_np *np)
np->np_transport->iscsit_free_np(np);
- spin_lock_bh(&np_lock);
+ mutex_lock(&np_lock);
list_del(&np->np_list);
- spin_unlock_bh(&np_lock);
+ mutex_unlock(&np_lock);
pr_debug("CORE[0] - Removed Network Portal: %s:%hu on %s\n",
np->np_ip, np->np_port, np->np_transport->name);
--
1.7.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 3/3] target/iscsi: hold np_lock mutex across adding new net portal to g_np_list
2014-01-25 0:18 [PATCH 0/3] target/iscsi networkportal race fixes Andy Grover
2014-01-25 0:18 ` [PATCH 1/3] target/iscsi: Set np_thread_state to ACTIVE before adding to g_np_list Andy Grover
2014-01-25 0:18 ` [PATCH 2/3] target/iscsi: Convert np_lock from a spinlock to a mutex Andy Grover
@ 2014-01-25 0:18 ` Andy Grover
2014-01-28 19:15 ` [PATCH 0/3] target/iscsi networkportal race fixes Nicholas A. Bellinger
3 siblings, 0 replies; 6+ messages in thread
From: Andy Grover @ 2014-01-25 0:18 UTC (permalink / raw)
To: target-devel; +Cc: linux-scsi
This prevents a race where two threads may attempt to create the same
network portal, resulting in one of them failing.
Signed-off-by: Andy Grover <agrover@redhat.com>
---
drivers/target/iscsi/iscsi_target.c | 14 +++++++++-----
1 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 45ec498..4e91586 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -307,6 +307,9 @@ bool iscsit_check_np_match(
return false;
}
+/*
+ * Called with mutex np_lock held
+ */
static struct iscsi_np *iscsit_get_np(
struct __kernel_sockaddr_storage *sockaddr,
int network_transport)
@@ -314,7 +317,6 @@ static struct iscsi_np *iscsit_get_np(
struct iscsi_np *np;
bool match;
- mutex_lock(&np_lock);
list_for_each_entry(np, &g_np_list, np_list) {
spin_lock_bh(&np->np_thread_lock);
if (np->np_thread_state != ISCSI_NP_THREAD_ACTIVE) {
@@ -331,12 +333,10 @@ static struct iscsi_np *iscsit_get_np(
*/
np->np_exports++;
spin_unlock_bh(&np->np_thread_lock);
- mutex_unlock(&np_lock);
return np;
}
spin_unlock_bh(&np->np_thread_lock);
}
- mutex_unlock(&np_lock);
return NULL;
}
@@ -350,12 +350,17 @@ struct iscsi_np *iscsit_add_np(
struct sockaddr_in6 *sock_in6;
struct iscsi_np *np;
int ret;
+
+ mutex_lock(&np_lock);
+
/*
* Locate the existing struct iscsi_np if already active..
*/
np = iscsit_get_np(sockaddr, network_transport);
- if (np)
+ if (np) {
+ mutex_unlock(&np_lock);
return np;
+ }
np = kzalloc(sizeof(struct iscsi_np), GFP_KERNEL);
if (!np) {
@@ -402,7 +407,6 @@ struct iscsi_np *iscsit_add_np(
np->np_exports = 1;
np->np_thread_state = ISCSI_NP_THREAD_ACTIVE;
- mutex_lock(&np_lock);
list_add_tail(&np->np_list, &g_np_list);
mutex_unlock(&np_lock);
--
1.7.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 0/3] target/iscsi networkportal race fixes
2014-01-25 0:18 [PATCH 0/3] target/iscsi networkportal race fixes Andy Grover
` (2 preceding siblings ...)
2014-01-25 0:18 ` [PATCH 3/3] target/iscsi: hold np_lock mutex across adding new net portal to g_np_list Andy Grover
@ 2014-01-28 19:15 ` Nicholas A. Bellinger
2014-01-28 20:04 ` Andy Grover
3 siblings, 1 reply; 6+ messages in thread
From: Nicholas A. Bellinger @ 2014-01-28 19:15 UTC (permalink / raw)
To: Andy Grover; +Cc: target-devel, linux-scsi
Hi Andy,
On Fri, 2014-01-24 at 16:18 -0800, Andy Grover wrote:
> see https://bugzilla.redhat.com/show_bug.cgi?id=1055064
>
> Summary: if thread doesn't run before 2nd identical portal is created,
> it misses the existing one and the kernel_bind returns an error.
>
> The first one-line patch fixes the issue for a single thread creating
> multiple network portals via configfs.
>
> The second two close the race between multiple threads creating
> network portals.
>
> Testing: failing test script now works.
>
Applied, squashed into a single bugfix patch, and adding a CC'ed to
stable.
FYI, on patch #3, there where a few exception path mutex_unlocks
missing, so the following incremental has been applied + squashed as
well.
Thanks,
--nab
diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 13868ff..1ab9b18 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -365,6 +365,7 @@ struct iscsi_np *iscsit_add_np(
np = kzalloc(sizeof(struct iscsi_np), GFP_KERNEL);
if (!np) {
pr_err("Unable to allocate memory for struct iscsi_np\n");
+ mutex_unlock(&np_lock);
return ERR_PTR(-ENOMEM);
}
@@ -387,6 +388,7 @@ struct iscsi_np *iscsit_add_np(
ret = iscsi_target_setup_login_socket(np, sockaddr);
if (ret != 0) {
kfree(np);
+ mutex_unlock(&np_lock);
return ERR_PTR(ret);
}
@@ -395,6 +397,7 @@ struct iscsi_np *iscsit_add_np(
pr_err("Unable to create kthread: iscsi_np\n");
ret = PTR_ERR(np->np_thread);
kfree(np);
+ mutex_unlock(&np_lock);
return ERR_PTR(ret);
}
/*
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 0/3] target/iscsi networkportal race fixes
2014-01-28 19:15 ` [PATCH 0/3] target/iscsi networkportal race fixes Nicholas A. Bellinger
@ 2014-01-28 20:04 ` Andy Grover
0 siblings, 0 replies; 6+ messages in thread
From: Andy Grover @ 2014-01-28 20:04 UTC (permalink / raw)
To: Nicholas A. Bellinger; +Cc: target-devel, linux-scsi
On 01/28/2014 11:15 AM, Nicholas A. Bellinger wrote:
> Hi Andy,
>
> On Fri, 2014-01-24 at 16:18 -0800, Andy Grover wrote:
>> see https://bugzilla.redhat.com/show_bug.cgi?id=1055064
>>
>> Summary: if thread doesn't run before 2nd identical portal is created,
>> it misses the existing one and the kernel_bind returns an error.
> Applied, squashed into a single bugfix patch, and adding a CC'ed to
> stable.
>
> FYI, on patch #3, there where a few exception path mutex_unlocks
> missing, so the following incremental has been applied + squashed as
> well.
Thanks for the review and fixes! -- Andy
^ permalink raw reply [flat|nested] 6+ messages in thread