nvmet: debug namespace noiob boundary emulation

Useful to exercise the host driver to noiob support without having device that actually require it. name it with a debug prefix such that it is clear that its a debug feature.

---
 drivers/nvme/target/admin-cmd.c |  2 ++
 drivers/nvme/target/configfs.c  | 30 ++++++++++++++++++++++++++++++
 drivers/nvme/target/nvmet.h     |  1 +
 3 files changed, 33 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..c85f6977548a 100644
@@ -555,6 +555,35 @@ static ssize_t nvmet_ns_buffered_io_store(struct config_item *item,
 
 CONFIGFS_ATTR(nvmet_ns_, buffered_io);
 
+static ssize_t nvmet_ns_debug_chunk_sectors_show(struct config_item *item,
+		char *page)
+{
+	return sprintf(page, "%u\n", to_nvmet_ns(item)->noiob);
+}
+
+static ssize_t nvmet_ns_debug_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_, debug_chunk_sectors);
+
 static struct configfs_attribute *nvmet_ns_attrs[] = {
 	&nvmet_ns_attr_device_path,
 	&nvmet_ns_attr_device_nguid,

@@ -562,6 +591,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_debug_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

Add a debug class for setting/getting namespace debug features.

---
 nvmet/nvme.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 7ab0846bf835..561a9a4c9ba2 100644
@@ -554,7 +554,7 @@ class Namespace(CFSNode):
             if nsid < 1 or nsid > self.MAX_NSID:
                 raise CFSError("NSID must be 1 to %d" % self.MAX_NSID)
 
-        self.attr_groups = ['device']
+        self.attr_groups = ['device', 'debug']
         self._subsystem = subsystem
         self._nsid = nsid
         self._path = "%s/namespaces/%d" % (self.subsystem.path, self.nsid)

2.17.1

Looks good.

?On 10/30/18, 10:57 AM, "Linux-nvme on behalf of Sagi Grimberg" <[email protected] on behalf of [email protected]> wrote:

    Useful to exercise the host driver to noiob support without
    having device that actually require it. name it with a debug
    prefix such that it is clear that its a debug feature.
    
    Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
    ---
     drivers/nvme/target/admin-cmd.c |  2 ++
     drivers/nvme/target/configfs.c  | 30 ++++++++++++++++++++++++++++++
     drivers/nvme/target/nvmet.h     |  1 +
     3 files changed, 33 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..c85f6977548a 100644
    --- a/drivers/nvme/target/configfs.c
    +++ b/drivers/nvme/target/configfs.c
    @@ -555,6 +555,35 @@ static ssize_t nvmet_ns_buffered_io_store(struct config_item *item,
     
     CONFIGFS_ATTR(nvmet_ns_, buffered_io);
     
    +static ssize_t nvmet_ns_debug_chunk_sectors_show(struct config_item *item,
    +		char *page)
    +{
    +	return sprintf(page, "%u\n", to_nvmet_ns(item)->noiob);
    +}
    +
    +static ssize_t nvmet_ns_debug_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_, debug_chunk_sectors);
    +
     static struct configfs_attribute *nvmet_ns_attrs[] = {
     	&nvmet_ns_attr_device_path,
     	&nvmet_ns_attr_device_nguid,
    @@ -562,6 +591,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_debug_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
    
    
    _______________________________________________
    Linux-nvme mailing list
    Linux-nvme at lists.infradead.org
    http://lists.infradead.org/mailman/listinfo/linux-nvme

I'd really prefer to have a new CONFIG_NVME_TARGET_DEBUG or similar config symbol so that we can not build these purely debug knobs if desired.

Really? I don't think this is worth it, we already have a tone of build time config options (https://lwn.net/Articles/733418/) and having another knob for small debug options for our nvmet is not helping. Also it will also introduce some annoying ifdefs and more 0-day testing complaints in the future. And, it is also sometimes annoying to have to rebuild a kernel (or module) especially when debugging on a station without access to modify the running source code.

So I'm not sure I agree with this preference. But I'm not completely against it, can we have more people share their thoughts here?

I'd honestly prefer the nvme target simply provide useful functionality for end users rather than take on the responsibility of a test vehicle for host drivers.

I don't contribute much to the target, though, so I'd yield to those who do if they really consider these features to be worth maintaining.