nvme: ANA transition timeout handling
PATCH v2 1/1 nvme: ANA transition timeout handling
Turn the ana_state array into an array of ana groups, and add a timer to each group for tracking ANA transition timeout. Once the timeout expires the controller will be reset.
--- drivers/nvme/host/core.c | 2 ++ drivers/nvme/host/multipath.c | 78 ++++++++++++++++++++++++++++++++++--------- drivers/nvme/host/nvme.h | 14 +++++++- 3 files changed, 77 insertions(+), 17 deletions(-)
drivers/nvme/host/core.c +2 -0
@@ -114,6 +114,7 @@ int nvme_reset_ctrl(struct nvme_ctrl *ctrl) {if (!nvme_change_ctrl_state (ctrl,NVME_CTRL_RESETTING ))return -EBUSY ; +nvme_stop_anatt (ctrl);if (!queue_work (nvme_reset_wq, &ctrl->reset_work))return -EBUSY ;return 0 ;
@@ -2378,6 +2379,7 @@ int nvme_init_identify(struct nvme_ctrl *ctrl) ctrl->kas =le16_to_cpu (id->kas); ctrl->max_namespaces =le32_to_cpu (id->mnan); ctrl->anacap = id->anacap; + ctrl->anatt = id->anatt; ctrl->nanagrpid =le32_to_cpu (id->nanagrpid); ctrl->anagrpmax =le32_to_cpu (id->anagrpmax);
drivers/nvme/host/multipath.c +62 -16
139 lines not on this page · raw message
drivers/nvme/host/nvme.h +13 -1
@@ -119,6 +119,13 @@ static inline struct nvme_request *nvme_req(struct request *req)return blk_mq_rq_to_pdu (req); } +struct nvme_ana_group { +u32 grpid; +struct nvme_ctrl *ctrl; +enum nvme_ana_state state; +struct timer_list anatt_timer; +}; +/* The below value is the specific amount of delay needed before checking * readiness in case of the PCI_DEVICE(0x1c58, 0x0003), which needs the * NVME_QUIRK_DELAY_BEFORE_CHK_RDY quirk enabled. The value (in ms) was
@@ -200,9 +207,10 @@ struct nvme_ctrl {/* asymmetric namespace access: */ u8 anacap; +u8 anatt;u32 anagrpmax;u32 nanagrpid; -enum nvme_ana_state *ana_state; +struct nvme_ana_group *ana_groups;size_t ana_log_size;struct nvme_ana_rsp_hdr *ana_log_buf;struct work_struct ana_work;
@@ -469,6 +477,7 @@ void nvme_mpath_add_disk(struct nvme_ns *ns);void nvme_mpath_remove_disk (struct nvme_ns_head *head);int nvme_configure_ana (struct nvme_ctrl *ctrl);void nvme_deconfigure_ana (struct nvme_ctrl *ctrl); +void nvme_stop_anatt (struct nvme_ctrl *ctrl);static inline void nvme_mpath_clear_current_path (struct nvme_ns *ns) {
@@ -531,6 +540,9 @@ static inline int nvme_configure_ana(struct nvme_ctrl *ctrl)static inline void nvme_deconfigure_ana (struct nvme_ctrl *ctrl) { } +static inline void nvme_stop_anatt (struct nvme_ctrl *ctrl) +{ +}#endif /* CONFIG_NVME_MULTIPATH */ #ifdef CONFIG_NVM
Signature · 1 line
2.12.3
Christoph Hellwig
maintainerPATCH v2 1/1 nvme: ANA transition timeout handling
drivers/nvme/host/multipath.c void nvme_failover_req(struct request *req) · line 76
Quoted 6 lines
drivers/nvme/host/multipath.c +6
-0+/* +* Use timer_reduce() to ensure we're not modifying +* an already running timer. +*/ +timer_reduce (&grp->anatt_timer, + ns->ctrl->anatt *HZ + jiffies);
That's a bit of a gross hack for a timer that never is actually reduced, just added.
drivers/nvme/host/multipath.c static int nvme_process_ana_log(struct nvme_ctrl *ctrl, bool groups_only)
Quoted 11 lines
drivers/nvme/host/multipath.c +9
-0+if (ctrl->state ==NVME_CTRL_LIVE && state ==NVME_ANA_CHANGE ) +timer_reduce (&grp->anatt_timer, + ctrl->anatt *HZ + jiffies); +else +del_timer (&grp->anatt_timer); +for (grpid =0 ; grpid < ctrl->anagrpmax; grpid++) { +struct nvme_ana_group *grp = &ctrl->ana_groups[grpid]; + +del_timer (&grp->anatt_timer);
Without del_timer_sync you are going to create use after free conditions.
Anyway, v4 of the ANA series I've just sent contains simple ANATT handling based on a per-controller timer, which should be sufficient to catch the intent behind ANATT. Comments welcome.
PATCH v2 1/1 nvme: ANA transition timeout handling
Quoted 10 lines · Christoph Hellwig
drivers/nvme/host/multipath.c void nvme_failover_req(struct request *req) · line 76
Quoted 6 lines
drivers/nvme/host/multipath.c +6
-0+/* +* Use timer_reduce() to ensure we're not modifying +* an already running timer. +*/ +timer_reduce (&grp->anatt_timer, + ns->ctrl->anatt *HZ + jiffies);That's a bit of a gross hack for a timer that never is actually reduced, just added.
Well, yes; but using timer_reduce allows me to use the timer code without having to take a lock; if I would've used add_timer() I always would have to have two statements (one for setting the timer, and on call to add_timer) which from my understanding would need to be protected by a lock.
Quoted 7 lines
Quoted 5 lines
drivers/nvme/host/multipath.c +5
-0+if (ctrl->state ==NVME_CTRL_LIVE && state ==NVME_ANA_CHANGE ) +timer_reduce (&grp->anatt_timer, + ctrl->anatt *HZ + jiffies); +else +del_timer (&grp->anatt_timer);
Quoted 8 lines
drivers/nvme/host/multipath.c static int nvme_process_ana_log(struct nvme_ctrl *ctrl, bool groups_only) · line 436
Quoted 4 lines
drivers/nvme/host/multipath.c +4
-0+for (grpid =0 ; grpid < ctrl->anagrpmax; grpid++) { +struct nvme_ana_group *grp = &ctrl->ana_groups[grpid]; + +del_timer (&grp->anatt_timer);Without
del_timer_syncyou are going to create use after free conditions.
Hmm. Possibly.
Will be looking into it if
Quoted 5 lines
Anyway, v4 of the ANA series I've just sent contains simple ANATT handling based on a per-controller timer, which should be sufficient to catch the intent behind ANATT. Comments welcome.
Actually, I looked into moving the timer handling into the controller, too.
But then the problem was that we'll catch only the most current transition; if we have two namespaces, both going into the 'change state' mode independently (as would be the case with our nvmet, if both namespaces are configured on different ANA groups), we would only able to register the most current transition.
If we have a scenario like
1. NS A: ANA change state
2. NS A: ANA AEN
-> start ANATT
3. NS B: ANA change state
4. NS B: ANA AEN
-> not starting ANATT as it's already running
5. NS A: final state
6. NS A: ANA AEN
-> kill ANATT
we will start ANATT for NS B (with the full transition timeout) only after 6., failing to reduce the timer by the time spent between 4. and 6.
If that's not an issue, okay, we can use that approach. I just didn't dare so, knowing as it has design flaws.
Cheers,
Hannes