LinuxLists.cc - [PATCH] nvme: update keep alive interval when kato is modified

2021-08-02 01:02:35

Subject: [PATCH] nvme: update keep alive interval when kato is modified

2021-08-02 04:34:31

Subject: Re: [PATCH] nvme: update keep alive interval when kato is modified

Hi sasaki,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on block/for-next]
[also build test WARNING on hch-configfs/for-next linus/master v5.14-rc3 next-20210730]
[cannot apply to linux-nvme/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url: https://github.com/0day-ci/linux/commits/sasaki-tatsuya/nvme-update-keep-alive-interval-when-kato-is-modified/20210802-090235
base: https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
config: i386-randconfig-s002-20210802 (attached as .config)
compiler: gcc-10 (Ubuntu 10.3.0-1ubuntu1~20.04) 10.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.3-341-g8af24329-dirty
# https://github.com/0day-ci/linux/commit/eda2903523c28b51997fb071c0ff3653081c8a79
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review sasaki-tatsuya/nvme-update-keep-alive-interval-when-kato-is-modified/20210802-090235
git checkout eda2903523c28b51997fb071c0ff3653081c8a79
# save the attached .config to linux build tree
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 SHELL=/bin/bash drivers/nvme/host/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>

sparse warnings: (new ones prefixed by >>)
>> drivers/nvme/host/ioctl.c:239:15: sparse: sparse: cast from restricted __le32
>> drivers/nvme/host/ioctl.c:241:41: sparse: sparse: restricted __le32 degrades to integer

vim +239 drivers/nvme/host/ioctl.c

189
190 static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
191 struct nvme_passthru_cmd __user *ucmd)
192 {
193 struct nvme_passthru_cmd cmd;
194 struct nvme_command c;
195 unsigned timeout = 0;
196 u64 result;
197 int status;
198
199 if (!capable(CAP_SYS_ADMIN))
200 return -EACCES;
201 if (copy_from_user(&cmd, ucmd, sizeof(cmd)))
202 return -EFAULT;
203 if (cmd.flags)
204 return -EINVAL;
205 if (!nvme_validate_passthru_nsid(ctrl, ns, cmd.nsid))
206 return -EINVAL;
207
208 memset(&c, 0, sizeof(c));
209 c.common.opcode = cmd.opcode;
210 c.common.flags = cmd.flags;
211 c.common.nsid = cpu_to_le32(cmd.nsid);
212 c.common.cdw2[0] = cpu_to_le32(cmd.cdw2);
213 c.common.cdw2[1] = cpu_to_le32(cmd.cdw3);
214 c.common.cdw10 = cpu_to_le32(cmd.cdw10);
215 c.common.cdw11 = cpu_to_le32(cmd.cdw11);
216 c.common.cdw12 = cpu_to_le32(cmd.cdw12);
217 c.common.cdw13 = cpu_to_le32(cmd.cdw13);
218 c.common.cdw14 = cpu_to_le32(cmd.cdw14);
219 c.common.cdw15 = cpu_to_le32(cmd.cdw15);
220
221 if (cmd.timeout_ms)
222 timeout = msecs_to_jiffies(cmd.timeout_ms);
223
224 status = nvme_submit_user_cmd(ns ? ns->queue : ctrl->admin_q, &c,
225 nvme_to_user_ptr(cmd.addr), cmd.data_len,
226 nvme_to_user_ptr(cmd.metadata), cmd.metadata_len,
227 0, &result, timeout);
228
229 if (status >= 0) {
230 if (put_user(result, &ucmd->result))
231 return -EFAULT;
232 }
233
234 /*
235 * Keep alive commands interval on the host should be updated
236 * when KATO is modified by Set Features commands.
237 */
238 if (!status && c.common.opcode == nvme_admin_set_features &&
> 239 ((u8)c.common.cdw10 & 0xFF) == NVME_FEAT_KATO) {
240 /* ms -> s */
> 241 unsigned int new_kato = DIV_ROUND_UP(c.common.cdw11, 1000);
242
243 dev_info(ctrl->device,
244 "keep alive commands interval on the host is updated from %u milliseconds to %u milliseconds\n",
245 ctrl->kato * 1000 / 2, new_kato * 1000 / 2);
246 nvme_stop_keep_alive(ctrl);
247 ctrl->kato = new_kato;
248 nvme_start_keep_alive(ctrl);
249 }
250
251 return status;
252 }
253

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]

Attachments:

(No filename) (4.17 kB)
.config.gz (39.62 kB)
Download all attachments

2021-08-03 02:15:16

by sasaki tatsuya

[permalink] [raw]

Subject: RE: [PATCH] nvme: update keep alive interval when kato is modified

Currently the connection between host and NVMe-oF target gets
disconnected by keep-alive timeout when a user connects to a target
with a relatively large kato value and then sets the smaller kato
with a set features command (e.g. connects with 60 seconds kato value
and then sets 10 seconds kato value).

The cause is that keep alive command interval on the host, which is
defined as unsigned int kato in nvme_ctrl structure, does not follow
the kato value changes.

This patch updates the keep alive interval in the following steps when
the kato is modified by a set features command: stops the keep alive
work queue, then sets the kato as new timer value and re-start the queue.

Signed-off-by: Tatsuya Sasaki <[email protected]>
---
drivers/nvme/host/core.c | 3 ++-
drivers/nvme/host/ioctl.c | 17 +++++++++++++++++
drivers/nvme/host/nvme.h | 1 +
3 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index dfd9dec0c1f6..89c52da15618 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1255,13 +1255,14 @@ static void nvme_keep_alive_work(struct work_struct *work)
blk_execute_rq_nowait(NULL, rq, 0, nvme_keep_alive_end_io);
}

-static void nvme_start_keep_alive(struct nvme_ctrl *ctrl)
+void nvme_start_keep_alive(struct nvme_ctrl *ctrl)
{
if (unlikely(ctrl->kato == 0))
return;

nvme_queue_keep_alive_work(ctrl);
}
+EXPORT_SYMBOL_GPL(nvme_start_keep_alive);

void nvme_stop_keep_alive(struct nvme_ctrl *ctrl)
{
diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
index 305ddd415e45..0066728e77b2 100644
--- a/drivers/nvme/host/ioctl.c
+++ b/drivers/nvme/host/ioctl.c
@@ -231,6 +231,23 @@ static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
return -EFAULT;
}

+ /*
+ * Keep alive commands interval on the host should be updated
+ * when KATO is modified by Set Features commands.
+ */
+ if (!status && cmd.opcode == nvme_admin_set_features &&
+ (cmd.cdw10 & 0xFF) == NVME_FEAT_KATO) {
+ /* ms -> s */
+ unsigned int new_kato = DIV_ROUND_UP(cmd.cdw11, 1000);
+
+ dev_info(ctrl->device,
+ "keep alive commands interval on the host is updated from %u milliseconds to %u milliseconds\n",
+ ctrl->kato * 1000 / 2, new_kato * 1000 / 2);
+ nvme_stop_keep_alive(ctrl);
+ ctrl->kato = new_kato;
+ nvme_start_keep_alive(ctrl);
+ }
+
return status;
}

diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 5cd1fa3b8464..d4066b7c5fc8 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -666,6 +666,7 @@ int nvme_get_features(struct nvme_ctrl *dev, unsigned int fid,
unsigned int dword11, void *buffer, size_t buflen,
u32 *result);
int nvme_set_queue_count(struct nvme_ctrl *ctrl, int *count);
+void nvme_start_keep_alive(struct nvme_ctrl *ctrl);
void nvme_stop_keep_alive(struct nvme_ctrl *ctrl);
int nvme_reset_ctrl(struct nvme_ctrl *ctrl);
int nvme_reset_ctrl_sync(struct nvme_ctrl *ctrl);
--
2.25.1

-----Original Message-----
From: kernel test robot <[email protected]>
Sent: Monday, August 2, 2021 1:32 PM
To: sasaki tatsuya($B:4!9LZ(B $BC#:H(B $B#K#I#C(B $B!{#S5;#C""#S#S3+!{#S#S0l(B) <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Cc: [email protected]
Subject: Re: [PATCH] nvme: update keep alive interval when kato is modified

Hi sasaki,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on block/for-next]
[also build test WARNING on hch-configfs/for-next linus/master v5.14-rc3 next-20210730]
[cannot apply to linux-nvme/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url: https://github.com/0day-ci/linux/commits/sasaki-tatsuya/nvme-update-keep-alive-interval-when-kato-is-modified/20210802-090235
base: https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next
config: i386-randconfig-s002-20210802 (attached as .config)
compiler: gcc-10 (Ubuntu 10.3.0-1ubuntu1~20.04) 10.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.3-341-g8af24329-dirty
# https://github.com/0day-ci/linux/commit/eda2903523c28b51997fb071c0ff3653081c8a79
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review sasaki-tatsuya/nvme-update-keep-alive-interval-when-kato-is-modified/20210802-090235
git checkout eda2903523c28b51997fb071c0ff3653081c8a79
# save the attached .config to linux build tree
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 SHELL=/bin/bash drivers/nvme/host/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>

sparse warnings: (new ones prefixed by >>)
>> drivers/nvme/host/ioctl.c:239:15: sparse: sparse: cast from restricted __le32
>> drivers/nvme/host/ioctl.c:241:41: sparse: sparse: restricted __le32 degrades to integer

vim +239 drivers/nvme/host/ioctl.c

189
190 static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
191 struct nvme_passthru_cmd __user *ucmd)
192 {
193 struct nvme_passthru_cmd cmd;
194 struct nvme_command c;
195 unsigned timeout = 0;
196 u64 result;
197 int status;
198
199 if (!capable(CAP_SYS_ADMIN))
200 return -EACCES;
201 if (copy_from_user(&cmd, ucmd, sizeof(cmd)))
202 return -EFAULT;
203 if (cmd.flags)
204 return -EINVAL;
205 if (!nvme_validate_passthru_nsid(ctrl, ns, cmd.nsid))
206 return -EINVAL;
207
208 memset(&c, 0, sizeof(c));
209 c.common.opcode = cmd.opcode;
210 c.common.flags = cmd.flags;
211 c.common.nsid = cpu_to_le32(cmd.nsid);
212 c.common.cdw2[0] = cpu_to_le32(cmd.cdw2);
213 c.common.cdw2[1] = cpu_to_le32(cmd.cdw3);
214 c.common.cdw10 = cpu_to_le32(cmd.cdw10);
215 c.common.cdw11 = cpu_to_le32(cmd.cdw11);
216 c.common.cdw12 = cpu_to_le32(cmd.cdw12);
217 c.common.cdw13 = cpu_to_le32(cmd.cdw13);
218 c.common.cdw14 = cpu_to_le32(cmd.cdw14);
219 c.common.cdw15 = cpu_to_le32(cmd.cdw15);
220
221 if (cmd.timeout_ms)
222 timeout = msecs_to_jiffies(cmd.timeout_ms);
223
224 status = nvme_submit_user_cmd(ns ? ns->queue : ctrl->admin_q, &c,
225 nvme_to_user_ptr(cmd.addr), cmd.data_len,
226 nvme_to_user_ptr(cmd.metadata), cmd.metadata_len,
227 0, &result, timeout);
228
229 if (status >= 0) {
230 if (put_user(result, &ucmd->result))
231 return -EFAULT;
232 }
233
234 /*
235 * Keep alive commands interval on the host should be updated
236 * when KATO is modified by Set Features commands.
237 */
238 if (!status && c.common.opcode == nvme_admin_set_features &&
> 239 ((u8)c.common.cdw10 & 0xFF) == NVME_FEAT_KATO) {
240 /* ms -> s */
> 241 unsigned int new_kato = DIV_ROUND_UP(c.common.cdw11, 1000);
242
243 dev_info(ctrl->device,
244 "keep alive commands interval on the host is updated from %u milliseconds to %u milliseconds\n",
245 ctrl->kato * 1000 / 2, new_kato * 1000 / 2);
246 nvme_stop_keep_alive(ctrl);
247 ctrl->kato = new_kato;
248 nvme_start_keep_alive(ctrl);
249 }
250
251 return status;
252 }
253

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]

2021-08-07 00:11:18

by Sagi Grimberg

[permalink] [raw]

Subject: Re: [PATCH] nvme: update keep alive interval when kato is modified

On 8/2/21 7:12 PM, sasaki tatsuya wrote:
> Currently the connection between host and NVMe-oF target gets
> disconnected by keep-alive timeout when a user connects to a target
> with a relatively large kato value and then sets the smaller kato
> with a set features command (e.g. connects with 60 seconds kato value
> and then sets 10 seconds kato value).
>
> The cause is that keep alive command interval on the host, which is
> defined as unsigned int kato in nvme_ctrl structure, does not follow
> the kato value changes.
>
> This patch updates the keep alive interval in the following steps when
> the kato is modified by a set features command: stops the keep alive
> work queue, then sets the kato as new timer value and re-start the queue.
>
> Signed-off-by: Tatsuya Sasaki <[email protected]>
> ---
> drivers/nvme/host/core.c | 3 ++-
> drivers/nvme/host/ioctl.c | 17 +++++++++++++++++
> drivers/nvme/host/nvme.h | 1 +
> 3 files changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index dfd9dec0c1f6..89c52da15618 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1255,13 +1255,14 @@ static void nvme_keep_alive_work(struct work_struct *work)
> blk_execute_rq_nowait(NULL, rq, 0, nvme_keep_alive_end_io);
> }
>
> -static void nvme_start_keep_alive(struct nvme_ctrl *ctrl)
> +void nvme_start_keep_alive(struct nvme_ctrl *ctrl)
> {
> if (unlikely(ctrl->kato == 0))
> return;
>
> nvme_queue_keep_alive_work(ctrl);
> }
> +EXPORT_SYMBOL_GPL(nvme_start_keep_alive);
>
> void nvme_stop_keep_alive(struct nvme_ctrl *ctrl)
> {
> diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
> index 305ddd415e45..0066728e77b2 100644
> --- a/drivers/nvme/host/ioctl.c
> +++ b/drivers/nvme/host/ioctl.c
> @@ -231,6 +231,23 @@ static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
> return -EFAULT;
> }
>
> + /*
> + * Keep alive commands interval on the host should be updated
> + * when KATO is modified by Set Features commands.
> + */

This is too specific. I'd move it to nvme_user_cmd_post similar to how
we handle passthru commands (with start/end that does stuff based on the
effects)

> + if (!status && cmd.opcode == nvme_admin_set_features &&
> + (cmd.cdw10 & 0xFF) == NVME_FEAT_KATO) {
> + /* ms -> s */
> + unsigned int new_kato = DIV_ROUND_UP(cmd.cdw11, 1000);
> +

The section below needs to move to a core rountine:
nvme_update_keep_alive()

> + dev_info(ctrl->device,
> + "keep alive commands interval on the host is updated from %u milliseconds to %u milliseconds\n",
> + ctrl->kato * 1000 / 2, new_kato * 1000 / 2);
> + nvme_stop_keep_alive(ctrl);
> + ctrl->kato = new_kato;
> + nvme_start_keep_alive(ctrl);
> + }
> +
> return status;
> }
>

2021-08-19 10:17:55

by sasaki tatsuya

[permalink] [raw]

Subject: RE: [PATCH] nvme: update keep alive interval when kato is modified

> This is too specific. I'd move it to nvme_user_cmd_post similar to how
> we handle passthru commands (with start/end that does stuff based on the
> effects)

Thank you for your comment.
I will recreate this patch which adds nvme_user_cmd_post
for update keep alive timer and move update routine to core.c.

-----Original Message-----
From: Sagi Grimberg <[email protected]>
Sent: Saturday, August 7, 2021 5:07 AM
To: sasaki tatsuya($B:4!9LZ(B $BC#:H(B $B#K#I#C(B $B!{#S5;#C""#S#S3+!{#S#S0l(B) <[email protected]>; kernel test robot <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH] nvme: update keep alive interval when kato is modified

On 8/2/21 7:12 PM, sasaki tatsuya wrote:
> Currently the connection between host and NVMe-oF target gets
> disconnected by keep-alive timeout when a user connects to a target
> with a relatively large kato value and then sets the smaller kato
> with a set features command (e.g. connects with 60 seconds kato value
> and then sets 10 seconds kato value).
>
> The cause is that keep alive command interval on the host, which is
> defined as unsigned int kato in nvme_ctrl structure, does not follow
> the kato value changes.
>
> This patch updates the keep alive interval in the following steps when
> the kato is modified by a set features command: stops the keep alive
> work queue, then sets the kato as new timer value and re-start the queue.
>
> Signed-off-by: Tatsuya Sasaki <[email protected]>
> ---
> drivers/nvme/host/core.c | 3 ++-
> drivers/nvme/host/ioctl.c | 17 +++++++++++++++++
> drivers/nvme/host/nvme.h | 1 +
> 3 files changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index dfd9dec0c1f6..89c52da15618 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1255,13 +1255,14 @@ static void nvme_keep_alive_work(struct work_struct *work)
> blk_execute_rq_nowait(NULL, rq, 0, nvme_keep_alive_end_io);
> }
>
> -static void nvme_start_keep_alive(struct nvme_ctrl *ctrl)
> +void nvme_start_keep_alive(struct nvme_ctrl *ctrl)
> {
> if (unlikely(ctrl->kato == 0))
> return;
>
> nvme_queue_keep_alive_work(ctrl);
> }
> +EXPORT_SYMBOL_GPL(nvme_start_keep_alive);
>
> void nvme_stop_keep_alive(struct nvme_ctrl *ctrl)
> {
> diff --git a/drivers/nvme/host/ioctl.c b/drivers/nvme/host/ioctl.c
> index 305ddd415e45..0066728e77b2 100644
> --- a/drivers/nvme/host/ioctl.c
> +++ b/drivers/nvme/host/ioctl.c
> @@ -231,6 +231,23 @@ static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
> return -EFAULT;
> }
>
> + /*
> + * Keep alive commands interval on the host should be updated
> + * when KATO is modified by Set Features commands.
> + */

This is too specific. I'd move it to nvme_user_cmd_post similar to how
we handle passthru commands (with start/end that does stuff based on the
effects)

> + if (!status && cmd.opcode == nvme_admin_set_features &&
> + (cmd.cdw10 & 0xFF) == NVME_FEAT_KATO) {
> + /* ms -> s */
> + unsigned int new_kato = DIV_ROUND_UP(cmd.cdw11, 1000);
> +

The section below needs to move to a core rountine:
nvme_update_keep_alive()

> + dev_info(ctrl->device,
> + "keep alive commands interval on the host is updated from %u milliseconds to %u milliseconds\n",
> + ctrl->kato * 1000 / 2, new_kato * 1000 / 2);
> + nvme_stop_keep_alive(ctrl);
> + ctrl->kato = new_kato;
> + nvme_start_keep_alive(ctrl);
> + }
> +
> return status;
> }
>