[PATCHv2] nvme: add 'iopolicy' module parameter

[PATCHv2] nvme: add 'iopolicy' module parameter

[Hannes Reinecke]

While the 'iopolicy' sysfs attribute can be set at runtime, most
storage arrays prefer to use the 'round-robin' iopolicy per default.
We can use udev rules to set this, but is getting rather unwieldy
for rebranded arrays as we would have to update the udev rules
anytime a new array shows up, leading to the same mess we currently
have in multipathd for configuring the RDAC arrays.

Hence this patch adds a module parameter 'iopolicy' to allow the
admin to switch the default, and to do away with the need for a
udev rule here.

Signed-off-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
---
 drivers/nvme/host/core.c      |  4 +---
 drivers/nvme/host/multipath.c | 10 ++++++++++
 drivers/nvme/host/nvme.h      |  4 ++++
 3 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 44c375a1edbb..be404004c235 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2747,9 +2747,7 @@ static int nvme_init_subsystem(struct nvme_ctrl *ctrl, struct nvme_id_ctrl *id)
 		return -EINVAL;
 	}
 	subsys->awupf = le16_to_cpu(id->awupf);
-#ifdef CONFIG_NVME_MULTIPATH
-	subsys->iopolicy = NVME_IOPOLICY_NUMA;
-#endif
+	nvme_mpath_default_iopolicy(subsys);
 
 	subsys->dev.class = nvme_subsys_class;
 	subsys->dev.release = nvme_release_subsystem;
diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index 7f2071f2460c..88dceeb73334 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -13,6 +13,16 @@ module_param(multipath, bool, 0444);
 MODULE_PARM_DESC(multipath,
 	"turn on native support for multiple controllers per subsystem");
 
+static int iopolicy = NVME_IOPOLICY_NUMA;
+module_param(iopolicy, int, 0444);
+MODULE_PARM_DESC(iopolicy,
+	"Default multipath I/O policy; 0 - NUMA (default), 1 - Round-robin");
+
+void nvme_mpath_default_iopolicy(struct nvme_subsystem *subsys)
+{
+	subsys->iopolicy = iopolicy;
+}
+
 void nvme_mpath_unfreeze(struct nvme_subsystem *subsys)
 {
 	struct nvme_ns_head *h;
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index a54096ba0552..fe224016418e 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -767,6 +767,7 @@ static inline bool nvme_ctrl_use_ana(struct nvme_ctrl *ctrl)
 void nvme_mpath_unfreeze(struct nvme_subsystem *subsys);
 void nvme_mpath_wait_freeze(struct nvme_subsystem *subsys);
 void nvme_mpath_start_freeze(struct nvme_subsystem *subsys);
+void nvme_mpath_default_iopolicy(struct nvme_subsystem *subsys);
 bool nvme_mpath_set_disk_name(struct nvme_ns *ns, char *disk_name, int *flags);
 void nvme_failover_req(struct request *req);
 void nvme_kick_requeue_lists(struct nvme_ctrl *ctrl);
@@ -864,6 +865,9 @@ static inline void nvme_mpath_wait_freeze(struct nvme_subsystem *subsys)
 static inline void nvme_mpath_start_freeze(struct nvme_subsystem *subsys)
 {
 }
+static inline void nvme_mpath_default_iopolicy(struct nvme_subsystem *subsys)
+{
+}
 #endif /* CONFIG_NVME_MULTIPATH */
 
 int nvme_revalidate_zones(struct nvme_ns *ns);
-- 
2.29.2

Re: [PATCHv2] nvme: add 'iopolicy' module parameter

[Daniel Wagner]

On Fri, Dec 17, 2021 at 04:02:22PM +0100, Hannes Reinecke wrote:
> +static int iopolicy = NVME_IOPOLICY_NUMA;
> +module_param(iopolicy, int, 0444);
> +MODULE_PARM_DESC(iopolicy,
> +	"Default multipath I/O policy; 0 - NUMA (default), 1 - Round-robin");
> +
> +void nvme_mpath_default_iopolicy(struct nvme_subsystem *subsys)
> +{
> +	subsys->iopolicy = iopolicy;

Wouldn't make sense to check if iopolicy has a valid value too?

Re: [PATCHv2] nvme: add 'iopolicy' module parameter

[Keith Busch]

On Fri, Dec 17, 2021 at 07:03:58PM +0100, Daniel Wagner wrote:
> On Fri, Dec 17, 2021 at 04:02:22PM +0100, Hannes Reinecke wrote:
> > +static int iopolicy = NVME_IOPOLICY_NUMA;
> > +module_param(iopolicy, int, 0444);
> > +MODULE_PARM_DESC(iopolicy,
> > +	"Default multipath I/O policy; 0 - NUMA (default), 1 - Round-robin");
> > +
> > +void nvme_mpath_default_iopolicy(struct nvme_subsystem *subsys)
> > +{
> > +	subsys->iopolicy = iopolicy;
> 
> Wouldn't make sense to check if iopolicy has a valid value too?
> 

Even better would be to ensure the iopolicy can never be invalid in the
first place. If the module param is invalid, then the sysfs attribute
for it will run off the end of the array, among other problems.

The sysfs toggle already ensures a valid iopolicy, so just need
something similar for the module parameter. You can append a callback to
the module parameter (module_param_cb) to enforce valid values only, and
I think using that would be appropriate here.

Re: [PATCHv2] nvme: add 'iopolicy' module parameter

[Hannes Reinecke]

On 12/17/21 7:03 PM, Daniel Wagner wrote:
> On Fri, Dec 17, 2021 at 04:02:22PM +0100, Hannes Reinecke wrote:
>> +static int iopolicy = NVME_IOPOLICY_NUMA;
>> +module_param(iopolicy, int, 0444);
>> +MODULE_PARM_DESC(iopolicy,
>> +	"Default multipath I/O policy; 0 - NUMA (default), 1 - Round-robin");
>> +
>> +void nvme_mpath_default_iopolicy(struct nvme_subsystem *subsys)
>> +{
>> +	subsys->iopolicy = iopolicy;
> 
> Wouldn't make sense to check if iopolicy has a valid value too?
> 
At this time it doesn't matter, as the only values we're checking of are 
'0' or '1' (or, rather, any non-zero value).
But yeah, one probably should check here.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer