+ */
+ 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);
+ }
+
Hey,
I had one more question.
With the above code, I think we have a race where if we clear the bit
above early and iscsi_target_sk_data_ready runs while
iscsi_target_do_login_rx is still running then we could queue the work
an extra time and get stuck. Because the bit is now not set, if
iscsi_target_sk_data_ready runs it will end up calling
schedule_delayed_work which will queue up the work again since the work
is running and not pending.
Yes. I was trying to allow queuing the delayed work early.
If that is correct and we hit the race what happens if this was the
last
login pdu, and we are supposed to go to full feature phase next? For
example if iscsi_target_do_login_rx runs an extra time, will we end up
stuck waiting in iscsi_target_do_login_rx's call to:
rc = conn->conn_transport->iscsit_get_login_rx(conn, login);
?
For the last login pdu, we may have race as you said. thanks for
pointing it out.
I was trying to image a case where we can hit the race, normally it is
case a).
a). initiator send last login pdu -> target received -> target replied
b). initiator send last login pdu -> target received -> initiator send
something -> target replied
in case b). we will queue another delayed work which we should not.
After the target replied
the last login pdu, conn->conn_login is freed. we might visited it in
the delayed work.
Just answering my own question. It looks like we do not get stuck. But
we either get nothing on the session so the login timeout fires and we
drop the session. Or, we get a PDU and the login thread reads it in
before the normal rx thread does, but it assumes it is a login related
and we most likely drop the session due to invalid fields.
I think in iscsi_target_restore_sock_callbacks we want to do a:
cancel_delayed_work(&conn->login_work)
after we reset the callbacks and drop the sk_callback_lock lock.
I am not very sure if we could or if it is good to cancel_delayed_work
from the work itself.
If it is ok then i am ok with it. Or in another way, I think we could
just clear
LOGIN_FLAGS_READ_ACTIVE and set LOGIN_FLAGS_WRITE_ACTIVE
after iscsi_target_restore_sock_callbacks when finish process last
login pdu.
That would look like (in iscsi_target_do_tx_login_io):
diff --git a/drivers/target/iscsi/iscsi_target_nego.c
b/drivers/target/iscsi/iscsi_target_nego.c
index 685d771b51d4..4d0658731382 100644
--- a/drivers/target/iscsi/iscsi_target_nego.c
+++ b/drivers/target/iscsi/iscsi_target_nego.c
@@ -328,6 +328,28 @@ static int iscsi_target_do_tx_login_io(struct
iscsi_conn *conn, struct iscsi_log
u32 padding = 0;
struct iscsi_login_rsp *login_rsp;
+ /*
+ * 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
+ * process a login pdu, so that sk_state_chage could do login
+ * cleanup as needed if the socket is closed. If a delayed 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);
+ if (!test_bit(LOGIN_FLAGS_INITIAL_PDU,
&conn->login_flags)) {
+ 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);
+ }
What do you think?
Thanks,
Hou
