Support read-only subsystem critical warning
Subsystems (2)
Sagi Grimberg
maintainercover Support read-only subsystem critical warning
In case a subsystem enters a read-only mode, the host shall issue a smart log page request and check the read-only bit in the critical warning section. In case its set, it shall set all the controller namespaces in read-only mode (or set the namespaces in read-write in case its cleared).
The nvmet implements emulation of this to exercise the host (might also be useful for other use-cases as well though). Add a configfs knob to trigger read-only subsystem critical event. Also when the host connects to a read-only subsystem, and AER is queued to notify the host on the critical warning.
Note that the spec does not refer to the scenario of fabrics connecting to an already read-only subsystem so the behavior I implemented is what I thought was most suitable. Also, the spec does not define (at least from what I found) what happens if a subsystem moves out from read-only state, so again, the behavior I implemented is what I found to be suitable.
Sagi Grimberg (2):
nvme: support smart read-only critical warning event
nvmet: expose option to emulate a nvm subsystem in read-only mode
drivers/nvme/host/core.c | 51 +++++++++++++++++++++++++++++++++++++++ drivers/nvme/host/multipath.c | 4 +++ drivers/nvme/host/nvme.h | 2 ++ drivers/nvme/target/admin-cmd.c | 3 +++ drivers/nvme/target/configfs.c | 34 ++++++++++++++++++++++++++ drivers/nvme/target/core.c | 2 +- drivers/nvme/target/fabrics-cmd.c | 3 +++ drivers/nvme/target/io-cmd.c | 8 +++++- drivers/nvme/target/nvmet.h | 3 +++ 9 files changed, 108 insertions(+), 2 deletions(-)
Signature · 1 line
2.14.1
Sagi Grimberg
maintainerRFC PATCH 1/2 nvme: support smart read-only critical warning event
In case a controller sent us a critical warning AER, we should query the smart information and in case the media was placed in read only mode, we need to set all the controller namespaces (and the namespaces heads) in read-only (or set to rw in case its not set).
Given that in fabrics we may also connect to a read-only subsystem, we add the disk as read-only to start with. Once the subsystem will be in rw mode, we restore the disk rw permissions.
--- drivers/nvme/host/core.c | 51 +++++++++++++++++++++++++++++++++++++++++++ drivers/nvme/host/multipath.c | 4 ++++ drivers/nvme/host/nvme.h | 2 ++ 3 files changed, 57 insertions(+)
drivers/nvme/host/core.c +51 -0
@@ -2949,6 +2949,9 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)kfree (id); +if (ctrl->subsys->read_only) +set_disk_ro (ns->disk, true); +device_add_disk (ctrl->device, ns->disk);if (sysfs_create_group (&disk_to_dev (ns->disk)->kobj, &nvme_ns_id_attr_group))
@@ -3192,6 +3195,49 @@ static void nvme_get_fw_slot_info(struct nvme_ctrl *ctrl)kfree (log); } +static void nvme_get_smart_log (struct nvme_ctrl *ctrl) +{ +struct nvme_smart_log *log; +struct nvme_ns *ns; + + log =kmalloc (sizeof (*log),GFP_KERNEL ); +if (!log) +return ; + +if (nvme_get_log (ctrl,NVME_LOG_SMART , log,sizeof (*log))) { +dev_warn (ctrl->device,"Get SMART log error\n" ); + goto out; + } + +if (log->critical_warning &NVME_SMART_CRIT_MEDIA ) { +dev_warn (ctrl->device, +"critical warning: media placed in read-only\n" ); + ctrl->subsys->read_only = true; + }else { +dev_info (ctrl->device, +"media placed in read-write mode\n" ); + ctrl->subsys->read_only = false; + } + +mutex_lock (&ctrl->namespaces_mutex); +list_for_each_entry (ns, &ctrl->namespaces, list) { +set_disk_ro (ns->disk, ctrl->subsys->read_only); +if (ns->head->disk) +set_disk_ro (ns->head->disk, ctrl->subsys->read_only); + } +mutex_unlock (&ctrl->namespaces_mutex); +out: +kfree (log); +} + +static void nvme_smart_work (struct work_struct *work) +{ +struct nvme_ctrl *ctrl = + container_of(work, struct nvme_ctrl, smart_work); + +nvme_get_smart_log (ctrl); +} +static void nvme_fw_act_work (struct work_struct *work) { struct nvme_ctrl *ctrl = container_of(work,
@@ -3251,6 +3297,9 @@ void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,case NVME_AER_NOTICE_FW_ACT_STARTING :queue_work (nvme_wq, &ctrl->fw_act_work);break ; +case NVME_AER_SMART : +queue_work (nvme_wq, &ctrl->smart_work); +break ;default :dev_warn (ctrl->device,"async event result %08x\n" , result); }
@@ -3263,6 +3312,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); +flush_work (&ctrl->smart_work);cancel_work_sync (&ctrl->fw_act_work); }EXPORT_SYMBOL_GPL (nvme_stop_ctrl);
@@ -3328,6 +3378,7 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,INIT_WORK (&ctrl->scan_work, nvme_scan_work);INIT_WORK (&ctrl->async_event_work, nvme_async_event_work);INIT_WORK (&ctrl->fw_act_work, nvme_fw_act_work); +INIT_WORK (&ctrl->smart_work, nvme_smart_work);INIT_WORK (&ctrl->delete_work, nvme_delete_ctrl_work); ret =ida_simple_get (&nvme_instance_ida,0 ,0 ,GFP_KERNEL );
drivers/nvme/host/multipath.c +4 -0
@@ -238,6 +238,10 @@ void nvme_mpath_add_disk(struct nvme_ns_head *head) {if (!head->disk)return ; + +if (head->subsys->read_only) +set_disk_ro (head->disk, true); +device_add_disk (&head->subsys->dev, head->disk);if (sysfs_create_group (&disk_to_dev (head->disk)->kobj, &nvme_ns_id_attr_group))
drivers/nvme/host/nvme.h +2 -0
@@ -181,6 +181,7 @@ struct nvme_ctrl {struct nvme_id_power_state psd[32 ];struct nvme_effects_log *effects;struct work_struct scan_work; +struct work_struct smart_work;struct work_struct async_event_work;struct delayed_work ka_work;struct work_struct fw_act_work;
@@ -226,6 +227,7 @@ struct nvme_subsystem {u8 cmic;u16 vendor_id;struct ida ns_ida; +bool read_only; };/*
Signature · 1 line
2.14.1
Christoph Hellwig
maintainerRFC PATCH 1/2 nvme: support smart read-only critical warning event
Quoted 10 lines · Sagi Grimberg
In case a controller sent us a critical warning AER, we should query the smart information and in case the media was placed in read only mode, we need to set all the controller namespaces (and the namespaces heads) in read-only (or set to rw in case its not set).
Given that in fabrics we may also connect to a read-only subsystem, we add the disk as read-only to start with. Once the subsystem will be in rw mode, we restore the disk rw permissions.
We are sending the smart log notification on to userspace, so with this we now have two parties that are trying to clear the log page..
Sagi Grimberg
maintainerRFC PATCH 1/2 nvme: support smart read-only critical warning event
Quoted 7 lines
Quoted 7 lines
In case a controller sent us a critical warning AER, we should query the smart information and in case the media was placed in read only mode, we need to set all the controller namespaces (and the namespaces heads) in read-only (or set to rw in case its not set).
Quoted 7 lines
Quoted 4 lines
Given that in fabrics we may also connect to a read-only subsystem, we add the disk as read-only to start with. Once the subsystem will be in rw mode, we restore the disk rw permissions.
We are sending the smart log notification on to userspace, so with this we now have two parties that are trying to clear the log page..
Not at the moment, but soon we will send smart logs to userspace, yes.
I guess we have two choices for this:
1. let userspace handle it and expose a knob to modify all controller namespaces to r/o.
2. handle the event in the kernel, issue the log page with retain bit on and only then notify userspace.
I personally prefer (2), what do others think?
RFC PATCH 1/2 nvme: support smart read-only critical warning event
Quoted 4 lines · Sagi Grimberg
2. handle the event in the kernel, issue the log page with retain bit on and only then notify userspace.
I personally prefer (2), what do others think?
Sorry for chiming in so late, but I agree with Sagi.
Byte,
Johannes
Signature · 6 lines
Johannes Thumshirn Storage jthumshirn at suse.de +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg GF: Felix Imend?rffer, Jane Smithard, Graham Norton HRB 21284 (AG N?rnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
Christoph Hellwig
maintainerRFC PATCH 1/2 nvme: support smart read-only critical warning event
In reply to Sagi GrimbergQuoted 2 lines · Sagi Grimberg
2. handle the event in the kernel, issue the log page with retain bit on and only then notify userspace.
The retain bit is a new optional feature in nvme 1.3. I fear we won't see all that many controllers respecting it.
Sagi Grimberg
maintainerRFC PATCH 1/2 nvme: support smart read-only critical warning event
Quoted 6 lines
Quoted 3 lines
2. handle the event in the kernel, issue the log page with retain bit on and only then notify userspace.
The retain bit is a new optional feature in nvme 1.3. I fear we won't see all that many controllers respecting it.
Well, for those devices user-space will see a cleared log page...
Unless you prefer option 1? Or have a better idea?
Sagi Grimberg
maintainerRFC PATCH 2/2 nvmet: expose option to emulate a nvm subsystem in read-only mode
In reply to Sagi GrimbergUseful for exercising the host. Also, if a host connects to a subsystem which is read-only to begin with, queue an AER to let it know about it.
In read-only state, we fail writes/dsms/write-zeros.
--- drivers/nvme/target/admin-cmd.c | 3 +++ drivers/nvme/target/configfs.c | 34 ++++++++++++++++++++++++++++++++++ drivers/nvme/target/core.c | 2 +- drivers/nvme/target/fabrics-cmd.c | 3 +++ drivers/nvme/target/io-cmd.c | 8 +++++++- drivers/nvme/target/nvmet.h | 3 +++ 6 files changed, 51 insertions(+), 2 deletions(-)
drivers/nvme/target/admin-cmd.c +3 -0
@@ -69,6 +69,9 @@ static u16 nvmet_get_smart_log_all(struct nvmet_req *req, ctrl = req->sq->ctrl; +if (req->sq->ctrl->subsys->read_only) + slog->critical_warning |=NVME_SMART_CRIT_MEDIA ; +rcu_read_lock ();list_for_each_entry_rcu (ns, &ctrl->subsys->namespaces, dev_link) { host_reads +=part_stat_read (ns->bdev->bd_part, ios[READ ]);
drivers/nvme/target/configfs.c +34 -0
@@ -711,6 +711,39 @@ static ssize_t nvmet_subsys_attr_allow_any_host_store(struct config_item *item,CONFIGFS_ATTR (nvmet_subsys_, attr_allow_any_host); +static ssize_t nvmet_subsys_attr_ro_show (struct config_item *item, +char *page) +{ +return snprintf (page,PAGE_SIZE ,"%d\n" , +to_subsys (item)->read_only); +} + +static ssize_t nvmet_subsys_attr_ro_store (struct config_item *item, +const char *page,size_t count) +{ +struct nvmet_subsys *subsys =to_subsys (item); +bool subsys_ro = subsys->read_only; +bool ro; + +if (strtobool (page, &ro)) +return -EINVAL ; + + subsys->read_only = ro; +if ((!subsys_ro && ro) || (subsys_ro && !ro)) { +struct nvmet_ctrl *ctrl; + +/* read only state change, send a SMART AER */ +mutex_lock (&subsys->lock); +list_for_each_entry (ctrl, &subsys->ctrls, subsys_entry) +nvmet_add_async_event (ctrl,NVME_AER_SMART ,0 ,0 ); +mutex_unlock (&subsys->lock); + } + +return count; +} + +CONFIGFS_ATTR (nvmet_subsys_, attr_ro); +static ssize_t nvmet_subsys_attr_version_show (struct config_item *item,char *page) {
@@ -770,6 +803,7 @@ CONFIGFS_ATTR(nvmet_subsys_, attr_serial);static struct configfs_attribute *nvmet_subsys_attrs[] = { &nvmet_subsys_attr_attr_allow_any_host, + &nvmet_subsys_attr_attr_ro, &nvmet_subsys_attr_attr_version, &nvmet_subsys_attr_attr_serial,NULL ,
drivers/nvme/target/core.c +1 -1
@@ -117,7 +117,7 @@ static void nvmet_async_event_work(struct work_struct *work) } } -static void nvmet_add_async_event (struct nvmet_ctrl *ctrl,u8 event_type, +void nvmet_add_async_event (struct nvmet_ctrl *ctrl,u8 event_type,u8 event_info,u8 log_page) {struct nvmet_async_event *aen;
drivers/nvme/target/fabrics-cmd.c +3 -0
@@ -177,6 +177,9 @@ static void nvmet_execute_admin_connect(struct nvmet_req *req) goto out; } +if (req->sq->ctrl->subsys->read_only) +nvmet_add_async_event (req->sq->ctrl,NVME_AER_SMART ,0 ,0 ); +pr_info ("creating controller %d for subsystem %s for NQN %s.\n" , ctrl->cntlid, ctrl->subsys->subsysnqn, ctrl->hostnqn); req->rsp->result.u16 =cpu_to_le16 (ctrl->cntlid);
drivers/nvme/target/io-cmd.c +7 -1
@@ -202,8 +202,10 @@ u16 nvmet_parse_io_cmd(struct nvmet_req *req)return NVME_SC_INVALID_NS |NVME_SC_DNR ;switch (cmd->common.opcode) { -case nvme_cmd_read:case nvme_cmd_write: +if (unlikely (req->sq->ctrl->subsys->read_only)) +return NVME_SC_READ_ONLY |NVME_SC_DNR ; +case nvme_cmd_read: req->execute = nvmet_execute_rw; req->data_len =nvmet_rw_len (req);return 0 ;
@@ -212,11 +214,15 @@ u16 nvmet_parse_io_cmd(struct nvmet_req *req) req->data_len =0 ;return 0 ;case nvme_cmd_dsm: +if (unlikely (req->sq->ctrl->subsys->read_only)) +return NVME_SC_READ_ONLY |NVME_SC_DNR ; req->execute = nvmet_execute_dsm; req->data_len = (le32_to_cpu (cmd->dsm.nr) +1 ) *sizeof (struct nvme_dsm_range );return 0 ;case nvme_cmd_write_zeroes: +if (unlikely (req->sq->ctrl->subsys->read_only)) +return NVME_SC_READ_ONLY |NVME_SC_DNR ; req->execute = nvmet_execute_write_zeroes;return 0 ;default :
drivers/nvme/target/nvmet.h +3 -0
@@ -144,6 +144,7 @@ struct nvmet_ctrl {struct nvmet_subsys {enum nvme_subsys_type type; +bool read_only;struct mutex lock;struct kref ref;
@@ -287,6 +288,8 @@ void nvmet_sq_destroy(struct nvmet_sq *sq);int nvmet_sq_init (struct nvmet_sq *sq);void nvmet_ctrl_fatal_error (struct nvmet_ctrl *ctrl); +void nvmet_add_async_event (struct nvmet_ctrl *ctrl,u8 event_type, +u8 event_info,u8 log_page);void nvmet_update_cc (struct nvmet_ctrl *ctrl,u32 new); u16 nvmet_alloc_ctrl(const char *subsysnqn, const char *hostnqn,
Signature · 1 line
2.14.1
Christoph Hellwig
maintainerRFC PATCH 2/2 nvmet: expose option to emulate a nvm subsystem in read-only mode
I don't think that is what the SMART log is inteded for. Please review the write protected namespace TP in the working group instead and make sure it passes soon so that we can use it.
Sagi Grimberg
maintainerRFC PATCH 2/2 nvmet: expose option to emulate a nvm subsystem in read-only mode
Hey Christoph,
Quoted 1 line
I don't think that is what the SMART log is inteded for.
Can you please elaborate what you mean? are you talking about this patch in particular (simply an emulation)? or support for this altogether? A subsystem can, at any point, be placed in read-only mode. In that case it needs to send SMART AER to indicate health status change. I think that if the media has been placed in read-only mode, we need to log it, and set all the subsystem namespaces in read-only mode.
Anything different you think we need to do here?
Quoted 2 lines
Please review the write protected namespace TP in the working group instead and make sure it passes soon so that we can use it.
I did look at write protect, this is not the use-case here. write-protect is a per-namespace attribute which is controlled by the host, this addresses NVMe subsystem critical warning scenario.
Christoph Hellwig
maintainerRFC PATCH 2/2 nvmet: expose option to emulate a nvm subsystem in read-only mode
Quoted 8 lines · Sagi Grimberg
Can you please elaborate what you mean? are you talking about this patch in particular (simply an emulation)? or support for this altogether? A subsystem can, at any point, be placed in read-only mode. In that case it needs to send SMART AER to indicate health status change. I think that if the media has been placed in read-only mode, we need to log it, and set all the subsystem namespaces in read-only mode.
Anything different you think we need to do here?
Yes, but for that we should trigger it off gettind a EROFS warning from the block device, not through sysfs. Having the explicit sysfs no, and the code to send the AER on connect suggests to me that this was intended to be used to emulate a r/o setting, but apparently I was wrong.
Sagi Grimberg
maintainerRFC PATCH 2/2 nvmet: expose option to emulate a nvm subsystem in read-only mode
Quoted 8 lines
Quoted 8 lines
Can you please elaborate what you mean? are you talking about this patch in particular (simply an emulation)? or support for this altogether? A subsystem can, at any point, be placed in read-only mode. In that case it needs to send SMART AER to indicate health status change. I think that if the media has been placed in read-only mode, we need to log it, and set all the subsystem namespaces in read-only mode.
Quoted 4 lines
Quoted 1 line
Anything different you think we need to do here?
Yes, but for that we should trigger it off gettind a EROFS warning from the block device, not through sysfs.
Just to be clear, that is exactly what I intended to do.
fs/block_dev.c checks bdev_read_only() in blkdev_write_iter and also there is a patchset to enforce this for several places in the block layer from Ilya Dryomov ("block: enforce ioctl(BLKROSET) and set_disk_ro()")
I wander if we need this also in generic_write_checks()?
Quoted 4 lines
Having the explicit sysfs
no, and the code to send the AER on connect suggests to me that this was intended to be used to emulate a r/o setting, but apparently I was wrong.
I'm not sure I understand your point here, can you explain what you mean?
nvmet sends a smart AER in case a host connected to a subsystem in r/o state because there no other way it will know that (other than failing writes when they come). Its just an attempt to notify the host right away instead of waiting for it to issue writes and fail later.
Christoph Hellwig
maintainerRFC PATCH 2/2 nvmet: expose option to emulate a nvm subsystem in read-only mode
Quoted 1 line · Sagi Grimberg
Just to be clear, that is exactly what I intended to do.
My point is that we should not have a configfs nob to turn on the r/o nob, but do it based on an error from the lower block device.
Sagi Grimberg
maintainerRFC PATCH 2/2 nvmet: expose option to emulate a nvm subsystem in read-only mode
Quoted 6 lines
Quoted 2 lines
Just to be clear, that is exactly what I intended to do.
My point is that we should not have a configfs nob to turn on the r/o nob, but do it based on an error from the lower block device.
What do you suggest we do when the subsystem exports two block devices of two different subsystems where only one gets into r/o state?