This patch adds helper functions which are used in the NVMeOF configfs
when the user is configuring the passthru subsystem. Here we ensure
that only one subsys is assigned to each nvme_ctrl by using an xarray
on the cntlid.
The subsystem's version number is overridden by the passed through
controller's version. However, if that version is less than 1.2.1,
then we bump the advertised version to that and print a warning
in dmesg.
Based-on-a-patch-by: Chaitanya Kulkarni <[email protected]>
Signed-off-by: Logan Gunthorpe <[email protected]>
Reviewed-by: Sagi Grimberg <[email protected]>
---
drivers/nvme/target/configfs.c | 4 ++
drivers/nvme/target/core.c | 10 +++-
drivers/nvme/target/nvmet.h | 12 +++++
drivers/nvme/target/passthru.c | 87 ++++++++++++++++++++++++++++++++++
4 files changed, 112 insertions(+), 1 deletion(-)
diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 58cabd7b6fc5..e0ce6e5feb3a 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -826,6 +826,10 @@ static ssize_t nvmet_subsys_attr_version_store(struct config_item *item,
int major, minor, tertiary = 0;
int ret;
+ /* passthru subsystems use the underlying controller's version */
+ if (nvmet_passthru_ctrl(subsys))
+ return -EINVAL;
+
ret = sscanf(page, "%d.%d.%d\n", &major, &minor, &tertiary);
if (ret != 2 && ret != 3)
return -EINVAL;
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 2547e0d8951c..a2cdd35ffb1d 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -522,6 +522,12 @@ int nvmet_ns_enable(struct nvmet_ns *ns)
mutex_lock(&subsys->lock);
ret = 0;
+
+ if (nvmet_passthru_ctrl(subsys)) {
+ pr_info("cannot enable both passthru and regular namespaces for a single subsystem");
+ goto out_unlock;
+ }
+
if (ns->enabled)
goto out_unlock;
@@ -1421,7 +1427,7 @@ struct nvmet_subsys *nvmet_subsys_alloc(const char *subsysnqn,
if (!subsys)
return ERR_PTR(-ENOMEM);
- subsys->ver = NVME_VS(1, 3, 0); /* NVMe 1.3.0 */
+ subsys->ver = NVMET_DEFAULT_VS;
/* generate a random serial number as our controllers are ephemeral: */
get_random_bytes(&subsys->serial, sizeof(subsys->serial));
@@ -1463,6 +1469,8 @@ static void nvmet_subsys_free(struct kref *ref)
WARN_ON_ONCE(!list_empty(&subsys->namespaces));
+ nvmet_passthru_subsys_free(subsys);
+
kfree(subsys->subsysnqn);
kfree_rcu(subsys->model, rcuhead);
kfree(subsys);
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index deb996c90804..76c3a7cb9c89 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -20,6 +20,8 @@
#include <linux/blkdev.h>
#include <linux/radix-tree.h>
+#define NVMET_DEFAULT_VS NVME_VS(1, 3, 0)
+
#define NVMET_ASYNC_EVENTS 4
#define NVMET_ERROR_LOG_SLOTS 128
#define NVMET_NO_ERROR_LOC ((u16)-1)
@@ -240,6 +242,7 @@ struct nvmet_subsys {
#ifdef CONFIG_NVME_TARGET_PASSTHRU
struct nvme_ctrl *passthru_ctrl;
+ char *passthru_ctrl_path;
#endif /* CONFIG_NVME_TARGET_PASSTHRU */
};
@@ -523,6 +526,9 @@ static inline u32 nvmet_dsm_len(struct nvmet_req *req)
}
#ifdef CONFIG_NVME_TARGET_PASSTHRU
+void nvmet_passthru_subsys_free(struct nvmet_subsys *subsys);
+int nvmet_passthru_ctrl_enable(struct nvmet_subsys *subsys);
+void nvmet_passthru_ctrl_disable(struct nvmet_subsys *subsys);
u16 nvmet_parse_passthru_admin_cmd(struct nvmet_req *req);
u16 nvmet_parse_passthru_io_cmd(struct nvmet_req *req);
static inline struct nvme_ctrl *nvmet_passthru_ctrl(struct nvmet_subsys *subsys)
@@ -530,6 +536,12 @@ static inline struct nvme_ctrl *nvmet_passthru_ctrl(struct nvmet_subsys *subsys)
return subsys->passthru_ctrl;
}
#else /* CONFIG_NVME_TARGET_PASSTHRU */
+static inline void nvmet_passthru_subsys_free(struct nvmet_subsys *subsys)
+{
+}
+static inline void nvmet_passthru_ctrl_disable(struct nvmet_subsys *subsys)
+{
+}
static inline u16 nvmet_parse_passthru_admin_cmd(struct nvmet_req *req)
{
return 0;
diff --git a/drivers/nvme/target/passthru.c b/drivers/nvme/target/passthru.c
index 15b23521e59f..a11c3ef34041 100644
--- a/drivers/nvme/target/passthru.c
+++ b/drivers/nvme/target/passthru.c
@@ -11,6 +11,11 @@
#include "../host/nvme.h"
#include "nvmet.h"
+/*
+ * xarray to maintain one passthru subsystem per nvme controller.
+ */
+static DEFINE_XARRAY(passthru_subsystems);
+
static void nvmet_passthru_execute_cmd_work(struct work_struct *w)
{
struct nvmet_req *req = container_of(w, struct nvmet_req, p.work);
@@ -458,3 +463,85 @@ u16 nvmet_parse_passthru_admin_cmd(struct nvmet_req *req)
return NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
}
}
+
+int nvmet_passthru_ctrl_enable(struct nvmet_subsys *subsys)
+{
+ struct nvme_ctrl *ctrl;
+ int ret = -EINVAL;
+ void *old;
+
+ mutex_lock(&subsys->lock);
+ if (!subsys->passthru_ctrl_path)
+ goto out_unlock;
+ if (subsys->passthru_ctrl)
+ goto out_unlock;
+
+ if (subsys->nr_namespaces) {
+ pr_info("cannot enable both passthru and regular namespaces for a single subsystem");
+ goto out_unlock;
+ }
+
+ ctrl = nvme_ctrl_get_by_path(subsys->passthru_ctrl_path);
+ if (IS_ERR(ctrl)) {
+ ret = PTR_ERR(ctrl);
+ pr_err("failed to open nvme controller %s\n",
+ subsys->passthru_ctrl_path);
+
+ goto out_unlock;
+ }
+
+ old = xa_cmpxchg(&passthru_subsystems, ctrl->cntlid, NULL,
+ subsys, GFP_KERNEL);
+ if (xa_is_err(old)) {
+ ret = xa_err(old);
+ goto out_put_ctrl;
+ }
+
+ if (old)
+ goto out_put_ctrl;
+
+ subsys->passthru_ctrl = ctrl;
+ subsys->ver = ctrl->vs;
+
+ if (subsys->ver < NVME_VS(1, 2, 1)) {
+ pr_warn("nvme controller version is too old: %d.%d.%d, advertising 1.2.1\n",
+ (int)NVME_MAJOR(subsys->ver),
+ (int)NVME_MINOR(subsys->ver),
+ (int)NVME_TERTIARY(subsys->ver));
+ subsys->ver = NVME_VS(1, 2, 1);
+ }
+
+ mutex_unlock(&subsys->lock);
+ return 0;
+
+out_put_ctrl:
+ nvme_put_ctrl(ctrl);
+out_unlock:
+ mutex_unlock(&subsys->lock);
+ return ret;
+}
+
+static void __nvmet_passthru_ctrl_disable(struct nvmet_subsys *subsys)
+{
+ if (subsys->passthru_ctrl) {
+ xa_erase(&passthru_subsystems, subsys->passthru_ctrl->cntlid);
+ nvme_put_ctrl(subsys->passthru_ctrl);
+ }
+ subsys->passthru_ctrl = NULL;
+ subsys->ver = NVMET_DEFAULT_VS;
+}
+
+void nvmet_passthru_ctrl_disable(struct nvmet_subsys *subsys)
+{
+ mutex_lock(&subsys->lock);
+ __nvmet_passthru_ctrl_disable(subsys);
+ mutex_unlock(&subsys->lock);
+}
+
+void nvmet_passthru_subsys_free(struct nvmet_subsys *subsys)
+{
+ mutex_lock(&subsys->lock);
+ __nvmet_passthru_ctrl_disable(subsys);
+ mutex_unlock(&subsys->lock);
+ kfree(subsys->passthru_ctrl_path);
+}
--
2.20.1
On 5/14/20 10:23 AM, Logan Gunthorpe wrote:
> + if (subsys->nr_namespaces) {
> + pr_info("cannot enable both passthru and regular namespaces for a single subsystem");
Let's try and keep the error message witin 80 char per line or split the
message into two pr_info() calls,how about this ?
pr_info("cannot enable passthru & regular namespaces\n")
On 2020-06-11 5:05 p.m., Chaitanya Kulkarni wrote:
> On 5/14/20 10:23 AM, Logan Gunthorpe wrote:
>> + if (subsys->nr_namespaces) {
>> + pr_info("cannot enable both passthru and regular namespaces for a single subsystem");
>
> Let's try and keep the error message witin 80 char per line or split the
> message into two pr_info() calls,how about this ?
>
>
> pr_info("cannot enable passthru & regular namespaces\n")
Honestly, I think that is too brief. The error message is only 74 chars
and there's a long standing exception for long lines in the kernel for
printks. Even Linus has recently suggested that keeping to the 80 char
limit is not recommended when it harms readability (though I don't
generally agree with this 100%)[1].
Logan
[1]
https://lwn.net/ml/linux-kernel/CAHk-=wjR0H3+2ba0UUWwoYzYBH0GX9yTf5dj2MZyo0xvyzvJnA@mail.gmail.com/
On 5/14/20 10:23 AM, Logan Gunthorpe wrote:
> + if (subsys->ver < NVME_VS(1, 2, 1)) {
> + pr_warn("nvme controller version is too old: %d.%d.%d, advertising 1.2.1\n",
Is it more than 80 char ? can it be ?
pr_warn("nvme controller version is too old: ");
pr_warn("%d.%d.%d, advertising 1.2.1\n",
> + (int)NVME_MAJOR(subsys->ver),
> + (int)NVME_MINOR(subsys->ver),
> + (int)NVME_TERTIARY(subsys->ver));
> + subsys->ver = NVME_VS(1, 2, 1);
> + }
NVMe blktests are running on QEMU based controller. This will generate
warning every-time.
Also, I didn't understand int type cast, I wonder under what condition
all these macros will return -ve values since ver of type u64 ?
On 2020-06-11 5:13 p.m., Chaitanya Kulkarni wrote:
> On 5/14/20 10:23 AM, Logan Gunthorpe wrote:
>> + if (subsys->ver < NVME_VS(1, 2, 1)) {
>> + pr_warn("nvme controller version is too old: %d.%d.%d, advertising 1.2.1\n",
> Is it more than 80 char ? can it be ?
> pr_warn("nvme controller version is too old: ");
> pr_warn("%d.%d.%d, advertising 1.2.1\n",
Splitting printks is a bad idea because they won't necessarily end up in
dmesg right next to each other -- other prints can be interleaved.
Moreover, it breaks any tools that are dealing with kernel logs as
single records (ie through /dev/kmsg). Continuations (or printks without
a trailing "\n") would require using pr_cont() and generally that's
discouraged.
>> + (int)NVME_MAJOR(subsys->ver),
>> + (int)NVME_MINOR(subsys->ver),
>> + (int)NVME_TERTIARY(subsys->ver));
>> + subsys->ver = NVME_VS(1, 2, 1);
>> + }
>
> NVMe blktests are running on QEMU based controller. This will generate
> warning every-time.
Yup.
> Also, I didn't understand int type cast, I wonder under what condition
> all these macros will return -ve values since ver of type u64 ?
Yes, I'm not sure what I was thinking when I put in the cast. Probably
better just to prints "%llu". I'll fix this for the next revision.
Thanks,
Logan
On 6/11/20 4:15 PM, Logan Gunthorpe wrote:
> Honestly, I think that is too brief. The error message is only 74 chars
> and there's a long standing exception for long lines in the kernel for
> printks. Even Linus has recently suggested that keeping to the 80 char
> limit is not recommended when it harms readability (though I don't
> generally agree with this 100%)[1].
I'm aware of that and main cause is it creates unnecessary breaking
which can lead to bugs, hard to read code and ugly wrapped code. I think
this case is different with printing message which is straight forward
to read and not going to introduce any bugs in actual logic.
Based on your earlier reply about splitting printk I'll let Christoph
and Sagi decide on what to do about this and whether to enforce strict
80 lines or not for NVMe subsystem for printing messages and for future
code.