[PATCH 00/12] nvmet: add error log page support

[PATCH 00/12] nvmet: add error log page support

[Chaitanya Kulkarni]

Hi,

This patch series adds support for the NVMe Over Fabrics target error
log page.

For I/O backend when the namespace is configured with bdev we just map
the blk_status to NVMe error status and for file backend we just map
the errno to the NVMe error status.

We add new fields to the nvmet_req to hold the error log information and
to the nvmet_ctrl to hold the error log page. We update the error log
page in the request completion path in the nvmet_req_complete() and
implement admin command so that host can read the error log page.

Regards,
Chaitanya

Chaitanya Kulkarni (12):
  nvme: remove nvme_common command cdw10 array
  nvme: add error log page slot definition
  nvmet: add error-log definitions
  nvmet: add interface to update error-log page
  nvmet: add error log support in the core
  nvmet: add error log support for bdev backend
  nvmet: add error log support for file backend
  nvmet: add error log support for fabrics-cmd
  nvmet: add error log support for rdma backend
  nvmet: add error log support for admin-cmd
  nvmet: add error log page cmd handler
  nvmet: update smart log with num err log entries

 drivers/nvme/host/core.c          |  18 ++---
 drivers/nvme/host/trace.h         |   4 +-
 drivers/nvme/target/admin-cmd.c   |  80 +++++++++++++++++-----
 drivers/nvme/target/core.c        | 107 +++++++++++++++++++++++++++---
 drivers/nvme/target/discovery.c   |  14 +++-
 drivers/nvme/target/fabrics-cmd.c |  50 ++++++++++----
 drivers/nvme/target/io-cmd-bdev.c |  84 ++++++++++++++++++++---
 drivers/nvme/target/io-cmd-file.c |  35 +++++-----
 drivers/nvme/target/nvmet.h       |  14 +++-
 drivers/nvme/target/rdma.c        |  10 ++-
 include/linux/nvme.h              |  21 +++++-
 11 files changed, 357 insertions(+), 80 deletions(-)

-- 
2.17.0

[PATCH 01/12] nvme: remove nvme_common command cdw10 array

[Chaitanya Kulkarni]

This is a preparation patch which removes the nvme common command cdw10
array and replace with individual fields. This is needed for the nvmet
error log page implementation make is error log page entry offset
assignment easier.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>
---
 drivers/nvme/host/core.c        | 18 +++++++++---------
 drivers/nvme/host/trace.h       |  4 ++--
 drivers/nvme/target/admin-cmd.c | 12 ++++++------
 drivers/nvme/target/discovery.c |  4 ++--
 drivers/nvme/target/nvmet.h     |  2 +-
 include/linux/nvme.h            |  7 ++++++-
 6 files changed, 26 insertions(+), 21 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index f90576862736..090ce8f69ef2 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1257,12 +1257,12 @@ static int nvme_user_cmd(struct nvme_ctrl *ctrl, struct nvme_ns *ns,
 	c.common.nsid = cpu_to_le32(cmd.nsid);
 	c.common.cdw2[0] = cpu_to_le32(cmd.cdw2);
 	c.common.cdw2[1] = cpu_to_le32(cmd.cdw3);
-	c.common.cdw10[0] = cpu_to_le32(cmd.cdw10);
-	c.common.cdw10[1] = cpu_to_le32(cmd.cdw11);
-	c.common.cdw10[2] = cpu_to_le32(cmd.cdw12);
-	c.common.cdw10[3] = cpu_to_le32(cmd.cdw13);
-	c.common.cdw10[4] = cpu_to_le32(cmd.cdw14);
-	c.common.cdw10[5] = cpu_to_le32(cmd.cdw15);
+	c.common.cdw10 = cpu_to_le32(cmd.cdw10);
+	c.common.cdw11 = cpu_to_le32(cmd.cdw11);
+	c.common.cdw12 = cpu_to_le32(cmd.cdw12);
+	c.common.cdw13 = cpu_to_le32(cmd.cdw13);
+	c.common.cdw14 = cpu_to_le32(cmd.cdw14);
+	c.common.cdw15 = cpu_to_le32(cmd.cdw15);
 
 	if (cmd.timeout_ms)
 		timeout = msecs_to_jiffies(cmd.timeout_ms);
@@ -1625,7 +1625,7 @@ static int nvme_pr_command(struct block_device *bdev, u32 cdw10,
 	memset(&c, 0, sizeof(c));
 	c.common.opcode = op;
 	c.common.nsid = cpu_to_le32(ns->head->ns_id);
-	c.common.cdw10[0] = cpu_to_le32(cdw10);
+	c.common.cdw10 = cpu_to_le32(cdw10);
 
 	ret = nvme_submit_sync_cmd(ns->queue, &c, data, 16);
 	nvme_put_ns_from_disk(head, srcu_idx);
@@ -1699,8 +1699,8 @@ int nvme_sec_submit(void *data, u16 spsp, u8 secp, void *buffer, size_t len,
 	else
 		cmd.common.opcode = nvme_admin_security_recv;
 	cmd.common.nsid = 0;
-	cmd.common.cdw10[0] = cpu_to_le32(((u32)secp) << 24 | ((u32)spsp) << 8);
-	cmd.common.cdw10[1] = cpu_to_le32(len);
+	cmd.common.cdw10 = cpu_to_le32(((u32)secp) << 24 | ((u32)spsp) << 8);
+	cmd.common.cdw11 = cpu_to_le32(len);
 
 	return __nvme_submit_sync_cmd(ctrl->admin_q, &cmd, NULL, buffer, len,
 				      ADMIN_TIMEOUT, NVME_QID_ANY, 1, 0);
diff --git a/drivers/nvme/host/trace.h b/drivers/nvme/host/trace.h
index 196d5bd56718..1978deb6fcc7 100644
--- a/drivers/nvme/host/trace.h
+++ b/drivers/nvme/host/trace.h
@@ -115,8 +115,8 @@ TRACE_EVENT(nvme_setup_cmd,
 		__entry->nsid = le32_to_cpu(cmd->common.nsid);
 		__entry->metadata = le64_to_cpu(cmd->common.metadata);
 		__assign_disk_name(__entry->disk, req->rq_disk);
-		memcpy(__entry->cdw10, cmd->common.cdw10,
-		       sizeof(__entry->cdw10));
+		memcpy(__entry->cdw10, &cmd->common.cdw10,
+			6 * sizeof(__entry->cdw10));
 	    ),
 	    TP_printk("nvme%d: %sqid=%d, cmdid=%u, nsid=%u, flags=0x%x, meta=0x%llx, cmd=(%s %s)",
 		      __entry->ctrl_id, __print_disk_name(__entry->disk),
diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 753515fc8028..721b041a6b3b 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -557,7 +557,7 @@ static u16 nvmet_write_protect_flush_sync(struct nvmet_req *req)
 
 static u16 nvmet_set_feat_write_protect(struct nvmet_req *req)
 {
-	u32 write_protect = le32_to_cpu(req->cmd->common.cdw10[1]);
+	u32 write_protect = le32_to_cpu(req->cmd->common.cdw11);
 	struct nvmet_subsys *subsys = req->sq->ctrl->subsys;
 	u16 status = NVME_SC_FEATURE_NOT_CHANGEABLE;
 
@@ -589,7 +589,7 @@ static u16 nvmet_set_feat_write_protect(struct nvmet_req *req)
 
 u16 nvmet_set_feat_kato(struct nvmet_req *req)
 {
-	u32 val32 = le32_to_cpu(req->cmd->common.cdw10[1]);
+	u32 val32 = le32_to_cpu(req->cmd->common.cdw11);
 
 	req->sq->ctrl->kato = DIV_ROUND_UP(val32, 1000);
 
@@ -600,7 +600,7 @@ u16 nvmet_set_feat_kato(struct nvmet_req *req)
 
 u16 nvmet_set_feat_async_event(struct nvmet_req *req, u32 mask)
 {
-	u32 val32 = le32_to_cpu(req->cmd->common.cdw10[1]);
+	u32 val32 = le32_to_cpu(req->cmd->common.cdw11);
 
 	if (val32 & ~mask)
 		return NVME_SC_INVALID_FIELD | NVME_SC_DNR;
@@ -614,7 +614,7 @@ u16 nvmet_set_feat_async_event(struct nvmet_req *req, u32 mask)
 static void nvmet_execute_set_features(struct nvmet_req *req)
 {
 	struct nvmet_subsys *subsys = req->sq->ctrl->subsys;
-	u32 cdw10 = le32_to_cpu(req->cmd->common.cdw10[0]);
+	u32 cdw10 = le32_to_cpu(req->cmd->common.cdw10);
 	u16 status = 0;
 
 	switch (cdw10 & 0xff) {
@@ -675,7 +675,7 @@ void nvmet_get_feat_async_event(struct nvmet_req *req)
 static void nvmet_execute_get_features(struct nvmet_req *req)
 {
 	struct nvmet_subsys *subsys = req->sq->ctrl->subsys;
-	u32 cdw10 = le32_to_cpu(req->cmd->common.cdw10[0]);
+	u32 cdw10 = le32_to_cpu(req->cmd->common.cdw10);
 	u16 status = 0;
 
 	switch (cdw10 & 0xff) {
@@ -715,7 +715,7 @@ static void nvmet_execute_get_features(struct nvmet_req *req)
 		break;
 	case NVME_FEAT_HOST_ID:
 		/* need 128-bit host identifier flag */
-		if (!(req->cmd->common.cdw10[1] & cpu_to_le32(1 << 0))) {
+		if (!(req->cmd->common.cdw11 & cpu_to_le32(1 << 0))) {
 			status = NVME_SC_INVALID_FIELD | NVME_SC_DNR;
 			break;
 		}
diff --git a/drivers/nvme/target/discovery.c b/drivers/nvme/target/discovery.c
index 4d8757ae8210..e1bb254671de 100644
--- a/drivers/nvme/target/discovery.c
+++ b/drivers/nvme/target/discovery.c
@@ -247,7 +247,7 @@ static void nvmet_execute_identify_disc_ctrl(struct nvmet_req *req)
 
 static void nvmet_execute_disc_set_features(struct nvmet_req *req)
 {
-	u32 cdw10 = le32_to_cpu(req->cmd->common.cdw10[0]);
+	u32 cdw10 = le32_to_cpu(req->cmd->common.cdw10);
 	u16 stat;
 
 	switch (cdw10 & 0xff) {
@@ -268,7 +268,7 @@ static void nvmet_execute_disc_set_features(struct nvmet_req *req)
 
 static void nvmet_execute_disc_get_features(struct nvmet_req *req)
 {
-	u32 cdw10 = le32_to_cpu(req->cmd->common.cdw10[0]);
+	u32 cdw10 = le32_to_cpu(req->cmd->common.cdw10);
 	u16 stat = 0;
 
 	switch (cdw10 & 0xff) {
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 89df51ee5bdf..dafee1af4829 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -349,7 +349,7 @@ struct nvmet_async_event {
 
 static inline void nvmet_clear_aen_bit(struct nvmet_req *req, u32 bn)
 {
-	int rae = le32_to_cpu(req->cmd->common.cdw10[0]) & 1 << 15;
+	int rae = le32_to_cpu(req->cmd->common.cdw10) & 1 << 15;
 
 	if (!rae)
 		clear_bit(bn, &req->sq->ctrl->aen_masked);
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 4d7907e3771e..b94fe8fadc4f 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -662,7 +662,12 @@ struct nvme_common_command {
 	__le32			cdw2[2];
 	__le64			metadata;
 	union nvme_data_ptr	dptr;
-	__le32			cdw10[6];
+	__le32			cdw10;
+	__le32			cdw11;
+	__le32			cdw12;
+	__le32			cdw13;
+	__le32			cdw14;
+	__le32			cdw15;
 };
 
 struct nvme_rw_command {
-- 
2.17.0

[PATCH 01/12] nvme: remove nvme_common command cdw10 array

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi at grimberg.me>

[PATCH 02/12] nvme: add error log page slot definition

[Chaitanya Kulkarni]

This patch adds the NVMe error slot definition from the spec.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>
---
 include/linux/nvme.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index b94fe8fadc4f..bbcc83886899 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -1168,6 +1168,20 @@ struct nvme_command {
 	};
 };
 
+struct nvme_error_slot {
+	__le64		error_count;
+	__le16		sqid;
+	__le16		cmdid;
+	__le16		status_field;
+	__le16		param_error_location;
+	__le64		lba;
+	__le32		nsid;
+	__u8		vs;
+	__u8		resv[3];
+	__le64		cs;
+	__u8		resv2[24];
+};
+
 static inline bool nvme_is_write(struct nvme_command *cmd)
 {
 	/*
-- 
2.17.0

[PATCH 02/12] nvme: add error log page slot definition

[Sagi Grimberg]

Looks fine,

Reviewed-by: Sagi Grimberg <sagi at grimberg.me>

[PATCH 03/12] nvmet: add error-log definitions

[Chaitanya Kulkarni]

This patch adds necessary fields in the target data structures to
support error log page. For a target controller, we add a new error log
field to maintain the error log, at any given point we maintain error
entries equal to NVMET_ERROR_LOG_SLOTS for each controller. In the
following patch, we also update the error log page entry in the I/O
completion path so we introduce a spinlock for synchronization of the
log.

For nvmet_req, we add a new field error_loc to hold the location of
the error in the command when the actual error occurs for each request
and a starting LBA if applicable.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>
---
 drivers/nvme/target/core.c  | 5 +++++
 drivers/nvme/target/nvmet.h | 6 ++++++
 2 files changed, 11 insertions(+)

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index e468100b9211..4ed0022b8450 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -769,6 +769,8 @@ bool nvmet_req_init(struct nvmet_req *req, struct nvmet_cq *cq,
 	req->rsp->status = 0;
 	req->rsp->sq_head = 0;
 	req->ns = NULL;
+	req->error_loc = 0;
+	req->error_slba = 0;
 
 	/* no support for fused commands yet */
 	if (unlikely(flags & (NVME_CMD_FUSE_FIRST | NVME_CMD_FUSE_SECOND))) {
@@ -1174,6 +1176,9 @@ u16 nvmet_alloc_ctrl(const char *subsysnqn, const char *hostnqn,
 	/* keep-alive timeout in seconds */
 	ctrl->kato = DIV_ROUND_UP(kato, 1000);
 
+	ctrl->counter = 0;
+	spin_lock_init(&ctrl->error_lock);
+
 	nvmet_start_keep_alive_timer(ctrl);
 
 	mutex_lock(&subsys->lock);
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index dafee1af4829..5a19f64de891 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -202,6 +202,10 @@ struct nvmet_ctrl {
 
 	struct device		*p2p_client;
 	struct radix_tree_root	p2p_ns_map;
+
+	spinlock_t		error_lock;
+	u64			counter;
+	struct nvme_error_slot	slots[NVMET_ERROR_LOG_SLOTS];
 };
 
 struct nvmet_subsys {
@@ -317,6 +321,8 @@ struct nvmet_req {
 
 	struct pci_dev		*p2p_dev;
 	struct device		*p2p_client;
+	u16			error_loc;
+	u64			error_slba;
 };
 
 extern struct workqueue_struct *buffered_io_wq;
-- 
2.17.0

[PATCH 03/12] nvmet: add error-log definitions

[Sagi Grimberg]

> diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
> index dafee1af4829..5a19f64de891 100644
> --- a/drivers/nvme/target/nvmet.h
> +++ b/drivers/nvme/target/nvmet.h
> @@ -202,6 +202,10 @@ struct nvmet_ctrl {
>   
>   	struct device		*p2p_client;
>   	struct radix_tree_root	p2p_ns_map;
> +
> +	spinlock_t		error_lock;
> +	u64			counter;
> +	struct nvme_error_slot	slots[NVMET_ERROR_LOG_SLOTS];
>   };

NVMET_ERROR_LOG_SLOTS is undefined AFAICT.

[PATCH 03/12] nvmet: add error-log definitions

[Chaitanya Kulkarni]

Hi Sagi,

NVMET_ERROR_LOG_SLOTS is already defined in nvmet.h:-

# cat -n   drivers/nvme/target/nvmet.h  
     1	/*
     2	 * Copyright (c) 2015-2016 HGST, a Western Digital Company.
     3	 *
     4	 * This program is free software; you can redistribute it and/or modify it
     5	 * under the terms and conditions of the GNU General Public License,
     6	 * version 2, as published by the Free Software Foundation.
     7	 *
     8	 * This program is distributed in the hope it will be useful, but WITHOUT
     9	 * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
    10 * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
    11 * more details.
    12 */
    13	
    14	#ifndef _NVMET_H
    15	#define _NVMET_H
    16	
    17	#include <linux/dma-mapping.h>
    18	#include <linux/types.h>
    19	#include <linux/device.h>
    20	#include <linux/kref.h>
    21	#include <linux/percpu-refcount.h>
    22	#include <linux/list.h>
    23	#include <linux/mutex.h>
    24	#include <linux/uuid.h>
    25	#include <linux/nvme.h>
    26	#include <linux/configfs.h>
    27	#include <linux/rcupdate.h>
    28	#include <linux/blkdev.h>
    29	#include <linux/radix-tree.h>
    30	
    31	#define NVMET_ASYNC_EVENTS		4
    32	#define NVMET_ERROR_LOG_SLOTS		128

We also use this in admin-cmd.c:-

309         id->frmw = (1 << 0) | (1 << 1);
310         id->lpa = (1 << 0) | (1 << 1) | (1 << 2);
311         id->elpe = NVMET_ERROR_LOG_SLOTS - 1;                                                                                                          
312         id->npss = 0;
313 



From: Sagi Grimberg <sagi@grimberg.me>
Sent: Monday, December 10, 2018 12:12 PM
To: Chaitanya Kulkarni; linux-nvme at lists.infradead.org
Cc: hch at lst.de; keith.busch at intel.com
Subject: Re: [PATCH 03/12] nvmet: add error-log definitions
? 


> diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
> index dafee1af4829..5a19f64de891 100644
> --- a/drivers/nvme/target/nvmet.h
> +++ b/drivers/nvme/target/nvmet.h
> @@ -202,6 +202,10 @@ struct nvmet_ctrl {
>?? 
>??????? struct device?????????? *p2p_client;
>??????? struct radix_tree_root? p2p_ns_map;
> +
> +???? spinlock_t????????????? error_lock;
> +???? u64???????????????????? counter;
> +???? struct nvme_error_slot? slots[NVMET_ERROR_LOG_SLOTS];
>?? };

NVMET_ERROR_LOG_SLOTS is undefined AFAICT.
    

[PATCH 04/12] nvmet: add interface to update error-log page

[Chaitanya Kulkarni]

This patch adds nvmet_req based interface to the nvmet-core so that
we can update the error log page. We update error log page in
the request completion path when status is not set to NVME_SC_SUCCESS.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>
---
 drivers/nvme/target/core.c  | 31 ++++++++++++++++++++++++++++++-
 drivers/nvme/target/nvmet.h |  5 -----
 2 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 4ed0022b8450..4fe70fc322b4 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -611,14 +611,43 @@ static void nvmet_update_sq_head(struct nvmet_req *req)
 	req->rsp->sq_head = cpu_to_le16(req->sq->sqhd & 0x0000FFFF);
 }
 
+static void nvmet_set_error(struct nvmet_req *req, u16 status)
+{
+	struct nvmet_ctrl *ctrl = req->sq->ctrl;
+	struct nvme_error_slot *new_error_slot;
+	unsigned long flags;
+
+	req->rsp->status = cpu_to_le16(status << 1);
+
+	if (!ctrl)
+		return;
+
+	spin_lock_irqsave(&ctrl->error_lock, flags);
+	ctrl->counter++;
+	new_error_slot = &ctrl->slots[ctrl->counter % NVMET_ERROR_LOG_SLOTS];
+
+	new_error_slot->error_count = cpu_to_le64(ctrl->counter);
+	new_error_slot->sqid = cpu_to_le16(req->sq->qid);
+	new_error_slot->cmdid = cpu_to_le16(req->cmd->common.command_id);
+	new_error_slot->status_field = cpu_to_le16(status << 1);
+	new_error_slot->param_error_location = cpu_to_le16(req->error_loc);
+	new_error_slot->lba = cpu_to_le64(req->error_slba);
+	new_error_slot->nsid = cpu_to_le32(req->cmd->common.nsid);
+	spin_unlock_irqrestore(&ctrl->error_lock, flags);
+
+	/* set the more bit for this request */
+	req->rsp->status |= cpu_to_le16(1 << 14);
+}
+
 static void __nvmet_req_complete(struct nvmet_req *req, u16 status)
 {
 	if (!req->sq->sqhd_disabled)
 		nvmet_update_sq_head(req);
 	req->rsp->sq_id = cpu_to_le16(req->sq->qid);
 	req->rsp->command_id = req->cmd->common.command_id;
+
 	if (unlikely(status))
-		nvmet_set_status(req, status);
+		nvmet_set_error(req, status);
 	if (req->ns)
 		nvmet_put_namespace(req->ns);
 	req->ops->queue_response(req);
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 5a19f64de891..662def80ff23 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -327,11 +327,6 @@ struct nvmet_req {
 
 extern struct workqueue_struct *buffered_io_wq;
 
-static inline void nvmet_set_status(struct nvmet_req *req, u16 status)
-{
-	req->rsp->status = cpu_to_le16(status << 1);
-}
-
 static inline void nvmet_set_result(struct nvmet_req *req, u32 result)
 {
 	req->rsp->result.u32 = cpu_to_le32(result);
-- 
2.17.0

[PATCH 04/12] nvmet: add interface to update error-log page

[Sagi Grimberg]

> +static void nvmet_set_error(struct nvmet_req *req, u16 status)
> +{
> +	struct nvmet_ctrl *ctrl = req->sq->ctrl;
> +	struct nvme_error_slot *new_error_slot;
> +	unsigned long flags;
> +
> +	req->rsp->status = cpu_to_le16(status << 1);
> +
> +	if (!ctrl)
> +		return;
> +
> +	spin_lock_irqsave(&ctrl->error_lock, flags);
> +	ctrl->counter++;

ctrl->err_counter reads out better.

[PATCH 04/12] nvmet: add interface to update error-log page

[Chaitanya Kulkarni]

Sounds good.




From: Sagi Grimberg <sagi@grimberg.me>
Sent: Monday, December 10, 2018 12:18 PM
To: Chaitanya Kulkarni; linux-nvme at lists.infradead.org
Cc: hch at lst.de; keith.busch at intel.com
Subject: Re: [PATCH 04/12] nvmet: add interface to update error-log page
? 


> +static void nvmet_set_error(struct nvmet_req *req, u16 status)
> +{
> +???? struct nvmet_ctrl *ctrl = req->sq->ctrl;
> +???? struct nvme_error_slot *new_error_slot;
> +???? unsigned long flags;
> +
> +???? req->rsp->status = cpu_to_le16(status << 1);
> +
> +???? if (!ctrl)
> +???????????? return;
> +
> +???? spin_lock_irqsave(&ctrl->error_lock, flags);
> +???? ctrl->counter++;

ctrl->err_counter reads out better.
    

[PATCH 05/12] nvmet: add error log support in the core

[Chaitanya Kulkarni]

This patch adds the support to maintain error log page for the
nvmet-core.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>
---
 drivers/nvme/target/core.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 4fe70fc322b4..5f0b281945e4 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -51,22 +51,28 @@ static struct nvmet_subsys *nvmet_find_get_subsys(struct nvmet_port *port,
 u16 nvmet_copy_to_sgl(struct nvmet_req *req, off_t off, const void *buf,
 		size_t len)
 {
-	if (sg_pcopy_from_buffer(req->sg, req->sg_cnt, buf, len, off) != len)
+	if (sg_pcopy_from_buffer(req->sg, req->sg_cnt, buf, len, off) != len) {
+		req->error_loc = offsetof(struct nvme_common_command, dptr);
 		return NVME_SC_SGL_INVALID_DATA | NVME_SC_DNR;
+	}
 	return 0;
 }
 
 u16 nvmet_copy_from_sgl(struct nvmet_req *req, off_t off, void *buf, size_t len)
 {
-	if (sg_pcopy_to_buffer(req->sg, req->sg_cnt, buf, len, off) != len)
+	if (sg_pcopy_to_buffer(req->sg, req->sg_cnt, buf, len, off) != len) {
+		req->error_loc = offsetof(struct nvme_common_command, dptr);
 		return NVME_SC_SGL_INVALID_DATA | NVME_SC_DNR;
+	}
 	return 0;
 }
 
 u16 nvmet_zero_sgl(struct nvmet_req *req, off_t off, size_t len)
 {
-	if (sg_zero_buffer(req->sg, req->sg_cnt, len, off) != len)
+	if (sg_zero_buffer(req->sg, req->sg_cnt, len, off) != len) {
+		req->error_loc = offsetof(struct nvme_common_command, dptr);
 		return NVME_SC_SGL_INVALID_DATA | NVME_SC_DNR;
+	}
 	return 0;
 }
 
@@ -768,14 +774,20 @@ static u16 nvmet_parse_io_cmd(struct nvmet_req *req)
 		return ret;
 
 	req->ns = nvmet_find_namespace(req->sq->ctrl, cmd->rw.nsid);
-	if (unlikely(!req->ns))
+	if (unlikely(!req->ns)) {
+		req->error_loc = offsetof(struct nvme_common_command, nsid);
 		return NVME_SC_INVALID_NS | NVME_SC_DNR;
+	}
 	ret = nvmet_check_ana_state(req->port, req->ns);
-	if (unlikely(ret))
+	if (unlikely(ret)) {
+		req->error_loc = offsetof(struct nvme_common_command, nsid);
 		return ret;
+	}
 	ret = nvmet_io_cmd_check_access(req);
-	if (unlikely(ret))
+	if (unlikely(ret)) {
+		req->error_loc = offsetof(struct nvme_common_command, nsid);
 		return ret;
+	}
 
 	if (req->ns->file)
 		return nvmet_file_parse_io_cmd(req);
@@ -803,6 +815,7 @@ bool nvmet_req_init(struct nvmet_req *req, struct nvmet_cq *cq,
 
 	/* no support for fused commands yet */
 	if (unlikely(flags & (NVME_CMD_FUSE_FIRST | NVME_CMD_FUSE_SECOND))) {
+		req->error_loc = offsetof(struct nvme_common_command, flags);
 		status = NVME_SC_INVALID_FIELD | NVME_SC_DNR;
 		goto fail;
 	}
@@ -813,6 +826,7 @@ bool nvmet_req_init(struct nvmet_req *req, struct nvmet_cq *cq,
 	 * byte aligned.
 	 */
 	if (unlikely((flags & NVME_CMD_SGL_ALL) != NVME_CMD_SGL_METABUF)) {
+		req->error_loc = offsetof(struct nvme_common_command, flags);
 		status = NVME_SC_INVALID_FIELD | NVME_SC_DNR;
 		goto fail;
 	}
@@ -858,9 +872,10 @@ EXPORT_SYMBOL_GPL(nvmet_req_uninit);
 
 void nvmet_req_execute(struct nvmet_req *req)
 {
-	if (unlikely(req->data_len != req->transfer_len))
+	if (unlikely(req->data_len != req->transfer_len)) {
+		req->error_loc = offsetof(struct nvme_common_command, dptr);
 		nvmet_req_complete(req, NVME_SC_SGL_INVALID_DATA | NVME_SC_DNR);
-	else
+	} else
 		req->execute(req);
 }
 EXPORT_SYMBOL_GPL(nvmet_req_execute);
-- 
2.17.0

[PATCH 05/12] nvmet: add error log support in the core

[Sagi Grimberg]

Looks good,

Reviewed-by: Sagi Grimberg <sagi at grimberg.me>

[PATCH 06/12] nvmet: add error log support for bdev backend

[Chaitanya Kulkarni]

This patch adds the support for the block device backend to populate the
error log entries. Here we map the blk_status_t to the NVMe status.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>
---
 drivers/nvme/target/io-cmd-bdev.c | 84 ++++++++++++++++++++++++++++---
 1 file changed, 76 insertions(+), 8 deletions(-)

diff --git a/drivers/nvme/target/io-cmd-bdev.c b/drivers/nvme/target/io-cmd-bdev.c
index c1cb2ed5531c..33b548240568 100644
--- a/drivers/nvme/target/io-cmd-bdev.c
+++ b/drivers/nvme/target/io-cmd-bdev.c
@@ -44,13 +44,73 @@ void nvmet_bdev_ns_disable(struct nvmet_ns *ns)
 	}
 }
 
+static u16 blk_to_nvme_status(struct nvmet_req *req, blk_status_t blk_sts)
+{
+	u16 status = NVME_SC_SUCCESS;
+
+	if (likely(blk_sts == BLK_STS_OK))
+		return status;
+	/*
+	 * Right now there exists M : 1 mapping between block layer error
+	 * to the NVMe status code (see nvme_error_status()). For consistency,
+	 * when we reverse map we use most appropriate NVMe Status code from
+	 * the group of the NVMe staus codes used in the nvme_error_status().
+	 */
+	switch (blk_sts) {
+	case BLK_STS_NOSPC:
+		status = NVME_SC_CAP_EXCEEDED | NVME_SC_DNR;
+		req->error_loc = offsetof(struct nvme_rw_command, length);
+		break;
+	case BLK_STS_TARGET:
+		status = NVME_SC_LBA_RANGE | NVME_SC_DNR;
+		req->error_loc = offsetof(struct nvme_rw_command, slba);
+		break;
+	case BLK_STS_NOTSUPP:
+		req->error_loc = offsetof(struct nvme_common_command, opcode);
+		switch (req->cmd->common.opcode) {
+		case nvme_cmd_dsm:
+		case nvme_cmd_write_zeroes:
+			status = NVME_SC_ONCS_NOT_SUPPORTED | NVME_SC_DNR;
+			break;
+		default:
+			status = NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
+		}
+		break;
+	case BLK_STS_MEDIUM:
+		status = NVME_SC_ACCESS_DENIED;
+		req->error_loc = offsetof(struct nvme_rw_command, nsid);
+		break;
+	case BLK_STS_PROTECTION:
+		status = NVME_SC_INVALID_PI;
+		req->error_loc = offsetof(struct nvme_rw_command, nsid);
+		break;
+	case BLK_STS_IOERR:
+		/* fallthru */
+	default:
+		status = NVME_SC_INTERNAL | NVME_SC_DNR;
+		req->error_loc = offsetof(struct nvme_common_command, opcode);
+	}
+
+	switch (req->cmd->common.opcode) {
+	case nvme_cmd_read:
+	case nvme_cmd_write:
+		req->error_slba = le64_to_cpu(req->cmd->rw.slba);
+		break;
+	case nvme_cmd_write_zeroes:
+		req->error_slba =
+			le64_to_cpu(req->cmd->write_zeroes.slba);
+		break;
+	default:
+		req->error_slba = 0;
+	}
+	return status;
+}
+
 static void nvmet_bio_done(struct bio *bio)
 {
 	struct nvmet_req *req = bio->bi_private;
 
-	nvmet_req_complete(req,
-		bio->bi_status ? NVME_SC_INTERNAL | NVME_SC_DNR : 0);
-
+	nvmet_req_complete(req, blk_to_nvme_status(req, bio->bi_status));
 	if (bio != &req->b.inline_bio)
 		bio_put(bio);
 }
@@ -137,18 +197,21 @@ u16 nvmet_bdev_flush(struct nvmet_req *req)
 	return 0;
 }
 
-static u16 nvmet_bdev_discard_range(struct nvmet_ns *ns,
+static u16 nvmet_bdev_discard_range(struct nvmet_req *req,
 		struct nvme_dsm_range *range, struct bio **bio)
 {
+	struct nvmet_ns *ns = req->ns;
 	int ret;
 
 	ret = __blkdev_issue_discard(ns->bdev,
 			le64_to_cpu(range->slba) << (ns->blksize_shift - 9),
 			le32_to_cpu(range->nlb) << (ns->blksize_shift - 9),
 			GFP_KERNEL, 0, bio);
-	if (ret && ret != -EOPNOTSUPP)
-		return NVME_SC_INTERNAL | NVME_SC_DNR;
-	return 0;
+
+	if (ret)
+		req->error_slba = le64_to_cpu(range->slba);
+
+	return blk_to_nvme_status(req, errno_to_blk_status(ret));
 }
 
 static void nvmet_bdev_execute_discard(struct nvmet_req *req)
@@ -164,7 +227,7 @@ static void nvmet_bdev_execute_discard(struct nvmet_req *req)
 		if (status)
 			break;
 
-		status = nvmet_bdev_discard_range(req->ns, &range, &bio);
+		status = nvmet_bdev_discard_range(req, &range, &bio);
 		if (status)
 			break;
 	}
@@ -205,6 +268,7 @@ static void nvmet_bdev_execute_write_zeroes(struct nvmet_req *req)
 	u16 status = NVME_SC_SUCCESS;
 	sector_t sector;
 	sector_t nr_sector;
+	int ret;
 
 	sector = le64_to_cpu(write_zeroes->slba) <<
 		(req->ns->blksize_shift - 9);
@@ -215,6 +279,9 @@ static void nvmet_bdev_execute_write_zeroes(struct nvmet_req *req)
 				GFP_KERNEL, &bio, 0))
 		status = NVME_SC_INTERNAL | NVME_SC_DNR;
 
+	ret = __blkdev_issue_zeroout(req->ns->bdev, sector, nr_sector,
+			GFP_KERNEL, &bio, 0);
+	status = blk_to_nvme_status(req, errno_to_blk_status(ret));
 	if (bio) {
 		bio->bi_private = req;
 		bio->bi_end_io = nvmet_bio_done;
@@ -249,6 +316,7 @@ u16 nvmet_bdev_parse_io_cmd(struct nvmet_req *req)
 	default:
 		pr_err("unhandled cmd %d on qid %d\n", cmd->common.opcode,
 		       req->sq->qid);
+		req->error_loc = offsetof(struct nvme_common_command, opcode);
 		return NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
 	}
 }
-- 
2.17.0

[PATCH 06/12] nvmet: add error log support for bdev backend

[Sagi Grimberg]

> +static u16 blk_to_nvme_status(struct nvmet_req *req, blk_status_t blk_sts)
> +{
> +	u16 status = NVME_SC_SUCCESS;
> +
> +	if (likely(blk_sts == BLK_STS_OK))
> +		return status;
> +	/*
> +	 * Right now there exists M : 1 mapping between block layer error
> +	 * to the NVMe status code (see nvme_error_status()). For consistency,
> +	 * when we reverse map we use most appropriate NVMe Status code from
> +	 * the group of the NVMe staus codes used in the nvme_error_status().
> +	 */
> +	switch (blk_sts) {
> +	case BLK_STS_NOSPC:
> +		status = NVME_SC_CAP_EXCEEDED | NVME_SC_DNR;
> +		req->error_loc = offsetof(struct nvme_rw_command, length);
> +		break;
> +	case BLK_STS_TARGET:
> +		status = NVME_SC_LBA_RANGE | NVME_SC_DNR;
> +		req->error_loc = offsetof(struct nvme_rw_command, slba);
> +		break;
> +	case BLK_STS_NOTSUPP:
> +		req->error_loc = offsetof(struct nvme_common_command, opcode);
> +		switch (req->cmd->common.opcode) {
> +		case nvme_cmd_dsm:
> +		case nvme_cmd_write_zeroes:
> +			status = NVME_SC_ONCS_NOT_SUPPORTED | NVME_SC_DNR;
> +			break;
> +		default:
> +			status = NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
> +		}
> +		break;
> +	case BLK_STS_MEDIUM:
> +		status = NVME_SC_ACCESS_DENIED;
> +		req->error_loc = offsetof(struct nvme_rw_command, nsid);
> +		break;
> +	case BLK_STS_PROTECTION:
> +		status = NVME_SC_INVALID_PI;
> +		req->error_loc = offsetof(struct nvme_rw_command, nsid);

Is this really the nsid (not PRINFO)? Moreover, I think we should not
propagate this if the host did not send us any PI...

> @@ -205,6 +268,7 @@ static void nvmet_bdev_execute_write_zeroes(struct nvmet_req *req)
>   	u16 status = NVME_SC_SUCCESS;
>   	sector_t sector;
>   	sector_t nr_sector;
> +	int ret;
>   
>   	sector = le64_to_cpu(write_zeroes->slba) <<
>   		(req->ns->blksize_shift - 9);
> @@ -215,6 +279,9 @@ static void nvmet_bdev_execute_write_zeroes(struct nvmet_req *req)
>   				GFP_KERNEL, &bio, 0))
>   		status = NVME_SC_INTERNAL | NVME_SC_DNR;
>   
> +	ret = __blkdev_issue_zeroout(req->ns->bdev, sector, nr_sector,
> +			GFP_KERNEL, &bio, 0);

Shouldn't this erase the former __blkdev_issue_zeroout?

[PATCH 06/12] nvmet: add error log support for bdev backend

[Chaitanya Kulkarni]

From: Sagi Grimberg <sagi@grimberg.me>
Sent: Monday, December 10, 2018 12:31 PM
To: Chaitanya Kulkarni; linux-nvme at lists.infradead.org
Cc: hch at lst.de; keith.busch at intel.com
Subject: Re: [PATCH 06/12] nvmet: add error log support for bdev backend
? 
 

> +static u16 blk_to_nvme_status(struct nvmet_req *req, blk_status_t blk_sts)
> +{
> +???? u16 status = NVME_SC_SUCCESS;
> +
> +???? if (likely(blk_sts == BLK_STS_OK))
> +???????????? return status;
> +???? /*
> +????? * Right now there exists M : 1 mapping between block layer error
> +????? * to the NVMe status code (see nvme_error_status()). For consistency,
> +????? * when we reverse map we use most appropriate NVMe Status code from
> +????? * the group of the NVMe staus codes used in the nvme_error_status().
> +????? */
> +???? switch (blk_sts) {
> +???? case BLK_STS_NOSPC:
> +???????????? status = NVME_SC_CAP_EXCEEDED | NVME_SC_DNR;
> +???????????? req->error_loc = offsetof(struct nvme_rw_command, length);
> +???????????? break;
> +???? case BLK_STS_TARGET:
> +???????????? status = NVME_SC_LBA_RANGE | NVME_SC_DNR;
> +???????????? req->error_loc = offsetof(struct nvme_rw_command, slba);
> +???????????? break;
> +???? case BLK_STS_NOTSUPP:
> +???????????? req->error_loc = offsetof(struct nvme_common_command, opcode);
> +???????????? switch (req->cmd->common.opcode) {
> +???????????? case nvme_cmd_dsm:
> +???????????? case nvme_cmd_write_zeroes:
> +???????????????????? status = NVME_SC_ONCS_NOT_SUPPORTED | NVME_SC_DNR;
> +???????????????????? break;
> +???????????? default:
> +???????????????????? status = NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
> +???????????? }
> +???????????? break;
> +???? case BLK_STS_MEDIUM:
> +???????????? status = NVME_SC_ACCESS_DENIED;
> +???????????? req->error_loc = offsetof(struct nvme_rw_command, nsid);
> +???????????? break;
> +???? case BLK_STS_PROTECTION:
> +???????????? status = NVME_SC_INVALID_PI;
> +???????????? req->error_loc = offsetof(struct nvme_rw_command, nsid);

Is this really the nsid (not PRINFO)? Moreover, I think we should not
propagate this if the host did not send us any PI...
[CK] Okay, I'll skip this error code.

> @@ -205,6 +268,7 @@ static void nvmet_bdev_execute_write_zeroes(struct nvmet_req *req)
>??????? u16 status = NVME_SC_SUCCESS;
>??????? sector_t sector;
>??????? sector_t nr_sector;
> +???? int ret;
>?? 
>??????? sector = le64_to_cpu(write_zeroes->slba) <<
>??????????????? (req->ns->blksize_shift - 9);
> @@ -215,6 +279,9 @@ static void nvmet_bdev_execute_write_zeroes(struct nvmet_req *req)
>??????????????????????????????? GFP_KERNEL, &bio, 0))
>??????????????? status = NVME_SC_INTERNAL | NVME_SC_DNR;
>?? 
> +???? ret = __blkdev_issue_zeroout(req->ns->bdev, sector, nr_sector,
> +???????????????????? GFP_KERNEL, &bio, 0);

Shouldn't this erase the former __blkdev_issue_zeroout?

[CK] My bad when porting the patches, will fix it in the next round.    

[PATCH 07/12] nvmet: add error log support for file backend

[Chaitanya Kulkarni]

This patch adds support for the file backend to populate the
error log entries. Here we map the errno to the NVMe status codes.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>
---
 drivers/nvme/target/core.c        | 42 +++++++++++++++++++++++++++++++
 drivers/nvme/target/io-cmd-file.c | 35 +++++++++++++++-----------
 drivers/nvme/target/nvmet.h       |  2 ++
 3 files changed, 64 insertions(+), 15 deletions(-)

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 5f0b281945e4..dee691f6e3b3 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -45,6 +45,48 @@ u32 nvmet_ana_group_enabled[NVMET_MAX_ANAGRPS + 1];
 u64 nvmet_ana_chgcnt;
 DECLARE_RWSEM(nvmet_ana_sem);
 
+inline u16 errno_to_nvme_status(struct nvmet_req *req, int errno)
+{
+	u16 status;
+
+	switch (errno) {
+	case -ENOSPC:
+		req->error_loc = offsetof(struct nvme_rw_command, length);
+		status = NVME_SC_CAP_EXCEEDED | NVME_SC_DNR;
+		break;
+	case -EREMOTEIO:
+		req->error_loc = offsetof(struct nvme_rw_command, slba);
+		status = NVME_SC_LBA_RANGE | NVME_SC_DNR;
+		break;
+	case -EOPNOTSUPP:
+		req->error_loc = offsetof(struct nvme_common_command, opcode);
+		switch (req->cmd->common.opcode) {
+		case nvme_cmd_dsm:
+		case nvme_cmd_write_zeroes:
+			status = NVME_SC_ONCS_NOT_SUPPORTED | NVME_SC_DNR;
+			break;
+		default:
+			status = NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
+		}
+		break;
+	case -ENODATA:
+		req->error_loc = offsetof(struct nvme_rw_command, nsid);
+		status = NVME_SC_ACCESS_DENIED;
+		break;
+	case -EILSEQ:
+		req->error_loc = offsetof(struct nvme_rw_command, nsid);
+		status = NVME_SC_INVALID_PI;
+		break;
+	case -EIO:
+		/* FALLTHRU */
+	default:
+		req->error_loc = offsetof(struct nvme_common_command, opcode);
+		status = NVME_SC_INTERNAL | NVME_SC_DNR;
+	}
+
+	return status;
+}
+
 static struct nvmet_subsys *nvmet_find_get_subsys(struct nvmet_port *port,
 		const char *subsysnqn);
 
diff --git a/drivers/nvme/target/io-cmd-file.c b/drivers/nvme/target/io-cmd-file.c
index 12eaa8ddc248..517522305e5c 100644
--- a/drivers/nvme/target/io-cmd-file.c
+++ b/drivers/nvme/target/io-cmd-file.c
@@ -112,6 +112,7 @@ static ssize_t nvmet_file_submit_bvec(struct nvmet_req *req, loff_t pos,
 static void nvmet_file_io_done(struct kiocb *iocb, long ret, long ret2)
 {
 	struct nvmet_req *req = container_of(iocb, struct nvmet_req, f.iocb);
+	u16 status = NVME_SC_SUCCESS;
 
 	if (req->f.bvec != req->inline_bvec) {
 		if (likely(req->f.mpool_alloc == false))
@@ -120,8 +121,9 @@ static void nvmet_file_io_done(struct kiocb *iocb, long ret, long ret2)
 			mempool_free(req->f.bvec, req->ns->bvec_pool);
 	}
 
-	nvmet_req_complete(req, ret != req->data_len ?
-			NVME_SC_INTERNAL | NVME_SC_DNR : 0);
+	if (unlikely(ret != req->data_len))
+		status = errno_to_nvme_status(req, ret);
+	nvmet_req_complete(req, status);
 }
 
 static bool nvmet_file_execute_io(struct nvmet_req *req, int ki_flags)
@@ -140,7 +142,7 @@ static bool nvmet_file_execute_io(struct nvmet_req *req, int ki_flags)
 
 	pos = le64_to_cpu(req->cmd->rw.slba) << req->ns->blksize_shift;
 	if (unlikely(pos + req->data_len > req->ns->size)) {
-		nvmet_req_complete(req, NVME_SC_LBA_RANGE | NVME_SC_DNR);
+		nvmet_req_complete(req, errno_to_nvme_status(req, -ENOSPC));
 		return true;
 	}
 
@@ -254,9 +256,7 @@ static void nvmet_file_execute_rw(struct nvmet_req *req)
 
 u16 nvmet_file_flush(struct nvmet_req *req)
 {
-	if (vfs_fsync(req->ns->file, 1) < 0)
-		return NVME_SC_INTERNAL | NVME_SC_DNR;
-	return 0;
+	return errno_to_nvme_status(req, vfs_fsync(req->ns->file, 1));
 }
 
 static void nvmet_file_flush_work(struct work_struct *w)
@@ -277,30 +277,34 @@ static void nvmet_file_execute_discard(struct nvmet_req *req)
 	int mode = FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE;
 	struct nvme_dsm_range range;
 	loff_t offset, len;
-	u16 ret;
+	u16 status = 0;
+	int ret;
 	int i;
 
 	for (i = 0; i <= le32_to_cpu(req->cmd->dsm.nr); i++) {
-		ret = nvmet_copy_from_sgl(req, i * sizeof(range), &range,
+		status = nvmet_copy_from_sgl(req, i * sizeof(range), &range,
 					sizeof(range));
-		if (ret)
+		if (status)
 			break;
 
 		offset = le64_to_cpu(range.slba) << req->ns->blksize_shift;
 		len = le32_to_cpu(range.nlb);
 		len <<= req->ns->blksize_shift;
 		if (offset + len > req->ns->size) {
-			ret = NVME_SC_LBA_RANGE | NVME_SC_DNR;
+			req->error_slba = le64_to_cpu(range.slba);
+			status = errno_to_nvme_status(req, -ENOSPC);
 			break;
 		}
 
-		if (vfs_fallocate(req->ns->file, mode, offset, len)) {
-			ret = NVME_SC_INTERNAL | NVME_SC_DNR;
+		ret = vfs_fallocate(req->ns->file, mode, offset, len);
+		if (ret) {
+			req->error_slba = le64_to_cpu(range.slba);
+			status = errno_to_nvme_status(req, ret);
 			break;
 		}
 	}
 
-	nvmet_req_complete(req, ret);
+	nvmet_req_complete(req, status);
 }
 
 static void nvmet_file_dsm_work(struct work_struct *w)
@@ -340,12 +344,12 @@ static void nvmet_file_write_zeroes_work(struct work_struct *w)
 			req->ns->blksize_shift);
 
 	if (unlikely(offset + len > req->ns->size)) {
-		nvmet_req_complete(req, NVME_SC_LBA_RANGE | NVME_SC_DNR);
+		nvmet_req_complete(req, errno_to_nvme_status(req, -ENOSPC));
 		return;
 	}
 
 	ret = vfs_fallocate(req->ns->file, mode, offset, len);
-	nvmet_req_complete(req, ret < 0 ? NVME_SC_INTERNAL | NVME_SC_DNR : 0);
+	nvmet_req_complete(req, ret < 0 ? errno_to_nvme_status(req, ret) : 0);
 }
 
 static void nvmet_file_execute_write_zeroes(struct nvmet_req *req)
@@ -380,6 +384,7 @@ u16 nvmet_file_parse_io_cmd(struct nvmet_req *req)
 	default:
 		pr_err("unhandled cmd for file ns %d on qid %d\n",
 				cmd->common.opcode, req->sq->qid);
+		req->error_loc = offsetof(struct nvme_common_command, opcode);
 		return NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
 	}
 }
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 662def80ff23..03f91cd3c24d 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -494,4 +494,6 @@ static inline u32 nvmet_rw_len(struct nvmet_req *req)
 	return ((u32)le16_to_cpu(req->cmd->rw.length) + 1) <<
 			req->ns->blksize_shift;
 }
+
+u16 errno_to_nvme_status(struct nvmet_req *req, int errno);
 #endif /* _NVMET_H */
-- 
2.17.0

[PATCH 07/12] nvmet: add error log support for file backend

[Sagi Grimberg]

> +	case -EILSEQ:
> +		req->error_loc = offsetof(struct nvme_rw_command, nsid);
> +		status = NVME_SC_INVALID_PI;

Same here...

[PATCH 07/12] nvmet: add error log support for file backend

[Chaitanya Kulkarni]

From: Sagi Grimberg <sagi@grimberg.me>
Sent: Monday, December 10, 2018 12:32 PM
To: Chaitanya Kulkarni; linux-nvme at lists.infradead.org
Cc: hch at lst.de; keith.busch at intel.com
Subject: Re: [PATCH 07/12] nvmet: add error log support for file backend
? 


> +???? case -EILSEQ:
> +???????????? req->error_loc = offsetof(struct nvme_rw_command, nsid);
> +???????????? status = NVME_SC_INVALID_PI;

Same here...
[CK] Will skip this errorno in the next round.
    

[PATCH 08/12] nvmet: add error log support for fabrics-cmd

[Chaitanya Kulkarni]

This patch adds the support to maintain error log page for the fabrics
prop get, prop set, and admin connect commands. Here we also update the
discovery.c and add update set/get features and parse functions to
support error log page.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>
---
 drivers/nvme/target/discovery.c   | 10 +++++++
 drivers/nvme/target/fabrics-cmd.c | 50 +++++++++++++++++++++++--------
 2 files changed, 47 insertions(+), 13 deletions(-)

diff --git a/drivers/nvme/target/discovery.c b/drivers/nvme/target/discovery.c
index e1bb254671de..d2cb71a0b419 100644
--- a/drivers/nvme/target/discovery.c
+++ b/drivers/nvme/target/discovery.c
@@ -259,6 +259,8 @@ static void nvmet_execute_disc_set_features(struct nvmet_req *req)
 						  NVMET_DISC_AEN_CFG_OPTIONAL);
 		break;
 	default:
+		req->error_loc =
+			offsetof(struct nvme_common_command, cdw10);
 		stat = NVME_SC_INVALID_FIELD | NVME_SC_DNR;
 		break;
 	}
@@ -279,6 +281,8 @@ static void nvmet_execute_disc_get_features(struct nvmet_req *req)
 		nvmet_get_feat_async_event(req);
 		break;
 	default:
+		req->error_loc =
+			offsetof(struct nvme_common_command, cdw10);
 		stat = NVME_SC_INVALID_FIELD | NVME_SC_DNR;
 		break;
 	}
@@ -293,6 +297,8 @@ u16 nvmet_parse_discovery_cmd(struct nvmet_req *req)
 	if (unlikely(!(req->sq->ctrl->csts & NVME_CSTS_RDY))) {
 		pr_err("got cmd %d while not ready\n",
 		       cmd->common.opcode);
+		req->error_loc =
+			offsetof(struct nvme_common_command, opcode);
 		return NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
 	}
 
@@ -323,6 +329,8 @@ u16 nvmet_parse_discovery_cmd(struct nvmet_req *req)
 		default:
 			pr_err("unsupported get_log_page lid %d\n",
 			       cmd->get_log_page.lid);
+			req->error_loc =
+				offsetof(struct nvme_get_log_page_command, lid);
 		return NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
 		}
 	case nvme_admin_identify:
@@ -335,10 +343,12 @@ u16 nvmet_parse_discovery_cmd(struct nvmet_req *req)
 		default:
 			pr_err("unsupported identify cns %d\n",
 			       cmd->identify.cns);
+			req->error_loc = offsetof(struct nvme_identify, cns);
 			return NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
 		}
 	default:
 		pr_err("unhandled cmd %d\n", cmd->common.opcode);
+		req->error_loc = offsetof(struct nvme_common_command, opcode);
 		return NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
 	}
 
diff --git a/drivers/nvme/target/fabrics-cmd.c b/drivers/nvme/target/fabrics-cmd.c
index ee7d84621d65..993a84e67a3a 100644
--- a/drivers/nvme/target/fabrics-cmd.c
+++ b/drivers/nvme/target/fabrics-cmd.c
@@ -17,23 +17,26 @@
 
 static void nvmet_execute_prop_set(struct nvmet_req *req)
 {
+	u64 val = le64_to_cpu(req->cmd->prop_set.value);
 	u16 status = 0;
 
-	if (!(req->cmd->prop_set.attrib & 1)) {
-		u64 val = le64_to_cpu(req->cmd->prop_set.value);
-
-		switch (le32_to_cpu(req->cmd->prop_set.offset)) {
-		case NVME_REG_CC:
-			nvmet_update_cc(req->sq->ctrl, val);
-			break;
-		default:
-			status = NVME_SC_INVALID_FIELD | NVME_SC_DNR;
-			break;
-		}
-	} else {
+	if (req->cmd->prop_set.attrib & 1) {
+		req->error_loc =
+			offsetof(struct nvmf_property_set_command, attrib);
 		status = NVME_SC_INVALID_FIELD | NVME_SC_DNR;
+		goto out;
 	}
 
+	switch (le32_to_cpu(req->cmd->prop_set.offset)) {
+	case NVME_REG_CC:
+		nvmet_update_cc(req->sq->ctrl, val);
+		break;
+	default:
+		req->error_loc =
+			offsetof(struct nvmf_property_set_command, offset);
+		status = NVME_SC_INVALID_FIELD | NVME_SC_DNR;
+	}
+out:
 	nvmet_req_complete(req, status);
 }
 
@@ -69,6 +72,16 @@ static void nvmet_execute_prop_get(struct nvmet_req *req)
 		}
 	}
 
+
+	if (status && req->cmd->prop_get.attrib & 1) {
+		req->error_loc =
+			offsetof(struct nvmf_property_get_command, offset);
+	} else {
+		req->error_loc =
+			offsetof(struct nvmf_property_get_command, attrib);
+	}
+
+
 	req->rsp->result.u64 = cpu_to_le64(val);
 	nvmet_req_complete(req, status);
 }
@@ -89,6 +102,7 @@ u16 nvmet_parse_fabrics_cmd(struct nvmet_req *req)
 	default:
 		pr_err("received unknown capsule type 0x%x\n",
 			cmd->fabrics.fctype);
+		req->error_loc = offsetof(struct nvmf_common_command, fctype);
 		return NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
 	}
 
@@ -105,10 +119,12 @@ static u16 nvmet_install_queue(struct nvmet_ctrl *ctrl, struct nvmet_req *req)
 	old = cmpxchg(&req->sq->ctrl, NULL, ctrl);
 	if (old) {
 		pr_warn("queue already connected!\n");
+		req->error_loc = offsetof(struct nvmf_connect_command, opcode);
 		return NVME_SC_CONNECT_CTRL_BUSY | NVME_SC_DNR;
 	}
 	if (!sqsize) {
 		pr_warn("queue size zero!\n");
+		req->error_loc = offsetof(struct nvmf_connect_command, sqsize);
 		return NVME_SC_CONNECT_INVALID_PARAM | NVME_SC_DNR;
 	}
 
@@ -157,6 +173,7 @@ static void nvmet_execute_admin_connect(struct nvmet_req *req)
 	if (c->recfmt != 0) {
 		pr_warn("invalid connect version (%d).\n",
 			le16_to_cpu(c->recfmt));
+		req->error_loc = offsetof(struct nvmf_connect_command, recfmt);
 		status = NVME_SC_CONNECT_FORMAT | NVME_SC_DNR;
 		goto out;
 	}
@@ -171,8 +188,13 @@ static void nvmet_execute_admin_connect(struct nvmet_req *req)
 
 	status = nvmet_alloc_ctrl(d->subsysnqn, d->hostnqn, req,
 				  le32_to_cpu(c->kato), &ctrl);
-	if (status)
+	if (status) {
+		if (status == (NVME_SC_INVALID_FIELD | NVME_SC_DNR))
+			req->error_loc =
+				offsetof(struct nvme_common_command, opcode);
 		goto out;
+	}
+
 	uuid_copy(&ctrl->hostid, &d->hostid);
 
 	status = nvmet_install_queue(ctrl, req);
@@ -259,11 +281,13 @@ u16 nvmet_parse_connect_cmd(struct nvmet_req *req)
 	if (cmd->common.opcode != nvme_fabrics_command) {
 		pr_err("invalid command 0x%x on unconnected queue.\n",
 			cmd->fabrics.opcode);
+		req->error_loc = offsetof(struct nvme_common_command, opcode);
 		return NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
 	}
 	if (cmd->fabrics.fctype != nvme_fabrics_type_connect) {
 		pr_err("invalid capsule type 0x%x on unconnected queue.\n",
 			cmd->fabrics.fctype);
+		req->error_loc = offsetof(struct nvmf_common_command, fctype);
 		return NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
 	}
 
-- 
2.17.0

[PATCH 08/12] nvmet: add error log support for fabrics-cmd

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi at grimberg.me>

[PATCH 09/12] nvmet: add error log support for rdma backend

[Chaitanya Kulkarni]

This patch adds the support to maintain the error log page for rdma
transport, we mainly focus here on the NVME_INVALID_FIELD errors.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>
---
 drivers/nvme/target/rdma.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
index 932242e27b03..b15c6915fb40 100644
--- a/drivers/nvme/target/rdma.c
+++ b/drivers/nvme/target/rdma.c
@@ -629,8 +629,11 @@ static u16 nvmet_rdma_map_sgl_inline(struct nvmet_rdma_rsp *rsp)
 	u64 off = le64_to_cpu(sgl->addr);
 	u32 len = le32_to_cpu(sgl->length);
 
-	if (!nvme_is_write(rsp->req.cmd))
+	if (!nvme_is_write(rsp->req.cmd)) {
+		rsp->req.error_loc =
+			offsetof(struct nvme_common_command, opcode);
 		return NVME_SC_INVALID_FIELD | NVME_SC_DNR;
+	}
 
 	if (off + len > rsp->queue->dev->inline_data_size) {
 		pr_err("invalid inline data offset!\n");
@@ -695,6 +698,8 @@ static u16 nvmet_rdma_map_sgl(struct nvmet_rdma_rsp *rsp)
 			return nvmet_rdma_map_sgl_inline(rsp);
 		default:
 			pr_err("invalid SGL subtype: %#x\n", sgl->type);
+			rsp->req.error_loc =
+				offsetof(struct nvme_common_command, dptr);
 			return NVME_SC_INVALID_FIELD | NVME_SC_DNR;
 		}
 	case NVME_KEY_SGL_FMT_DATA_DESC:
@@ -705,10 +710,13 @@ static u16 nvmet_rdma_map_sgl(struct nvmet_rdma_rsp *rsp)
 			return nvmet_rdma_map_sgl_keyed(rsp, sgl, false);
 		default:
 			pr_err("invalid SGL subtype: %#x\n", sgl->type);
+			rsp->req.error_loc =
+				offsetof(struct nvme_common_command, dptr);
 			return NVME_SC_INVALID_FIELD | NVME_SC_DNR;
 		}
 	default:
 		pr_err("invalid SGL type: %#x\n", sgl->type);
+		rsp->req.error_loc = offsetof(struct nvme_common_command, dptr);
 		return NVME_SC_SGL_INVALID_TYPE | NVME_SC_DNR;
 	}
 }
-- 
2.17.0

[PATCH 09/12] nvmet: add error log support for rdma backend

[Sagi Grimberg]

Signed-off-by: Sagi Grimberg <sagi at grimberg.me>

[PATCH 10/12] nvmet: add error log support for admin-cmd

[Chaitanya Kulkarni]

This patch adds the support to maintain the error log page for admin
commands.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>
---
 drivers/nvme/target/admin-cmd.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 721b041a6b3b..fa62db7a5e9e 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -47,6 +47,7 @@ static u16 nvmet_get_smart_log_nsid(struct nvmet_req *req,
 	if (!ns) {
 		pr_err("Could not find namespace id : %d\n",
 				le32_to_cpu(req->cmd->get_log_page.nsid));
+		req->error_loc = offsetof(struct nvme_rw_command, nsid);
 		return NVME_SC_INVALID_NS;
 	}
 
@@ -380,6 +381,7 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)
 	u16 status = 0;
 
 	if (le32_to_cpu(req->cmd->identify.nsid) == NVME_NSID_ALL) {
+		req->error_loc = offsetof(struct nvme_identify, nsid);
 		status = NVME_SC_INVALID_NS | NVME_SC_DNR;
 		goto out;
 	}
@@ -500,6 +502,7 @@ static void nvmet_execute_identify_desclist(struct nvmet_req *req)
 
 	ns = nvmet_find_namespace(req->sq->ctrl, req->cmd->identify.nsid);
 	if (!ns) {
+		req->error_loc = offsetof(struct nvme_identify, nsid);
 		status = NVME_SC_INVALID_NS | NVME_SC_DNR;
 		goto out;
 	}
@@ -562,8 +565,10 @@ static u16 nvmet_set_feat_write_protect(struct nvmet_req *req)
 	u16 status = NVME_SC_FEATURE_NOT_CHANGEABLE;
 
 	req->ns = nvmet_find_namespace(req->sq->ctrl, req->cmd->rw.nsid);
-	if (unlikely(!req->ns))
+	if (unlikely(!req->ns)) {
+		req->error_loc = offsetof(struct nvme_common_command, nsid);
 		return status;
+	}
 
 	mutex_lock(&subsys->lock);
 	switch (write_protect) {
@@ -602,8 +607,10 @@ u16 nvmet_set_feat_async_event(struct nvmet_req *req, u32 mask)
 {
 	u32 val32 = le32_to_cpu(req->cmd->common.cdw11);
 
-	if (val32 & ~mask)
+	if (val32 & ~mask) {
+		req->error_loc = offsetof(struct nvme_common_command, cdw11);
 		return NVME_SC_INVALID_FIELD | NVME_SC_DNR;
+	}
 
 	WRITE_ONCE(req->sq->ctrl->aen_enabled, val32);
 	nvmet_set_result(req, val32);
@@ -635,6 +642,7 @@ static void nvmet_execute_set_features(struct nvmet_req *req)
 		status = nvmet_set_feat_write_protect(req);
 		break;
 	default:
+		req->error_loc = offsetof(struct nvme_common_command, cdw10);
 		status = NVME_SC_INVALID_FIELD | NVME_SC_DNR;
 		break;
 	}
@@ -648,9 +656,10 @@ static u16 nvmet_get_feat_write_protect(struct nvmet_req *req)
 	u32 result;
 
 	req->ns = nvmet_find_namespace(req->sq->ctrl, req->cmd->common.nsid);
-	if (!req->ns)
+	if (!req->ns)  {
+		req->error_loc = offsetof(struct nvme_common_command, nsid);
 		return NVME_SC_INVALID_NS | NVME_SC_DNR;
-
+	}
 	mutex_lock(&subsys->lock);
 	if (req->ns->readonly == true)
 		result = NVME_NS_WRITE_PROTECT;
@@ -716,6 +725,8 @@ static void nvmet_execute_get_features(struct nvmet_req *req)
 	case NVME_FEAT_HOST_ID:
 		/* need 128-bit host identifier flag */
 		if (!(req->cmd->common.cdw11 & cpu_to_le32(1 << 0))) {
+			req->error_loc =
+				offsetof(struct nvme_common_command, cdw11);
 			status = NVME_SC_INVALID_FIELD | NVME_SC_DNR;
 			break;
 		}
@@ -727,6 +738,8 @@ static void nvmet_execute_get_features(struct nvmet_req *req)
 		status = nvmet_get_feat_write_protect(req);
 		break;
 	default:
+		req->error_loc =
+			offsetof(struct nvme_common_command, cdw10);
 		status = NVME_SC_INVALID_FIELD | NVME_SC_DNR;
 		break;
 	}
@@ -848,5 +861,6 @@ u16 nvmet_parse_admin_cmd(struct nvmet_req *req)
 
 	pr_err("unhandled cmd %d on qid %d\n", cmd->common.opcode,
 	       req->sq->qid);
+	req->error_loc = offsetof(struct nvme_common_command, opcode);
 	return NVME_SC_INVALID_OPCODE | NVME_SC_DNR;
 }
-- 
2.17.0

[PATCH 10/12] nvmet: add error log support for admin-cmd

[Sagi Grimberg]

> @@ -380,6 +381,7 @@ static void nvmet_execute_identify_ns(struct nvmet_req *req)
>   	u16 status = 0;
>   
>   	if (le32_to_cpu(req->cmd->identify.nsid) == NVME_NSID_ALL) {
> +		req->error_loc = offsetof(struct nvme_identify, nsid);
>   		status = NVME_SC_INVALID_NS | NVME_SC_DNR;

I'm wandering how we can guarantee that we don't forget to set
req->error_loc for all future errors...

Anyways..

Reviewed-by: Sagi Grimberg <sagi at grimberg.me>

[PATCH 11/12] nvmet: add error log page cmd handler

[Chaitanya Kulkarni]

Now that we have support for all the major parts of the target we add
a NVMe error log page handler so that host can read the log page.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>
---
 drivers/nvme/target/admin-cmd.c | 36 ++++++++++++++++++++++++++-------
 1 file changed, 29 insertions(+), 7 deletions(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index fa62db7a5e9e..f22c0ff8f163 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -37,6 +37,34 @@ static void nvmet_execute_get_log_page_noop(struct nvmet_req *req)
 	nvmet_req_complete(req, nvmet_zero_sgl(req, 0, req->data_len));
 }
 
+static void nvmet_execute_get_log_page_error(struct nvmet_req *req)
+{
+	struct nvmet_ctrl *ctrl = req->sq->ctrl;
+	u16 status = NVME_SC_SUCCESS;
+	unsigned long flags;
+	off_t offset = 0;
+	u64 slot;
+	u64 i;
+
+	spin_lock_irqsave(&ctrl->error_lock, flags);
+	slot = ctrl->counter % NVMET_ERROR_LOG_SLOTS;
+
+	for (i = 0; i < NVMET_ERROR_LOG_SLOTS; i++) {
+		status = nvmet_copy_to_sgl(req, offset, &ctrl->slots[slot],
+				sizeof(struct nvme_error_slot));
+		if (status)
+			break;
+
+		if (slot == 0)
+			slot = NVMET_ERROR_LOG_SLOTS - 1;
+		else
+			slot--;
+		offset += sizeof(struct nvme_error_slot);
+	}
+	spin_unlock_irqrestore(&ctrl->error_lock, flags);
+	nvmet_req_complete(req, status);
+}
+
 static u16 nvmet_get_smart_log_nsid(struct nvmet_req *req,
 		struct nvme_smart_log *slog)
 {
@@ -789,13 +817,7 @@ u16 nvmet_parse_admin_cmd(struct nvmet_req *req)
 
 		switch (cmd->get_log_page.lid) {
 		case NVME_LOG_ERROR:
-			/*
-			 * We currently never set the More bit in the status
-			 * field, so all error log entries are invalid and can
-			 * be zeroed out.  This is called a minum viable
-			 * implementation (TM) of this mandatory log page.
-			 */
-			req->execute = nvmet_execute_get_log_page_noop;
+			req->execute = nvmet_execute_get_log_page_error;
 			return 0;
 		case NVME_LOG_SMART:
 			req->execute = nvmet_execute_get_log_page_smart;
-- 
2.17.0

[PATCH 12/12] nvmet: update smart log with num err log entries

[Chaitanya Kulkarni]

Now that we have error log page implementation update smart log command
handler to provide number of error log entries in the lifetime of the
controller field.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>
---
 drivers/nvme/target/admin-cmd.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index f22c0ff8f163..2824c3edb96e 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -135,6 +135,7 @@ static void nvmet_execute_get_log_page_smart(struct nvmet_req *req)
 {
 	struct nvme_smart_log *log;
 	u16 status = NVME_SC_INTERNAL;
+	unsigned long flags;
 
 	if (req->data_len != sizeof(*log))
 		goto out;
@@ -150,6 +151,10 @@ static void nvmet_execute_get_log_page_smart(struct nvmet_req *req)
 	if (status)
 		goto out_free_log;
 
+	spin_lock_irqsave(&req->sq->ctrl->error_lock, flags);
+	put_unaligned_le64(req->sq->ctrl->counter, &log->num_err_log_entries);
+	spin_unlock_irqrestore(&req->sq->ctrl->error_lock, flags);
+
 	status = nvmet_copy_to_sgl(req, 0, log, sizeof(*log));
 out_free_log:
 	kfree(log);
-- 
2.17.0

[PATCH 10/12] nvmet: add error log support for admin-cmd

[hch]

On Mon, Dec 10, 2018@10:55:08PM +0000, Chaitanya Kulkarni wrote:
> I'm wandering how we can guarantee that we don't forget to set
> req->error_loc for all future errors...
> 
> Anyways..
> 
> [CK] One way to do that is to have a default centralized mapping from the error codes,
> not sure if that is worth an effort though, will skip this for now.

Well, the M bit and the error log page are optional features.
Maybe we should only set the M bit and generate a log page if error_loc
is set?  Although for that we'd need to initialize it to -1 as 0
is a valid error location.

We could then also add a debug option to print a warning for errors
without error_loc if we care enough.

[PATCH 10/12] nvmet: add error log support for admin-cmd

[Sagi Grimberg]

>> I'm wandering how we can guarantee that we don't forget to set
>> req->error_loc for all future errors...
>>
>> Anyways..
>>
>> [CK] One way to do that is to have a default centralized mapping from the error codes,
>> not sure if that is worth an effort though, will skip this for now.
> 
> Well, the M bit and the error log page are optional features.
> Maybe we should only set the M bit and generate a log page if error_loc
> is set?  Although for that we'd need to initialize it to -1 as 0
> is a valid error location.
> 
> We could then also add a debug option to print a warning for errors
> without error_loc if we care enough.

Sounds good to me...

[PATCH 10/12] nvmet: add error log support for admin-cmd

[Chaitanya Kulkarni]

From: Sagi Grimberg <sagi@grimberg.me>
Sent: Tuesday, December 11, 2018 3:59 PM
To: hch at lst.de; Chaitanya Kulkarni
Cc: linux-nvme at lists.infradead.org; keith.busch at intel.com
Subject: Re: [PATCH 10/12] nvmet: add error log support for admin-cmd
? 
 

>> I'm wandering how we can guarantee that we don't forget to set
>> req->error_loc for all future errors...
>>
>> Anyways..
>>
>> [CK] One way to do that is to have a default centralized mapping from the error codes,
>> not sure if that is worth an effort though, will skip this for now.
> 
> Well, the M bit and the error log page are optional features.
> Maybe we should only set the M bit and generate a log page if error_loc
> is set?? Although for that we'd need to initialize it to -1 as 0
> is a valid error location.
> 
> We could then also add a debug option to print a warning for errors
> without error_loc if we care enough.

Sounds good to me...



[CK] Okay, will send a new series with Christoph's suggestion.