srp_daemon: Use maximum initiator to target IU size

srp_daemon/srp_daemon.c static int add_non_exist_target(struct target_details *target) · line 571

Quoted 12 lines · Honggang LI

srp_daemon/srp_daemon.c +12 -0

+	if (config->print_max_it_iu_size) {
+		len += snprintf(target_config_str+len,
+				sizeof(target_config_str) - len,
+				",max_it_iu_size=%d",
+				be32toh(target->ioc_prof.send_size));
+
+		if (len >= sizeof(target_config_str)) {
+			pr_err("Target config string is too long, ignoring target\n");
+			closedir(dir);
+			return -1;
+		}
+	}

Hi Honggang,

I think this patch will make srp_daemon incompatible with versions of

the ib_srp kernel driver that do not support the max_it_iu_size parameter and also that that's unacceptable. How about the following approach:
* Do not add a new command-line option.

* Add max_it_iu_size at the end. I think that approach will trigger a

warning with older versions of the SRP kernel driver but also that it won't break SRP login.

Thanks,

Bart.

[ ... ]

Quoted 1 line · Honggang LI

[ ... ]

Hi Honggang,

The approach of your patch seems suboptimal to me. I think it would be cumbersome for users to have to modify the [email protected] file to suppress a kernel warning or to pass the max_it_iu_size parameter during login.

Had you noticed that the SRP initiator stops parsing parameters if it encounters an unrecognized parameter?

From ib_srp.c, function srp_parse_options():

	default:
		pr_warn("unknown parameter or missing value '%s' in"
			" target creation request\n", p);
			goto out;
		}

The "goto out" breaks out of the while loop that parses options. We cannot change this behavior in existing versions of the SRP initiator kernel driver. In other words, if the max_it_iu_size parameter is not specified at the very end of the login string it will cause some login parameters to be ignored.

I think we should use one of the following two approaches:
* Not add a new command-line option to srp_daemon and add the max_it_iu_size at the very end of the login string.

* Modify the ib_srp driver such that it exports the login parameters

srp_daemon/srp_daemon.c +12 -0

+static bool use_imm_data(void)
+{
+#ifdef __linux__
+	bool ret = false;
+	char flag = 0;
+	int cnt;
+	int fd = open("/sys/module/ib_srp/parameters/use_imm_data", O_RDONLY);
+
+	if (fd < 0)
+		return false;
+	cnt = read(fd, &flag, 1);
+	if (cnt != 1)

Quoted 17 lines · Honggang LI

srp_daemon/srp_daemon.c +17 -0

+static bool use_imm_data(void)
+{
+#ifdef __linux__
+	bool ret = false;
+	char flag = 0;
+	int cnt;
+	int fd = open("/sys/module/ib_srp/parameters/use_imm_data", O_RDONLY);
+
+	if (fd < 0)
+		return false;
+	cnt = read(fd, &flag, 1);
+	if (cnt != 1)
+		return false;
+
+	if (!strncmp(&flag, "Y", 1))
+		ret = true;
+	close(fd);

srp_daemon/srp_daemon.c static int is_enabled_by_rules_file(struct target_details *target) · line 426

Quoted 5 lines

srp_daemon/srp_daemon.c +5 -0

+	return ret;
+#else
+	return false;
+#endif
+}

There is already plenty of Linux-specific code in srp_daemon. The #ifdef __linux__ / #endif guard does not seem useful to me.

There is a file descriptor leak in the above function, namely if read() returns another value than 1.

The use_imm_data kernel module parameter was introduced in kernel v5.0 (commit 882981f4a411; "RDMA/srp: Add support for immediate data"). The max_it_iu_size will be introduced in kernel v5.5 (commit 547ed331bbe8; "RDMA/srp: Add parse function for maximum initiator to target IU size").

So the above check will help for kernel versions before v5.0 but not for kernel versions [v5.0..v5.5).

If that is really what you want, please
explain this in a comment above the use_imm_data() function.

Quoted 18 lines · Bart Van Assche

Quoted 17 lines · Honggang LI

(quoted diff) +17 -0

+static bool use_imm_data(void)
+{
+#ifdef __linux__
+	bool ret = false;
+	char flag = 0;
+	int cnt;
+	int fd = open("/sys/module/ib_srp/parameters/use_imm_data", O_RDONLY);
+
+	if (fd < 0)
+		return false;
+	cnt = read(fd, &flag, 1);
+	if (cnt != 1)
+		return false;
+
+	if (!strncmp(&flag, "Y", 1))
+		ret = true;
+	close(fd);

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+	return ret;
+#else
+	return false;
+#endif
+}

There is already plenty of Linux-specific code in srp_daemon. The #ifdef __linux__ / #endif guard does not seem useful to me.

Will delete the guard.

Quoted 3 lines

There is a file descriptor leak in the above function, namely if read() returns another value than 1.

Yes, will fix it.

Quoted 6 lines

The use_imm_data kernel module parameter was introduced in kernel v5.0 (commit 882981f4a411; "RDMA/srp: Add support for immediate data"). The max_it_iu_size will be introduced in kernel v5.5 (commit 547ed331bbe8; "RDMA/srp: Add parse function for maximum initiator to target IU size").

Quoted 2 lines

So the above check will help for kernel versions before v5.0 but not for kernel versions [v5.0..v5.5).

Yes, you are right. The patch does not fix the issue for kernel versions [v5.0..v5.5). But it also does not do anything bad for kernel versions before v5.5 (commit 547ed331bbe8). It will fix the issue for kernel after 547ed331bbe8.

drivers/infiniband/ulp/srp/ib_srp.c +5 -0

index b7f7a5f7bd98..96434f743a91 100644
@@ -74,6 +74,7 @@ static bool allow_ext_sg;
 static bool prefer_fr = true;
 static bool register_always = true;
 static bool never_register;
+static bool has_max_it_iu_size = true;
 static int topspin_workarounds = 1;
 
 module_param(srp_sg_tablesize, uint, 0444);

@@ -103,6 +104,10 @@ module_param(register_always, bool, 0444);
 MODULE_PARM_DESC(register_always,
 		 "Use memory registration even for contiguous memory regions");
 
+module_param(has_max_it_iu_size, bool, 0444);
+MODULE_PARM_DESC(has_max_it_iu_size,
+		  "Indicate the module supports max_it_iu_size login parameter");
+
 module_param(never_register, bool, 0444);
 MODULE_PARM_DESC(never_register, "Never register memory");
 

==============
Then, srp_daemon should check file "/sys/module/ib_srp/parameters/has_max_it_iu_size" instead of "/sys/module/ib_srp/parameters/use_imm_data".

This should work but I'm not sure this is the best approach we can come up with. Will one new kernel module parameter be added to the ib_srp kernel module every time a login parameter is added?