nvme: ANA support
Subsystems (2)
v2 cover nvme: ANA support
Hi all,
here's now the second round of adding ANA support to the nvme subsystem. It's the combined patchset from hch and me, and I've included all reviews I've had so far.
There are actually two parts to it:
The first patches update AEN configuration support as ANA relies on AENs to be sent. So we should better make sure that we have them enabled.
The second part is then the ANA support proper.
As usual, comments and reviews are welcome.
Christoph Hellwig (8):
nvme.h: untangle AEN notice definitions
nvmet: refactor AER handling
nvme.h: add ANA definitions
nvme: add support for the log specific field
nvme: always failover on path or transport errors
nvme: add ANA support
nvmet: add a new nvmet_zero_sgl helper
nvmet: split log page implementation
Hannes Reinecke (3):
nvme: submit AEN event configuration on startup
nvmet: Add AEN configuration support
nvmet: ANA support
drivers/nvme/host/core.c | 390 ++++++++++++++++++++++++++++++++++++---- drivers/nvme/host/lightnvm.c | 2 +- drivers/nvme/host/multipath.c | 43 ++++- drivers/nvme/host/nvme.h | 20 ++- drivers/nvme/target/admin-cmd.c | 247 +++++++++++++++++++------ drivers/nvme/target/configfs.c | 381 +++++++++++++++++++++++++++++++++++++++ drivers/nvme/target/core.c | 120 +++++++++++-- drivers/nvme/target/discovery.c | 49 +++-- drivers/nvme/target/io-cmd.c | 36 ++++ drivers/nvme/target/nvmet.h | 52 ++++++ include/linux/nvme.h | 65 ++++++- 11 files changed, 1275 insertions(+), 130 deletions(-)
Signature · 1 line
2.12.3
PATCH v2 1/11 nvme.h: untangle AEN notice definitions
From: Christoph Hellwig <[email protected]>
Stop including the event type in the definitions for the notice type.
--- drivers/nvme/host/core.c | 30 ++++++++++++++++++------------ include/linux/nvme.h | 8 ++++++-- 2 files changed, 24 insertions(+), 14 deletions(-)
drivers/nvme/host/core.c +18 -12
@@ -3343,6 +3343,21 @@ static void nvme_fw_act_work(struct work_struct *work)nvme_get_fw_slot_info (ctrl); } +static void nvme_handle_aen_notice (struct nvme_ctrl *ctrl,u32 result) +{ +switch ((result &0xff00 ) >>8 ) { +case NVME_AER_NOTICE_NS_CHANGED : +dev_info (ctrl->device,"rescanning\n" ); +nvme_queue_scan (ctrl); +break ; +case NVME_AER_NOTICE_FW_ACT_STARTING : +queue_work (nvme_wq, &ctrl->fw_act_work); +break ; +default : +dev_warn (ctrl->device,"async event result %08x\n" , result); + } +} +void nvme_complete_async_event (struct nvme_ctrl *ctrl,__le16 status,union nvme_result *res) {
@@ -3352,6 +3367,9 @@ void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,return ;switch (result &0x7 ) { +case NVME_AER_NOTICE : +nvme_handle_aen_notice (ctrl, result); +break ;case NVME_AER_ERROR :case NVME_AER_SMART :case NVME_AER_CSS :
@@ -3361,18 +3379,6 @@ void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,default :break ; } - -switch (result &0xff07 ) { -case NVME_AER_NOTICE_NS_CHANGED : -dev_info (ctrl->device,"rescanning\n" ); -nvme_queue_scan (ctrl); -break ; -case NVME_AER_NOTICE_FW_ACT_STARTING : -queue_work (nvme_wq, &ctrl->fw_act_work); -break ; -default : -dev_warn (ctrl->device,"async event result %08x\n" , result); - }queue_work (nvme_wq, &ctrl->async_event_work); }EXPORT_SYMBOL_GPL (nvme_complete_async_event);
include/linux/nvme.h +6 -2
@@ -436,10 +436,14 @@ enum {enum {NVME_AER_ERROR =0 ,NVME_AER_SMART =1 , +NVME_AER_NOTICE =2 ,NVME_AER_CSS =6 ,NVME_AER_VS =7 , -NVME_AER_NOTICE_NS_CHANGED =0x0002 , -NVME_AER_NOTICE_FW_ACT_STARTING =0x0102 , +}; + +enum { +NVME_AER_NOTICE_NS_CHANGED =0x00 , +NVME_AER_NOTICE_FW_ACT_STARTING =0x01 , };struct nvme_lba_range_type {
Signature · 1 line
2.12.3
PATCH v2 2/11 nvme: submit AEN event configuration on startup
In reply to Hannes ReineckeWe should register for AEN events; some law-abiding targets might not be sending us AENs otherwise.
--- drivers/nvme/host/core.c | 19 +++++++++++++++++++ drivers/nvme/host/nvme.h | 1 + include/linux/nvme.h | 5 +++++ 3 files changed, 25 insertions(+)
drivers/nvme/host/core.c +19 -0
@@ -1029,6 +1029,22 @@ int nvme_set_queue_count(struct nvme_ctrl *ctrl, int *count) }EXPORT_SYMBOL_GPL (nvme_set_queue_count); +void nvme_enable_aen (struct nvme_ctrl *ctrl) +{ +u32 aen_cfg =NVME_SMART_CRIT_SPARE |NVME_SMART_CRIT_TEMPERATURE | +NVME_SMART_CRIT_RELIABILITY |NVME_SMART_CRIT_MEDIA | +NVME_SMART_CRIT_VOLATILE_MEMORY ; +u32 result; +int status; + + aen_cfg |= ctrl->aen_cfg; + status =nvme_set_features (ctrl,NVME_FEAT_ASYNC_EVENT , + aen_cfg,NULL ,0 , &result); +if (status) +dev_warn (ctrl->device,"Failed to configure AEN (cfg %x)\n" , + aen_cfg); +} +static int nvme_submit_io (struct nvme_ns *ns,struct nvme_user_io __user *uio) {struct nvme_user_io io;
@@ -2345,6 +2361,8 @@ int nvme_init_identify(struct nvme_ctrl *ctrl) ctrl->oacs =le16_to_cpu (id->oacs); ctrl->oncs =le16_to_cpup (&id->oncs); + ctrl->aen_cfg =le32_to_cpu (id->oaes) & + (NVME_AEN_CFG_NS_ATTR |NVME_AEN_CFG_FW_ACT );atomic_set (&ctrl->abort_limit, id->acl +1 ); ctrl->vwc = id->vwc; ctrl->cntlid =le16_to_cpup (&id->cntlid);
@@ -3401,6 +3419,7 @@ void nvme_start_ctrl(struct nvme_ctrl *ctrl)if (ctrl->queue_count >1 ) {nvme_queue_scan (ctrl); +nvme_enable_aen (ctrl);queue_work (nvme_wq, &ctrl->async_event_work);nvme_start_queues (ctrl); }
drivers/nvme/host/nvme.h +1 -0
@@ -176,6 +176,7 @@ struct nvme_ctrl {u16 kas;u8 npss;u8 apsta; +u32 aen_cfg;u32 aen_result;unsigned int shutdown_timeout;unsigned int kato;
include/linux/nvme.h +5 -0
@@ -446,6 +446,11 @@ enum { NVME_AER_NOTICE_FW_ACT_STARTING = 0x01, }; +enum { +NVME_AEN_CFG_NS_ATTR =1 <<8 , +NVME_AEN_CFG_FW_ACT =1 <<9 , +}; +struct nvme_lba_range_type {__u8 type;__u8 attributes;
Signature · 1 line
2.12.3
Christoph Hellwig
maintainerPATCH v2 2/11 nvme: submit AEN event configuration on startup
drivers/nvme/host/core.c int nvme_set_queue_count(struct nvme_ctrl *ctrl, int *count) · line 1036
Quoted 5 lines
drivers/nvme/host/core.c +5
-0+void nvme_enable_aen (struct nvme_ctrl *ctrl) +{ +u32 aen_cfg =NVME_SMART_CRIT_SPARE |NVME_SMART_CRIT_TEMPERATURE | +NVME_SMART_CRIT_RELIABILITY |NVME_SMART_CRIT_MEDIA | +NVME_SMART_CRIT_VOLATILE_MEMORY ;
Currently the kernel doesn't really care about smart warnings at all. Should we make this a sysfs to that people only enable if they actually care?
drivers/nvme/host/core.c int nvme_set_queue_count(struct nvme_ctrl *ctrl, int *count) · line 1047
Quoted 11 lines
drivers/nvme/host/core.c +11
-0+u32 result; +int status; + + aen_cfg |= ctrl->aen_cfg; + status =nvme_set_features (ctrl,NVME_FEAT_ASYNC_EVENT , + aen_cfg,NULL ,0 , &result); +if (status) +dev_warn (ctrl->device,"Failed to configure AEN (cfg %x)\n" , + aen_cfg); +} +
If status is negative we should abort the reset, so we need to pass that on.
drivers/nvme/host/core.c int nvme_init_identify(struct nvme_ctrl *ctrl) · line 2365
Quoted 6 lines
drivers/nvme/host/core.c +2
-0@@ -2345,6 +2361,8 @@ int nvme_init_identify(struct nvme_ctrl *ctrl) ctrl->oacs =le16_to_cpu (id->oacs); ctrl->oncs =le16_to_cpup (&id->oncs); + ctrl->aen_cfg =le32_to_cpu (id->oaes) & + (NVME_AEN_CFG_NS_ATTR |NVME_AEN_CFG_FW_ACT );
Hmm. I'd rather store the oaes value in nvme_ctrl and do this calculation in nvme_enable_aen.
PATCH v2 2/11 nvme: submit AEN event configuration on startup
Quoted 10 lines · Christoph Hellwig
drivers/nvme/host/core.c int nvme_set_queue_count(struct nvme_ctrl *ctrl, int *count) · line 1036
Quoted 5 lines
drivers/nvme/host/core.c +5
-0+void nvme_enable_aen (struct nvme_ctrl *ctrl) +{ +u32 aen_cfg =NVME_SMART_CRIT_SPARE |NVME_SMART_CRIT_TEMPERATURE | +NVME_SMART_CRIT_RELIABILITY |NVME_SMART_CRIT_MEDIA | +NVME_SMART_CRIT_VOLATILE_MEMORY ;Currently the kernel doesn't really care about smart warnings at all. Should we make this a sysfs to that people only enable if they actually care?
Yeah, can do.
Quoted 6 lines
Quoted 6 lines
drivers/nvme/host/core.c +6
-0+u32 result; +int status; + + aen_cfg |= ctrl->aen_cfg; + status =nvme_set_features (ctrl,NVME_FEAT_ASYNC_EVENT , + aen_cfg,NULL ,0 , &result);
Quoted 9 lines
drivers/nvme/host/core.c int nvme_set_queue_count(struct nvme_ctrl *ctrl, int *count) · line 1047
Quoted 5 lines
drivers/nvme/host/core.c +5
-0+if (status) +dev_warn (ctrl->device,"Failed to configure AEN (cfg %x)\n" , + aen_cfg); +} +If status is negative we should abort the reset, so we need to pass that on.
Ah. I would've thought as this is technically a new feature, so we might encounter spurious errors here (which we've never noticed until now). But if you say so ...
Quoted 10 lines
drivers/nvme/host/core.c int nvme_init_identify(struct nvme_ctrl *ctrl) · line 2365
Quoted 6 lines
drivers/nvme/host/core.c +2
-0@@ -2345,6 +2361,8 @@ int nvme_init_identify(struct nvme_ctrl *ctrl) ctrl->oacs =le16_to_cpu (id->oacs); ctrl->oncs =le16_to_cpup (&id->oncs); + ctrl->aen_cfg =le32_to_cpu (id->oaes) & + (NVME_AEN_CFG_NS_ATTR |NVME_AEN_CFG_FW_ACT );Hmm. I'd rather store the oaes value in
nvme_ctrland do this calculation innvme_enable_aen.
Ok. Will be doing so.
Cheers,
Hannes
PATCH v2 3/11 nvmet: refactor AER handling
In reply to Hannes ReineckeFrom: Christoph Hellwig <[email protected]>
Create a helper to iterate all controlles in a subsystem instead of open coding the loop. Also use the NVME_AER_NOTICE_NS_CHANGED definition instead of plain 0.
--- drivers/nvme/target/core.c | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-)
drivers/nvme/target/core.c +15 -8
@@ -137,6 +137,19 @@ static void nvmet_add_async_event(struct nvmet_ctrl *ctrl, u8 event_type,schedule_work (&ctrl->async_event_work); } +static void nvmet_send_ns_changed_event (struct nvmet_subsys *subsys) +{ +/* +* XXX: should also include the log id, but the changed namespace list +* log needs to be implemented first.. +*/ +struct nvmet_ctrl *ctrl; + +list_for_each_entry (ctrl, &subsys->ctrls, subsys_entry) +nvmet_add_async_event (ctrl,NVME_AER_TYPE_NOTICE , +NVME_AER_NOTICE_NS_CHANGED ,0 ); +} +int nvmet_register_transport (const struct nvmet_fabrics_ops *ops) {int ret =0 ;
@@ -274,7 +287,6 @@ void nvmet_put_namespace(struct nvmet_ns *ns)int nvmet_ns_enable (struct nvmet_ns *ns) {struct nvmet_subsys *subsys = ns->subsys; -struct nvmet_ctrl *ctrl;int ret =0 ;mutex_lock (&subsys->lock);
@@ -320,9 +332,7 @@ int nvmet_ns_enable(struct nvmet_ns *ns)list_add_tail_rcu (&ns->dev_link, &old->dev_link); } -list_for_each_entry (ctrl, &subsys->ctrls, subsys_entry) -nvmet_add_async_event (ctrl,NVME_AER_TYPE_NOTICE ,0 ,0 ); - +nvmet_send_ns_changed_event (subsys); ns->enabled = true; ret =0 ; out_unlock:
@@ -337,7 +347,6 @@ int nvmet_ns_enable(struct nvmet_ns *ns)void nvmet_ns_disable (struct nvmet_ns *ns) {struct nvmet_subsys *subsys = ns->subsys; -struct nvmet_ctrl *ctrl;mutex_lock (&subsys->lock);if (!ns->enabled)
@@ -363,9 +372,7 @@ void nvmet_ns_disable(struct nvmet_ns *ns)percpu_ref_exit (&ns->ref);mutex_lock (&subsys->lock); -list_for_each_entry (ctrl, &subsys->ctrls, subsys_entry) -nvmet_add_async_event (ctrl,NVME_AER_TYPE_NOTICE ,0 ,0 ); - +nvmet_send_ns_changed_event (subsys);if (ns->bdev)blkdev_put (ns->bdev,FMODE_WRITE |FMODE_READ ); out_unlock:
Signature · 1 line
2.12.3
PATCH v2 4/11 nvmet: Add AEN configuration support
In reply to Hannes ReineckeFrom: Hannes Reinecke <[email protected]>
AEN configuration via the 'Get Features' and 'Set Features' admin command is mandatory, so we should be implemeting handling for it.
--- drivers/nvme/target/admin-cmd.c | 8 +++++++- drivers/nvme/target/core.c | 6 ++++-- drivers/nvme/target/nvmet.h | 1 + 3 files changed, 12 insertions(+), 3 deletions(-)
drivers/nvme/target/admin-cmd.c +7 -1
@@ -447,6 +447,11 @@ static void nvmet_execute_set_features(struct nvmet_req *req) req->sq->ctrl->kato =DIV_ROUND_UP (val32,1000 );nvmet_set_result (req, req->sq->ctrl->kato);break ; +case NVME_FEAT_ASYNC_EVENT : + val32 =le32_to_cpu (req->cmd->common.cdw10[1 ]); + req->sq->ctrl->aen_cfg = val32 &NVME_AEN_CFG_NS_ATTR ; +nvmet_set_result (req, req->sq->ctrl->aen_cfg); +break ;case NVME_FEAT_HOST_ID : status =NVME_SC_CMD_SEQ_ERROR |NVME_SC_DNR ;break ;
@@ -485,9 +490,10 @@ static void nvmet_execute_get_features(struct nvmet_req *req)break ;case NVME_FEAT_WRITE_ATOMIC :break ; +#endif case NVME_FEAT_ASYNC_EVENT : +nvmet_set_result (req, req->sq->ctrl->aen_cfg);break ; -#endif case NVME_FEAT_VOLATILE_WC :nvmet_set_result (req,1 );break ;
drivers/nvme/target/core.c +4 -2
@@ -146,8 +146,9 @@ static void nvmet_send_ns_changed_event(struct nvmet_subsys *subsys)struct nvmet_ctrl *ctrl;list_for_each_entry (ctrl, &subsys->ctrls, subsys_entry) -nvmet_add_async_event (ctrl,NVME_AER_TYPE_NOTICE , -NVME_AER_NOTICE_NS_CHANGED ,0 ); +if (ctrl->aen_cfg &NVME_AEN_CFG_NS_ATTR ) +nvmet_add_async_event (ctrl,NVME_AER_TYPE_NOTICE , +NVME_AER_NOTICE_NS_CHANGED ,0 ); }int nvmet_register_transport (const struct nvmet_fabrics_ops *ops)
@@ -816,6 +817,7 @@ u16 nvmet_alloc_ctrl(const char *subsysnqn, const char *hostnqn,kref_init (&ctrl->ref); ctrl->subsys = subsys; + ctrl->aen_cfg =NVME_AEN_CFG_NS_ATTR ; ctrl->cqs = kcalloc(subsys->max_qid + 1, sizeof(struct nvmet_cq *),
drivers/nvme/target/nvmet.h +1 -0
@@ -120,6 +120,7 @@ struct nvmet_ctrl {u16 cntlid;u32 kato; +u32 aen_cfg;struct nvmet_req *async_event_cmds[NVMET_ASYNC_EVENTS ];unsigned int nr_async_event_cmds;struct list_head async_events;
Signature · 1 line
2.12.3
Christoph Hellwig
maintainerPATCH v2 4/11 nvmet: Add AEN configuration support
drivers/nvme/target/admin-cmd.c static void nvmet_execute_set_features(struct nvmet_req *req) · line 452
Quoted 3 lines
drivers/nvme/target/admin-cmd.c +3
-0+case NVME_FEAT_ASYNC_EVENT : + val32 =le32_to_cpu (req->cmd->common.cdw10[1 ]); + req->sq->ctrl->aen_cfg = val32 &NVME_AEN_CFG_NS_ATTR ;
This needs a validity check, and either be under a lock or very carefully accessed using WRITE_ONCE/READ_ONCE.
PATCH v2 4/11 nvmet: Add AEN configuration support
Quoted 7 lines · Christoph Hellwig
drivers/nvme/target/admin-cmd.c static void nvmet_execute_set_features(struct nvmet_req *req) · line 452
Quoted 3 lines
drivers/nvme/target/admin-cmd.c +3
-0+case NVME_FEAT_ASYNC_EVENT : + val32 =le32_to_cpu (req->cmd->common.cdw10[1 ]); + req->sq->ctrl->aen_cfg = val32 &NVME_AEN_CFG_NS_ATTR ;This needs a validity check, and either be under a lock or very carefully accessed using WRITE_ONCE/READ_ONCE.
Okay; a controller lock would be good here. Let's see what we have.
Cheers,
Hannes
PATCH v2 5/11 nvme.h: add ANA definitions
In reply to Hannes ReineckeFrom: Christoph Hellwig <[email protected]>
Add various defintions from NVMe 1.3 TP 4004.
--- include/linux/nvme.h | 50 +++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 47 insertions(+), 3 deletions(-)
include/linux/nvme.h +47 -3
@@ -242,7 +242,12 @@ struct nvme_id_ctrl {__le32 sanicap;__le32 hmminds;__le16 hmmaxd; -__u8 rsvd338[174 ]; +__u8 rsvd338[4 ]; +__u8 anatt; +__u8 anacap; +__le32 anagrpmax; +__le32 nanagrpid; +__u8 rsvd352[160 ];__u8 sqes;__u8 cqes;__le16 maxcmd;
@@ -258,7 +263,8 @@ struct nvme_id_ctrl {__le16 acwu;__u8 rsvd534[2 ];__le32 sgls; -__u8 rsvd540[228 ]; +__le32 mnan; +__u8 rsvd544[224 ];char subnqn[256 ];__u8 rsvd1024[768 ];__le32 ioccsz;
@@ -312,7 +318,9 @@ struct nvme_id_ns {__le16 nabspf;__le16 noiob;__u8 nvmcap[16 ]; -__u8 rsvd64[40 ]; +__u8 rsvd64[28 ]; +__le32 anagrpid; +__u8 rsvd96[8 ];__u8 nguid[16 ];__u8 eui64[8 ];struct nvme_lbaf lbaf[16 ];
@@ -425,6 +433,32 @@ struct nvme_effects_log {__u8 resv[2048 ]; }; +enum nvme_ana_state { +NVME_ANA_OPTIMIZED =0x01 , +NVME_ANA_NONOPTIMIZED =0x02 , +NVME_ANA_INACCESSIBLE =0x03 , +NVME_ANA_PERSISTENT_LOSS =0x04 , +NVME_ANA_CHANGE =0x0f , +}; + +struct nvme_ana_group_desc { +__le32 grpid; +__le32 nnsids; +__le64 chgcnt; +__u8 state; +__u8 rsvd17[7 ]; +__le32 nsids[]; +}; + +/* flag for the log specific field of the ANA log */ +#define NVME_ANA_LOG_RGO (1 << 0) + +struct nvme_ana_rsp_hdr { +__le64 chgcnt; +__le16 ngrps; +__le16 rsvd10[3 ]; +}; +enum {NVME_SMART_CRIT_SPARE =1 <<0 ,NVME_SMART_CRIT_TEMPERATURE =1 <<1 ,
@@ -444,11 +478,13 @@ enum {enum {NVME_AER_NOTICE_NS_CHANGED =0x00 ,NVME_AER_NOTICE_FW_ACT_STARTING =0x01 , +NVME_AER_NOTICE_ANA =0x03 , };enum {NVME_AEN_CFG_NS_ATTR =1 <<8 ,NVME_AEN_CFG_FW_ACT =1 <<9 , +NVME_AEN_CFG_ANA_CHANGE =1 <<11 , };struct nvme_lba_range_type {
@@ -757,6 +793,7 @@ enum {NVME_LOG_SMART =0x02 ,NVME_LOG_FW_SLOT =0x03 ,NVME_LOG_CMD_EFFECTS =0x05 , +NVME_LOG_ANA =0x0c ,NVME_LOG_DISC =0x70 ,NVME_LOG_RESERVATION =0x80 , NVME_FWACT_REPL = (0 << 3),
@@ -1177,6 +1214,13 @@ enum {NVME_SC_ACCESS_DENIED =0x286 ,NVME_SC_UNWRITTEN_BLOCK =0x287 , +/* +* Path-related Errors: +*/ +NVME_SC_ANA_PERSISTENT_LOSS =0x301 , +NVME_SC_ANA_INACCESSIBLE =0x302 , +NVME_SC_ANA_TRANSITION =0x303 , + NVME_SC_DNR = 0x4000, };
Signature · 1 line
2.12.3
PATCH v2 6/11 nvme: add support for the log specific field
In reply to Hannes ReineckeFrom: Christoph Hellwig <[email protected]>
NVMe 1.3 added a new log specific field to the get log page CQ defintion. Add the field and support passing it to nvme_get_log_ext.
--- drivers/nvme/host/core.c | 6 +++--- drivers/nvme/host/lightnvm.c | 2 +- drivers/nvme/host/nvme.h | 2 +- include/linux/nvme.h | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-)
drivers/nvme/host/core.c +3 -3
@@ -2245,8 +2245,7 @@ static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id) }int nvme_get_log_ext (struct nvme_ctrl *ctrl,struct nvme_ns *ns, -u8 log_page,void *log, -size_t size,u64 offset) +u8 log_page,u8 lsp,void *log,size_t size,u64 offset) {struct nvme_command c = { };unsigned long dwlen = size /4 -1 ;
@@ -2259,6 +2258,7 @@ int nvme_get_log_ext(struct nvme_ctrl *ctrl, struct nvme_ns *ns, c.get_log_page.nsid =cpu_to_le32 (NVME_NSID_ALL ); c.get_log_page.lid = log_page; + c.get_log_page.lsp = lsp; c.get_log_page.numdl =cpu_to_le16 (dwlen & ((1 <<16 ) -1 )); c.get_log_page.numdu =cpu_to_le16 (dwlen >>16 ); c.get_log_page.lpol =cpu_to_le32 (lower_32_bits (offset));
@@ -2270,7 +2270,7 @@ int nvme_get_log_ext(struct nvme_ctrl *ctrl, struct nvme_ns *ns,static int nvme_get_log (struct nvme_ctrl *ctrl,u8 log_page,void *log,size_t size) { -return nvme_get_log_ext (ctrl,NULL , log_page, log, size,0 ); +return nvme_get_log_ext (ctrl,NULL , log_page,0 , log, size,0 ); }static int nvme_get_effects_log (struct nvme_ctrl *ctrl)
drivers/nvme/host/lightnvm.c +1 -1
@@ -598,7 +598,7 @@ static int nvme_nvm_get_chk_meta(struct nvm_dev *ndev,while (left) { len = min_t(unsigned int, left, ctrl->max_hw_sectors << 9); - ret =nvme_get_log_ext (ctrl, ns,NVME_NVM_LOG_REPORT_CHUNK , + ret =nvme_get_log_ext (ctrl, ns,NVME_NVM_LOG_REPORT_CHUNK ,0 , dev_meta, len, offset);if (ret) {dev_err (ctrl->device,"Get REPORT CHUNK log error\n" );
drivers/nvme/host/nvme.h +1 -1
@@ -432,7 +432,7 @@ int nvme_delete_ctrl(struct nvme_ctrl *ctrl);int nvme_delete_ctrl_sync (struct nvme_ctrl *ctrl);int nvme_get_log_ext (struct nvme_ctrl *ctrl,struct nvme_ns *ns, -u8 log_page,void *log,size_t size,u64 offset); +u8 log_page,u8 lsp,void *log,size_t size,u64 offset);extern const struct attribute_group nvme_ns_id_attr_group;extern const struct block_device_operations nvme_ns_head_ops;
include/linux/nvme.h +1 -1
@@ -914,7 +914,7 @@ struct nvme_get_log_page_command {__u64 rsvd2[2 ];union nvme_data_ptr dptr;__u8 lid; -__u8 rsvd10; +__u8 lsp;/* upper 4 bits reserved */ __le16 numdl;__le16 numdu;__u16 rsvd11;
Signature · 1 line
2.12.3
PATCH v2 7/11 nvme: always failover on path or transport errors
In reply to Hannes ReineckeFrom: Christoph Hellwig <[email protected]>
NVMe 1.3 TP 4028 introduced a new status code type 3h for "Path Related Status". We should always retry on another path for this class of errors, without even trying to decode them.
--- drivers/nvme/host/multipath.c | 2 ++ 1 file changed, 2 insertions(+)
drivers/nvme/host/multipath.c +2 -0
@@ -37,6 +37,8 @@ bool nvme_req_needs_failover(struct request *req, blk_status_t error) {if (!(req->cmd_flags &REQ_NVME_MPATH ))return false; +if (nvme_req (req)->status &0x300 ) +return true;return blk_path_error (error); }
Signature · 1 line
2.12.3
Christoph Hellwig
maintainerPATCH v2 7/11 nvme: always failover on path or transport errors
drivers/nvme/host/multipath.c bool nvme_req_needs_failover(struct request *req, blk_status_t error) · line 41
Quoted 4 lines · Hannes Reinecke
drivers/nvme/host/multipath.c +2
-0if (!(req->cmd_flags &REQ_NVME_MPATH ))return false; +if (nvme_req (req)->status &0x300 ) +return true;
As I love to correct everyone including myself:
This actually needs to be
if ((nvme_req(req)->status & 0x700) == 0x300)
to accomodate for future new SCT values.
PATCH v2 8/11 nvme: add ANA support
In reply to Hannes ReineckeFrom: Christoph Hellwig <[email protected]>
Adding sypport for Asymmetric Namespace Access (ANA)
--- drivers/nvme/host/core.c | 335 +++++++++++++++++++++++++++++++++++++++--- drivers/nvme/host/multipath.c | 41 +++++- drivers/nvme/host/nvme.h | 17 +++ 3 files changed, 366 insertions(+), 27 deletions(-)
drivers/nvme/host/core.c +313 -22
@@ -68,6 +68,10 @@ static bool streams;module_param (streams, bool,0644 );MODULE_PARM_DESC (streams,"turn on support for Streams write directives" ); +static unsigned long ana_log_delay =500 ; +module_param (ana_log_delay, ulong,0644 ); +MODULE_PARM_DESC (ana_log_delay,"Delay in msecs before retrieving ANA log" ); +/* * nvme_wq - hosts nvme related works that are not reset or delete * nvme_reset_wq - hosts nvme reset works
@@ -1246,6 +1250,25 @@ static void nvme_put_ns_from_disk(struct nvme_ns_head *head, int idx)srcu_read_unlock (&head->srcu, idx); } +static struct nvme_ns *nvme_find_get_ns (struct nvme_ctrl *ctrl,unsigned nsid) +{ +struct nvme_ns *ns, *ret =NULL ; + +down_read (&ctrl->namespaces_rwsem); +list_for_each_entry (ns, &ctrl->namespaces, list) { +if (ns->head->ns_id == nsid) { +if (!kref_get_unless_zero (&ns->kref)) +continue ; + ret = ns; +break ; + } +if (ns->head->ns_id > nsid) +break ; + } +up_read (&ctrl->namespaces_rwsem); +return ret; +} +static int nvme_ns_ioctl (struct nvme_ns *ns,unsigned cmd,unsigned long arg) {switch (cmd) {
@@ -1357,6 +1380,228 @@ static void nvme_init_integrity(struct gendisk *disk, u16 ms, u8 pi_type) }#endif /* CONFIG_BLK_DEV_INTEGRITY */ +static const char *nvme_ana_state_names[] = { + [0 ] ="invalid state" , + [NVME_ANA_OPTIMIZED ] ="optimized" , + [NVME_ANA_NONOPTIMIZED ] ="non-optimized" , + [NVME_ANA_INACCESSIBLE ] ="inaccessible" , + [NVME_ANA_PERSISTENT_LOSS ] ="persistent-loss" , + [NVME_ANA_CHANGE ] ="change" , +}; + +static int nvme_process_ana_log (struct nvme_ctrl *ctrl,bool groups_only) +{ +void *base = ctrl->ana_log_buf; +size_t offset =sizeof (struct nvme_ana_rsp_hdr ); +int error, i, j; +size_t mdts = ctrl->max_hw_sectors <<9 ; +size_t ana_log_size = ctrl->ana_log_size, ana_xfer_size; +off_t ana_log_offset =0 ; + +/* +* If anagrpid never changes we don't need to process the namespace +* lists. +*/ +if (ctrl->anacap & (1 <<7 )) + groups_only = true; + +if (groups_only) + ana_log_size =sizeof (struct nvme_ana_rsp_hdr ) + + ctrl->nanagrpid *sizeof (struct nvme_ana_group_desc ); + + ana_xfer_size = ana_log_size; +if (mdts && ana_log_size > mdts) { +dev_warn (ctrl->device, +"ANA log too large (%lu, max %zu), truncating\n" , + ana_log_size, mdts); + ana_xfer_size = mdts; + } +resubmit: + error =nvme_get_log_ext (ctrl,NULL ,NVME_LOG_ANA , + groups_only ?NVME_ANA_LOG_RGO :0 , + ctrl->ana_log_buf + ana_log_offset, + ana_xfer_size, ana_log_offset); +if (error) { +dev_warn (ctrl->device,"Failed to get ANA log: %d\n" , error); +return -EIO ; + } + ana_log_offset += ana_xfer_size; +if (ana_log_size > ana_log_offset) { +if (ana_log_size - ana_log_offset < mdts) + ana_xfer_size = ana_log_size - ana_log_offset; + goto resubmit; + } + +for (i =0 ; i <le16_to_cpu (ctrl->ana_log_buf->ngrps); i++) { +struct nvme_ana_group_desc *desc = base + offset; +u32 grpid; +u32 nr_nsids; +size_t nsid_buf_size; +struct nvme_ns *ns; + +if (WARN_ON_ONCE (offset > ana_log_size -sizeof (*desc))) +return -EINVAL ; + grpid =le32_to_cpu (desc->grpid); +if (WARN_ON_ONCE (grpid ==0 )) +return -EINVAL ; +if (WARN_ON_ONCE (grpid > ctrl->anagrpmax)) +return -EINVAL ; +if (WARN_ON_ONCE (desc->state ==0 )) +return -EINVAL ; +if (WARN_ON_ONCE (desc->state >NVME_ANA_CHANGE )) +return -EINVAL ; + + offset +=sizeof (*desc); +if (groups_only) { +down_write (&ctrl->namespaces_rwsem); +list_for_each_entry (ns, &ctrl->namespaces, list) { +if (ns->anagrpid != grpid) +continue ; +if (desc->state ==NVME_SC_ANA_TRANSITION ) + error = -EAGAIN ; +if (ns->ana_state != desc->state) { + ns->ana_state = desc->state; +nvme_mpath_clear_current_path (ns); + } +break ; + } +up_write (&ctrl->namespaces_rwsem); +continue ; + } + nr_nsids =le32_to_cpu (desc->nnsids); +if (!nr_nsids) +continue ; + + nsid_buf_size = nr_nsids *sizeof (__le32); +if (WARN_ON_ONCE (offset > ana_log_size - nsid_buf_size)) +return -EINVAL ; + +for (j =0 ; j < nr_nsids; j++) { +u32 nsid =le32_to_cpu (desc->nsids[j]); + + ns =nvme_find_get_ns (ctrl, nsid); +if (!ns) +continue ; + +if (ns->anagrpid != grpid || + ns->ana_state != desc->state) { + ns->anagrpid = grpid; + ns->ana_state = desc->state; +nvme_mpath_clear_current_path (ns); +if (desc->state ==NVME_SC_ANA_TRANSITION ) + error = -EAGAIN ; + } +nvme_put_ns (ns); + } + + offset += nsid_buf_size; + } + +return error; +} + +static enum nvme_ana_state nvme_get_ana_state (struct nvme_ctrl *ctrl, +unsigned int grpid, +unsigned int nsid) +{ +void *base = ctrl->ana_log_buf; +size_t offset =sizeof (struct nvme_ana_rsp_hdr ); +int i, j; + +if (!ctrl->ana_log_buf) +return NVME_ANA_OPTIMIZED ; + +for (i =0 ; i <le16_to_cpu (ctrl->ana_log_buf->ngrps); i++) { +struct nvme_ana_group_desc *desc = base + offset; +u32 nr_nsids; +size_t nsid_buf_size; + +if (offset > ctrl->ana_log_size -sizeof (*desc)) +break ; + + offset +=sizeof (*desc); + nr_nsids =le32_to_cpu (desc->nnsids); +if (!nr_nsids) +continue ; + + nsid_buf_size = nr_nsids *sizeof (__le32); +if (offset > ctrl->ana_log_size - nsid_buf_size) +break ; + +if (grpid ==le32_to_cpu (desc->grpid)) { +for (j =0 ; j < nr_nsids; j++) { +if (le32_to_cpu (desc->nsids[j]) != nsid) +continue ; + +dev_info (ctrl->device, +"nsid %d: ANA group %d, %s.\n" , + nsid, grpid, + nvme_ana_state_names[desc->state]); +return desc->state; + } + } + offset += nsid_buf_size; + } + +return NVME_ANA_OPTIMIZED ; +} + +static void nvme_ana_work (struct work_struct *work) +{ + struct nvme_ctrl *ctrl = container_of(work, struct nvme_ctrl, + ana_work.work); +int ret; + +if (!ctrl->ana_log_buf) +return ; +if (ctrl->state !=NVME_CTRL_LIVE ) +return ; + ret =nvme_process_ana_log (ctrl, false); +if (ret == -EAGAIN || ret == -EIO ) { +unsigned long log_delay = ctrl->anatt *HZ ; + +if (ret == -EIO ) + log_delay =msecs_to_jiffies (ana_log_delay); +queue_delayed_work (nvme_wq, &ctrl->ana_work, log_delay); + } +nvme_kick_requeue_lists (ctrl); +} + +int nvme_configure_ana (struct nvme_ctrl *ctrl) +{ +int error; + +if (!nvme_ctrl_has_ana (ctrl)) +return 0 ; + +INIT_DELAYED_WORK (&ctrl->ana_work, nvme_ana_work); + ctrl->ana_log_size =sizeof (struct nvme_ana_rsp_hdr ) + + ctrl->nanagrpid *sizeof (struct nvme_ana_group_desc ) + + ctrl->max_namespaces *sizeof (__le32); + ctrl->ana_log_buf =kzalloc (ctrl->ana_log_size,GFP_KERNEL ); +if (!ctrl->ana_log_buf) +return -ENOMEM ; + + error =nvme_process_ana_log (ctrl, false); +if (error) { +if (error != -EAGAIN && error != -EIO ) { +kfree (ctrl->ana_log_buf); + ctrl->ana_log_buf =NULL ; + }else + error =0 ; + } +return error; +} + +void nvme_deconfigure_ana (struct nvme_ctrl *ctrl) +{ +if (ctrl->ana_log_buf) { +cancel_delayed_work_sync (&ctrl->ana_work); +kfree (ctrl->ana_log_buf); + ctrl->ana_log_buf =NULL ; + } +} +static void nvme_set_chunk_size (struct nvme_ns *ns) {u32 chunk_size = (((u32 )ns->noiob) << (ns->lba_shift -9 ));
@@ -1446,7 +1691,9 @@ static void nvme_update_disk_info(struct gendisk *disk,if (ns->ms && !nvme_ns_has_pi (ns) && !blk_get_integrity (disk)) capacity =0 ; -set_capacity (disk, capacity); +if (ns->ana_state ==NVME_ANA_OPTIMIZED || + ns->ana_state ==NVME_ANA_NONOPTIMIZED ) +set_capacity (disk, capacity);nvme_config_discard (ns);blk_mq_unfreeze_queue (disk->queue); }
@@ -1462,6 +1709,7 @@ static void __nvme_revalidate_disk(struct gendisk *disk, struct nvme_id_ns *id) ns->lba_shift = id->lbaf[id->flbas &NVME_NS_FLBAS_LBA_MASK ].ds;if (ns->lba_shift ==0 ) ns->lba_shift =9 ; + ns->anagrpid =le32_to_cpu (id->anagrpid); ns->noiob =le16_to_cpu (id->noiob); ns->ext = ns->ms && (id->flbas &NVME_NS_FLBAS_META_EXT ); ns->ms =le16_to_cpu (id->lbaf[id->flbas &NVME_NS_FLBAS_LBA_MASK ].ms);
@@ -1488,6 +1736,7 @@ static int nvme_revalidate_disk(struct gendisk *disk)struct nvme_ctrl *ctrl = ns->ctrl;struct nvme_id_ns *id;struct nvme_ns_ids ids; +unsigned int grpid;int ret =0 ;if (test_bit (NVME_NS_DEAD , &ns->flags)) {
@@ -1504,6 +1753,19 @@ static int nvme_revalidate_disk(struct gendisk *disk) goto out; } + grpid =le32_to_cpu (id->anagrpid); +if (grpid) { +if (ns->anagrpid != grpid) { +dev_warn (ctrl->device,"ns %x ANA group ID changed\n" , + ns->head->ns_id); + ns->anagrpid = grpid; +queue_delayed_work (nvme_wq, &ctrl->ana_work, +msecs_to_jiffies (ana_log_delay)); + } + ns->ana_state =nvme_get_ana_state (ctrl, grpid, + ns->head->ns_id); + } +__nvme_revalidate_disk (disk, id);nvme_report_ns_ids (ctrl, ns->head->ns_id, id, &ids);if (!nvme_ns_ids_equal (&ns->head->ids, &ids)) {
@@ -2363,6 +2625,8 @@ int nvme_init_identify(struct nvme_ctrl *ctrl) ctrl->oncs =le16_to_cpup (&id->oncs); ctrl->aen_cfg =le32_to_cpu (id->oaes) & (NVME_AEN_CFG_NS_ATTR |NVME_AEN_CFG_FW_ACT ); +if (nvme_ctrl_has_ana (ctrl)) + ctrl->aen_cfg |=NVME_AEN_CFG_ANA_CHANGE ;atomic_set (&ctrl->abort_limit, id->acl +1 ); ctrl->vwc = id->vwc; ctrl->cntlid =le16_to_cpup (&id->cntlid);
@@ -2376,6 +2640,11 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)nvme_set_queue_limits (ctrl, ctrl->admin_q); ctrl->sgls =le32_to_cpu (id->sgls); 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);if (id->rtd3e) {/* us -> s */
@@ -2445,7 +2714,7 @@ int nvme_init_identify(struct nvme_ctrl *ctrl) ret =nvme_configure_apst (ctrl);if (ret <0 )return ret; - + ret =nvme_configure_timestamp (ctrl);if (ret <0 )return ret;
@@ -2454,6 +2723,10 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)if (ret <0 )return ret; + ret =nvme_configure_ana (ctrl); +if (ret <0 ) +return ret; + ctrl->identified = true;return 0 ;
@@ -2649,12 +2922,30 @@ static ssize_t nsid_show(struct device *dev, struct device_attribute *attr, }static DEVICE_ATTR_RO (nsid); +static ssize_t ana_grpid_show (struct device *dev,struct device_attribute *attr, +char *buf) +{ +return sprintf (buf,"%d\n" ,nvme_get_ns_from_dev (dev)->anagrpid); +} +DEVICE_ATTR_RO (ana_grpid); + +static ssize_t ana_state_show (struct device *dev,struct device_attribute *attr, +char *buf) +{ +struct nvme_ns *ns =nvme_get_ns_from_dev (dev); + +return sprintf (buf,"%s\n" , nvme_ana_state_names[ns->ana_state]); +} +DEVICE_ATTR_RO (ana_state); +static struct attribute *nvme_ns_id_attrs[] = { &dev_attr_wwid.attr, &dev_attr_uuid.attr, &dev_attr_nguid.attr, &dev_attr_eui.attr, &dev_attr_nsid.attr, + &dev_attr_ana_grpid.attr, + &dev_attr_ana_state.attr,NULL , };
@@ -2663,6 +2954,7 @@ static umode_t nvme_ns_id_attrs_are_visible(struct kobject *kobj, { struct device *dev = container_of(kobj, struct device, kobj);struct nvme_ns_ids *ids = &dev_to_ns_head (dev)->ids; +struct nvme_ns *ns =nvme_get_ns_from_dev (dev);if (a == &dev_attr_uuid.attr) { if (uuid_is_null(&ids->uuid) &&
@@ -2677,6 +2969,10 @@ static umode_t nvme_ns_id_attrs_are_visible(struct kobject *kobj,if (!memchr_inv (ids->eui64,0 ,sizeof (ids->eui64)))return 0 ; } +if (a == &dev_attr_ana_grpid.attr || a == &dev_attr_ana_state.attr) { +if (!ns->anagrpid) +return 0 ; + }return a->mode; }
@@ -2939,25 +3235,6 @@ static int ns_cmp(void *priv, struct list_head *a, struct list_head *b)return nsa->head->ns_id - nsb->head->ns_id; } -static struct nvme_ns *nvme_find_get_ns (struct nvme_ctrl *ctrl,unsigned nsid) -{ -struct nvme_ns *ns, *ret =NULL ; - -down_read (&ctrl->namespaces_rwsem); -list_for_each_entry (ns, &ctrl->namespaces, list) { -if (ns->head->ns_id == nsid) { -if (!kref_get_unless_zero (&ns->kref)) -continue ; - ret = ns; -break ; - } -if (ns->head->ns_id > nsid) -break ; - } -up_read (&ctrl->namespaces_rwsem); -return ret; -} -static int nvme_setup_streams_ns (struct nvme_ctrl *ctrl,struct nvme_ns *ns) {struct streams_directive_params s;
@@ -2991,6 +3268,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)struct nvme_id_ns *id;char disk_name[DISK_NAME_LEN ];int node =dev_to_node (ctrl->dev), flags =GENHD_FL_EXT_DEVT ; +unsigned int grpid; ns =kzalloc_node (sizeof (*ns),GFP_KERNEL , node);if (!ns)
@@ -3002,6 +3280,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)blk_queue_flag_set (QUEUE_FLAG_NONROT , ns->queue); ns->queue->queuedata = ns; ns->ctrl = ctrl; + ns->ana_state =NVME_ANA_OPTIMIZED ;kref_init (&ns->kref); ns->lba_shift =9 ;/* set to a default value for 512 until disk is validated */
@@ -3019,7 +3298,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)if (nvme_init_ns_head (ns, nsid, id)) goto out_free_id;nvme_setup_streams_ns (ctrl, ns); - +#ifdef CONFIG_NVME_MULTIPATH /* * If multipathing is enabled we need to always use the subsystem
@@ -3063,6 +3342,10 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)memcpy (disk->disk_name, disk_name,DISK_NAME_LEN ); ns->disk = disk; + grpid =le32_to_cpu (id->anagrpid); +if (grpid) + ns->ana_state =nvme_get_ana_state (ctrl, grpid, nsid); +__nvme_revalidate_disk (disk, id);down_write (&ctrl->namespaces_rwsem);
@@ -3371,6 +3654,12 @@ static void nvme_handle_aen_notice(struct nvme_ctrl *ctrl, u32 result)case NVME_AER_NOTICE_FW_ACT_STARTING :queue_work (nvme_wq, &ctrl->fw_act_work);break ; +case NVME_AER_NOTICE_ANA : +if (WARN_ON_ONCE (!ctrl->ana_log_buf)) +break ; +queue_delayed_work (nvme_wq, &ctrl->ana_work, +msecs_to_jiffies (ana_log_delay)); +break ;default :dev_warn (ctrl->device,"async event result %08x\n" , result); }
@@ -3406,6 +3695,7 @@ void nvme_stop_ctrl(struct nvme_ctrl *ctrl)nvme_stop_keep_alive (ctrl);flush_work (&ctrl->async_event_work);flush_work (&ctrl->scan_work); +cancel_delayed_work_sync (&ctrl->ana_work);cancel_work_sync (&ctrl->fw_act_work);if (ctrl->ops->stop_ctrl) ctrl->ops->stop_ctrl (ctrl);
@@ -3440,6 +3730,7 @@ static void nvme_free_ctrl(struct device *dev)ida_simple_remove (&nvme_instance_ida, ctrl->instance);kfree (ctrl->effects); +nvme_deconfigure_ana (ctrl);if (subsys) {mutex_lock (&subsys->lock);
drivers/nvme/host/multipath.c +36 -5
63 lines not on this page · raw message
drivers/nvme/host/nvme.h +17 -0
@@ -169,6 +169,7 @@ struct nvme_ctrl {u16 oacs;u16 nssa;u16 nr_streams; +u32 max_namespaces;atomic_t abort_limit;u8 vwc;u32 vs;
@@ -190,6 +191,15 @@ struct nvme_ctrl {struct nvme_command ka_cmd;struct work_struct fw_act_work; +/* asymmetric namespace access: */ +u8 anacap; +u8 anatt; +u32 anagrpmax; +u32 nanagrpid; +size_t ana_log_size; +struct nvme_ana_rsp_hdr *ana_log_buf; +struct delayed_work ana_work; +/* Power saving configuration */ u64 ps_max_latency_us;bool apst_enabled;
@@ -295,6 +305,8 @@ struct nvme_ns {#define NVME_NS_REMOVING 0#define NVME_NS_DEAD 1u16 noiob; +u32 anagrpid; +enum nvme_ana_state ana_state;#ifdef CONFIG_FAULT_INJECTION_DEBUG_FS struct nvme_fault_inject fault_inject;
@@ -437,6 +449,11 @@ int nvme_get_log_ext(struct nvme_ctrl *ctrl, struct nvme_ns *ns,extern const struct attribute_group nvme_ns_id_attr_group;extern const struct block_device_operations nvme_ns_head_ops; +static inline bool nvme_ctrl_has_ana (struct nvme_ctrl *ctrl) +{ +return ctrl->subsys->cmic & (1 <<3 ); +} +#ifdef CONFIG_NVME_MULTIPATH void nvme_failover_req (struct request *req);bool nvme_req_needs_failover (struct request *req,blk_status_t error);
Signature · 1 line
2.12.3
PATCH v2 8/11 nvme: add ANA support
Thanks for addressing our comments. Overall looks good and have few comments as below:
1) I guess it should be bit 6 not bit 7.
- if (ctrl->anacap & (1 << 7))
+if (ctrl->anacap & (1 << 6))
2) If you resubmit because ana_xfer_size is restricted by mdts, you need to get nvme_ana_rsp_hdr at last and revalidate chgcnt. Otherwise the log you read might be corrupt.
3) NVME_SC_ANA_TRANSITION used instead of NVME_ANA_CHANGE in nvme_process_ana_log (two places)
4) Can you add info level debug for ANA state changes so its helpful for debugging.
Regards,
~Sriram
-----Original Message-----
From: Linux-nvme <[email protected]> On Behalf Of Hannes Reinecke
Sent: Tuesday, May 22, 2018 2:40 PM
To: Keith Busch <keith.busch at intel.com>
Cc: Hannes Reinecke <hare at suse.com>; Johannes Thumshirn <jth at kernel.org>; Sagi Grimberg <sagi at grimberg.me>; linux-nvme at lists.infradead.org; Christoph Hellwig <hch at lst.de>
Subject: [PATCHv2 08/11] nvme: add ANA support
From: Christoph Hellwig <[email protected]>
Adding sypport for Asymmetric Namespace Access (ANA)
--- drivers/nvme/host/core.c | 335 +++++++++++++++++++++++++++++++++++++++--- drivers/nvme/host/multipath.c | 41 +++++- drivers/nvme/host/nvme.h | 17 +++ 3 files changed, 366 insertions(+), 27 deletions(-)
drivers/nvme/host/core.c +4 -0
@@ -68,6 +68,10 @@ static bool streams;module_param (streams, bool,0644 );MODULE_PARM_DESC (streams,"turn on support for Streams write directives" ); +static unsigned long ana_log_delay =500 ;module_param (ana_log_delay, +ulong, 0644); MODULE_PARM_DESC(ana_log_delay, "Delay in msecs before +retrieving ANA log"); +/* * nvme_wq - hosts nvme related works that are not reset or delete * nvme_reset_wq - hosts nvme reset works @@ -1246,6 +1250,25 @@ static void nvme_put_ns_from_disk(struct nvme_ns_head *head, int idx)
srcu_read_unlock(&head->srcu, idx);
}
+static struct nvme_ns *nvme_find_get_ns(struct nvme_ctrl *ctrl,
+unsigned nsid) {
+ struct nvme_ns *ns, *ret = NULL;
+
+ down_read(&ctrl->namespaces_rwsem);
+ list_for_each_entry(ns, &ctrl->namespaces, list) {
+ if (ns->head->ns_id == nsid) {
+ if (!kref_get_unless_zero(&ns->kref))
+ continue;
+ ret = ns;
+ break;
+ }
+ if (ns->head->ns_id > nsid)
+ break;
+ }
+ up_read(&ctrl->namespaces_rwsem);
+ return ret;
+}
+
static int nvme_ns_ioctl(struct nvme_ns *ns, unsigned cmd, unsigned long arg) {
switch (cmd) {
@@ -1357,6 +1380,228 @@ static void nvme_init_integrity(struct gendisk *disk, u16 ms, u8 pi_type) } #endif /* CONFIG_BLK_DEV_INTEGRITY */
+static const char *nvme_ana_state_names[] = {
+ [0] = "invalid state",
+ [NVME_ANA_OPTIMIZED] = "optimized",
+ [NVME_ANA_NONOPTIMIZED] = "non-optimized",
+ [NVME_ANA_INACCESSIBLE] = "inaccessible",
+ [NVME_ANA_PERSISTENT_LOSS] = "persistent-loss",
+ [NVME_ANA_CHANGE] = "change",
+};
+
+static int nvme_process_ana_log(struct nvme_ctrl *ctrl, bool
+groups_only) {
+ void *base = ctrl->ana_log_buf;
+ size_t offset = sizeof(struct nvme_ana_rsp_hdr);
+ int error, i, j;
+ size_t mdts = ctrl->max_hw_sectors << 9;
+ size_t ana_log_size = ctrl->ana_log_size, ana_xfer_size;
+ off_t ana_log_offset = 0;
+
+ /*
+ * If anagrpid never changes we don't need to process the namespace
+ * lists.
+ */
+ if (ctrl->anacap & (1 << 7))
+ groups_only = true;
+
+ if (groups_only)
+ ana_log_size = sizeof(struct nvme_ana_rsp_hdr) +
+ ctrl->nanagrpid * sizeof(struct nvme_ana_group_desc);
+
+ ana_xfer_size = ana_log_size;
+ if (mdts && ana_log_size > mdts) {
+ dev_warn(ctrl->device,
+ "ANA log too large (%lu, max %zu), truncating\n",
+ ana_log_size, mdts);
+ ana_xfer_size = mdts;
+ }
+resubmit:
+ error = nvme_get_log_ext(ctrl, NULL, NVME_LOG_ANA,
+ groups_only ? NVME_ANA_LOG_RGO : 0,
+ ctrl->ana_log_buf + ana_log_offset,
+ ana_xfer_size, ana_log_offset);
+ if (error) {
+ dev_warn(ctrl->device, "Failed to get ANA log: %d\n", error);
+ return -EIO;
+ }
+ ana_log_offset += ana_xfer_size;
+ if (ana_log_size > ana_log_offset) {
+ if (ana_log_size - ana_log_offset < mdts)
+ ana_xfer_size = ana_log_size - ana_log_offset;
+ goto resubmit;
+ }
+
+ for (i = 0; i < le16_to_cpu(ctrl->ana_log_buf->ngrps); i++) {
+ struct nvme_ana_group_desc *desc = base + offset;
+ u32 grpid;
+ u32 nr_nsids;
+ size_t nsid_buf_size;
+ struct nvme_ns *ns;
+
+ if (WARN_ON_ONCE(offset > ana_log_size - sizeof(*desc)))
+ return -EINVAL;
+ grpid = le32_to_cpu(desc->grpid);
+ if (WARN_ON_ONCE(grpid == 0))
+ return -EINVAL;
+ if (WARN_ON_ONCE(grpid > ctrl->anagrpmax))
+ return -EINVAL;
+ if (WARN_ON_ONCE(desc->state == 0))
+ return -EINVAL;
+ if (WARN_ON_ONCE(desc->state > NVME_ANA_CHANGE))
+ return -EINVAL;
+
+ offset += sizeof(*desc);
+ if (groups_only) {
+ down_write(&ctrl->namespaces_rwsem);
+ list_for_each_entry(ns, &ctrl->namespaces, list) {
+ if (ns->anagrpid != grpid)
+ continue;
+ if (desc->state == NVME_SC_ANA_TRANSITION)
+ error = -EAGAIN;
+ if (ns->ana_state != desc->state) {
+ ns->ana_state = desc->state;
+ nvme_mpath_clear_current_path(ns);
+ }
+ break;
+ }
+ up_write(&ctrl->namespaces_rwsem);
+ continue;
+ }
+ nr_nsids = le32_to_cpu(desc->nnsids);
+ if (!nr_nsids)
+ continue;
+
+ nsid_buf_size = nr_nsids * sizeof(__le32);
+ if (WARN_ON_ONCE(offset > ana_log_size - nsid_buf_size))
+ return -EINVAL;
+
+ for (j = 0; j < nr_nsids; j++) {
+ u32 nsid = le32_to_cpu(desc->nsids[j]);
+
+ ns = nvme_find_get_ns(ctrl, nsid);
+ if (!ns)
+ continue;
+
+ if (ns->anagrpid != grpid ||
+ ns->ana_state != desc->state) {
+ ns->anagrpid = grpid;
+ ns->ana_state = desc->state;
+ nvme_mpath_clear_current_path(ns);
+ if (desc->state == NVME_SC_ANA_TRANSITION)
+ error = -EAGAIN;
+ }
+ nvme_put_ns(ns);
+ }
+
+ offset += nsid_buf_size;
+ }
+
+ return error;
+}
+
+static enum nvme_ana_state nvme_get_ana_state(struct nvme_ctrl *ctrl,
+ unsigned int grpid,
+ unsigned int nsid)
+{
+ void *base = ctrl->ana_log_buf;
+ size_t offset = sizeof(struct nvme_ana_rsp_hdr);
+ int i, j;
+
+ if (!ctrl->ana_log_buf)
+ return NVME_ANA_OPTIMIZED;
+
+ for (i = 0; i < le16_to_cpu(ctrl->ana_log_buf->ngrps); i++) {
+ struct nvme_ana_group_desc *desc = base + offset;
+ u32 nr_nsids;
+ size_t nsid_buf_size;
+
+ if (offset > ctrl->ana_log_size - sizeof(*desc))
+ break;
+
+ offset += sizeof(*desc);
+ nr_nsids = le32_to_cpu(desc->nnsids);
+ if (!nr_nsids)
+ continue;
+
+ nsid_buf_size = nr_nsids * sizeof(__le32);
+ if (offset > ctrl->ana_log_size - nsid_buf_size)
+ break;
+
+ if (grpid == le32_to_cpu(desc->grpid)) {
+ for (j = 0; j < nr_nsids; j++) {
+ if (le32_to_cpu(desc->nsids[j]) != nsid)
+ continue;
+
+ dev_info(ctrl->device,
+ "nsid %d: ANA group %d, %s.\n",
+ nsid, grpid,
+ nvme_ana_state_names[desc->state]);
+ return desc->state;
+ }
+ }
+ offset += nsid_buf_size;
+ }
+
+ return NVME_ANA_OPTIMIZED;
+}
+
+static void nvme_ana_work(struct work_struct *work) {
+ struct nvme_ctrl *ctrl = container_of(work, struct nvme_ctrl,
+ ana_work.work);
+ int ret;
+
+ if (!ctrl->ana_log_buf)
+ return;
+ if (ctrl->state != NVME_CTRL_LIVE)
+ return;
+ ret = nvme_process_ana_log(ctrl, false);
+ if (ret == -EAGAIN || ret == -EIO) {
+ unsigned long log_delay = ctrl->anatt * HZ;
+
+ if (ret == -EIO)
+ log_delay = msecs_to_jiffies(ana_log_delay);
+ queue_delayed_work(nvme_wq, &ctrl->ana_work, log_delay);
+ }
+ nvme_kick_requeue_lists(ctrl);
+}
+
+int nvme_configure_ana(struct nvme_ctrl *ctrl) {
+ int error;
+
+ if (!nvme_ctrl_has_ana(ctrl))
+ return 0;
+
+ INIT_DELAYED_WORK(&ctrl->ana_work, nvme_ana_work);
+ ctrl->ana_log_size = sizeof(struct nvme_ana_rsp_hdr) +
+ ctrl->nanagrpid * sizeof(struct nvme_ana_group_desc) +
+ ctrl->max_namespaces * sizeof(__le32);
+ ctrl->ana_log_buf = kzalloc(ctrl->ana_log_size, GFP_KERNEL);
+ if (!ctrl->ana_log_buf)
+ return -ENOMEM;
+
+ error = nvme_process_ana_log(ctrl, false);
+ if (error) {
+ if (error != -EAGAIN && error != -EIO) {
+ kfree(ctrl->ana_log_buf);
+ ctrl->ana_log_buf = NULL;
+ } else
+ error = 0;
+ }
+ return error;
+}
+
+void nvme_deconfigure_ana(struct nvme_ctrl *ctrl) {
+ if (ctrl->ana_log_buf) {
+ cancel_delayed_work_sync(&ctrl->ana_work);
+ kfree(ctrl->ana_log_buf);
+ ctrl->ana_log_buf = NULL;
+ }
+}
+
static void nvme_set_chunk_size(struct nvme_ns *ns) {
u32 chunk_size = (((u32)ns->noiob) << (ns->lba_shift - 9)); @@ -1446,7 +1691,9 @@ static void nvme_update_disk_info(struct gendisk *disk, if (ns->ms && !nvme_ns_has_pi(ns) && !blk_get_integrity(disk))
capacity = 0;
- set_capacity(disk, capacity);
+ if (ns->ana_state == NVME_ANA_OPTIMIZED ||
+ ns->ana_state == NVME_ANA_NONOPTIMIZED)
+ set_capacity(disk, capacity);
nvme_config_discard(ns);
blk_mq_unfreeze_queue(disk->queue);
}
@@ -1462,6 +1709,7 @@ static void __nvme_revalidate_disk(struct gendisk *disk, struct nvme_id_ns *id)
ns->lba_shift = id->lbaf[id->flbas & NVME_NS_FLBAS_LBA_MASK].ds;
if (ns->lba_shift == 0)
ns->lba_shift = 9;
+ ns->anagrpid = le32_to_cpu(id->anagrpid);
ns->noiob = le16_to_cpu(id->noiob);
ns->ext = ns->ms && (id->flbas & NVME_NS_FLBAS_META_EXT);
ns->ms = le16_to_cpu(id->lbaf[id->flbas & NVME_NS_FLBAS_LBA_MASK].ms); @@ -1488,6 +1736,7 @@ static int nvme_revalidate_disk(struct gendisk *disk) struct nvme_ctrl *ctrl = ns->ctrl;
struct nvme_id_ns *id;
struct nvme_ns_ids ids;
+ unsigned int grpid;
int ret = 0;
if (test_bit(NVME_NS_DEAD, &ns->flags)) { @@ -1504,6 +1753,19 @@ static int nvme_revalidate_disk(struct gendisk *disk)
goto out;
}
+ grpid = le32_to_cpu(id->anagrpid);
+ if (grpid) {
+ if (ns->anagrpid != grpid) {
+ dev_warn(ctrl->device, "ns %x ANA group ID changed\n",
+ ns->head->ns_id);
+ ns->anagrpid = grpid;
+ queue_delayed_work(nvme_wq, &ctrl->ana_work,
+ msecs_to_jiffies(ana_log_delay));
+ }
+ ns->ana_state = nvme_get_ana_state(ctrl, grpid,
+ ns->head->ns_id);
+ }
+
__nvme_revalidate_disk(disk, id);
nvme_report_ns_ids(ctrl, ns->head->ns_id, id, &ids);
if (!nvme_ns_ids_equal(&ns->head->ids, &ids)) { @@ -2363,6 +2625,8 @@ int nvme_init_identify(struct nvme_ctrl *ctrl) ctrl->oncs = le16_to_cpup(&id->oncs);
ctrl->aen_cfg = le32_to_cpu(id->oaes) &
(NVME_AEN_CFG_NS_ATTR | NVME_AEN_CFG_FW_ACT);
+ if (nvme_ctrl_has_ana(ctrl))
+ ctrl->aen_cfg |= NVME_AEN_CFG_ANA_CHANGE;
atomic_set(&ctrl->abort_limit, id->acl + 1);
ctrl->vwc = id->vwc;
ctrl->cntlid = le16_to_cpup(&id->cntlid); @@ -2376,6 +2640,11 @@ int nvme_init_identify(struct nvme_ctrl *ctrl) nvme_set_queue_limits(ctrl, ctrl->admin_q);
ctrl->sgls = le32_to_cpu(id->sgls);
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);
if (id->rtd3e) {
/* us -> s */
@@ -2445,7 +2714,7 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
ret = nvme_configure_apst(ctrl);
if (ret < 0)
return ret;
-
+
ret = nvme_configure_timestamp(ctrl);
if (ret < 0)
return ret;
@@ -2454,6 +2723,10 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
if (ret < 0)
return ret;
+ ret = nvme_configure_ana(ctrl);
+ if (ret < 0)
+ return ret;
+
ctrl->identified = true;
return 0;
@@ -2649,12 +2922,30 @@ static ssize_t nsid_show(struct device *dev, struct device_attribute *attr, } static DEVICE_ATTR_RO(nsid);
+static ssize_t ana_grpid_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ return sprintf(buf, "%d\n", nvme_get_ns_from_dev(dev)->anagrpid);
+}
+DEVICE_ATTR_RO(ana_grpid);
+
+static ssize_t ana_state_show(struct device *dev, struct device_attribute *attr,
+ char *buf)
+{
+ struct nvme_ns *ns = nvme_get_ns_from_dev(dev);
+
+ return sprintf(buf, "%s\n", nvme_ana_state_names[ns->ana_state]);
+}
+DEVICE_ATTR_RO(ana_state);
+
static struct attribute *nvme_ns_id_attrs[] = {
&dev_attr_wwid.attr,
&dev_attr_uuid.attr,
&dev_attr_nguid.attr,
&dev_attr_eui.attr,
&dev_attr_nsid.attr,
+ &dev_attr_ana_grpid.attr,
+ &dev_attr_ana_state.attr,
NULL,
};
@@ -2663,6 +2954,7 @@ static umode_t nvme_ns_id_attrs_are_visible(struct kobject *kobj, {
struct device *dev = container_of(kobj, struct device, kobj); struct nvme_ns_ids *ids = &dev_to_ns_head(dev)->ids;
+ struct nvme_ns *ns = nvme_get_ns_from_dev(dev);
if (a == &dev_attr_uuid.attr) {
if (uuid_is_null(&ids->uuid) &&
@@ -2677,6 +2969,10 @@ static umode_t nvme_ns_id_attrs_are_visible(struct kobject *kobj,
if (!memchr_inv(ids->eui64, 0, sizeof(ids->eui64)))
return 0;
}
+ if (a == &dev_attr_ana_grpid.attr || a == &dev_attr_ana_state.attr) {
+ if (!ns->anagrpid)
+ return 0;
+ }
return a->mode;
}
@@ -2939,25 +3235,6 @@ static int ns_cmp(void *priv, struct list_head *a, struct list_head *b)
return nsa->head->ns_id - nsb->head->ns_id; }
-static struct nvme_ns *nvme_find_get_ns(struct nvme_ctrl *ctrl, unsigned nsid) -{
- struct nvme_ns *ns, *ret = NULL;
-
- down_read(&ctrl->namespaces_rwsem);
- list_for_each_entry(ns, &ctrl->namespaces, list) {
- if (ns->head->ns_id == nsid) {
- if (!kref_get_unless_zero(&ns->kref))
- continue;
- ret = ns;
- break;
- }
- if (ns->head->ns_id > nsid)
- break;
- }
- up_read(&ctrl->namespaces_rwsem);
- return ret;
-}
-
static int nvme_setup_streams_ns(struct nvme_ctrl *ctrl, struct nvme_ns *ns) {
struct streams_directive_params s;
@@ -2991,6 +3268,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
struct nvme_id_ns *id;
char disk_name[DISK_NAME_LEN];
int node = dev_to_node(ctrl->dev), flags = GENHD_FL_EXT_DEVT;
+ unsigned int grpid;
ns = kzalloc_node(sizeof(*ns), GFP_KERNEL, node);
if (!ns)
@@ -3002,6 +3280,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
blk_queue_flag_set(QUEUE_FLAG_NONROT, ns->queue); ns->queue->queuedata = ns;
ns->ctrl = ctrl;
+ ns->ana_state = NVME_ANA_OPTIMIZED;
kref_init(&ns->kref);
ns->lba_shift = 9; /* set to a default value for 512 until disk is validated */ @@ -3019,7 +3298,7 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid) if (nvme_init_ns_head(ns, nsid, id))
goto out_free_id;
nvme_setup_streams_ns(ctrl, ns);
-
+
#ifdef CONFIG_NVME_MULTIPATH
/*
* If multipathing is enabled we need to always use the subsystem @@ -3063,6 +3342,10 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
memcpy(disk->disk_name, disk_name, DISK_NAME_LEN);
ns->disk = disk;
+ grpid = le32_to_cpu(id->anagrpid);
+ if (grpid)
+ ns->ana_state = nvme_get_ana_state(ctrl, grpid, nsid);
+
__nvme_revalidate_disk(disk, id);
down_write(&ctrl->namespaces_rwsem);
@@ -3371,6 +3654,12 @@ static void nvme_handle_aen_notice(struct nvme_ctrl *ctrl, u32 result)
case NVME_AER_NOTICE_FW_ACT_STARTING:
queue_work(nvme_wq, &ctrl->fw_act_work);
break;
+ case NVME_AER_NOTICE_ANA:
+ if (WARN_ON_ONCE(!ctrl->ana_log_buf))
+ break;
+ queue_delayed_work(nvme_wq, &ctrl->ana_work,
+ msecs_to_jiffies(ana_log_delay));
+ break;
default:
dev_warn(ctrl->device, "async event result %08x\n", result);
}
@@ -3406,6 +3695,7 @@ void nvme_stop_ctrl(struct nvme_ctrl *ctrl)
nvme_stop_keep_alive(ctrl);
flush_work(&ctrl->async_event_work);
flush_work(&ctrl->scan_work);
+ cancel_delayed_work_sync(&ctrl->ana_work);
cancel_work_sync(&ctrl->fw_act_work);
if (ctrl->ops->stop_ctrl)
ctrl->ops->stop_ctrl(ctrl);
@@ -3440,6 +3730,7 @@ static void nvme_free_ctrl(struct device *dev)
ida_simple_remove(&nvme_instance_ida, ctrl->instance); kfree(ctrl->effects);
+ nvme_deconfigure_ana(ctrl);
if (subsys) {
mutex_lock(&subsys->lock);
drivers/nvme/host/multipath.c +36 -5
62 lines not on this page · raw message
drivers/nvme/host/nvme.h +12 -0
@@ -169,6 +169,7 @@ struct nvme_ctrl {u16 oacs;u16 nssa;u16 nr_streams; +u32 max_namespaces;atomic_t abort_limit;u8 vwc;u32 vs;
@@ -190,6 +191,15 @@ struct nvme_ctrl {struct nvme_command ka_cmd;struct work_struct fw_act_work; +/* asymmetric namespace access: */ +u8 anacap; +u8 anatt; +u32 anagrpmax; +u32 nanagrpid; +size_t ana_log_size; +struct nvme_ana_rsp_hdr *ana_log_buf; +struct delayed_work ana_work; +/* Power saving configuration */ u64 ps_max_latency_us;bool apst_enabled;
@@ -295,6 +305,8 @@ struct nvme_ns {#define NVME_NS_REMOVING 0#define NVME_NS_DEAD 1u16 noiob; +u32 anagrpid; +enum nvme_ana_state ana_state;#ifdef CONFIG_FAULT_INJECTION_DEBUG_FS struct nvme_fault_inject fault_inject; @@ -437,6 +449,11 @@ int nvme_get_log_ext(struct nvme_ctrl *ctrl, struct nvme_ns *ns, extern const struct attribute_group nvme_ns_id_attr_group; extern const struct block_device_operations nvme_ns_head_ops;
+static inline bool nvme_ctrl_has_ana(struct nvme_ctrl *ctrl) {
+ return ctrl->subsys->cmic & (1 << 3);
+}
+
#ifdef CONFIG_NVME_MULTIPATH
void nvme_failover_req(struct request *req); bool nvme_req_needs_failover(struct request *req, blk_status_t error);
--
2.12.3
_______________________________________________
Linux-nvme mailing list
Linux-nvme at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
PATCH v2 8/11 nvme: add ANA support
Quoted 5 lines · Sriram
Thanks for addressing our comments. Overall looks good and have few comments as below:
1) I guess it should be bit 6 not bit 7.
- if (ctrl->anacap& (1 << 7))
+if (ctrl->anacap& (1 << 6))
Ah. Correct.
Will be fixing it up.
Quoted 2 lines
2) If you resubmit because
ana_xfer_sizeis restricted by mdts, you need to getnvme_ana_rsp_hdrat last and revalidate chgcnt. Otherwise the log you read might be corrupt.
Indeed. This whole mdts stuff is pretty raw, so I'm not surprised that we're getting errors here.
Maybe it's worthwhile moving it into the generic function; let's see.
Quoted 1 line
3)
NVME_SC_ANA_TRANSITIONused instead ofNVME_ANA_CHANGEinnvme_process_ana_log(two places)
Did I? That would've been wrong indeed. Will be fixing it up.
Quoted 2 lines
4) Can you add info level debug for ANA state changes so its helpful for debugging.
Thought about it myself (and actually did so during development), but then decided against it as the entire nvme stack is pretty silent. Probably I'll make it a conditional logging entry here.
Thanks for the review.
Cheers,
Hannes
Christoph Hellwig
maintainerPATCH v2 8/11 nvme: add ANA support
In reply to Popuri, SriramQuoted 6 lines · Sriram
Thanks for addressing our comments. Overall looks good and have few comments as below:
1) I guess it should be bit 6 not bit 7.
- if (ctrl->anacap& (1 << 7))
+if (ctrl->anacap& (1 << 6))
2) If you resubmit becauseana_xfer_sizeis restricted by mdts, you need to getnvme_ana_rsp_hdrat last and revalidate chgcnt. Otherwise the log you read might be corrupt.
We can sensibly chunk the ANA log page anyway. So if a controller has NANAGRPID/MNAN config where we can't sensible read it in a single operation we better reject the ANA support as buggy early on.
Quoted 3 lines
3)
NVME_SC_ANA_TRANSITIONused instead ofNVME_ANA_CHANGEinnvme_process_ana_log(two places)4) Can you add info level debug for ANA state changes so its helpful for debugging.
My original code had, and Hannes code has it at least in some places.
Christoph Hellwig
maintainerPATCH v2 8/11 nvme: add ANA support
In reply to Hannes ReineckeQuoted 6 lines · Hannes Reinecke
From: Christoph Hellwig <hch at lst.de>
Adding sypport for Asymmetric Namespace Access (ANA)
Signed-off-by Christoph Hellwig <hch at lst.de>Signed-off-by Hannes Reinecke <hare at suse.com>
I just did a quick diff, and this really doesn't look like my old code. Quick difference I spotted:
- lots of code in core.c instead of multipath.c, where it should be
- adds a per-ns ana_state value, including all the crap to main it instead of the simple per-controller state table
- fucks of the sysfs attributes to also who on the multipath node, where they can't work, and probably even corrupt memory
- adds a get_log_page call per namespace, which is completely counter to the ANA design to not have craptons of roundtrip
And probably a few more things I didn't notice due to all the code moves.
Quoted 11 lines
--- drivers/nvme/host/core.c | 335 +++++++++++++++++++++++++++++++++++++++--- drivers/nvme/host/multipath.c | 41 +++++- drivers/nvme/host/nvme.h | 17 +++ 3 files changed, 366 insertions(+), 27 deletions(-)drivers/nvme/host/core.c +0
-0
drivers/nvme/host/core.c static bool streams; · line 73
Quoted 6 lines
drivers/nvme/host/core.c +3
-0@@ -68,6 +68,10 @@ static bool streams;module_param (streams, bool,0644 );MODULE_PARM_DESC (streams,"turn on support for Streams write directives" ); +static unsigned long ana_log_delay =500 ; +module_param (ana_log_delay, ulong,0644 ); +MODULE_PARM_DESC (ana_log_delay,"Delay in msecs before retrieving ANA log" );
Why? There really is no point in delaying reading the state information and just messing things up.
PATCH v2 8/11 nvme: add ANA support
Quoted 21 lines · On Fri, 25 May 2018 15:28:40 +0200 Christoph Hellwig <[email protected]> wrote:
Quoted 6 lines · Hannes Reinecke
From: Christoph Hellwig <hch at lst.de>
Adding sypport for Asymmetric Namespace Access (ANA)
Signed-off-by Christoph Hellwig <hch at lst.de>Signed-off-by Hannes Reinecke <hare at suse.com>I just did a quick diff, and this really doesn't look like my old code. Quick difference I spotted:
- lots of code in
core.cinstead ofmultipath.c, where it should be
- adds a per-nsana_statevalue, including all the crap to main it instead of the simple per-controller state table
- fucks of the sysfs attributes to also who on the multipath node, where they can't work, and probably even corrupt memory
- adds aget_log_pagecall per namespace, which is completely counter to the ANA design to not have craptons of roundtrip
Quoted 3 lines
And probably a few more things I didn't notice due to all the code moves.
Okay, will be moving back to storing the ana state in the per-controller buffer.
Quoted 7 lines
Quoted 7 lines
--- drivers/nvme/host/core.c | 335 +++++++++++++++++++++++++++++++++++++++--- drivers/nvme/host/multipath.c | 41 +++++- drivers/nvme/host/nvme.h | 17 +++ 3 files changed, 366insertions(+), 27 deletions(-)
Quoted 15 lines
drivers/nvme/host/core.c static bool streams; · line 74
Quoted 12 lines
drivers/nvme/host/core.c +3
-0@@ -68,6 +68,10 @@ static bool streams;module_param (streams, bool,0644 );MODULE_PARM_DESC (streams,"turn on support for Streams write directives"); +static unsigned long ana_log_delay =500 ; +module_param (ana_log_delay, ulong,0644 ); +MODULE_PARM_DESC(ana_log_delay, "Delay in msecs before retrieving ANA log");Why? There really is no point in delaying reading the state information and just messing things up.
This delay will be used if we fail to get the ANA log page; in that case we _should_ be retrying it.
The amount of retries need to be capped (hence ana_log_retries), and we shouldn't retry immediately to give the target (and possibly lower layers) a chance to recover (that's what ana_log_delay is for).
If you don't like having them as module parameters, fine, I can make them as #define or something.
Cheers,
Hannes
Sagi Grimberg
maintainerPATCH v2 8/11 nvme: add ANA support
In reply to Hannes ReineckeQuoted 3 lines
From: Christoph Hellwig <hch at lst.de>
Adding sypport for Asymmetric Namespace Access (ANA)
Would be nice to have a slightly more detailed change log for a substantial addition.
Quoted 27 lines
Signed-off-by Christoph Hellwig <hch at lst.de>Signed-off-by Hannes Reinecke <hare at suse.com>--- drivers/nvme/host/core.c | 335 +++++++++++++++++++++++++++++++++++++++--- drivers/nvme/host/multipath.c | 41 +++++- drivers/nvme/host/nvme.h | 17 +++ 3 files changed, 366 insertions(+), 27 deletions(-)drivers/nvme/host/core.c +4
-0@@ -68,6 +68,10 @@ static bool streams;module_param (streams, bool,0644 );MODULE_PARM_DESC (streams,"turn on support for Streams write directives" ); +static unsigned long ana_log_delay =500 ; +module_param (ana_log_delay, ulong,0644 ); +MODULE_PARM_DESC (ana_log_delay,"Delay in msecs before retrieving ANA log" ); +/* * nvme_wq - hosts nvme related works that are not reset or delete * nvme_reset_wq - hosts nvme reset works @@ -1246,6 +1250,25 @@ static void nvme_put_ns_from_disk(struct nvme_ns_head *head, int idx)srcu_read_unlock (&head->srcu, idx); }
drivers/nvme/host/core.c static void nvme_put_ns_from_disk(struct nvme_ns_head *head, int idx) · line 1261
Quoted 10 lines
drivers/nvme/host/core.c +9
-0+static struct nvme_ns *nvme_find_get_ns (struct nvme_ctrl *ctrl,unsigned nsid) +{ +struct nvme_ns *ns, *ret =NULL ; + +down_read (&ctrl->namespaces_rwsem); +list_for_each_entry (ns, &ctrl->namespaces, list) { +if (ns->head->ns_id == nsid) { +if (!kref_get_unless_zero (&ns->kref)) +continue ;
Why continue?
Quoted 18 lines
drivers/nvme/host/core.c +11
-0+ ret = ns; +break ; + } +if (ns->head->ns_id > nsid) +break ; + } +up_read (&ctrl->namespaces_rwsem); +return ret; +} +static int nvme_ns_ioctl (struct nvme_ns *ns,unsigned cmd,unsigned long arg) {switch (cmd) {@@ -1357,6 +1380,228 @@ static void nvme_init_integrity(struct gendisk *disk, u16 ms, u8 pi_type) }#endif /* CONFIG_BLK_DEV_INTEGRITY */ +static const char *nvme_ana_state_names[] = {
drivers/nvme/host/core.c static void nvme_init_integrity(struct gendisk *disk, u16 ms, u8 pi_type) · line 1393
Quoted 10 lines
drivers/nvme/host/core.c +10
-0+ [0] = "invalid state", + [NVME_ANA_OPTIMIZED] = "optimized", + [NVME_ANA_NONOPTIMIZED] = "non-optimized", + [NVME_ANA_INACCESSIBLE] = "inaccessible", + [NVME_ANA_PERSISTENT_LOSS] = "persistent-loss", + [NVME_ANA_CHANGE] = "change", +}; + +static int nvme_process_ana_log (struct nvme_ctrl *ctrl,bool groups_only) +{
This is always called with groups_only=false - is it really needed?
drivers/nvme/host/core.c static void nvme_init_integrity(struct gendisk *disk, u16 ms, u8 pi_type) · line 1405
Quoted 12 lines
drivers/nvme/host/core.c +12
-0+void *base = ctrl->ana_log_buf; +size_t offset =sizeof (struct nvme_ana_rsp_hdr ); +int error, i, j; +size_t mdts = ctrl->max_hw_sectors <<9 ; +size_t ana_log_size = ctrl->ana_log_size, ana_xfer_size; +off_t ana_log_offset =0 ; + +/* +* If anagrpid never changes we don't need to process the namespace +* lists. +*/ +if (ctrl->anacap & (1 <<7 ))
Shouldn't this be bit 6?
Bit 6 if set to ?1? then the ANAGRPID field in the Identify Namespace data structure (refer to Figure 114) does not change while the namespace is attached to any controller. If cleared to ?0?, then the ANAGRPID field may change while the namespace is attached to any controller. Refer to section 8.18.2.
And can we give this a meaningful name?
Quoted 49 lines
drivers/nvme/host/core.c +49
-0+ groups_only = true; + +if (groups_only) + ana_log_size =sizeof (struct nvme_ana_rsp_hdr ) + + ctrl->nanagrpid *sizeof (struct nvme_ana_group_desc ); + + ana_xfer_size = ana_log_size; +if (mdts && ana_log_size > mdts) { +dev_warn (ctrl->device, +"ANA log too large (%lu, max %zu), truncating\n" , + ana_log_size, mdts); + ana_xfer_size = mdts; + } +resubmit: + error =nvme_get_log_ext (ctrl,NULL ,NVME_LOG_ANA , + groups_only ?NVME_ANA_LOG_RGO :0 , + ctrl->ana_log_buf + ana_log_offset, + ana_xfer_size, ana_log_offset); +if (error) { +dev_warn (ctrl->device,"Failed to get ANA log: %d\n" , error); +return -EIO ; + } + ana_log_offset += ana_xfer_size; +if (ana_log_size > ana_log_offset) { +if (ana_log_size - ana_log_offset < mdts) + ana_xfer_size = ana_log_size - ana_log_offset; + goto resubmit; + } + +for (i =0 ; i <le16_to_cpu (ctrl->ana_log_buf->ngrps); i++) { +struct nvme_ana_group_desc *desc = base + offset; +u32 grpid; +u32 nr_nsids; +size_t nsid_buf_size; +struct nvme_ns *ns; + +if (WARN_ON_ONCE (offset > ana_log_size -sizeof (*desc))) +return -EINVAL ; + grpid =le32_to_cpu (desc->grpid); +if (WARN_ON_ONCE (grpid ==0 )) +return -EINVAL ; +if (WARN_ON_ONCE (grpid > ctrl->anagrpmax)) +return -EINVAL ; +if (WARN_ON_ONCE (desc->state ==0 )) +return -EINVAL ; +if (WARN_ON_ONCE (desc->state >NVME_ANA_CHANGE )) +return -EINVAL ; + + offset +=sizeof (*desc);
drivers/nvme/host/core.c static void nvme_init_integrity(struct gendisk *disk, u16 ms, u8 pi_type) · line 1464
Quoted 10 lines
drivers/nvme/host/core.c +10
-0+if (groups_only) { +down_write (&ctrl->namespaces_rwsem); +list_for_each_entry (ns, &ctrl->namespaces, list) { +if (ns->anagrpid != grpid) +continue ; +if (desc->state ==NVME_SC_ANA_TRANSITION ) + error = -EAGAIN ; +if (ns->ana_state != desc->state) { + ns->ana_state = desc->state; +nvme_mpath_clear_current_path (ns);
Should the path clear unconditionally? even if its not the current path?
drivers/nvme/host/core.c static void nvme_init_integrity(struct gendisk *disk, u16 ms, u8 pi_type) · line 1470
Quoted 6 lines
drivers/nvme/host/core.c +6
-0+ } +break ; + } +up_write (&ctrl->namespaces_rwsem); +continue ; + }
Would be easier to follow if this can be splitted into grpid handling and nsid handling that both receive desc.
Quoted 25 lines
drivers/nvme/host/core.c +25
-0+ nr_nsids =le32_to_cpu (desc->nnsids); +if (!nr_nsids) +continue ; + + nsid_buf_size = nr_nsids *sizeof (__le32); +if (WARN_ON_ONCE (offset > ana_log_size - nsid_buf_size)) +return -EINVAL ; + +for (j =0 ; j < nr_nsids; j++) { +u32 nsid =le32_to_cpu (desc->nsids[j]); + + ns =nvme_find_get_ns (ctrl, nsid); +if (!ns) +continue ; + +if (ns->anagrpid != grpid || + ns->ana_state != desc->state) { + ns->anagrpid = grpid; + ns->ana_state = desc->state; +nvme_mpath_clear_current_path (ns); +if (desc->state ==NVME_SC_ANA_TRANSITION ) + error = -EAGAIN ; + } +nvme_put_ns (ns); + }
drivers/nvme/host/core.c static void nvme_init_integrity(struct gendisk *disk, u16 ms, u8 pi_type) · line 1505
Quoted 10 lines
drivers/nvme/host/core.c +10
-0+ + offset += nsid_buf_size; + } + +return error; +} + +static enum nvme_ana_state nvme_get_ana_state (struct nvme_ctrl *ctrl, +unsigned int grpid, +unsigned int nsid)
any reason why not make grpid and nsid u32?
drivers/nvme/host/core.c static void nvme_init_integrity(struct gendisk *disk, u16 ms, u8 pi_type) · line 1512
Quoted 7 lines
drivers/nvme/host/core.c +7
-0+{ +void *base = ctrl->ana_log_buf; +size_t offset =sizeof (struct nvme_ana_rsp_hdr ); +int i, j; + +if (!ctrl->ana_log_buf) +return NVME_ANA_OPTIMIZED ;
can be goto out (before the last return statement). Also is this a case of this triggers before we allocated the ana_log_buf? because if not maybe better to ask on:
nvme_ctrl_has_ana(ctrl)
Quoted 34 lines
drivers/nvme/host/core.c +34
-0+ +for (i =0 ; i <le16_to_cpu (ctrl->ana_log_buf->ngrps); i++) { +struct nvme_ana_group_desc *desc = base + offset; +u32 nr_nsids; +size_t nsid_buf_size; + +if (offset > ctrl->ana_log_size -sizeof (*desc)) +break ; + + offset +=sizeof (*desc); + nr_nsids =le32_to_cpu (desc->nnsids); +if (!nr_nsids) +continue ; + + nsid_buf_size = nr_nsids *sizeof (__le32); +if (offset > ctrl->ana_log_size - nsid_buf_size) +break ; + +if (grpid ==le32_to_cpu (desc->grpid)) { +for (j =0 ; j < nr_nsids; j++) { +if (le32_to_cpu (desc->nsids[j]) != nsid) +continue ; + +dev_info (ctrl->device, +"nsid %d: ANA group %d, %s.\n" , + nsid, grpid, + nvme_ana_state_names[desc->state]); +return desc->state; + } + } + offset += nsid_buf_size; + } + +return NVME_ANA_OPTIMIZED ;
drivers/nvme/host/core.c static void nvme_init_integrity(struct gendisk *disk, u16 ms, u8 pi_type) · line 1556
Quoted 10 lines
drivers/nvme/host/core.c +10
-0+} + +static void nvme_ana_work (struct work_struct *work) +{ + struct nvme_ctrl *ctrl = container_of(work, struct nvme_ctrl, + ana_work.work); +int ret; + +if (!ctrl->ana_log_buf) +return ;
I think it will be clearer to ask on:
nvme_ctrl_has_ana(ctrl)
Quoted 32 lines
drivers/nvme/host/core.c +32
-0+if (ctrl->state !=NVME_CTRL_LIVE ) +return ; + ret =nvme_process_ana_log (ctrl, false); +if (ret == -EAGAIN || ret == -EIO ) { +unsigned long log_delay = ctrl->anatt *HZ ; + +if (ret == -EIO ) + log_delay =msecs_to_jiffies (ana_log_delay); +queue_delayed_work (nvme_wq, &ctrl->ana_work, log_delay); + } +nvme_kick_requeue_lists (ctrl); +} + +int nvme_configure_ana (struct nvme_ctrl *ctrl) +{ +int error; + +if (!nvme_ctrl_has_ana (ctrl)) +return 0 ; + +INIT_DELAYED_WORK (&ctrl->ana_work, nvme_ana_work); + ctrl->ana_log_size =sizeof (struct nvme_ana_rsp_hdr ) + + ctrl->nanagrpid *sizeof (struct nvme_ana_group_desc ) + + ctrl->max_namespaces *sizeof (__le32); + ctrl->ana_log_buf =kzalloc (ctrl->ana_log_size,GFP_KERNEL ); +if (!ctrl->ana_log_buf) +return -ENOMEM ; + + error =nvme_process_ana_log (ctrl, false); +if (error) { +if (error != -EAGAIN && error != -EIO ) { +kfree (ctrl->ana_log_buf);
drivers/nvme/host/core.c static void nvme_init_integrity(struct gendisk *disk, u16 ms, u8 pi_type) · line 1598
Quoted 10 lines
drivers/nvme/host/core.c +10
-0+ ctrl->ana_log_buf =NULL ; + }else + error =0 ; + } +return error; +} + +void nvme_deconfigure_ana (struct nvme_ctrl *ctrl) +{ +if (ctrl->ana_log_buf) {
if (nvme_ctrl_has_ana(ctrl)) {Quoted 10 lines
drivers/nvme/host/core.c +6
-0+cancel_delayed_work_sync (&ctrl->ana_work); +kfree (ctrl->ana_log_buf); + ctrl->ana_log_buf =NULL ; + } +} +static void nvme_set_chunk_size (struct nvme_ns *ns) {u32 chunk_size = (((u32 )ns->noiob) << (ns->lba_shift -9 ));
drivers/nvme/host/core.c static void nvme_update_disk_info(struct gendisk *disk, · line 1696
Quoted 7 lines
drivers/nvme/host/core.c +3
-1@@ -1446,7 +1691,9 @@ static void nvme_update_disk_info(struct gendisk *disk,if (ns->ms && !nvme_ns_has_pi (ns) && !blk_get_integrity (disk)) capacity =0 ; -set_capacity (disk, capacity); +if (ns->ana_state ==NVME_ANA_OPTIMIZED || + ns->ana_state ==NVME_ANA_NONOPTIMIZED ) +set_capacity (disk, capacity);
Should we really only set capacity for accessible? why is this the case?
Quoted 61 lines
drivers/nvme/host/core.c +22
-0nvme_config_discard (ns);blk_mq_unfreeze_queue (disk->queue); }@@ -1462,6 +1709,7 @@ static void __nvme_revalidate_disk(struct gendisk *disk, struct nvme_id_ns *id) ns->lba_shift = id->lbaf[id->flbas &NVME_NS_FLBAS_LBA_MASK ].ds;if (ns->lba_shift ==0 ) ns->lba_shift =9 ; + ns->anagrpid =le32_to_cpu (id->anagrpid); ns->noiob =le16_to_cpu (id->noiob); ns->ext = ns->ms && (id->flbas &NVME_NS_FLBAS_META_EXT ); ns->ms =le16_to_cpu (id->lbaf[id->flbas &NVME_NS_FLBAS_LBA_MASK ].ms);@@ -1488,6 +1736,7 @@ static int nvme_revalidate_disk(struct gendisk *disk)struct nvme_ctrl *ctrl = ns->ctrl;struct nvme_id_ns *id;struct nvme_ns_ids ids; +unsigned int grpid;int ret =0 ;if (test_bit (NVME_NS_DEAD , &ns->flags)) {@@ -1504,6 +1753,19 @@ static int nvme_revalidate_disk(struct gendisk *disk) goto out; } + grpid =le32_to_cpu (id->anagrpid); +if (grpid) { +if (ns->anagrpid != grpid) { +dev_warn (ctrl->device,"ns %x ANA group ID changed\n" , + ns->head->ns_id); + ns->anagrpid = grpid; +queue_delayed_work (nvme_wq, &ctrl->ana_work, +msecs_to_jiffies (ana_log_delay)); + } + ns->ana_state =nvme_get_ana_state (ctrl, grpid, + ns->head->ns_id); + } +__nvme_revalidate_disk (disk, id);nvme_report_ns_ids (ctrl, ns->head->ns_id, id, &ids);if (!nvme_ns_ids_equal (&ns->head->ids, &ids)) {@@ -2363,6 +2625,8 @@ int nvme_init_identify(struct nvme_ctrl *ctrl) ctrl->oncs =le16_to_cpup (&id->oncs); ctrl->aen_cfg =le32_to_cpu (id->oaes) & (NVME_AEN_CFG_NS_ATTR |NVME_AEN_CFG_FW_ACT ); +if (nvme_ctrl_has_ana (ctrl)) + ctrl->aen_cfg |=NVME_AEN_CFG_ANA_CHANGE ;atomic_set (&ctrl->abort_limit, id->acl +1 ); ctrl->vwc = id->vwc; ctrl->cntlid =le16_to_cpup (&id->cntlid);@@ -2376,6 +2640,11 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)nvme_set_queue_limits (ctrl, ctrl->admin_q); ctrl->sgls =le32_to_cpu (id->sgls); 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);if (id->rtd3e) {/* us -> s */
drivers/nvme/host/core.c int nvme_init_identify(struct nvme_ctrl *ctrl) · line 2717
Quoted 5 lines
drivers/nvme/host/core.c +1
-1@@ -2445,7 +2714,7 @@ int nvme_init_identify(struct nvme_ctrl *ctrl) ret =nvme_configure_apst (ctrl);if (ret <0 )return ret; - +
??
Quoted 76 lines
drivers/nvme/host/core.c +27
-8ret =nvme_configure_timestamp (ctrl);if (ret <0 )return ret;@@ -2454,6 +2723,10 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)if (ret <0 )return ret; + ret =nvme_configure_ana (ctrl); +if (ret <0 ) +return ret; + ctrl->identified = true;return 0 ;@@ -2649,12 +2922,30 @@ static ssize_t nsid_show(struct device *dev, struct device_attribute *attr, }static DEVICE_ATTR_RO (nsid); +static ssize_t ana_grpid_show (struct device *dev,struct device_attribute *attr, +char *buf) +{ +return sprintf (buf,"%d\n" ,nvme_get_ns_from_dev (dev)->anagrpid); +} +DEVICE_ATTR_RO (ana_grpid); + +static ssize_t ana_state_show (struct device *dev,struct device_attribute *attr, +char *buf) +{ +struct nvme_ns *ns =nvme_get_ns_from_dev (dev); + +return sprintf (buf,"%s\n" , nvme_ana_state_names[ns->ana_state]); +} +DEVICE_ATTR_RO (ana_state); +static struct attribute *nvme_ns_id_attrs[] = { &dev_attr_wwid.attr, &dev_attr_uuid.attr, &dev_attr_nguid.attr, &dev_attr_eui.attr, &dev_attr_nsid.attr, + &dev_attr_ana_grpid.attr, + &dev_attr_ana_state.attr,NULL , };@@ -2663,6 +2954,7 @@ static umode_t nvme_ns_id_attrs_are_visible(struct kobject *kobj, { struct device *dev = container_of(kobj, struct device, kobj);struct nvme_ns_ids *ids = &dev_to_ns_head (dev)->ids; +struct nvme_ns *ns =nvme_get_ns_from_dev (dev);if (a == &dev_attr_uuid.attr) { if (uuid_is_null(&ids->uuid) &&@@ -2677,6 +2969,10 @@ static umode_t nvme_ns_id_attrs_are_visible(struct kobject *kobj,if (!memchr_inv (ids->eui64,0 ,sizeof (ids->eui64)))return 0 ; } +if (a == &dev_attr_ana_grpid.attr || a == &dev_attr_ana_state.attr) { +if (!ns->anagrpid) +return 0 ; + }return a->mode; }@@ -2939,25 +3235,6 @@ static int ns_cmp(void *priv, struct list_head *a, struct list_head *b)return nsa->head->ns_id - nsb->head->ns_id; } -static struct nvme_ns *nvme_find_get_ns (struct nvme_ctrl *ctrl,unsigned nsid) -{ -struct nvme_ns *ns, *ret =NULL ; - -down_read (&ctrl->namespaces_rwsem); -list_for_each_entry (ns, &ctrl->namespaces, list) { -if (ns->head->ns_id == nsid) { -if (!kref_get_unless_zero (&ns->kref))
drivers/nvme/host/core.c static int ns_cmp(void *priv, struct list_head *a, struct list_head *b) · line 2959
Quoted 10 lines
drivers/nvme/host/core.c +0
-10-continue ; - ret = ns; -break ; - } -if (ns->head->ns_id > nsid) -break ; - } -up_read (&ctrl->namespaces_rwsem); -return ret; -}
OK, now I see this was copied...
PATCH v2 9/11 nvmet: add a new nvmet_zero_sgl helper
In reply to Hannes ReineckeFrom: Christoph Hellwig <[email protected]>
Zeroes the SGL in the payload.
--- drivers/nvme/target/core.c | 7 +++++++ drivers/nvme/target/nvmet.h | 1 + 2 files changed, 8 insertions(+)
drivers/nvme/target/core.c +7 -0
@@ -57,6 +57,13 @@ u16 nvmet_copy_from_sgl(struct nvmet_req *req, off_t off, void *buf, size_t len)return 0 ; } +u16 nvmet_zero_sgl (struct nvmet_req *req,off_t off,size_t len) +{ +if (sg_zero_buffer (req->sg, req->sg_cnt, len, off) != len) +return NVME_SC_SGL_INVALID_DATA |NVME_SC_DNR ; +return 0 ; +} +static unsigned int nvmet_max_nsid (struct nvmet_subsys *subsys) {struct nvmet_ns *ns;
drivers/nvme/target/nvmet.h +1 -0
@@ -317,6 +317,7 @@ u16 nvmet_copy_to_sgl(struct nvmet_req *req, off_t off, const void *buf, size_t len);u16 nvmet_copy_from_sgl (struct nvmet_req *req,off_t off,void *buf,size_t len); +u16 nvmet_zero_sgl (struct nvmet_req *req,off_t off,size_t len);u32 nvmet_get_log_page_len (struct nvme_command *cmd);
Signature · 1 line
2.12.3
PATCH v2 10/11 nvmet: split log page implementation
In reply to Hannes ReineckeFrom: Christoph Hellwig <[email protected]>
Remove the common code to allocate a buffer and copy it into the SGL. Instead the two no-op implementations just zero the SGL directly, and the smart log allocates a buffer on its own. This prepares for the more elaborate ANA log page.
--- drivers/nvme/target/admin-cmd.c | 101 +++++++++++++++------------------------- 1 file changed, 37 insertions(+), 64 deletions(-)
drivers/nvme/target/admin-cmd.c +37 -64
@@ -32,6 +32,11 @@ u32 nvmet_get_log_page_len(struct nvme_command *cmd)return len; } +static void nvmet_execute_get_log_page_noop (struct nvmet_req *req) +{ +nvmet_req_complete (req,nvmet_zero_sgl (req,0 , req->data_len)); +} +static u16 nvmet_get_smart_log_nsid (struct nvmet_req *req,struct nvme_smart_log *slog) {
@@ -89,74 +94,26 @@ static u16 nvmet_get_smart_log_all(struct nvmet_req *req,return NVME_SC_SUCCESS ; } -static u16 nvmet_get_smart_log (struct nvmet_req *req, -struct nvme_smart_log *slog) +static void nvmet_execute_get_log_page_smart (struct nvmet_req *req) { -u16 status; +struct nvme_smart_log *log; +u16 status =NVME_SC_INTERNAL ; -WARN_ON (req ==NULL || slog ==NULL ); -if (req->cmd->get_log_page.nsid ==cpu_to_le32 (NVME_NSID_ALL )) - status =nvmet_get_smart_log_all (req, slog); -else - status =nvmet_get_smart_log_nsid (req, slog); -return status; -} - -static void nvmet_execute_get_log_page (struct nvmet_req *req) -{ -struct nvme_smart_log *smart_log; -size_t data_len =nvmet_get_log_page_len (req->cmd); -void *buf; -u16 status =0 ; - - buf =kzalloc (data_len,GFP_KERNEL ); -if (!buf) { - status =NVME_SC_INTERNAL ; +if (req->data_len !=sizeof (*log)) goto out; - } -switch (req->cmd->get_log_page.lid) { -case NVME_LOG_ERROR : -/* -* We currently never set the More bit in the status field, -* so all error log entries are invalid and can be zeroed out. -* This is called a minum viable implementation (TM) of this -* mandatory log page. -*/ -break ; -case NVME_LOG_SMART : -/* -* XXX: fill out actual smart log -* -* We might have a hard time coming up with useful values for -* many of the fields, and even when we have useful data -* available (e.g. units or commands read/written) those aren't -* persistent over power loss. -*/ -if (data_len !=sizeof (*smart_log)) { - status =NVME_SC_INTERNAL ; - goto err; - } - smart_log = buf; - status =nvmet_get_smart_log (req, smart_log); -if (status) - goto err; -break ; -case NVME_LOG_FW_SLOT : -/* -* We only support a single firmware slot which always is -* active, so we can zero out the whole firmware slot log and -* still claim to fully implement this mandatory log page. -*/ -break ; -default : -BUG (); - } + log =kzalloc (sizeof (*log),GFP_KERNEL ); +if (!log) + goto out; - status =nvmet_copy_to_sgl (req,0 , buf, data_len); +if (req->cmd->get_log_page.nsid ==cpu_to_le32 (NVME_NSID_ALL )) + status =nvmet_get_smart_log_all (req, log); +else + status =nvmet_get_smart_log_nsid (req, log); +if (status) + goto out; -err: -kfree (buf); + status =nvmet_copy_to_sgl (req,0 , log,sizeof (*log)); out:nvmet_req_complete (req, status); }
@@ -566,9 +523,25 @@ u16 nvmet_parse_admin_cmd(struct nvmet_req *req)switch (cmd->get_log_page.lid) {case NVME_LOG_ERROR : -case NVME_LOG_SMART : +/* +* We currently never set the More bit in the status +* field, so all error log entries are invalid and can +* be zeroed out. This is called a minum viable +* implementation (TM) of this mandatory log page. +*/ + req->execute = nvmet_execute_get_log_page_noop; +return 0 ;case NVME_LOG_FW_SLOT : - req->execute = nvmet_execute_get_log_page; +/* +* We only support a single firmware slot which always +* is active, so we can zero out the whole firmware slot +* log and still claim to fully implement this mandatory +* log page. +*/ + req->execute = nvmet_execute_get_log_page_noop; +return 0 ; +case NVME_LOG_SMART : + req->execute = nvmet_execute_get_log_page_smart;return 0 ; }break ;
Signature · 1 line
2.12.3
PATCH v2 11/11 nvmet: ANA support
In reply to Hannes ReineckeAdd ANA support to the nvme target. The ANA configuration is optional, and doesn't interfere with existing configurations.
ANA groups are created under each subsystem, the namespaces must to be linked into this group to become part of the ANA group.
If ANA groups are created it is required to link the ANA groups into the respective ports. Each ANA group can only be part of one port. Linking the entire subsystem will be refused is ANA groups are specified. Also this implementation has a limit of one single ANA state per group, irrespective of the path. So when different ANA states are required one needs
to create different ANA groups, one for each state.
--- drivers/nvme/target/admin-cmd.c | 156 +++++++++++++++- drivers/nvme/target/configfs.c | 381 ++++++++++++++++++++++++++++++++++++++++ drivers/nvme/target/core.c | 94 +++++++++- drivers/nvme/target/discovery.c | 49 ++++-- drivers/nvme/target/io-cmd.c | 36 ++++ drivers/nvme/target/nvmet.h | 50 ++++++ 6 files changed, 747 insertions(+), 19 deletions(-)
drivers/nvme/target/admin-cmd.c +154 -2
@@ -118,6 +118,79 @@ static void nvmet_execute_get_log_page_smart(struct nvmet_req *req)nvmet_req_complete (req, status); } +static u32 nvmet_format_ana_group (struct nvmet_req *req,struct nvmet_ag *ag, +struct nvme_ana_group_desc *desc) +{ +struct nvmet_ctrl *ctrl = req->sq->ctrl; +struct nvmet_ns *ns; +u32 count =0 ; + +if (!(req->cmd->get_log_page.lsp &NVME_ANA_LOG_RGO )) { +struct nvmet_ns_link *n; + +rcu_read_lock (); +if (!ag->allow_any_ns && !list_empty (&ag->ns_link)) { +list_for_each_entry_rcu (n, &ag->ns_link, entry) + desc->nsids[count++] =cpu_to_le32 (n->ns->nsid); + }else { +list_for_each_entry_rcu (ns, &ctrl->subsys->namespaces, + dev_link) + desc->nsids[count++] =cpu_to_le32 (ns->nsid); + } +rcu_read_unlock (); + } + + desc->grpid =cpu_to_le32 (ag->grpid); + desc->nnsids =cpu_to_le32 (count); + desc->chgcnt =cpu_to_le64 (ag->change_count); + desc->state = ag->state; +memset (desc->rsvd17,0 ,sizeof (desc->rsvd17)); + +return sizeof (struct nvme_ana_group_desc ) + count *sizeof (__le32); +} + +static void nvmet_execute_get_log_page_ana (struct nvmet_req *req) +{ +struct nvmet_ctrl *ctrl = req->sq->ctrl; +struct nvme_ana_rsp_hdr hdr = {0 , }; +struct nvme_ana_group_desc *desc; +struct nvmet_ag_link *a; +size_t offset =sizeof (struct nvme_ana_rsp_hdr );/* start beyond hdr */ +size_t len; +u16 ngrps =0 ; +u16 status; + + status =NVME_SC_INTERNAL ; + desc =kzalloc (sizeof (struct nvme_ana_group_desc ) + + ctrl->subsys->nr_namespaces *sizeof (__le32), +GFP_KERNEL ); +if (!desc) + goto out; + +down_read (&nvmet_config_sem); +list_for_each_entry (a, &req->port->ags, entry) { +if (a->ag->subsys != ctrl->subsys) +continue ; + len =nvmet_format_ana_group (req, a->ag, desc); + status =nvmet_copy_to_sgl (req, offset, desc, len); +if (status) +break ; + offset += len; + ngrps++; + } + + hdr.chgcnt =cpu_to_le64 (ctrl->subsys->ana_chgcnt); + hdr.ngrps =cpu_to_le16 (ngrps); +up_read (&nvmet_config_sem); + +kfree (desc); + +/* copy the header last once we know the information */ + status =nvmet_copy_to_sgl (req,0 , &hdr,sizeof (hdr)); +out: +nvmet_req_complete (req, status); +} +static void nvmet_execute_identify_ctrl (struct nvmet_req *req) {struct nvmet_ctrl *ctrl = req->sq->ctrl;
@@ -149,9 +222,31 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)* the safest is to leave it as zeroes. */ -/* we support multiple ports and multiples hosts: */ +/* we support multiple ports and multiple hosts */ id->cmic = (1 <<0 ) | (1 <<1 ); +/* ANA support */ +if (!list_empty (&ctrl->subsys->ana_groups)) { +u32 grpmax =0 , grpnum =0 ; +struct nvmet_ag *ag; + + id->cmic |= (1 <<3 ); + +/* All ANA states are supported */ + id->anacap = (1 <<0 ) | (1 <<1 ) | (1 <<2 ) | \ + (1 <<3 ) | (1 <<4 ); + id->anatt = ctrl->subsys->anatt; +down_read (&nvmet_config_sem); +list_for_each_entry (ag, &ctrl->subsys->ana_groups, entry) { +if (grpmax < ag->grpid) + grpmax = ag->grpid; + grpnum++; + } +up_read (&nvmet_config_sem); + id->anagrpmax =cpu_to_le32 (grpmax); + id->nanagrpid =cpu_to_le32 (grpnum); + } +/* no limit on data transfer sizes for now */ id->mdts =0 ; id->cntlid =cpu_to_le16 (ctrl->cntlid);
@@ -188,6 +283,7 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req) id->maxcmd =cpu_to_le16 (NVMET_MAX_CMD ); id->nn =cpu_to_le32 (ctrl->subsys->max_nsid); + id->mnan =cpu_to_le32 (ctrl->subsys->nr_namespaces); id->oncs =cpu_to_le16 (NVME_CTRL_ONCS_DSM |NVME_CTRL_ONCS_WRITE_ZEROES );
@@ -236,7 +332,11 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req) {struct nvmet_ns *ns;struct nvme_id_ns *id; +struct nvmet_ag_link *a; +struct nvmet_ns_link *n; +struct nvmet_ag *ag =NULL ;u16 status =0 ; +u8 ag_state =0 ; ns =nvmet_find_namespace (req->sq->ctrl, req->cmd->identify.nsid);if (!ns) {
@@ -244,18 +344,44 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req) goto out; } +list_for_each_entry (a, &req->port->ags, entry) { +if (a->ag->subsys != req->sq->ctrl->subsys) +continue ; +list_for_each_entry (n, &a->ag->ns_link, entry) { +if (a->ag->allow_any_ns || n->ns == ns) { + ag = a->ag; +break ; + } + } + } +if (!ag) { + status =NVME_SC_INVALID_NS |NVME_SC_DNR ; + goto out; + } id =kzalloc (sizeof (*id),GFP_KERNEL );if (!id) { status =NVME_SC_INTERNAL ; goto out_put_ns; } +if (ag) { + id->anagrpid =cpu_to_le32 (ag->grpid); + ag_state = ag->state; + } +/* * nuse = ncap = nsze isn't always true, but we have no way to find * that out from the underlying device. */ id->ncap = id->nuse = id->nsze =cpu_to_le64 (ns->size >> ns->blksize_shift); +/* +* nuse and nsze should be zero for inaccessible or +* persistent loss ANA state. +*/ +if ((ag_state ==NVME_ANA_INACCESSIBLE ) || + (ag_state ==NVME_ANA_PERSISTENT_LOSS )) + id->nuse = id->nsze =0 ;/* * We just provide a single LBA format that matches what the
@@ -300,6 +426,27 @@ static void nvmet_execute_identify_nslist(struct nvmet_req *req) }rcu_read_lock (); +if (!list_empty (&req->port->ags)) { +struct nvmet_ag_link *a; + +list_for_each_entry_rcu (a, &req->port->ags, entry) { +struct nvmet_ns_link *n; + +if (a->ag->subsys != ctrl->subsys) +continue ; +if (a->ag->allow_any_ns) + goto list_subsys; +list_for_each_entry (n, &a->ag->ns_link, entry) { +if (n->ns->nsid <= min_nsid) +continue ; + list[i++] =cpu_to_le32 (n->ns->nsid); +if (i == buf_size /sizeof (__le32)) +break ; + } + } + goto skip_subsys; + } +list_subsys:list_for_each_entry_rcu (ns, &ctrl->subsys->namespaces, dev_link) {if (ns->nsid <= min_nsid)continue ;
@@ -307,6 +454,7 @@ static void nvmet_execute_identify_nslist(struct nvmet_req *req)if (i == buf_size /sizeof (__le32))break ; } +skip_subsys:rcu_read_unlock (); status =nvmet_copy_to_sgl (req,0 , list, buf_size);
@@ -406,7 +554,8 @@ static void nvmet_execute_set_features(struct nvmet_req *req)break ;case NVME_FEAT_ASYNC_EVENT : val32 =le32_to_cpu (req->cmd->common.cdw10[1 ]); - req->sq->ctrl->aen_cfg = val32 &NVME_AEN_CFG_NS_ATTR ; + req->sq->ctrl->aen_cfg = val32 & + (NVME_AEN_CFG_NS_ATTR |NVME_AEN_CFG_ANA_CHANGE );nvmet_set_result (req, req->sq->ctrl->aen_cfg);break ;case NVME_FEAT_HOST_ID :
@@ -543,6 +692,9 @@ u16 nvmet_parse_admin_cmd(struct nvmet_req *req)case NVME_LOG_SMART : req->execute = nvmet_execute_get_log_page_smart;return 0 ; +case NVME_LOG_ANA : + req->execute = nvmet_execute_get_log_page_ana; +return 0 ; }break ;case nvme_admin_identify:
drivers/nvme/target/configfs.c +381 -0
427 lines not on this page · raw message
drivers/nvme/target/core.c +87 -7
163 lines not on this page · raw message
drivers/nvme/target/discovery.c +39 -10
64 lines not on this page · raw message
drivers/nvme/target/io-cmd.c +36 -0
@@ -189,6 +189,38 @@ static void nvmet_execute_write_zeroes(struct nvmet_req *req) } } +static u16 nvmet_check_ana_state (struct nvmet_req *req) +{ +struct nvmet_ag_link *a; +struct nvmet_ns_link *n; +struct nvmet_ag *ag =NULL ; +enum nvme_ana_state state =0 ; + +if (list_empty (&req->port->ags)) +return 0 ; + +list_for_each_entry (a, &req->port->ags, entry) { +list_for_each_entry (n, &a->ag->ns_link, entry) { +if (n->ns == req->ns) { + ag = a->ag; +break ; + } + } + } +if (ag) { +if (work_pending (&ag->state_change_work.work)) +return NVME_SC_ANA_TRANSITION ; + state = ag->state; + } +if (unlikely (state ==NVME_ANA_INACCESSIBLE )) +return NVME_SC_ANA_INACCESSIBLE ; +if (unlikely (state ==NVME_ANA_PERSISTENT_LOSS )) +return NVME_SC_ANA_PERSISTENT_LOSS ; +if (unlikely (state ==NVME_ANA_CHANGE )) +return NVME_SC_ANA_TRANSITION ; +return 0 ; +} +u16 nvmet_parse_io_cmd (struct nvmet_req *req) {struct nvme_command *cmd = req->cmd;
@@ -204,6 +236,10 @@ u16 nvmet_parse_io_cmd(struct nvmet_req *req)if (unlikely (!req->ns))return NVME_SC_INVALID_NS |NVME_SC_DNR ; + ret =nvmet_check_ana_state (req); +if (unlikely (ret)) +return ret; +switch (cmd->common.opcode) {case nvme_cmd_read:case nvme_cmd_write:
drivers/nvme/target/nvmet.h +50 -0
106 lines not on this page · raw message
Signature · 1 line
2.12.3
Christoph Hellwig
maintainerPATCH v2 11/11 nvmet: ANA support
Quoted 12 lines · Hannes Reinecke
Add ANA support to the nvme target. The ANA configuration is optional, and doesn't interfere with existing configurations.
ANA groups are created under each subsystem, the namespaces must to be linked into this group to become part of the ANA group.
If ANA groups are created it is required to link the ANA groups into the respective ports. Each ANA group can only be part of one port. Linking the entire subsystem will be refused is ANA groups are specified. Also this implementation has a limit of one single ANA state per group, irrespective of the path. So when different ANA states are required one needs
to create different ANA groups, one for each state.Signed-off-by Hannes Reinecke <hare at suse.com>
Quoted 8 lines
--- drivers/nvme/target/admin-cmd.c | 156 +++++++++++++++- drivers/nvme/target/configfs.c | 381 ++++++++++++++++++++++++++++++++++++++++ drivers/nvme/target/core.c | 94 +++++++++- drivers/nvme/target/discovery.c | 49 ++++-- drivers/nvme/target/io-cmd.c | 36 ++++ drivers/nvme/target/nvmet.h | 50 ++++++ 6 files changed, 747 insertions(+), 19 deletions(-)
What is the advantage over my implementation, which is a lot simpler?
Especially the configuration here seems extremely complex for no real gain.
PATCH v2 11/11 nvmet: ANA support
Quoted 22 lines · Christoph Hellwig
Quoted 12 lines · Hannes Reinecke
Add ANA support to the nvme target. The ANA configuration is optional, and doesn't interfere with existing configurations.
ANA groups are created under each subsystem, the namespaces must to be linked into this group to become part of the ANA group.
If ANA groups are created it is required to link the ANA groups into the respective ports. Each ANA group can only be part of one port. Linking the entire subsystem will be refused is ANA groups are specified. Also this implementation has a limit of one single ANA state per group, irrespective of the path. So when different ANA states are required one needs
to create different ANA groups, one for each state.Signed-off-by Hannes Reinecke <hare at suse.com>Quoted 8 lines
--- drivers/nvme/target/admin-cmd.c | 156 +++++++++++++++- drivers/nvme/target/configfs.c | 381 ++++++++++++++++++++++++++++++++++++++++ drivers/nvme/target/core.c | 94 +++++++++- drivers/nvme/target/discovery.c | 49 ++++-- drivers/nvme/target/io-cmd.c | 36 ++++ drivers/nvme/target/nvmet.h | 50 ++++++ 6 files changed, 747 insertions(+), 19 deletions(-)
Quoted 5 lines
What is the advantage over my implementation, which is a lot simpler?
Especially the configuration here seems extremely complex for no real gain.
Primarily this is designed to be a testbed for the various ANA configurations. Plus ANA support can be disabled, too :-) So with this we can do all sorts of compability testing like running against various revisions, and we can to scalability tests etc.
_And_ you can have a simple ANA configuration with enabling
echo 1 > allow_any_ns
then you only need to link the ANA groups into the ports. I don't really think it's _that_ complicated.
Cheers,
Hannes
Christoph Hellwig
maintainerPATCH v2 11/11 nvmet: ANA support
Quoted 2 lines · Hannes Reinecke
Primarily this is designed to be a testbed for the various ANA configurations.
Well, hat isn't really what we need in mainline.
Quoted 1 line
Plus ANA support can be disabled, too :-)
Great. That should be worth about 20 lines if we really need it, not 370 lines of additional code over the initial 315. And not require iterating the list of all ANA groups in the ANA fast path. And so on.
Sagi Grimberg
maintainerPATCH v2 11/11 nvmet: ANA support
Quoted 4 lines
Quoted 3 lines
Primarily this is designed to be a testbed for the various ANA configurations.
Quoted 8 lines
Well, hat isn't really what we need in mainline.
Quoted 1 line
Plus ANA support can be disabled, too :-)
Great. That should be worth about 20 lines if we really need it, not 370 lines of additional code over the initial 315. And not require iterating the list of all ANA groups in the ANA fast path. And so on.
Hey Hannes, Christoph,
I'm trying to catch up, and reviewed this set. Didn't see a v2 for this but only "NVMe ANA fixups".. is this supposed to be v2?