nvmet: emulate noiob boundary

Useful to exercise the host driver to noiob support without having device that actually require it.

---
 drivers/nvme/target/admin-cmd.c |  2 ++
 drivers/nvme/target/configfs.c  | 29 +++++++++++++++++++++++++++++
 drivers/nvme/target/nvmet.h     |  1 +
 3 files changed, 32 insertions(+)

index 3554cb4c6387..7e187815e0ea 100644
@@ -427,6 +427,8 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)
 
 	id->lbaf[0].ds = ns->blksize_shift;
 
+	id->noiob = cpu_to_le16(ns->noiob);
+
 	if (ns->readonly)
 		id->nsattr |= (1 << 0);
 	nvmet_put_namespace(ns);

index 46d0a0bc630b..5225715269cb 100644
@@ -555,6 +555,34 @@ static ssize_t nvmet_ns_buffered_io_store(struct config_item *item,
 
 CONFIGFS_ATTR(nvmet_ns_, buffered_io);
 
+static ssize_t nvmet_ns_chunk_sectors_show(struct config_item *item, char *page)
+{
+	return sprintf(page, "%u\n", to_nvmet_ns(item)->noiob);
+}
+
+static ssize_t nvmet_ns_chunk_sectors_store(struct config_item *item,
+		const char *page, size_t count)
+{
+	struct nvmet_ns *ns = to_nvmet_ns(item);
+	u16 noiob;
+
+	if (kstrtou16(page, 0, &noiob))
+		return -EINVAL;
+
+	mutex_lock(&ns->subsys->lock);
+	if (ns->enabled) {
+		pr_err("disable ns before setting chunk_sectors\n");
+		mutex_unlock(&ns->subsys->lock);
+		return -EINVAL;
+	}
+
+	ns->noiob = noiob;
+	mutex_unlock(&ns->subsys->lock);
+	return count;
+}
+
+CONFIGFS_ATTR(nvmet_ns_, chunk_sectors);
+
 static struct configfs_attribute *nvmet_ns_attrs[] = {
 	&nvmet_ns_attr_device_path,
 	&nvmet_ns_attr_device_nguid,

@@ -562,6 +590,7 @@ static struct configfs_attribute *nvmet_ns_attrs[] = {
 	&nvmet_ns_attr_ana_grpid,
 	&nvmet_ns_attr_enable,
 	&nvmet_ns_attr_buffered_io,
+	&nvmet_ns_attr_chunk_sectors,
 	NULL,
 };
 

index 10c96d43868d..cb8b93c83c5b 100644
@@ -67,6 +67,7 @@ struct nvmet_ns {
 	u8			nguid[16];
 	uuid_t			uuid;
 	u32			anagrpid;
+	u16			noiob;
 
 	bool			buffered_io;
 	bool			enabled;

2.17.1

This code looks ok to me, but I'm rather worried about adding these just for fun features - we'll eventually have a configfs mess just like the scsi target code if we just keep adding random options.

Well, this helped me debug a sub-page bio split code path that I wouldn't be able to generate without, so I think its beneficial to have a knob for it.

In general, I would be in favor of us having the kernel target allow one to exercise the host code in various forms, but would not want to have endless attributes to setup. Perhaps we would want to have these sort of stuff with a debug prefix such that its clear that only debug stuff belong there?

Thoughts?

From: Linux-nvme <[email protected]> on behalf of Sagi Grimberg <[email protected]>
Sent: Monday, October 22, 2018 5:29 PM
To: Christoph Hellwig
Cc: Keith Busch; linux-nvme at lists.infradead.org
Subject: Re: [PATCH RFC] nvmet: emulate noiob boundary
?

Well, this helped me debug a sub-page bio split code path that I wouldn't be able to generate without, so I think its beneficial to have a knob for it.

In general, I would be in favor of us having the kernel target allow one to exercise the host code in various forms, but would not want to have endless attributes to setup.

[CK] We can get away with the target component based debug bitmask and have one configfs entry to avoid the configfs attribute pollution.

Perhaps we would want to have
these sort of stuff with a debug prefix such that its clear that only debug stuff belong there?

Thoughts?

[CK] I like the idea of having debug code handy.
However,?if we start adding?debug code in each file?it will spread the debug code?all?over the places in the future.?How about we centralize?the debug code in nvmet-debug.[c|h] ? (not sure how we are going to do this in this case though).

?

_______________________________________________
Linux-nvme mailing list
Linux-nvme at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

That is not a better user experience in my mind...

I don't think the comment was on debug code layout, more about keeping a clean and simple user interface without tons of irrelevant knobs a user can see (or I might have misunderstood hch here).

Yes, maybe we could have a config option to turn on this pure debug code. For noiob in particular it might make sense to just pass it up if the lower level device has it, though.

I'm not sure what would be the benefit of having the host splitting bios instead of the target.