[PATCH rfc 0/2] Support read-only subsystem critical warning

[PATCH rfc 0/2] Support read-only subsystem critical warning

[Sagi Grimberg]

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(-)

-- 
2.14.1

[PATCH rfc 1/2] nvme: support smart read-only critical warning event

[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.

Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/core.c      | 51 +++++++++++++++++++++++++++++++++++++++++++
 drivers/nvme/host/multipath.c |  4 ++++
 drivers/nvme/host/nvme.h      |  2 ++
 3 files changed, 57 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 25e3168cec44..eaae1b4f6a4e 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -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);
diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index 1218a9fca846..b833ba3c78cb 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -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))
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 4361b71891e9..caa8f7130ec6 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -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;
 };
 
 /*
-- 
2.14.1

[PATCH rfc 2/2] nvmet: expose option to emulate a nvm subsystem in read-only mode

[Sagi Grimberg]

Useful 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.

Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 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(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 6b921c253618..aab646f44a66 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -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]);
diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 8f378030c8e4..c966228439f6 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -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,
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 841a37dfb987..5b4d30db9776 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -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;
diff --git a/drivers/nvme/target/fabrics-cmd.c b/drivers/nvme/target/fabrics-cmd.c
index 62315cc4b5b0..fc9ef44d2b49 100644
--- a/drivers/nvme/target/fabrics-cmd.c
+++ b/drivers/nvme/target/fabrics-cmd.c
@@ -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);
diff --git a/drivers/nvme/target/io-cmd.c b/drivers/nvme/target/io-cmd.c
index 0a4372a016f2..6f121d2f2ea3 100644
--- a/drivers/nvme/target/io-cmd.c
+++ b/drivers/nvme/target/io-cmd.c
@@ -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:
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index d69d948c8f36..a9fb763020b6 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -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,
-- 
2.14.1

[PATCH rfc 2/2] nvmet: expose option to emulate a nvm subsystem in read-only mode

[Christoph Hellwig]

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.

[PATCH rfc 2/2] nvmet: expose option to emulate a nvm subsystem in read-only mode

[Sagi Grimberg]

Hey Christoph,

> 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?

> 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.

[PATCH rfc 2/2] nvmet: expose option to emulate a nvm subsystem in read-only mode

[Christoph Hellwig]

On Mon, Dec 04, 2017@08:11:14PM +0200, Sagi Grimberg wrote:
> 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.

[PATCH rfc 1/2] nvme: support smart read-only critical warning event

[Christoph Hellwig]

On Mon, Dec 04, 2017@11:24:34AM +0200, Sagi Grimberg wrote:
> 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..

[PATCH rfc 2/2] nvmet: expose option to emulate a nvm subsystem in read-only mode

[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.

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()?

> 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.

[PATCH rfc 1/2] nvme: support smart read-only critical warning event

[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..

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?

[PATCH rfc 1/2] nvme: support smart read-only critical warning event

[Johannes Thumshirn]

Sagi Grimberg <sagi at grimberg.me> writes:
> 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
-- 
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

[PATCH rfc 2/2] nvmet: expose option to emulate a nvm subsystem in read-only mode

[Christoph Hellwig]

On Tue, Dec 05, 2017@09:22:05AM +0200, Sagi Grimberg wrote:
> 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.

[PATCH rfc 1/2] nvme: support smart read-only critical warning event

[Christoph Hellwig]

On Tue, Dec 05, 2017@09:28:53AM +0200, Sagi Grimberg wrote:
> 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.

[PATCH rfc 2/2] nvmet: expose option to emulate a nvm subsystem in read-only mode

[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.

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?

[PATCH rfc 1/2] nvme: support smart read-only critical warning event

[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.

Well, for those devices user-space will see a cleared log
page...

Unless you prefer option 1? Or have a better idea?