[PATCH RFC] nvmet: emulate noiob boundary

[PATCH RFC] nvmet: emulate noiob boundary

[Sagi Grimberg]

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

Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/target/admin-cmd.c |  2 ++
 drivers/nvme/target/configfs.c  | 29 +++++++++++++++++++++++++++++
 drivers/nvme/target/nvmet.h     |  1 +
 3 files changed, 32 insertions(+)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 3554cb4c6387..7e187815e0ea 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -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);
diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 46d0a0bc630b..5225715269cb 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -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,
 };
 
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 10c96d43868d..cb8b93c83c5b 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -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

[PATCH RFC] nvmet: emulate noiob boundary

[Christoph Hellwig]

On Fri, Oct 19, 2018@03:24:37PM -0700, Sagi Grimberg wrote:
> Useful to exercise the host driver to noiob support without having device
> that actually require it.

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.

[PATCH RFC] nvmet: emulate noiob boundary

[Sagi Grimberg]

>> Useful to exercise the host driver to noiob support without having device
>> that actually require it.
> 
> 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?

[PATCH RFC] nvmet: emulate noiob boundary

[Chaitanya Kulkarni]

From: Linux-nvme <linux-nvme-bounces@lists.infradead.org> on behalf of Sagi Grimberg <sagi@grimberg.me>
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
? 
 

>> Useful to exercise the host driver to noiob support without having device
>> that actually require it.
> 
> 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.

[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
    

[PATCH RFC] nvmet: emulate noiob boundary

[Sagi Grimberg]

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

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

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

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

[PATCH RFC] nvmet: emulate noiob boundary

[Christoph Hellwig]

On Mon, Oct 22, 2018@05:29:55PM -0700, Sagi Grimberg wrote:
> 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?

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.

[PATCH RFC] nvmet: emulate noiob boundary

[Sagi Grimberg]

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