Hi Maged,
On Mon, 2016-02-08 at 17:43 -0700, Maged Mokhtar wrote:
Hi Nick
Please find patch below:
Apologies for the delayed follow-up. Comments below.
Solves an issue with frequent "iSCSI Login negotiation failed." observed
during target discovery from Microsoft client initiator running inside a
virtual machine to a target running in another virtual machine inside the
same host under VMWare.
iscsi_target_sk_data_ready() callback is received too soon before
LOGIN_FLAGS_READY flag is set.
The patch fixes the problem by moving the section that checks the various
LOGIN_FLAGS_* from the iscsi_target_sk_data_ready() callback to the
worker
function iscsi_target_do_login_rx() and in case LOGIN_FLAGS_READY is not
set, sleeps and checks it a second time before exiting.
Maged Mokhtar mmokhtar@xxxxxxxxxxxxxxxxxx
diff -up a/drivers/target/iscsi/iscsi_target_nego.c
b/drivers/target/iscsi/iscsi_target_nego.c
--- a/drivers/target/iscsi/iscsi_target_nego.c 2016-02-08
18:07:32.282055751 -0500
+++ b/drivers/target/iscsi/iscsi_target_nego.c 2016-02-08
18:28:59.655937903 -0500
@@ -409,34 +409,11 @@ static void iscsi_target_sk_data_ready(s
bool rc;
pr_debug("Entering iscsi_target_sk_data_ready: conn: %p\n",
conn);
-
- write_lock_bh(&sk->sk_callback_lock);
- if (!sk->sk_user_data) {
- write_unlock_bh(&sk->sk_callback_lock);
- return;
- }
- if (!test_bit(LOGIN_FLAGS_READY, &conn->login_flags)) {
- write_unlock_bh(&sk->sk_callback_lock);
- pr_debug("Got LOGIN_FLAGS_READY=0, conn: %p >>>>\n",
conn);
- return;
- }
- if (test_bit(LOGIN_FLAGS_CLOSED, &conn->login_flags)) {
- write_unlock_bh(&sk->sk_callback_lock);
- pr_debug("Got LOGIN_FLAGS_CLOSED=1, conn: %p >>>>\n",
conn);
- return;
- }
- if (test_and_set_bit(LOGIN_FLAGS_READ_ACTIVE,
&conn->login_flags)) {
- write_unlock_bh(&sk->sk_callback_lock);
- pr_debug("Got LOGIN_FLAGS_READ_ACTIVE=1, conn: %p
>>>>\n", conn);
- return;
- }
-
rc = schedule_delayed_work(&conn->login_work, 0);
if (!rc) {
pr_debug("iscsi_target_sk_data_ready,
schedule_delayed_work"
" got false\n");
}
- write_unlock_bh(&sk->sk_callback_lock);
}
static void iscsi_target_sk_state_change(struct sock *);
@@ -551,6 +528,35 @@ static void iscsi_target_do_login_rx(str
if (conn->sock) {
struct sock *sk = conn->sock->sk;
+ write_lock_bh(&sk->sk_callback_lock);
+ if (!sk->sk_user_data) {
+ write_unlock_bh(&sk->sk_callback_lock);
+ return;
+ }
+ if (!test_bit(LOGIN_FLAGS_READY, &conn->login_flags)) {
+ write_unlock_bh(&sk->sk_callback_lock);
+ pr_debug("Got LOGIN_FLAGS_READY=0, conn: %p
>>>>\n", conn);
+ /* retry a second time in case we are called too
soon before LOGIN_FLAGS_READY is set */
+ msleep(100);
+ write_lock_bh(&sk->sk_callback_lock);
+ if (!test_bit(LOGIN_FLAGS_READY,
&conn->login_flags)) {
+ write_unlock_bh(&sk->sk_callback_lock);
+ pr_debug("Second retry. Got
LOGIN_FLAGS_READY=0, conn: %p >>>>\n", conn);
+ return;
+ }
+ }
+ if (test_bit(LOGIN_FLAGS_CLOSED, &conn->login_flags)) {
+ write_unlock_bh(&sk->sk_callback_lock);
+ pr_debug("Got LOGIN_FLAGS_CLOSED=1, conn: %p
>>>>\n", conn);
+ return;
+ }
+ if (test_and_set_bit(LOGIN_FLAGS_READ_ACTIVE,
&conn->login_flags)) {
+ write_unlock_bh(&sk->sk_callback_lock);
+ pr_debug("Got LOGIN_FLAGS_READ_ACTIVE=1, conn: %p
>>>>\n", conn);
+ return;
+ }
+
+ write_unlock_bh(&sk->sk_callback_lock);
read_lock_bh(&sk->sk_callback_lock);
state = iscsi_target_sk_state_check(sk);
So pondering this further, an earlier initial LOGIN_FLAGS_READY bit
assignment might work as a simpler patch:
diff --git a/drivers/target/iscsi/iscsi_target_nego.c
b/drivers/target/iscsi/iscsi_target_nego.c
index 9fc9117..8720daf 100644
--- a/drivers/target/iscsi/iscsi_target_nego.c
+++ b/drivers/target/iscsi/iscsi_target_nego.c
@@ -1246,16 +1246,16 @@ int iscsi_target_start_negotiation(
{
int ret;
- ret = iscsi_target_do_login(conn, login);
- if (!ret) {
- if (conn->sock) {
- struct sock *sk = conn->sock->sk;
+ if (conn->sock) {
+ struct sock *sk = conn->sock->sk;
- write_lock_bh(&sk->sk_callback_lock);
- set_bit(LOGIN_FLAGS_READY, &conn->login_flags);
- write_unlock_bh(&sk->sk_callback_lock);
- }
- } else if (ret < 0) {
+ write_lock_bh(&sk->sk_callback_lock);
+ set_bit(LOGIN_FLAGS_READY, &conn->login_flags);
+ write_unlock_bh(&sk->sk_callback_lock);
+ }
+
+ ret = iscsi_target_do_login(conn, login);
+ if (ret < 0) {
cancel_delayed_work_sync(&conn->login_work);
cancel_delayed_work_sync(&conn->login_cleanup_work);
iscsi_target_restore_sock_callbacks(conn);
Note each iscsi_target_do_login_rx() work_struct callback is still
clearing LOGIN_FLAGS_READ_ACTIVE after iscsi_target_do_login() is
invoked each time to handle subsequent login request PDUs.
So it's unclear if you'll hit a similar problem with clear_bit for
LOGIN_FLAGS_READ_ACTIVE in subsequent login PDUs, or if the above to
kick-off ->sk_data_ready() work_struct processing after initial login
request PDU is received does the trick.
Either way, please give the above a shot on your setup, and let's see
where we end up.
Thanks again,
--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
