diff mbox series

[v2,09/13] scsi: target: iscsit: Fix isert disconnect handling during login

Message ID 20230112030832.110143-10-michael.christie@oracle.com (mailing list archive)
State Changes Requested
Headers show
Series target task management fixes | expand

Commit Message

Mike Christie Jan. 12, 2023, 3:08 a.m. UTC
If we get a disconnect event while logging in we can end up in a state
where will never be able to relogin. This happens when:

1. login thread has put us into TARG_CONN_STATE_IN_LOGIN
2. isert then does

isert_disconnected_handler -> iscsit_cause_connection_reinstatement

which sets the conn connection_reinstatement flag. Nothing else happens
because we are only in IN_LOGIN. The tx/rx threads are not running yet
so we can't start recovery from those contexts at this time.

3. The login thread finishes processing the login pdu and thinks login is
done. It sets us into TARG_CONN_STATE_LOGGED_IN/TARG_SESS_STATE_LOGGED_IN.
This starts the rx/tx threads.

4. The initiator thought it disconnected the connection at 2, and has
since sent a new connect which is now handled. This leads us to eventually
run:

iscsi_check_for_session_reinstatement-> iscsit_stop_session ->
iscsit_cause_connection_reinstatement

iscsit_stop_session sees the old conn and does
iscsit_cause_connection_reinstatement which sees connection_reinstatement
is set so it just returns instead of trying to kill the tx/rx threads
which would have caused recovery to start.

5. iscsit_stop_session then waits on session_wait_comp which will never
happen since we didn't kill the tx/rx threads.

This has the iscsit login code check if a fabric driver ran
iscsit_cause_connection_reinstatement during the login process similar
to how we check for the sk state for tcp based iscsit. This will prevent
us from getting into 3 and creating a ghost session.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/target/iscsi/iscsi_target_nego.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Dmitry Bogdanov Jan. 13, 2023, 2:08 p.m. UTC | #1
On Wed, Jan 11, 2023 at 09:08:28PM -0600, Mike Christie wrote:
> 
> If we get a disconnect event while logging in we can end up in a state
> where will never be able to relogin. This happens when:
> 
> 1. login thread has put us into TARG_CONN_STATE_IN_LOGIN
> 2. isert then does
> 
> isert_disconnected_handler -> iscsit_cause_connection_reinstatement
> 
> which sets the conn connection_reinstatement flag. Nothing else happens
> because we are only in IN_LOGIN. The tx/rx threads are not running yet
> so we can't start recovery from those contexts at this time.
> 
> 3. The login thread finishes processing the login pdu and thinks login is
> done. It sets us into TARG_CONN_STATE_LOGGED_IN/TARG_SESS_STATE_LOGGED_IN.
> This starts the rx/tx threads.
> 
> 4. The initiator thought it disconnected the connection at 2, and has
> since sent a new connect which is now handled. This leads us to eventually
> run:
> 
> iscsi_check_for_session_reinstatement-> iscsit_stop_session ->
> iscsit_cause_connection_reinstatement
> 
> iscsit_stop_session sees the old conn and does
> iscsit_cause_connection_reinstatement which sees connection_reinstatement
> is set so it just returns instead of trying to kill the tx/rx threads
> which would have caused recovery to start.
> 
> 5. iscsit_stop_session then waits on session_wait_comp which will never
> happen since we didn't kill the tx/rx threads.
> 
> This has the iscsit login code check if a fabric driver ran
> iscsit_cause_connection_reinstatement during the login process similar
> to how we check for the sk state for tcp based iscsit. This will prevent
> us from getting into 3 and creating a ghost session.
> 
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
>  drivers/target/iscsi/iscsi_target_nego.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/target/iscsi/iscsi_target_nego.c b/drivers/target/iscsi/iscsi_target_nego.c
> index ff49c8f3fe24..2dd81752d4c9 100644
> --- a/drivers/target/iscsi/iscsi_target_nego.c
> +++ b/drivers/target/iscsi/iscsi_target_nego.c
> @@ -350,6 +350,16 @@ static int iscsi_target_do_tx_login_io(struct iscsit_conn *conn, struct iscsi_lo
>                                             ISCSI_LOGIN_STATUS_NO_RESOURCES);
>                         return -1;
>                 }
> +
> +               /*
> +                * isert doesn't know the iscsit state and uses
> +                * iscsit_cause_connection_reinstatement as a generic error
> +                * notification system. It may call it before we are in FFP.
> +                * Handle this now in case it signaled a failure before the
> +                * rx/tx threads were up and could start recovery.
> +                */
> +               if (atomic_read(&conn->connection_reinstatement))
> +                       goto err;

Why only for login->login_complete case? In other case the session will
not hang? Will it be droppped on login timeout or something else?

May be the root cause is point 2 itself - calling iscsit_cause_connection_reinstatement
in not ISER_CONN_FULL_FEATURE state where there are no TX_RX threads?
I mean that was a misuse of iscsit_cause_connection_reinstatement?

>         }
> 
>         if (conn->conn_transport->iscsit_put_login_tx(conn, login,
> --
> 2.31.1
> 
>
Mike Christie Jan. 18, 2023, 2:14 a.m. UTC | #2
On 1/13/23 08:08, Dmitry Bogdanov wrote:
> On Wed, Jan 11, 2023 at 09:08:28PM -0600, Mike Christie wrote:
>>
>> If we get a disconnect event while logging in we can end up in a state
>> where will never be able to relogin. This happens when:
>>
>> 1. login thread has put us into TARG_CONN_STATE_IN_LOGIN
>> 2. isert then does
>>
>> isert_disconnected_handler -> iscsit_cause_connection_reinstatement
>>
>> which sets the conn connection_reinstatement flag. Nothing else happens
>> because we are only in IN_LOGIN. The tx/rx threads are not running yet
>> so we can't start recovery from those contexts at this time.
>>
>> 3. The login thread finishes processing the login pdu and thinks login is
>> done. It sets us into TARG_CONN_STATE_LOGGED_IN/TARG_SESS_STATE_LOGGED_IN.
>> This starts the rx/tx threads.
>>
>> 4. The initiator thought it disconnected the connection at 2, and has
>> since sent a new connect which is now handled. This leads us to eventually
>> run:
>>
>> iscsi_check_for_session_reinstatement-> iscsit_stop_session ->
>> iscsit_cause_connection_reinstatement
>>
>> iscsit_stop_session sees the old conn and does
>> iscsit_cause_connection_reinstatement which sees connection_reinstatement
>> is set so it just returns instead of trying to kill the tx/rx threads
>> which would have caused recovery to start.
>>
>> 5. iscsit_stop_session then waits on session_wait_comp which will never
>> happen since we didn't kill the tx/rx threads.
>>
>> This has the iscsit login code check if a fabric driver ran
>> iscsit_cause_connection_reinstatement during the login process similar
>> to how we check for the sk state for tcp based iscsit. This will prevent
>> us from getting into 3 and creating a ghost session.
>>
>> Signed-off-by: Mike Christie <michael.christie@oracle.com>
>> ---
>>  drivers/target/iscsi/iscsi_target_nego.c | 10 ++++++++++
>>  1 file changed, 10 insertions(+)
>>
>> diff --git a/drivers/target/iscsi/iscsi_target_nego.c b/drivers/target/iscsi/iscsi_target_nego.c
>> index ff49c8f3fe24..2dd81752d4c9 100644
>> --- a/drivers/target/iscsi/iscsi_target_nego.c
>> +++ b/drivers/target/iscsi/iscsi_target_nego.c
>> @@ -350,6 +350,16 @@ static int iscsi_target_do_tx_login_io(struct iscsit_conn *conn, struct iscsi_lo
>>                                             ISCSI_LOGIN_STATUS_NO_RESOURCES);
>>                         return -1;
>>                 }
>> +
>> +               /*
>> +                * isert doesn't know the iscsit state and uses
>> +                * iscsit_cause_connection_reinstatement as a generic error
>> +                * notification system. It may call it before we are in FFP.
>> +                * Handle this now in case it signaled a failure before the
>> +                * rx/tx threads were up and could start recovery.
>> +                */
>> +               if (atomic_read(&conn->connection_reinstatement))
>> +                       goto err;
> 
> Why only for login->login_complete case? In other case the session will
> not hang? Will it be droppped on login timeout or something else?

It will not hang. If you hit an error with isert before we think we can go into
FFP then the login timeout currently cleans up the conn and session.

> 
> May be the root cause is point 2 itself - calling iscsit_cause_connection_reinstatement
> in not ISER_CONN_FULL_FEATURE state where there are no TX_RX threads?
> I mean that was a misuse of iscsit_cause_connection_reinstatement?

Let me drop this patch for now. After writing the response above about normally it just
times out, and thinking about your question here I think to really fix this we want to
fully integrate isert login into iscsit.

The root problem is that isert login is not integrated into iscsit, so there is really
no error handling. iscsit_cause_connection_reinstatement was supposed to do it, but it
doesn't do anything.

So we need to separate the LOGIN_FLAGS_CLOSED tests and setting from the iscsi_target_sk*
code. Add a helper to set that bit and do some state checks, and make the checks generic
in the login code (not tied to having a socket). Convert iscsit and isert to use the new
helper. Then handle the LOGIN_FLAGS_READ_ACTIVE/LOGIN_FLAGS_WRITE_ACTIVE and login_work
stuff. Then review cxgb?

I'll do that later after fixing the command cleanup stuff in this patchsdet and the
other patchsets I have outstanding.
Sagi Grimberg Jan. 23, 2023, 9:53 a.m. UTC | #3
On 1/18/23 04:14, Mike Christie wrote:
> On 1/13/23 08:08, Dmitry Bogdanov wrote:
>> On Wed, Jan 11, 2023 at 09:08:28PM -0600, Mike Christie wrote:
>>>
>>> If we get a disconnect event while logging in we can end up in a state
>>> where will never be able to relogin. This happens when:
>>>
>>> 1. login thread has put us into TARG_CONN_STATE_IN_LOGIN
>>> 2. isert then does
>>>
>>> isert_disconnected_handler -> iscsit_cause_connection_reinstatement
>>>
>>> which sets the conn connection_reinstatement flag. Nothing else happens
>>> because we are only in IN_LOGIN. The tx/rx threads are not running yet
>>> so we can't start recovery from those contexts at this time.
>>>
>>> 3. The login thread finishes processing the login pdu and thinks login is
>>> done. It sets us into TARG_CONN_STATE_LOGGED_IN/TARG_SESS_STATE_LOGGED_IN.
>>> This starts the rx/tx threads.
>>>
>>> 4. The initiator thought it disconnected the connection at 2, and has
>>> since sent a new connect which is now handled. This leads us to eventually
>>> run:
>>>
>>> iscsi_check_for_session_reinstatement-> iscsit_stop_session ->
>>> iscsit_cause_connection_reinstatement
>>>
>>> iscsit_stop_session sees the old conn and does
>>> iscsit_cause_connection_reinstatement which sees connection_reinstatement
>>> is set so it just returns instead of trying to kill the tx/rx threads
>>> which would have caused recovery to start.
>>>
>>> 5. iscsit_stop_session then waits on session_wait_comp which will never
>>> happen since we didn't kill the tx/rx threads.
>>>
>>> This has the iscsit login code check if a fabric driver ran
>>> iscsit_cause_connection_reinstatement during the login process similar
>>> to how we check for the sk state for tcp based iscsit. This will prevent
>>> us from getting into 3 and creating a ghost session.
>>>
>>> Signed-off-by: Mike Christie <michael.christie@oracle.com>
>>> ---
>>>   drivers/target/iscsi/iscsi_target_nego.c | 10 ++++++++++
>>>   1 file changed, 10 insertions(+)
>>>
>>> diff --git a/drivers/target/iscsi/iscsi_target_nego.c b/drivers/target/iscsi/iscsi_target_nego.c
>>> index ff49c8f3fe24..2dd81752d4c9 100644
>>> --- a/drivers/target/iscsi/iscsi_target_nego.c
>>> +++ b/drivers/target/iscsi/iscsi_target_nego.c
>>> @@ -350,6 +350,16 @@ static int iscsi_target_do_tx_login_io(struct iscsit_conn *conn, struct iscsi_lo
>>>                                              ISCSI_LOGIN_STATUS_NO_RESOURCES);
>>>                          return -1;
>>>                  }
>>> +
>>> +               /*
>>> +                * isert doesn't know the iscsit state and uses
>>> +                * iscsit_cause_connection_reinstatement as a generic error
>>> +                * notification system. It may call it before we are in FFP.
>>> +                * Handle this now in case it signaled a failure before the
>>> +                * rx/tx threads were up and could start recovery.
>>> +                */
>>> +               if (atomic_read(&conn->connection_reinstatement))
>>> +                       goto err;
>>
>> Why only for login->login_complete case? In other case the session will
>> not hang? Will it be droppped on login timeout or something else?
> 
> It will not hang. If you hit an error with isert before we think we can go into
> FFP then the login timeout currently cleans up the conn and session.
> 
>>
>> May be the root cause is point 2 itself - calling iscsit_cause_connection_reinstatement
>> in not ISER_CONN_FULL_FEATURE state where there are no TX_RX threads?
>> I mean that was a misuse of iscsit_cause_connection_reinstatement?
> 
> Let me drop this patch for now. After writing the response above about normally it just
> times out, and thinking about your question here I think to really fix this we want to
> fully integrate isert login into iscsit.
> 
> The root problem is that isert login is not integrated into iscsit, so there is really
> no error handling. iscsit_cause_connection_reinstatement was supposed to do it, but it
> doesn't do anything.
> 
> So we need to separate the LOGIN_FLAGS_CLOSED tests and setting from the iscsi_target_sk*
> code. Add a helper to set that bit and do some state checks, and make the checks generic
> in the login code (not tied to having a socket). Convert iscsit and isert to use the new
> helper. Then handle the LOGIN_FLAGS_READ_ACTIVE/LOGIN_FLAGS_WRITE_ACTIVE and login_work
> stuff. Then review cxgb?
> 
> I'll do that later after fixing the command cleanup stuff in this patchsdet and the
> other patchsets I have outstanding.

Perhaps the fix is as simple as checking the iscsi state to be also 
full-featured and if not, do a simple error handling within
iser_disconnected_handler?
Mike Christie Jan. 24, 2023, 2:40 a.m. UTC | #4
On 1/23/23 3:53 AM, Sagi Grimberg wrote:
> 
> 
> On 1/18/23 04:14, Mike Christie wrote:
>> On 1/13/23 08:08, Dmitry Bogdanov wrote:
>>> On Wed, Jan 11, 2023 at 09:08:28PM -0600, Mike Christie wrote:
>>>>
>>>> If we get a disconnect event while logging in we can end up in a state
>>>> where will never be able to relogin. This happens when:
>>>>
>>>> 1. login thread has put us into TARG_CONN_STATE_IN_LOGIN
>>>> 2. isert then does
>>>>
>>>> isert_disconnected_handler -> iscsit_cause_connection_reinstatement
>>>>
>>>> which sets the conn connection_reinstatement flag. Nothing else happens
>>>> because we are only in IN_LOGIN. The tx/rx threads are not running yet
>>>> so we can't start recovery from those contexts at this time.
>>>>
>>>> 3. The login thread finishes processing the login pdu and thinks login is
>>>> done. It sets us into TARG_CONN_STATE_LOGGED_IN/TARG_SESS_STATE_LOGGED_IN.
>>>> This starts the rx/tx threads.
>>>>
>>>> 4. The initiator thought it disconnected the connection at 2, and has
>>>> since sent a new connect which is now handled. This leads us to eventually
>>>> run:
>>>>
>>>> iscsi_check_for_session_reinstatement-> iscsit_stop_session ->
>>>> iscsit_cause_connection_reinstatement
>>>>
>>>> iscsit_stop_session sees the old conn and does
>>>> iscsit_cause_connection_reinstatement which sees connection_reinstatement
>>>> is set so it just returns instead of trying to kill the tx/rx threads
>>>> which would have caused recovery to start.
>>>>
>>>> 5. iscsit_stop_session then waits on session_wait_comp which will never
>>>> happen since we didn't kill the tx/rx threads.
>>>>
>>>> This has the iscsit login code check if a fabric driver ran
>>>> iscsit_cause_connection_reinstatement during the login process similar
>>>> to how we check for the sk state for tcp based iscsit. This will prevent
>>>> us from getting into 3 and creating a ghost session.
>>>>
>>>> Signed-off-by: Mike Christie <michael.christie@oracle.com>
>>>> ---
>>>>   drivers/target/iscsi/iscsi_target_nego.c | 10 ++++++++++
>>>>   1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/drivers/target/iscsi/iscsi_target_nego.c b/drivers/target/iscsi/iscsi_target_nego.c
>>>> index ff49c8f3fe24..2dd81752d4c9 100644
>>>> --- a/drivers/target/iscsi/iscsi_target_nego.c
>>>> +++ b/drivers/target/iscsi/iscsi_target_nego.c
>>>> @@ -350,6 +350,16 @@ static int iscsi_target_do_tx_login_io(struct iscsit_conn *conn, struct iscsi_lo
>>>>                                              ISCSI_LOGIN_STATUS_NO_RESOURCES);
>>>>                          return -1;
>>>>                  }
>>>> +
>>>> +               /*
>>>> +                * isert doesn't know the iscsit state and uses
>>>> +                * iscsit_cause_connection_reinstatement as a generic error
>>>> +                * notification system. It may call it before we are in FFP.
>>>> +                * Handle this now in case it signaled a failure before the
>>>> +                * rx/tx threads were up and could start recovery.
>>>> +                */
>>>> +               if (atomic_read(&conn->connection_reinstatement))
>>>> +                       goto err;
>>>
>>> Why only for login->login_complete case? In other case the session will
>>> not hang? Will it be droppped on login timeout or something else?
>>
>> It will not hang. If you hit an error with isert before we think we can go into
>> FFP then the login timeout currently cleans up the conn and session.
>>
>>>
>>> May be the root cause is point 2 itself - calling iscsit_cause_connection_reinstatement
>>> in not ISER_CONN_FULL_FEATURE state where there are no TX_RX threads?
>>> I mean that was a misuse of iscsit_cause_connection_reinstatement?
>>
>> Let me drop this patch for now. After writing the response above about normally it just
>> times out, and thinking about your question here I think to really fix this we want to
>> fully integrate isert login into iscsit.
>>
>> The root problem is that isert login is not integrated into iscsit, so there is really
>> no error handling. iscsit_cause_connection_reinstatement was supposed to do it, but it
>> doesn't do anything.
>>
>> So we need to separate the LOGIN_FLAGS_CLOSED tests and setting from the iscsi_target_sk*
>> code. Add a helper to set that bit and do some state checks, and make the checks generic
>> in the login code (not tied to having a socket). Convert iscsit and isert to use the new
>> helper. Then handle the LOGIN_FLAGS_READ_ACTIVE/LOGIN_FLAGS_WRITE_ACTIVE and login_work
>> stuff. Then review cxgb?
>>
>> I'll do that later after fixing the command cleanup stuff in this patchsdet and the
>> other patchsets I have outstanding.
> 
> Perhaps the fix is as simple as checking the iscsi state to be also full-featured and if not, do a simple error handling within
> iser_disconnected_handler?

Yeah that's what I'm thinking above. The catch was what is the simple error handler going to be.

For iscsit tcp it mostly just sets LOGIN_FLAGS_CLOSED to signal the failure. So I was saying
above if we could just expose that to isert, then all it has to do is call some helper iscsit
helper:

isert_disconnected_handler
...
        case ISER_CONN_BOUND:
		iscsit_login_failed();
		break;
	case ISER_CONN_FULL_FEATURE
		iscsit_cause_connection_reinstatement(isert_conn->conn, 0);

*I think isert_login_send_done would also need a change.

The iscsit code then would work similarly for both isert and iscsit tcp. There's then just
cxgb which I think just times out like isert.

The other option is for the simple case is just let it timeout like cxgb. And for isert
just do nothing:

        case ISER_CONN_BOUND:
		isert_err("Failure on unbound connection. Timing out login\n");
		break;
	case ISER_CONN_FULL_FEATURE
		iscsit_cause_connection_reinstatement(isert_conn->conn, 0);
		break;
diff mbox series

Patch

diff --git a/drivers/target/iscsi/iscsi_target_nego.c b/drivers/target/iscsi/iscsi_target_nego.c
index ff49c8f3fe24..2dd81752d4c9 100644
--- a/drivers/target/iscsi/iscsi_target_nego.c
+++ b/drivers/target/iscsi/iscsi_target_nego.c
@@ -350,6 +350,16 @@  static int iscsi_target_do_tx_login_io(struct iscsit_conn *conn, struct iscsi_lo
 					    ISCSI_LOGIN_STATUS_NO_RESOURCES);
 			return -1;
 		}
+
+		/*
+		 * isert doesn't know the iscsit state and uses
+		 * iscsit_cause_connection_reinstatement as a generic error
+		 * notification system. It may call it before we are in FFP.
+		 * Handle this now in case it signaled a failure before the
+		 * rx/tx threads were up and could start recovery.
+		 */
+		if (atomic_read(&conn->connection_reinstatement))
+			goto err;
 	}
 
 	if (conn->conn_transport->iscsit_put_login_tx(conn, login,