Fix keep-alive mechanism for fabrics

Keep-alive mechanism is an admin queue property and should be activated/deactivated during admin queue creation/destruction.

Hi all,
I've been debugging the KA mechanism lately and found a lack of coordination between the target and host implementations.

Johannes,
Sorry for reverting your commit - I'll use nvme_start_keep_alive for my fix.

I've noticed that there is no clear definition in the NVMe spec regarding the keep-alive mechanism association. IMO, it should be a property of the admin queue and should be triggered as soon as the admin queue configured successfuly.

Idan/Christoph/Sagi,
Any thoughts on that proposal ?

This patchset was tested using RDMA transport only:
I've created 20 subsystems, 5 namespaces per subsystem and exposed all through 8 portals (total 160 ctrl's created) on 1 target. I used 1 initiator (host) and connected successfuly.
Later on I've destroyed the target and caused a reconnection flow in the initiator side.
Ater ~30-50 seconds, I've configured the target again but the initiator couldn't reconnect to it (after many retries).
The reason for this was that the keep-alive timer expired at the target side, caused ctrl fatal error and the io-queue connect failed to find the ctrl. This loop never converged.

With the patches below, the test passed successfully after 1/2 reconnection attempts.

Quoted 9 lines

Hi all,
I've been debugging the KA mechanism lately and found a lack of coordination between the target and host implementations.

Johannes,
Sorry for reverting your commit - I'll use nvme_start_keep_alive for my fix.

Quoted 7 lines

I've noticed that there is no clear definition in the NVMe spec regarding the keep-alive mechanism association. IMO, it should be a property of the admin queue and should be triggered as soon as the admin queue configured successfuly.

Idan/Christoph/Sagi,
Any thoughts on that proposal ?

I don't understand what you mean by a "property of the admin queue", there is no such definition in the spec. In any event, the keep alive mechanism was defined to detect host/controller health on a periodic basis.

The spec clearly states that the host needs to send keep alive at a faster rate than the Keep alive timeout accounting things such as transport roundtrip times, transport delays, keep alive timer granularity and so on.

To me, this is a clear case where your use-case requires a larger keep alive timeout. I'm not sure why sprinkling the keep alive execution around solve/help anything. To me this looks completely redundant (sorry).

We could theoretically make NVME_KATO_GRACE configurable, but I seriously doubt its useful at all...

As a side note, Note that this should be solved by "Traffic Based Keep Alive" TP which is approaching ratification. This is yet another example showing that Having keep alive timer firing regardless of command execution is hurtful.

Quoted 13 lines

This patchset was tested using RDMA transport only:
I've created 20 subsystems, 5 namespaces per subsystem and exposed all through 8 portals (total 160 ctrl's created) on 1 target. I used 1 initiator (host) and connected successfuly.
Later on I've destroyed the target and caused a reconnection flow in the initiator side.
Ater ~30-50 seconds, I've configured the target again but the initiator couldn't reconnect to it (after many retries).
The reason for this was that the keep-alive timer expired at the target side, caused ctrl fatal error and the io-queue connect failed to find the ctrl. This loop never converged.

Quoted 2 lines

With the patches below, the test passed successfully after 1/2 reconnection attempts.

Hence my comment above. As I said, I think you should either increase the keep alive timeout for this use-case, or wait for TBKAS to ratify.

Quoted 11 lines

I don't understand what you mean by a "property of the admin queue", there is no such definition in the spec. In any event, the keep alive mechanism was defined to detect host/controller health on a periodic basis.

The spec clearly states that the host needs to send keep alive at a faster rate than the Keep alive timeout accounting things such as transport roundtrip times, transport delays, keep alive timer granularity and so on.

Quoted 4 lines

To me, this is a clear case where your use-case requires a larger keep alive timeout. I'm not sure why sprinkling the keep alive execution around solve/help anything. To me this looks completely redundant (sorry).

I think that starting keep-alive timer at the target side after admin connect and starting keep-alive timer at the host side after all io-queues connect is wrong.
In my solution I start keep-alive mechanism in the host side also after admin connect (exactly as the target side).

In this way I'll make sure the "health" is ok during the io-queues connection establishment (establisment of 4000-5000 IO queues can take long time and we hope sending a KA packet will not take more than 15 seconds no matter what :) ).

Why to have inconsistant implementation ?
Increasing the timeout is not a solution (maybe can WA some scenarios). Also TBKA comes to solve different problem.

Quoted 4 lines

As a side note, Note that this should be solved by "Traffic Based Keep Alive" TP which is approaching ratification. This is yet another example showing that Having keep alive timer firing regardless of command execution is hurtful.

TBKA will work as nop-in/out in iSCSI AKAIK. But again, it will no solve the issue I described above.

Quoted 16 lines

Quoted 11 lines

I don't understand what you mean by a "property of the admin queue", there is no such definition in the spec. In any event, the keep alive mechanism was defined to detect host/controller health on a periodic basis.

The spec clearly states that the host needs to send keep alive at a faster rate than the Keep alive timeout accounting things such as transport roundtrip times, transport delays, keep alive timer granularity and so on.

Quoted 4 lines

To me, this is a clear case where your use-case requires a larger keep alive timeout. I'm not sure why sprinkling the keep alive execution around solve/help anything. To me this looks completely redundant (sorry).

Quoted 5 lines

I think that starting keep-alive timer at the target side after admin connect and starting keep-alive timer at the host side after all io-queues connect is wrong.
In my solution I start keep-alive mechanism in the host side also after admin connect (exactly as the target side).

That does not guarantee anything either. as you were simply able to minimize the effect.

An alternative patch would also be able to minimize the effect:

drivers/nvme/host/core.c +1 -1

index e1b708ee6783..a84198b1e0d8 100644
@@ -847,7 +847,7 @@ static void nvme_start_keep_alive(struct nvme_ctrl 
 *ctrl)
 ??????? INIT_DELAYED_WORK(&ctrl->ka_work, nvme_keep_alive_work);
 ??????? memset(&ctrl->ka_cmd, 0, sizeof(ctrl->ka_cmd));
 ??????? ctrl->ka_cmd.common.opcode = nvme_admin_keep_alive;
-?????? schedule_delayed_work(&ctrl->ka_work, ctrl->kato * HZ);
+?????? schedule_delayed_work(&ctrl->ka_work, 0);
 ?}
 

?void nvme_stop_keep_alive(struct nvme_ctrl *ctrl)

Signature · 2 lines

This way the first keep alive will execute immediately the first time
and not kato after...

Quoted 5 lines

In this way I'll make sure the "health" is ok during the io-queues connection establishment (establisment of 4000-5000 IO queues can take long time and we hope sending a KA packet will not take more than 15 seconds no matter what :) ).

No you won't. nvme_start_keep_alive will execute a keep alive kato after it was started. and why do you need to detect the health during connection establishment? queue creation will fail if health is lost.

Quoted 3 lines

Why to have inconsistant implementation ? Increasing the timeout is not a solution (maybe can WA some scenarios). Also TBKA comes to solve different problem.

No, it came to solve a false positive of heart-bit lost as a result of keep alive starvation, which is exactly what this scenario is.

Quoted 5 lines

Quoted 4 lines

As a side note, Note that this should be solved by "Traffic Based Keep Alive" TP which is approaching ratification. This is yet another example showing that Having keep alive timer firing regardless of command execution is hurtful.

Quoted 2 lines

TBKA will work as nop-in/out in iSCSI AKAIK. But again, it will no solve the issue I described above.

Yes it will, fabrics connect is a traffic indication and will reset the keep alive timer.

Quoted 17 lines · Sagi Grimberg

Quoted 17 lines

Quoted 16 lines

I don't understand what you mean by a "property of the admin queue", there is no such definition in the spec. In any event, the keep alive mechanism was defined to detect host/controller health on a periodic basis.

The spec clearly states that the host needs to send keep alive at a faster rate than the Keep alive timeout accounting things such as transport roundtrip times, transport delays, keep alive timer granularity and so on.

To me, this is a clear case where your use-case requires a larger keep alive timeout. I'm not sure why sprinkling the keep alive execution
around solve/help anything. To me this looks completely redundant (sorry).

Quoted 5 lines

Quoted 5 lines

I think that starting keep-alive timer at the target side after admin connect and starting keep-alive timer at the host side after all io-queues connect is wrong.
In my solution I start keep-alive mechanism in the host side also after admin connect (exactly as the target side).

Quoted 19 lines

That does not guarantee anything either. as you were simply able to minimize the effect.

An alternative patch would also be able to minimize the effect:

drivers/nvme/host/core.c +1 -1

index e1b708ee6783..a84198b1e0d8 100644
@@ -847,7 +847,7 @@ static void nvme_start_keep_alive(struct 
 nvme_ctrl *ctrl)
 ???????? INIT_DELAYED_WORK(&ctrl->ka_work, nvme_keep_alive_work);
 ???????? memset(&ctrl->ka_cmd, 0, sizeof(ctrl->ka_cmd));
 ???????? ctrl->ka_cmd.common.opcode = nvme_admin_keep_alive;
-?????? schedule_delayed_work(&ctrl->ka_work, ctrl->kato * HZ);
+?????? schedule_delayed_work(&ctrl->ka_work, 0);
 ??}
 

Quoted 5 lines

??void nvme_stop_keep_alive(struct nvme_ctrl *ctrl)

Signature · 2 lines

This way the first keep alive will execute immediately the first time
and not kato after...

This is a good patch, and I can add it to the series.

Quoted 20 lines · Max Gurtovoy

Quoted 17 lines · Sagi Grimberg

Quoted 17 lines

Quoted 16 lines

I don't understand what you mean by a "property of the admin queue", there is no such definition in the spec. In any event, the keep alive mechanism was defined to detect host/controller health on a periodic basis.

The spec clearly states that the host needs to send keep alive at a faster rate than the Keep alive timeout accounting things such as transport roundtrip times, transport delays, keep alive timer granularity and so on.

To me, this is a clear case where your use-case requires a larger keep alive timeout. I'm not sure why sprinkling the keep alive execution
around solve/help anything. To me this looks completely redundant (sorry).

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

I think that starting keep-alive timer at the target side after admin connect and starting keep-alive timer at the host side after all io-queues connect is wrong.
In my solution I start keep-alive mechanism in the host side also after admin connect (exactly as the target side).

well - true, but I believe it should be started after the controller is enabled - not just that the admin queue has been created.? I don't think even the admin queue can be serviced till the adapter is enabled. Thus, I would agree with moving nvme_start_keep_alive() from nvme_start_ctrl() to nvme_enable_ctrl().?? We would also want the "enablement" to detect the keepalive style (today's keepalive or TBKA) supported by the time that call is made.?? Given that needs to be known, I believe there will always be a small window where a couple of things have to be accessed before the KA starts.

I strongly support Sagi's statements on trying to keep the KA start/stop in common code rather than sprinkling it around.?? I agree with the current placement of the stop code in nvme_stop_ctrl().?? The stop ctrl should always be called prior to the admin queue being torn down in the transport (as part of kicking off the reset of the controller).

Note: the FC patch is way off..? Also note that I've killed the xxx_is_ready() routines in the updated if_ready patch I sent. So your mods wouldn't apply (soon).

Quoted 23 lines

Quoted 23 lines

That does not guarantee anything either. as you were simply able to minimize the effect.

An alternative patch would also be able to minimize the effect:

drivers/nvme/host/core.c +1 -1

index e1b708ee6783..a84198b1e0d8 100644
@@ -847,7 +847,7 @@ static void nvme_start_keep_alive(struct 
 nvme_ctrl *ctrl)
 ???????? INIT_DELAYED_WORK(&ctrl->ka_work, nvme_keep_alive_work);
 ???????? memset(&ctrl->ka_cmd, 0, sizeof(ctrl->ka_cmd));
 ???????? ctrl->ka_cmd.common.opcode = nvme_admin_keep_alive;
-?????? schedule_delayed_work(&ctrl->ka_work, ctrl->kato * HZ);
+?????? schedule_delayed_work(&ctrl->ka_work, 0);
 ??}
 

??void nvme_stop_keep_alive(struct nvme_ctrl *ctrl)

Quoted 4 lines

Quoted 2 lines

This way the first keep alive will execute immediately the first time and not kato after...

This is a good patch, and I can add it to the series.

Uh... I wouldn't want to do this.? You can easily burn lots of cycles sending a KA.

I would lean toward making people get used to setting KATO to a large enough value that it can be used equally well with TBKA - thus it should be something like this instead:
?????? schedule_delayed_work(&ctrl->ka_work, (ctrl->kato * HZ) / 2);

This is somewhat independent from the grace value - as grace is a scaling factor to accommodate congestion and delayed processing.

-- james

Quoted 27 lines · James Smart

Quoted 20 lines · Max Gurtovoy

Quoted 17 lines · Sagi Grimberg

Quoted 17 lines

Quoted 16 lines

I don't understand what you mean by a "property of the admin queue", there is no such definition in the spec. In any event, the keep alive mechanism was defined to detect host/controller health on a periodic basis.

The spec clearly states that the host needs to send keep alive at a faster rate than the Keep alive timeout accounting things such as transport roundtrip times, transport delays, keep alive timer granularity and so on.

To me, this is a clear case where your use-case requires a larger keep alive timeout. I'm not sure why sprinkling the keep alive execution
around solve/help anything. To me this looks completely redundant (sorry).

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

I think that starting keep-alive timer at the target side after admin connect and starting keep-alive timer at the host side after all io-queues connect is wrong.
In my solution I start keep-alive mechanism in the host side also after admin connect (exactly as the target side).

Quoted 9 lines

well - true, but I believe it should be started after the controller is enabled - not just that the admin queue has been created.? I don't think even the admin queue can be serviced till the adapter is enabled. Thus, I would agree with moving nvme_start_keep_alive() from nvme_start_ctrl() to nvme_enable_ctrl().?? We would also want the "enablement" to detect the keepalive style (today's keepalive or TBKA) supported by the time that call is made.?? Given that needs to be known, I believe there will always be a small window where a couple of things have to be accessed before the KA starts.

At least I see that you agree to start keep-alive timer before creating the IO queues so that's a progress :).
The fact that I didn't put it in nvme_enable_ctrl is because the FC code never calls nvme_disable_ctrl (and there we probably should stop the timer).
I'm trying to solve an issue without changing the whole design of the FC implementation. We can do it incrementaly.

Quoted 7 lines

I strongly support Sagi's statements on trying to keep the KA start/stop in common code rather than sprinkling it around.?? I agree with the current placement of the stop code in nvme_stop_ctrl().?? The stop ctrl should always be called prior to the admin queue being torn down in the transport (as part of kicking off the reset of the controller).

If we'll add KA start to nvme_enable_ctrl then the KA stop should be added to nvme_disable_ctrl. We should be symmetric.

Quoted 8 lines

At least I see that you agree to start keep-alive timer before creating the IO queues so that's a progress :).
The fact that I didn't put it in nvme_enable_ctrl is because the FC code never calls nvme_disable_ctrl (and there we probably should stop the timer).
I'm trying to solve an issue without changing the whole design of the FC implementation. We can do it incrementaly.

Please don't touch any of the transport drivers for any of this, its the wrong place to handle keep alive.

Quoted 9 lines

Quoted 8 lines

I strongly support Sagi's statements on trying to keep the KA start/stop in common code rather than sprinkling it around. I agree with the current placement of the stop code in nvme_stop_ctrl().?? The stop ctrl should always be called prior to the admin queue being torn down in the transport (as part of kicking off the reset of the controller).

Quoted 2 lines

If we'll add KA start to nvme_enable_ctrl then the KA stop should be added to nvme_disable_ctrl. We should be symmetric.

Not a must. even after the controller is disabled we want to have a keep alive to it as we would need to enable it again at some point.

Quoted 20 lines · Max Gurtovoy

Quoted 17 lines · Sagi Grimberg

Quoted 17 lines

Quoted 16 lines

I don't understand what you mean by a "property of the admin queue", there is no such definition in the spec. In any event, the keep alive mechanism was defined to detect host/controller health on a periodic basis.

The spec clearly states that the host needs to send keep alive at a faster rate than the Keep alive timeout accounting things such as transport roundtrip times, transport delays, keep alive timer granularity and so on.

To me, this is a clear case where your use-case requires a larger keep alive timeout. I'm not sure why sprinkling the keep alive execution
around solve/help anything. To me this looks completely redundant (sorry).

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

I think that starting keep-alive timer at the target side after admin connect and starting keep-alive timer at the host side after all io-queues connect is wrong.
In my solution I start keep-alive mechanism in the host side also after admin connect (exactly as the target side).

well - true, but I believe it should be started after the controller is enabled - not just that the admin queue has been created.? I don't think even the admin queue can be serviced till the adapter is enabled. Thus, I would agree with moving nvme_start_keep_alive() from nvme_start_ctrl() to nvme_enable_ctrl().

? We would also want the "enablement" to detect
the keepalive style (today's keepalive or TBKA) supported by the time that call is made.?? Given that needs to be known, I believe there will always be a small window where a couple of things have to be accessed before the KA starts.

I strongly support Sagi's statements on trying to keep the KA start/stop in common code rather than sprinkling it around.?? I agree with the current placement of the stop code in nvme_stop_ctrl().?? The stop ctrl should always be called prior to the admin queue being torn down in the transport (as part of kicking off the reset of the controller).

Quoted 21 lines

Quoted 21 lines

That does not guarantee anything either. as you were simply able to minimize the effect.

An alternative patch would also be able to minimize the effect:

drivers/nvme/host/core.c +1 -1

index e1b708ee6783..a84198b1e0d8 100644
@@ -847,7 +847,7 @@ static void nvme_start_keep_alive(struct 
 nvme_ctrl *ctrl)
 ???????? INIT_DELAYED_WORK(&ctrl->ka_work, nvme_keep_alive_work);
 ???????? memset(&ctrl->ka_cmd, 0, sizeof(ctrl->ka_cmd));
 ???????? ctrl->ka_cmd.common.opcode = nvme_admin_keep_alive;
-?????? schedule_delayed_work(&ctrl->ka_work, ctrl->kato * HZ);
+?????? schedule_delayed_work(&ctrl->ka_work, 0);
 ??}
 

??void nvme_stop_keep_alive(struct nvme_ctrl *ctrl)

Quoted 4 lines

Quoted 2 lines

This way the first keep alive will execute immediately the first time and not kato after...

This is a good patch, and I can add it to the series.

Uh... I wouldn't want to do this.? You can easily burn lots of cycles sending a KA.

I would lean toward making people get used to setting KATO to a large enough value that it can be used equally well with TBKA - thus it should be something like this instead:
?????? schedule_delayed_work(&ctrl->ka_work, (ctrl->kato * HZ) / 2);

This is somewhat independent from the grace value - as grace is a scaling factor to accommodate congestion and delayed processing.

Quoted 27 lines · James Smart

Quoted 20 lines · Max Gurtovoy

Quoted 17 lines · Sagi Grimberg

Quoted 17 lines

Quoted 16 lines

I don't understand what you mean by a "property of the admin queue", there is no such definition in the spec. In any event, the keep alive mechanism was defined to detect host/controller health on a periodic basis.

The spec clearly states that the host needs to send keep alive at a faster rate than the Keep alive timeout accounting things such as transport roundtrip times, transport delays, keep alive timer granularity and so on.

To me, this is a clear case where your use-case requires a larger keep alive timeout. I'm not sure why sprinkling the keep alive execution
around solve/help anything. To me this looks completely redundant (sorry).

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

I think that starting keep-alive timer at the target side after admin connect and starting keep-alive timer at the host side after all io-queues connect is wrong.
In my solution I start keep-alive mechanism in the host side also after admin connect (exactly as the target side).

Quoted 5 lines

well - true, but I believe it should be started after the controller is enabled - not just that the admin queue has been created.? I don't think even the admin queue can be serviced till the adapter is enabled. Thus, I would agree with moving nvme_start_keep_alive() from nvme_start_ctrl() to nvme_enable_ctrl().

I'm fine with that.

Quoted 4 lines

? We would also want the "enablement" to detect the keepalive style (today's keepalive or TBKA) supported by the time that call is made. Given that needs to be known, I believe there will always be a small window where a couple of things have to be accessed before the KA starts.

I honestly don't understand why should this even be an issue. If someone is running a heavy load where keep alive timeout is not sufficient, then it should use a larger keep alive.

Quoted 6 lines

I strongly support Sagi's statements on trying to keep the KA start/stop in common code rather than sprinkling it around.?? I agree with the current placement of the stop code in nvme_stop_ctrl().?? The stop ctrl should always be called prior to the admin queue being torn down in the transport (as part of kicking off the reset of the controller).

Yea, we need to keep it where it is. the start can move to controller enable, and in fact, the target should also do that for the target and update the timer in nvmet_start_ctrl.

drivers/nvme/target/core.c +1 -0

index fe151d672241..a81bf4d5e60c 100644
@@ -643,6 +643,7 @@ static void nvmet_start_ctrl(struct nvmet_ctrl *ctrl)
 ??????? }
 
 ??????? ctrl->csts = NVME_CSTS_RDY;
+?????? mod_delayed_work(system_wq, &ctrl->ka_work, ctrl->kato * HZ);
 ?}
 
 ?static void nvmet_clear_ctrl(struct nvmet_ctrl *ctrl)

Signature · 6 lines

This would get the correct behavior and still be able to
detect host death before the controller was enabled.

This together with moving the keep alive timer start in nvme_enable_ctrl
should be enough and not propagating the handle to every transport
driver, we have enough of that already.

drivers/nvme/host/core.c +8 -1

index e1b708ee6783..4b5c3f7addeb 100644
@@ -1739,7 +1739,14 @@ int nvme_enable_ctrl(struct nvme_ctrl *ctrl, u64 
 cap)
 ??????? ret = ctrl->ops->reg_write32(ctrl, NVME_REG_CC, 
 ctrl->ctrl_config);
 ??????? if (ret)
 ??????????????? return ret;
-?????? return nvme_wait_ready(ctrl, cap, true);
+
+?????? ret = nvme_wait_ready(ctrl, cap, true);
+?????? if (ret)
+?????????????? return ret;
+
+?????? nvme_start_keep_alive(ctrl);
+
+?????? return 0;
 ?}

?EXPORT_SYMBOL_GPL(nvme_enable_ctrl);

drivers/nvme/host/core.c +0 -3

@@ -3393,9 +3400,6 @@ EXPORT_SYMBOL_GPL(nvme_stop_ctrl);
 
 ?void nvme_start_ctrl(struct nvme_ctrl *ctrl)
 ?{
-?????? if (ctrl->kato)
-?????????????? nvme_start_keep_alive(ctrl);
-
 ??????? if (ctrl->queue_count > 1) {
 ??????????????? nvme_queue_scan(ctrl);
 ??????????????? queue_work(nvme_wq, &ctrl->async_event_work);

Quoted 21 lines

Quoted 21 lines

Quoted 21 lines

That does not guarantee anything either. as you were simply able to minimize the effect.

An alternative patch would also be able to minimize the effect:

drivers/nvme/host/core.c +1 -1

index e1b708ee6783..a84198b1e0d8 100644
@@ -847,7 +847,7 @@ static void nvme_start_keep_alive(struct 
 nvme_ctrl *ctrl)
 ???????? INIT_DELAYED_WORK(&ctrl->ka_work, nvme_keep_alive_work);
 ???????? memset(&ctrl->ka_cmd, 0, sizeof(ctrl->ka_cmd));
 ???????? ctrl->ka_cmd.common.opcode = nvme_admin_keep_alive;
-?????? schedule_delayed_work(&ctrl->ka_work, ctrl->kato * HZ);
+?????? schedule_delayed_work(&ctrl->ka_work, 0);
 ??}
 

??void nvme_stop_keep_alive(struct nvme_ctrl *ctrl)

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

This way the first keep alive will execute immediately the first time and not kato after...

This is a good patch, and I can add it to the series.

Uh... I wouldn't want to do this.? You can easily burn lots of cycles sending a KA.

Its only for the first invocation. but I think its useless anyways...

Quoted 8 lines

I would lean toward making people get used to setting KATO to a large enough value that it can be used equally well with TBKA - thus it should be something like this instead:
??????? schedule_delayed_work(&ctrl->ka_work, (ctrl->kato * HZ) / 2);

This is somewhat independent from the grace value - as grace is a scaling factor to accommodate congestion and delayed processing.

This is fine too.

Quoted 4 lines

This together with moving the keep alive timer start in nvme_enable_ctrl should be enough and not propagating the handle to every transport driver, we have enough of that already.

Great, this is what I wanted to do in the first place but I was concerned about stopping the keep-alive in nvme_disable_ctrl. Are you sending a new series or I will ?

Quoted 8 lines

Quoted 4 lines

This together with moving the keep alive timer start in nvme_enable_ctrl should be enough and not propagating the handle to every transport driver, we have enough of that already.

Great, this is what I wanted to do in the first place but I was concerned about stopping the keep-alive in nvme_disable_ctrl. Are you sending a new series or I will ?

I can send it. Given that its your report and your intention in the first place, can I place you as the author of these?

Quoted 6 lines

Quoted 3 lines

TBKA will work as nop-in/out in iSCSI AKAIK. But again, it will no solve the issue I described above.

Yes it will, fabrics connect is a traffic indication and will reset the keep alive timer.

Very good. But it will reset a KA timer that is not even started (in the initiator) ?

Quoted 4 lines

Quoted 4 lines

Quoted 3 lines

TBKA will work as nop-in/out in iSCSI AKAIK. But again, it will no solve the issue I described above.

Quoted 5 lines

Quoted 2 lines

Yes it will, fabrics connect is a traffic indication and will reset the keep alive timer.

Very good. But it will reset a KA timer that is not even started (in the initiator) ?

No, host indication of traffic is completions, so the completion would reset it.

Quoted 10 lines

Quoted 9 lines

Quoted 6 lines

Quoted 3 lines

TBKA will work as nop-in/out in iSCSI AKAIK. But again, it will no solve the issue I described above.

Yes it will, fabrics connect is a traffic indication and will reset the keep alive timer.

Very good. But it will reset a KA timer that is not even started (in the initiator) ?

Quoted 2 lines

No, host indication of traffic is completions, so the completion would reset it.

I meant that in the current host implementation (that starts KA timer after IO queues connection establishment) there will be nothing to reset during io queues connection establishment and completions.

I've noticed that there is no clear definition in the NVMe spec regarding the keep-alive mechanism association. IMO, it should be a property of the admin queue and should be triggered as soon as the admin queue configured successfuly.