On Fri, Oct 18, 2019 at 10:10:05AM -0700, Bart Van Assche wrote:
> On 10/18/19 8:22 AM, Honggang LI wrote:
> > [ ... ]
>
> Hi Honggang,
>
> The approach of your patch seems suboptimal to me. I think it would be
> cumbersome for users to have to modify the srp_daemon_port@.service 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 see.
>
> 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.
Yes, I will remove the new command-line and append max_it_iu_size at
the very end of login string.
> * Modify the ib_srp driver such that it exports the login parameters
This hints me to check the value of
/sys/module/ib_srp/parameters/use_imm_data .
The regression issue was introduced because of using immediate data.
The new patch will append max_it_iu_size only when use_imm_data enabled.
For old ib_srp client does not support immediate date, the new patch will
not use max_it_iu_size. It will not trigger the kernel warning message
of unsupported parameter.
Please see this patch.
diff --git a/srp_daemon/srp_daemon.c b/srp_daemon/srp_daemon.c
index f0bcf923..24451b87 100644
--- a/srp_daemon/srp_daemon.c
+++ b/srp_daemon/srp_daemon.c
@@ -402,6 +402,28 @@ static int is_enabled_by_rules_file(struct target_details *target)
}
+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);
+ return ret;
+#else
+ return false;
+#endif
+}
static int add_non_exist_target(struct target_details *target)
{
@@ -581,6 +603,19 @@ static int add_non_exist_target(struct target_details *target)
}
}
+ if (use_imm_data()) {
+ 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;
+ }
+ }
+
target_config_str[len] = '\0';
pr_cmd(target_config_str, not_connected);
@@ -1360,6 +1395,10 @@ static void print_config(struct config_t *conf)
printf(" Print initiator_ext\n");
else
printf(" Do not print initiator_ext\n");
+ if (use_imm_data())
+ printf(" Print maximum initiator to target IU size\n");
+ else
+ printf(" Do not print maximum initiator to target IU size\n");
if (conf->recalc_time)
printf(" Performs full target rescan every %d seconds\n", conf->recalc_time);
else
