* [PATCH 0/2] iscsi-target: fix login error when receiving is too fast
@ 2020-04-15 8:08 ` Pu Hou
0 siblings, 0 replies; 12+ messages in thread
From: Pu Hou @ 2020-04-15 8:08 UTC (permalink / raw)
To: martin.petersen, linux-scsi, target-devel; +Cc: mchristi, Pu Hou
Hi,
We encountered "iSCSI Login negotiation failed" several times.
After enable debug log in iscsi_target_nego.c of iSCSI target.
Error shows:
"Got LOGIN_FLAGS_READ_ACTIVE=1, conn: xxxxxxxxxx >>>>"
Patch 1 is trying to fix this problem. Please see comment in patch 1
for details.
Pu Hou (2):
iscsi-target: fix login error when receiving is too fast
iscsi-target: Fix inconsistent debug message in
__iscsi_target_sk_check_close
drivers/target/iscsi/iscsi_target_nego.c | 31 ++++++++++++++++++++++++++-----
include/target/iscsi/iscsi_target_core.h | 1 +
2 files changed, 27 insertions(+), 5 deletions(-)
--
2.11.0
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 0/2] iscsi-target: fix login error when receiving is too fast
@ 2020-04-15 8:08 ` Pu Hou
0 siblings, 0 replies; 12+ messages in thread
From: Pu Hou @ 2020-04-15 8:08 UTC (permalink / raw)
To: martin.petersen, linux-scsi, target-devel; +Cc: mchristi, Pu Hou
Hi,
We encountered "iSCSI Login negotiation failed" several times.
After enable debug log in iscsi_target_nego.c of iSCSI target.
Error shows:
"Got LOGIN_FLAGS_READ_ACTIVE=1, conn: xxxxxxxxxx >>>>"
Patch 1 is trying to fix this problem. Please see comment in patch 1
for details.
Pu Hou (2):
iscsi-target: fix login error when receiving is too fast
iscsi-target: Fix inconsistent debug message in
__iscsi_target_sk_check_close
drivers/target/iscsi/iscsi_target_nego.c | 31 ++++++++++++++++++++++++++-----
include/target/iscsi/iscsi_target_core.h | 1 +
2 files changed, 27 insertions(+), 5 deletions(-)
--
2.11.0
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/2] iscsi-target: fix login error when receiving is too fast
2020-04-15 8:08 ` Pu Hou
@ 2020-04-15 8:08 ` Pu Hou
-1 siblings, 0 replies; 12+ messages in thread
From: Pu Hou @ 2020-04-15 8:08 UTC (permalink / raw)
To: martin.petersen, linux-scsi, target-devel; +Cc: mchristi, Pu Hou
iscsi_target_sk_data_ready() could be invoked indirectly
by iscsi_target_do_login_rx() from workqueue like following:
iscsi_target_do_login_rx()
iscsi_target_do_login()
iscsi_target_do_tx_login_io()
iscsit_put_login_tx()
iscsi_login_tx_data()
tx_data()
sock_sendmsg_nosec()
tcp_sendmsg()
release_sock()
sk_backlog_rcv()
tcp_v4_do_rcv()
tcp_data_ready()
iscsi_target_sk_data_ready()
At that time LOGIN_FLAGS_READ_ACTIVE is not cleared.
and iscsi_target_sk_data_ready will not read data
from the socket. And some iscsi initiator(i.e. libiscsi)
will wait forever for a reply.
LOGIN_FLAGS_READ_ACTIVE should be cleared early just after
doing the receive and before writing to the socket in
iscsi_target_do_login_rx.
But sad news is that LOGIN_FLAGS_READ_ACTIVE is also used
by sk_state_change to do login cleanup if a socket was closed
at login time. It is supposed to be cleared after the login
pdu is successfully processed and reponsed.
So introduce another flag LOGIN_FLAGS_WRITE_ACTIVE to cover
the transmit part so that sk_state_change could work well
and clear LOGIN_FLAGS_READ_ACTIVE early so that sk_data_ready
could also work.
Signed-off-by: Pu Hou <houpu@bytedance.com>
---
drivers/target/iscsi/iscsi_target_nego.c | 29 +++++++++++++++++++++++++----
include/target/iscsi/iscsi_target_core.h | 1 +
2 files changed, 26 insertions(+), 4 deletions(-)
diff --git a/drivers/target/iscsi/iscsi_target_nego.c b/drivers/target/iscsi/iscsi_target_nego.c
index 685d771b51d4..950339655778 100644
--- a/drivers/target/iscsi/iscsi_target_nego.c
+++ b/drivers/target/iscsi/iscsi_target_nego.c
@@ -622,6 +622,26 @@ static void iscsi_target_do_login_rx(struct work_struct *work)
if (rc < 0)
goto err;
+ /*
+ * LOGIN_FLAGS_READ_ACTIVE is cleared so that sk_data_ready
+ * could be trigger again after this.
+ *
+ * LOGIN_FLAGS_WRITE_ACTIVE is cleared after we successfully
+ * processing a login pdu. So that sk_state_chage could do login
+ * cleanup as need if the socket is closed. If a delay work is
+ * ongoing LOGIN_FLAGS_WRITE_ACTIVE or LOGIN_FLAGS_READ_ACTIVE),
+ * sk_state_change will leave the cleanup to the delayed work or
+ * it will schedule a delayed work to do cleanup.
+ */
+ if (conn->sock) {
+ struct sock *sk = conn->sock->sk;
+
+ write_lock_bh(&sk->sk_callback_lock);
+ clear_bit(LOGIN_FLAGS_READ_ACTIVE, &conn->login_flags);
+ set_bit(LOGIN_FLAGS_WRITE_ACTIVE, &conn->login_flags);
+ write_unlock_bh(&sk->sk_callback_lock);
+ }
+
pr_debug("iscsi_target_do_login_rx after rx_login_io, %p, %s:%d\n",
conn, current->comm, current->pid);
@@ -629,7 +649,7 @@ static void iscsi_target_do_login_rx(struct work_struct *work)
if (rc < 0) {
goto err;
} else if (!rc) {
- if (iscsi_target_sk_check_and_clear(conn, LOGIN_FLAGS_READ_ACTIVE))
+ if (iscsi_target_sk_check_and_clear(conn, LOGIN_FLAGS_WRITE_ACTIVE))
goto err;
} else if (rc = 1) {
iscsi_target_nego_release(conn);
@@ -670,9 +690,10 @@ static void iscsi_target_sk_state_change(struct sock *sk)
state = __iscsi_target_sk_check_close(sk);
pr_debug("__iscsi_target_sk_close_change: state: %d\n", state);
- if (test_bit(LOGIN_FLAGS_READ_ACTIVE, &conn->login_flags)) {
- pr_debug("Got LOGIN_FLAGS_READ_ACTIVE=1 sk_state_change"
- " conn: %p\n", conn);
+ if (test_bit(LOGIN_FLAGS_READ_ACTIVE, &conn->login_flags) ||
+ test_bit(LOGIN_FLAGS_WRITE_ACTIVE, &conn->login_flags)) {
+ pr_debug("Got LOGIN_FLAGS_{READ|WRITE}_ACTIVE=1"
+ "sk_state_change conn: %p\n", conn);
if (state)
set_bit(LOGIN_FLAGS_CLOSED, &conn->login_flags);
write_unlock_bh(&sk->sk_callback_lock);
diff --git a/include/target/iscsi/iscsi_target_core.h b/include/target/iscsi/iscsi_target_core.h
index 591cd9e4692c..0c2dfc81bf8b 100644
--- a/include/target/iscsi/iscsi_target_core.h
+++ b/include/target/iscsi/iscsi_target_core.h
@@ -570,6 +570,7 @@ struct iscsi_conn {
#define LOGIN_FLAGS_CLOSED 2
#define LOGIN_FLAGS_READY 4
#define LOGIN_FLAGS_INITIAL_PDU 8
+#define LOGIN_FLAGS_WRITE_ACTIVE 12
unsigned long login_flags;
struct delayed_work login_work;
struct iscsi_login *login;
--
2.11.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 1/2] iscsi-target: fix login error when receiving is too fast
@ 2020-04-15 8:08 ` Pu Hou
0 siblings, 0 replies; 12+ messages in thread
From: Pu Hou @ 2020-04-15 8:08 UTC (permalink / raw)
To: martin.petersen, linux-scsi, target-devel; +Cc: mchristi, Pu Hou
iscsi_target_sk_data_ready() could be invoked indirectly
by iscsi_target_do_login_rx() from workqueue like following:
iscsi_target_do_login_rx()
iscsi_target_do_login()
iscsi_target_do_tx_login_io()
iscsit_put_login_tx()
iscsi_login_tx_data()
tx_data()
sock_sendmsg_nosec()
tcp_sendmsg()
release_sock()
sk_backlog_rcv()
tcp_v4_do_rcv()
tcp_data_ready()
iscsi_target_sk_data_ready()
At that time LOGIN_FLAGS_READ_ACTIVE is not cleared.
and iscsi_target_sk_data_ready will not read data
from the socket. And some iscsi initiator(i.e. libiscsi)
will wait forever for a reply.
LOGIN_FLAGS_READ_ACTIVE should be cleared early just after
doing the receive and before writing to the socket in
iscsi_target_do_login_rx.
But sad news is that LOGIN_FLAGS_READ_ACTIVE is also used
by sk_state_change to do login cleanup if a socket was closed
at login time. It is supposed to be cleared after the login
pdu is successfully processed and reponsed.
So introduce another flag LOGIN_FLAGS_WRITE_ACTIVE to cover
the transmit part so that sk_state_change could work well
and clear LOGIN_FLAGS_READ_ACTIVE early so that sk_data_ready
could also work.
Signed-off-by: Pu Hou <houpu@bytedance.com>
---
drivers/target/iscsi/iscsi_target_nego.c | 29 +++++++++++++++++++++++++----
include/target/iscsi/iscsi_target_core.h | 1 +
2 files changed, 26 insertions(+), 4 deletions(-)
diff --git a/drivers/target/iscsi/iscsi_target_nego.c b/drivers/target/iscsi/iscsi_target_nego.c
index 685d771b51d4..950339655778 100644
--- a/drivers/target/iscsi/iscsi_target_nego.c
+++ b/drivers/target/iscsi/iscsi_target_nego.c
@@ -622,6 +622,26 @@ static void iscsi_target_do_login_rx(struct work_struct *work)
if (rc < 0)
goto err;
+ /*
+ * LOGIN_FLAGS_READ_ACTIVE is cleared so that sk_data_ready
+ * could be trigger again after this.
+ *
+ * LOGIN_FLAGS_WRITE_ACTIVE is cleared after we successfully
+ * processing a login pdu. So that sk_state_chage could do login
+ * cleanup as need if the socket is closed. If a delay work is
+ * ongoing LOGIN_FLAGS_WRITE_ACTIVE or LOGIN_FLAGS_READ_ACTIVE),
+ * sk_state_change will leave the cleanup to the delayed work or
+ * it will schedule a delayed work to do cleanup.
+ */
+ if (conn->sock) {
+ struct sock *sk = conn->sock->sk;
+
+ write_lock_bh(&sk->sk_callback_lock);
+ clear_bit(LOGIN_FLAGS_READ_ACTIVE, &conn->login_flags);
+ set_bit(LOGIN_FLAGS_WRITE_ACTIVE, &conn->login_flags);
+ write_unlock_bh(&sk->sk_callback_lock);
+ }
+
pr_debug("iscsi_target_do_login_rx after rx_login_io, %p, %s:%d\n",
conn, current->comm, current->pid);
@@ -629,7 +649,7 @@ static void iscsi_target_do_login_rx(struct work_struct *work)
if (rc < 0) {
goto err;
} else if (!rc) {
- if (iscsi_target_sk_check_and_clear(conn, LOGIN_FLAGS_READ_ACTIVE))
+ if (iscsi_target_sk_check_and_clear(conn, LOGIN_FLAGS_WRITE_ACTIVE))
goto err;
} else if (rc == 1) {
iscsi_target_nego_release(conn);
@@ -670,9 +690,10 @@ static void iscsi_target_sk_state_change(struct sock *sk)
state = __iscsi_target_sk_check_close(sk);
pr_debug("__iscsi_target_sk_close_change: state: %d\n", state);
- if (test_bit(LOGIN_FLAGS_READ_ACTIVE, &conn->login_flags)) {
- pr_debug("Got LOGIN_FLAGS_READ_ACTIVE=1 sk_state_change"
- " conn: %p\n", conn);
+ if (test_bit(LOGIN_FLAGS_READ_ACTIVE, &conn->login_flags) ||
+ test_bit(LOGIN_FLAGS_WRITE_ACTIVE, &conn->login_flags)) {
+ pr_debug("Got LOGIN_FLAGS_{READ|WRITE}_ACTIVE=1"
+ "sk_state_change conn: %p\n", conn);
if (state)
set_bit(LOGIN_FLAGS_CLOSED, &conn->login_flags);
write_unlock_bh(&sk->sk_callback_lock);
diff --git a/include/target/iscsi/iscsi_target_core.h b/include/target/iscsi/iscsi_target_core.h
index 591cd9e4692c..0c2dfc81bf8b 100644
--- a/include/target/iscsi/iscsi_target_core.h
+++ b/include/target/iscsi/iscsi_target_core.h
@@ -570,6 +570,7 @@ struct iscsi_conn {
#define LOGIN_FLAGS_CLOSED 2
#define LOGIN_FLAGS_READY 4
#define LOGIN_FLAGS_INITIAL_PDU 8
+#define LOGIN_FLAGS_WRITE_ACTIVE 12
unsigned long login_flags;
struct delayed_work login_work;
struct iscsi_login *login;
--
2.11.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/2] iscsi-target: Fix inconsistent debug message in __iscsi_target_sk_check_close
2020-04-15 8:08 ` Pu Hou
@ 2020-04-15 8:08 ` Pu Hou
-1 siblings, 0 replies; 12+ messages in thread
From: Pu Hou @ 2020-04-15 8:08 UTC (permalink / raw)
To: martin.petersen, linux-scsi, target-devel; +Cc: mchristi, Pu Hou
Following commit changed the return value of __iscsi_target_sk_check_close.
But the pr_debug is still printing FALSE when returing TRUE which is a little
confusing.
commit 25cdda95fda78d22d44157da15aa7ea34be3c804
Author: Nicholas Bellinger <nab@linux-iscsi.org>
Date: Wed May 24 21:47:09 2017 -0700
iscsi-target: Fix initial login PDU asynchronous socket close OOPs
Signed-off-by: Pu Hou <houpu@bytedance.com>
---
drivers/target/iscsi/iscsi_target_nego.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/target/iscsi/iscsi_target_nego.c b/drivers/target/iscsi/iscsi_target_nego.c
index 950339655778..56686e880d23 100644
--- a/drivers/target/iscsi/iscsi_target_nego.c
+++ b/drivers/target/iscsi/iscsi_target_nego.c
@@ -481,7 +481,7 @@ static bool __iscsi_target_sk_check_close(struct sock *sk)
{
if (sk->sk_state = TCP_CLOSE_WAIT || sk->sk_state = TCP_CLOSE) {
pr_debug("__iscsi_target_sk_check_close: TCP_CLOSE_WAIT|TCP_CLOSE,"
- "returning FALSE\n");
+ "returning TRUE\n");
return true;
}
return false;
--
2.11.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/2] iscsi-target: Fix inconsistent debug message in __iscsi_target_sk_check_close
@ 2020-04-15 8:08 ` Pu Hou
0 siblings, 0 replies; 12+ messages in thread
From: Pu Hou @ 2020-04-15 8:08 UTC (permalink / raw)
To: martin.petersen, linux-scsi, target-devel; +Cc: mchristi, Pu Hou
Following commit changed the return value of __iscsi_target_sk_check_close.
But the pr_debug is still printing FALSE when returing TRUE which is a little
confusing.
commit 25cdda95fda78d22d44157da15aa7ea34be3c804
Author: Nicholas Bellinger <nab@linux-iscsi.org>
Date: Wed May 24 21:47:09 2017 -0700
iscsi-target: Fix initial login PDU asynchronous socket close OOPs
Signed-off-by: Pu Hou <houpu@bytedance.com>
---
drivers/target/iscsi/iscsi_target_nego.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/target/iscsi/iscsi_target_nego.c b/drivers/target/iscsi/iscsi_target_nego.c
index 950339655778..56686e880d23 100644
--- a/drivers/target/iscsi/iscsi_target_nego.c
+++ b/drivers/target/iscsi/iscsi_target_nego.c
@@ -481,7 +481,7 @@ static bool __iscsi_target_sk_check_close(struct sock *sk)
{
if (sk->sk_state == TCP_CLOSE_WAIT || sk->sk_state == TCP_CLOSE) {
pr_debug("__iscsi_target_sk_check_close: TCP_CLOSE_WAIT|TCP_CLOSE,"
- "returning FALSE\n");
+ "returning TRUE\n");
return true;
}
return false;
--
2.11.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] iscsi-target: fix login error when receiving is too fast
2020-04-15 8:08 ` Pu Hou
@ 2020-04-22 23:12 ` Mike Christie
-1 siblings, 0 replies; 12+ messages in thread
From: Mike Christie @ 2020-04-22 23:12 UTC (permalink / raw)
To: Pu Hou, martin.petersen, linux-scsi, target-devel
On 4/15/20 3:08 AM, Pu Hou wrote:
> iscsi_target_sk_data_ready() could be invoked indirectly
> by iscsi_target_do_login_rx() from workqueue like following:
>
> iscsi_target_do_login_rx()
> iscsi_target_do_login()
> iscsi_target_do_tx_login_io()
> iscsit_put_login_tx()
> iscsi_login_tx_data()
> tx_data()
> sock_sendmsg_nosec()
> tcp_sendmsg()
> release_sock()
> sk_backlog_rcv()
> tcp_v4_do_rcv()
> tcp_data_ready()
> iscsi_target_sk_data_ready()
>
> At that time LOGIN_FLAGS_READ_ACTIVE is not cleared.
> and iscsi_target_sk_data_ready will not read data
> from the socket. And some iscsi initiator(i.e. libiscsi)
> will wait forever for a reply.
>
> LOGIN_FLAGS_READ_ACTIVE should be cleared early just after
> doing the receive and before writing to the socket in
> iscsi_target_do_login_rx.
>
> But sad news is that LOGIN_FLAGS_READ_ACTIVE is also used
> by sk_state_change to do login cleanup if a socket was closed
> at login time. It is supposed to be cleared after the login
> pdu is successfully processed and reponsed.
>
> So introduce another flag LOGIN_FLAGS_WRITE_ACTIVE to cover
> the transmit part so that sk_state_change could work well
> and clear LOGIN_FLAGS_READ_ACTIVE early so that sk_data_ready
> could also work.
>
> Signed-off-by: Pu Hou <houpu@bytedance.com>
> ---
> drivers/target/iscsi/iscsi_target_nego.c | 29 +++++++++++++++++++++++++----
> include/target/iscsi/iscsi_target_core.h | 1 +
> 2 files changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/target/iscsi/iscsi_target_nego.c b/drivers/target/iscsi/iscsi_target_nego.c
> index 685d771b51d4..950339655778 100644
> --- a/drivers/target/iscsi/iscsi_target_nego.c
> +++ b/drivers/target/iscsi/iscsi_target_nego.c
> @@ -622,6 +622,26 @@ static void iscsi_target_do_login_rx(struct work_struct *work)
> if (rc < 0)
> goto err;
>
> + /*
> + * LOGIN_FLAGS_READ_ACTIVE is cleared so that sk_data_ready
> + * could be trigger again after this.
> + *
> + * LOGIN_FLAGS_WRITE_ACTIVE is cleared after we successfully
> + * processing a login pdu. So that sk_state_chage could do login
I think we need to drop "ing" from processing and do:
process a login pdu, so that sk_state_chage could do login
> + * cleanup as need if the socket is closed. If a delay work is
> + * ongoing LOGIN_FLAGS_WRITE_ACTIVE or LOGIN_FLAGS_READ_ACTIVE),
> + * sk_state_change will leave the cleanup to the delayed work or
> + * it will schedule a delayed work to do cleanup.
> + */
> + if (conn->sock) {
> + struct sock *sk = conn->sock->sk;
> +
> + write_lock_bh(&sk->sk_callback_lock);
> + clear_bit(LOGIN_FLAGS_READ_ACTIVE, &conn->login_flags);
> + set_bit(LOGIN_FLAGS_WRITE_ACTIVE, &conn->login_flags);
> + write_unlock_bh(&sk->sk_callback_lock);
> + }
> +
> pr_debug("iscsi_target_do_login_rx after rx_login_io, %p, %s:%d\n",
> conn, current->comm, current->pid);
>
> @@ -629,7 +649,7 @@ static void iscsi_target_do_login_rx(struct work_struct *work)
> if (rc < 0) {
> goto err;
> } else if (!rc) {
> - if (iscsi_target_sk_check_and_clear(conn, LOGIN_FLAGS_READ_ACTIVE))
> + if (iscsi_target_sk_check_and_clear(conn, LOGIN_FLAGS_WRITE_ACTIVE))
> goto err;
> } else if (rc = 1) {
> iscsi_target_nego_release(conn);
> @@ -670,9 +690,10 @@ static void iscsi_target_sk_state_change(struct sock *sk)
> state = __iscsi_target_sk_check_close(sk);
> pr_debug("__iscsi_target_sk_close_change: state: %d\n", state);
>
> - if (test_bit(LOGIN_FLAGS_READ_ACTIVE, &conn->login_flags)) {
> - pr_debug("Got LOGIN_FLAGS_READ_ACTIVE=1 sk_state_change"
> - " conn: %p\n", conn);
> + if (test_bit(LOGIN_FLAGS_READ_ACTIVE, &conn->login_flags) ||
> + test_bit(LOGIN_FLAGS_WRITE_ACTIVE, &conn->login_flags)) {
> + pr_debug("Got LOGIN_FLAGS_{READ|WRITE}_ACTIVE=1"
> + "sk_state_change conn: %p\n", conn);
> if (state)
> set_bit(LOGIN_FLAGS_CLOSED, &conn->login_flags);
> write_unlock_bh(&sk->sk_callback_lock);
> diff --git a/include/target/iscsi/iscsi_target_core.h b/include/target/iscsi/iscsi_target_core.h
> index 591cd9e4692c..0c2dfc81bf8b 100644
> --- a/include/target/iscsi/iscsi_target_core.h
> +++ b/include/target/iscsi/iscsi_target_core.h
> @@ -570,6 +570,7 @@ struct iscsi_conn {
> #define LOGIN_FLAGS_CLOSED 2
> #define LOGIN_FLAGS_READY 4
> #define LOGIN_FLAGS_INITIAL_PDU 8
> +#define LOGIN_FLAGS_WRITE_ACTIVE 12
12 works but seems odd. The current code uses test/set/clear_bit, so we
want these to be:
#define LOGIN_FLAGS_CLOSED 0
#define LOGIN_FLAGS_READY 1
#define LOGIN_FLAGS_INITIAL_PDU 2
#define LOGIN_FLAGS_WRITE_ACTIVE 3
right?
2, 4, 8 was probably from a time we set the bits our self like:
login_flags |= LOGIN_FLAGS_CLOSED;
> unsigned long login_flags;
> struct delayed_work login_work;
> struct iscsi_login *login;
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] iscsi-target: fix login error when receiving is too fast
@ 2020-04-22 23:12 ` Mike Christie
0 siblings, 0 replies; 12+ messages in thread
From: Mike Christie @ 2020-04-22 23:12 UTC (permalink / raw)
To: Pu Hou, martin.petersen, linux-scsi, target-devel
On 4/15/20 3:08 AM, Pu Hou wrote:
> iscsi_target_sk_data_ready() could be invoked indirectly
> by iscsi_target_do_login_rx() from workqueue like following:
>
> iscsi_target_do_login_rx()
> iscsi_target_do_login()
> iscsi_target_do_tx_login_io()
> iscsit_put_login_tx()
> iscsi_login_tx_data()
> tx_data()
> sock_sendmsg_nosec()
> tcp_sendmsg()
> release_sock()
> sk_backlog_rcv()
> tcp_v4_do_rcv()
> tcp_data_ready()
> iscsi_target_sk_data_ready()
>
> At that time LOGIN_FLAGS_READ_ACTIVE is not cleared.
> and iscsi_target_sk_data_ready will not read data
> from the socket. And some iscsi initiator(i.e. libiscsi)
> will wait forever for a reply.
>
> LOGIN_FLAGS_READ_ACTIVE should be cleared early just after
> doing the receive and before writing to the socket in
> iscsi_target_do_login_rx.
>
> But sad news is that LOGIN_FLAGS_READ_ACTIVE is also used
> by sk_state_change to do login cleanup if a socket was closed
> at login time. It is supposed to be cleared after the login
> pdu is successfully processed and reponsed.
>
> So introduce another flag LOGIN_FLAGS_WRITE_ACTIVE to cover
> the transmit part so that sk_state_change could work well
> and clear LOGIN_FLAGS_READ_ACTIVE early so that sk_data_ready
> could also work.
>
> Signed-off-by: Pu Hou <houpu@bytedance.com>
> ---
> drivers/target/iscsi/iscsi_target_nego.c | 29 +++++++++++++++++++++++++----
> include/target/iscsi/iscsi_target_core.h | 1 +
> 2 files changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/target/iscsi/iscsi_target_nego.c b/drivers/target/iscsi/iscsi_target_nego.c
> index 685d771b51d4..950339655778 100644
> --- a/drivers/target/iscsi/iscsi_target_nego.c
> +++ b/drivers/target/iscsi/iscsi_target_nego.c
> @@ -622,6 +622,26 @@ static void iscsi_target_do_login_rx(struct work_struct *work)
> if (rc < 0)
> goto err;
>
> + /*
> + * LOGIN_FLAGS_READ_ACTIVE is cleared so that sk_data_ready
> + * could be trigger again after this.
> + *
> + * LOGIN_FLAGS_WRITE_ACTIVE is cleared after we successfully
> + * processing a login pdu. So that sk_state_chage could do login
I think we need to drop "ing" from processing and do:
process a login pdu, so that sk_state_chage could do login
> + * cleanup as need if the socket is closed. If a delay work is
> + * ongoing LOGIN_FLAGS_WRITE_ACTIVE or LOGIN_FLAGS_READ_ACTIVE),
> + * sk_state_change will leave the cleanup to the delayed work or
> + * it will schedule a delayed work to do cleanup.
> + */
> + if (conn->sock) {
> + struct sock *sk = conn->sock->sk;
> +
> + write_lock_bh(&sk->sk_callback_lock);
> + clear_bit(LOGIN_FLAGS_READ_ACTIVE, &conn->login_flags);
> + set_bit(LOGIN_FLAGS_WRITE_ACTIVE, &conn->login_flags);
> + write_unlock_bh(&sk->sk_callback_lock);
> + }
> +
> pr_debug("iscsi_target_do_login_rx after rx_login_io, %p, %s:%d\n",
> conn, current->comm, current->pid);
>
> @@ -629,7 +649,7 @@ static void iscsi_target_do_login_rx(struct work_struct *work)
> if (rc < 0) {
> goto err;
> } else if (!rc) {
> - if (iscsi_target_sk_check_and_clear(conn, LOGIN_FLAGS_READ_ACTIVE))
> + if (iscsi_target_sk_check_and_clear(conn, LOGIN_FLAGS_WRITE_ACTIVE))
> goto err;
> } else if (rc == 1) {
> iscsi_target_nego_release(conn);
> @@ -670,9 +690,10 @@ static void iscsi_target_sk_state_change(struct sock *sk)
> state = __iscsi_target_sk_check_close(sk);
> pr_debug("__iscsi_target_sk_close_change: state: %d\n", state);
>
> - if (test_bit(LOGIN_FLAGS_READ_ACTIVE, &conn->login_flags)) {
> - pr_debug("Got LOGIN_FLAGS_READ_ACTIVE=1 sk_state_change"
> - " conn: %p\n", conn);
> + if (test_bit(LOGIN_FLAGS_READ_ACTIVE, &conn->login_flags) ||
> + test_bit(LOGIN_FLAGS_WRITE_ACTIVE, &conn->login_flags)) {
> + pr_debug("Got LOGIN_FLAGS_{READ|WRITE}_ACTIVE=1"
> + "sk_state_change conn: %p\n", conn);
> if (state)
> set_bit(LOGIN_FLAGS_CLOSED, &conn->login_flags);
> write_unlock_bh(&sk->sk_callback_lock);
> diff --git a/include/target/iscsi/iscsi_target_core.h b/include/target/iscsi/iscsi_target_core.h
> index 591cd9e4692c..0c2dfc81bf8b 100644
> --- a/include/target/iscsi/iscsi_target_core.h
> +++ b/include/target/iscsi/iscsi_target_core.h
> @@ -570,6 +570,7 @@ struct iscsi_conn {
> #define LOGIN_FLAGS_CLOSED 2
> #define LOGIN_FLAGS_READY 4
> #define LOGIN_FLAGS_INITIAL_PDU 8
> +#define LOGIN_FLAGS_WRITE_ACTIVE 12
12 works but seems odd. The current code uses test/set/clear_bit, so we
want these to be:
#define LOGIN_FLAGS_CLOSED 0
#define LOGIN_FLAGS_READY 1
#define LOGIN_FLAGS_INITIAL_PDU 2
#define LOGIN_FLAGS_WRITE_ACTIVE 3
right?
2, 4, 8 was probably from a time we set the bits our self like:
login_flags |= LOGIN_FLAGS_CLOSED;
> unsigned long login_flags;
> struct delayed_work login_work;
> struct iscsi_login *login;
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [External] Re: [PATCH 1/2] iscsi-target: fix login error when receiving is too fast
2020-04-22 23:12 ` Mike Christie
@ 2020-04-23 4:24 ` Hou Pu
-1 siblings, 0 replies; 12+ messages in thread
From: Hou Pu @ 2020-04-23 4:24 UTC (permalink / raw)
To: Mike Christie; +Cc: martin.petersen, linux-scsi, target-devel
> > + /*
> > + * LOGIN_FLAGS_READ_ACTIVE is cleared so that sk_data_ready
> > + * could be trigger again after this.
> > + *
> > + * LOGIN_FLAGS_WRITE_ACTIVE is cleared after we successfully
> > + * processing a login pdu. So that sk_state_chage could do login
>
> I think we need to drop "ing" from processing and do:
>
> process a login pdu, so that sk_state_chage could do login
>
Sure. Thanks for helping me with my language. ^-^
Will change this in v2.
>
> > diff --git a/include/target/iscsi/iscsi_target_core.h b/include/target/iscsi/iscsi_target_core.h
> > index 591cd9e4692c..0c2dfc81bf8b 100644
> > --- a/include/target/iscsi/iscsi_target_core.h
> > +++ b/include/target/iscsi/iscsi_target_core.h
> > @@ -570,6 +570,7 @@ struct iscsi_conn {
> > #define LOGIN_FLAGS_CLOSED 2
> > #define LOGIN_FLAGS_READY 4
> > #define LOGIN_FLAGS_INITIAL_PDU 8
> > +#define LOGIN_FLAGS_WRITE_ACTIVE 12
>
> 12 works but seems odd. The current code uses test/set/clear_bit, so we
> want these to be:
>
> #define LOGIN_FLAGS_CLOSED 0
> #define LOGIN_FLAGS_READY 1
> #define LOGIN_FLAGS_INITIAL_PDU 2
> #define LOGIN_FLAGS_WRITE_ACTIVE 3
>
> right?
>
Yes, it is a little odd. What about this? I also changed the order of them
to be in sequence that happened.
--- a/include/target/iscsi/iscsi_target_core.h
+++ b/include/target/iscsi/iscsi_target_core.h
@@ -566,10 +566,11 @@ struct iscsi_conn {
struct socket *sock;
void (*orig_data_ready)(struct sock *);
void (*orig_state_change)(struct sock *);
-#define LOGIN_FLAGS_READ_ACTIVE 1
-#define LOGIN_FLAGS_CLOSED 2
-#define LOGIN_FLAGS_READY 4
-#define LOGIN_FLAGS_INITIAL_PDU 8
+#define LOGIN_FLAGS_READY 0
+#define LOGIN_FLAGS_INITIAL_PDU 1
+#define LOGIN_FLAGS_READ_ACTIVE 2
+#define LOGIN_FLAGS_WRITE_ACTIVE 3
+#define LOGIN_FLAGS_CLOSED 4
Thanks,
Hou
> 2, 4, 8 was probably from a time we set the bits our self like:
>
> login_flags |= LOGIN_FLAGS_CLOSED;
>
>
> > unsigned long login_flags;
> > struct delayed_work login_work;
> > struct iscsi_login *login;
> >
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [External] Re: [PATCH 1/2] iscsi-target: fix login error when receiving is too fast
@ 2020-04-23 4:24 ` Hou Pu
0 siblings, 0 replies; 12+ messages in thread
From: Hou Pu @ 2020-04-23 4:24 UTC (permalink / raw)
To: Mike Christie; +Cc: martin.petersen, linux-scsi, target-devel
> > + /*
> > + * LOGIN_FLAGS_READ_ACTIVE is cleared so that sk_data_ready
> > + * could be trigger again after this.
> > + *
> > + * LOGIN_FLAGS_WRITE_ACTIVE is cleared after we successfully
> > + * processing a login pdu. So that sk_state_chage could do login
>
> I think we need to drop "ing" from processing and do:
>
> process a login pdu, so that sk_state_chage could do login
>
Sure. Thanks for helping me with my language. ^-^
Will change this in v2.
>
> > diff --git a/include/target/iscsi/iscsi_target_core.h b/include/target/iscsi/iscsi_target_core.h
> > index 591cd9e4692c..0c2dfc81bf8b 100644
> > --- a/include/target/iscsi/iscsi_target_core.h
> > +++ b/include/target/iscsi/iscsi_target_core.h
> > @@ -570,6 +570,7 @@ struct iscsi_conn {
> > #define LOGIN_FLAGS_CLOSED 2
> > #define LOGIN_FLAGS_READY 4
> > #define LOGIN_FLAGS_INITIAL_PDU 8
> > +#define LOGIN_FLAGS_WRITE_ACTIVE 12
>
> 12 works but seems odd. The current code uses test/set/clear_bit, so we
> want these to be:
>
> #define LOGIN_FLAGS_CLOSED 0
> #define LOGIN_FLAGS_READY 1
> #define LOGIN_FLAGS_INITIAL_PDU 2
> #define LOGIN_FLAGS_WRITE_ACTIVE 3
>
> right?
>
Yes, it is a little odd. What about this? I also changed the order of them
to be in sequence that happened.
--- a/include/target/iscsi/iscsi_target_core.h
+++ b/include/target/iscsi/iscsi_target_core.h
@@ -566,10 +566,11 @@ struct iscsi_conn {
struct socket *sock;
void (*orig_data_ready)(struct sock *);
void (*orig_state_change)(struct sock *);
-#define LOGIN_FLAGS_READ_ACTIVE 1
-#define LOGIN_FLAGS_CLOSED 2
-#define LOGIN_FLAGS_READY 4
-#define LOGIN_FLAGS_INITIAL_PDU 8
+#define LOGIN_FLAGS_READY 0
+#define LOGIN_FLAGS_INITIAL_PDU 1
+#define LOGIN_FLAGS_READ_ACTIVE 2
+#define LOGIN_FLAGS_WRITE_ACTIVE 3
+#define LOGIN_FLAGS_CLOSED 4
Thanks,
Hou
> 2, 4, 8 was probably from a time we set the bits our self like:
>
> login_flags |= LOGIN_FLAGS_CLOSED;
>
>
> > unsigned long login_flags;
> > struct delayed_work login_work;
> > struct iscsi_login *login;
> >
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [External] Re: [PATCH 1/2] iscsi-target: fix login error when receiving is too fast
2020-04-23 4:24 ` Hou Pu
@ 2020-04-23 15:31 ` Mike Christie
-1 siblings, 0 replies; 12+ messages in thread
From: Mike Christie @ 2020-04-23 15:31 UTC (permalink / raw)
To: Hou Pu; +Cc: martin.petersen, linux-scsi, target-devel
On 4/22/20 11:24 PM, Hou Pu wrote:
>>> + /*
>>> + * LOGIN_FLAGS_READ_ACTIVE is cleared so that sk_data_ready
>>> + * could be trigger again after this.
>>> + *
>>> + * LOGIN_FLAGS_WRITE_ACTIVE is cleared after we successfully
>>> + * processing a login pdu. So that sk_state_chage could do login
>>
>> I think we need to drop "ing" from processing and do:
>>
>> process a login pdu, so that sk_state_chage could do login
>>
> Sure. Thanks for helping me with my language. ^-^
> Will change this in v2.
>>
>
>>> diff --git a/include/target/iscsi/iscsi_target_core.h b/include/target/iscsi/iscsi_target_core.h
>>> index 591cd9e4692c..0c2dfc81bf8b 100644
>>> --- a/include/target/iscsi/iscsi_target_core.h
>>> +++ b/include/target/iscsi/iscsi_target_core.h
>>> @@ -570,6 +570,7 @@ struct iscsi_conn {
>>> #define LOGIN_FLAGS_CLOSED 2
>>> #define LOGIN_FLAGS_READY 4
>>> #define LOGIN_FLAGS_INITIAL_PDU 8
>>> +#define LOGIN_FLAGS_WRITE_ACTIVE 12
>>
>> 12 works but seems odd. The current code uses test/set/clear_bit, so we
>> want these to be:
>>
>> #define LOGIN_FLAGS_CLOSED 0
>> #define LOGIN_FLAGS_READY 1
>> #define LOGIN_FLAGS_INITIAL_PDU 2
>> #define LOGIN_FLAGS_WRITE_ACTIVE 3
>>
>> right?
>>
> Yes, it is a little odd. What about this? I also changed the order of them
> to be in sequence that happened.
>
> --- a/include/target/iscsi/iscsi_target_core.h
> +++ b/include/target/iscsi/iscsi_target_core.h
> @@ -566,10 +566,11 @@ struct iscsi_conn {
> struct socket *sock;
> void (*orig_data_ready)(struct sock *);
> void (*orig_state_change)(struct sock *);
> -#define LOGIN_FLAGS_READ_ACTIVE 1
> -#define LOGIN_FLAGS_CLOSED 2
> -#define LOGIN_FLAGS_READY 4
> -#define LOGIN_FLAGS_INITIAL_PDU 8
> +#define LOGIN_FLAGS_READY 0
> +#define LOGIN_FLAGS_INITIAL_PDU 1
> +#define LOGIN_FLAGS_READ_ACTIVE 2
> +#define LOGIN_FLAGS_WRITE_ACTIVE 3
> +#define LOGIN_FLAGS_CLOSED 4
>
Looks ok to me.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [External] Re: [PATCH 1/2] iscsi-target: fix login error when receiving is too fast
@ 2020-04-23 15:31 ` Mike Christie
0 siblings, 0 replies; 12+ messages in thread
From: Mike Christie @ 2020-04-23 15:31 UTC (permalink / raw)
To: Hou Pu; +Cc: martin.petersen, linux-scsi, target-devel
On 4/22/20 11:24 PM, Hou Pu wrote:
>>> + /*
>>> + * LOGIN_FLAGS_READ_ACTIVE is cleared so that sk_data_ready
>>> + * could be trigger again after this.
>>> + *
>>> + * LOGIN_FLAGS_WRITE_ACTIVE is cleared after we successfully
>>> + * processing a login pdu. So that sk_state_chage could do login
>>
>> I think we need to drop "ing" from processing and do:
>>
>> process a login pdu, so that sk_state_chage could do login
>>
> Sure. Thanks for helping me with my language. ^-^
> Will change this in v2.
>>
>
>>> diff --git a/include/target/iscsi/iscsi_target_core.h b/include/target/iscsi/iscsi_target_core.h
>>> index 591cd9e4692c..0c2dfc81bf8b 100644
>>> --- a/include/target/iscsi/iscsi_target_core.h
>>> +++ b/include/target/iscsi/iscsi_target_core.h
>>> @@ -570,6 +570,7 @@ struct iscsi_conn {
>>> #define LOGIN_FLAGS_CLOSED 2
>>> #define LOGIN_FLAGS_READY 4
>>> #define LOGIN_FLAGS_INITIAL_PDU 8
>>> +#define LOGIN_FLAGS_WRITE_ACTIVE 12
>>
>> 12 works but seems odd. The current code uses test/set/clear_bit, so we
>> want these to be:
>>
>> #define LOGIN_FLAGS_CLOSED 0
>> #define LOGIN_FLAGS_READY 1
>> #define LOGIN_FLAGS_INITIAL_PDU 2
>> #define LOGIN_FLAGS_WRITE_ACTIVE 3
>>
>> right?
>>
> Yes, it is a little odd. What about this? I also changed the order of them
> to be in sequence that happened.
>
> --- a/include/target/iscsi/iscsi_target_core.h
> +++ b/include/target/iscsi/iscsi_target_core.h
> @@ -566,10 +566,11 @@ struct iscsi_conn {
> struct socket *sock;
> void (*orig_data_ready)(struct sock *);
> void (*orig_state_change)(struct sock *);
> -#define LOGIN_FLAGS_READ_ACTIVE 1
> -#define LOGIN_FLAGS_CLOSED 2
> -#define LOGIN_FLAGS_READY 4
> -#define LOGIN_FLAGS_INITIAL_PDU 8
> +#define LOGIN_FLAGS_READY 0
> +#define LOGIN_FLAGS_INITIAL_PDU 1
> +#define LOGIN_FLAGS_READ_ACTIVE 2
> +#define LOGIN_FLAGS_WRITE_ACTIVE 3
> +#define LOGIN_FLAGS_CLOSED 4
>
Looks ok to me.
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-04-23 15:31 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-15 8:08 [PATCH 0/2] iscsi-target: fix login error when receiving is too fast Pu Hou
2020-04-15 8:08 ` Pu Hou
2020-04-15 8:08 ` [PATCH 1/2] " Pu Hou
2020-04-15 8:08 ` Pu Hou
2020-04-22 23:12 ` Mike Christie
2020-04-22 23:12 ` Mike Christie
2020-04-23 4:24 ` [External] " Hou Pu
2020-04-23 4:24 ` Hou Pu
2020-04-23 15:31 ` Mike Christie
2020-04-23 15:31 ` Mike Christie
2020-04-15 8:08 ` [PATCH 2/2] iscsi-target: Fix inconsistent debug message in __iscsi_target_sk_check_close Pu Hou
2020-04-15 8:08 ` Pu Hou
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.