Skip to contentSkip to conversation

nvmet: add error log page support

PATCH Superseded Chaitanya Kulkarni 3 participants 30 messages Dec 10, 2018 – Dec 12, 2018 1 message not in the archive

Subsystems (2)
NVMe NVMe target

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Chaitanya Kulkarni

author
#0 lore raw

cover nvmet: add error log page support

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(-)
Signature · 1 line
2.17.0
author
#1 lore raw

PATCH 1/12 nvme: remove nvme_common command cdw10 array

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(-)
drivers/nvme/host/core.c +9 -9
index f90576862736..090ce8f69ef2 100644
@@ -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);
drivers/nvme/host/trace.h +2 -2
index 196d5bd56718..1978deb6fcc7 100644
@@ -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),
drivers/nvme/target/admin-cmd.c +6 -6
index 753515fc8028..721b041a6b3b 100644
@@ -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;
 		}
drivers/nvme/target/discovery.c +2 -2
index 4d8757ae8210..e1bb254671de 100644
@@ -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) {
drivers/nvme/target/nvmet.h +1 -1
index 89df51ee5bdf..dafee1af4829 100644
@@ -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);
include/linux/nvme.h +6 -1
index 4d7907e3771e..b94fe8fadc4f 100644
@@ -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 {
Signature · 1 line
2.17.0
maintainer
#2 lore raw

PATCH 1/12 nvme: remove nvme_common command cdw10 array

Reviewed-by Sagi Grimberg <sagi at grimberg.me>
author
#3 lore raw

PATCH 2/12 nvme: add error log page slot definition

In reply to 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(+)
include/linux/nvme.h +14 -0
index b94fe8fadc4f..bbcc83886899 100644
@@ -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)
 {
 	/*
Signature · 1 line
2.17.0
maintainer
#4 lore raw

PATCH 2/12 nvme: add error log page slot definition

Looks fine,

Reviewed-by Sagi Grimberg <sagi at grimberg.me>
author
#5 lore raw

PATCH 3/12 nvmet: add error-log definitions

In reply to 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(+)
drivers/nvme/target/core.c +5 -0
index e468100b9211..4ed0022b8450 100644
@@ -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);
drivers/nvme/target/nvmet.h +6 -0
index dafee1af4829..5a19f64de891 100644
@@ -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;
Signature · 1 line
2.17.0
maintainer
#6 lore raw

PATCH 3/12 nvmet: add error-log definitions

Quoted 6 lines
drivers/nvme/target/nvmet.h +0 -0
index dafee1af4829..5a19f64de891 100644

drivers/nvme/target/nvmet.h struct nvmet_ctrl { · line 209

Quoted 8 lines
drivers/nvme/target/nvmet.h +4 -0
@@ -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.

author
#7 lore raw

PATCH 3/12 nvmet: add error-log definitions

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 <[email protected]>
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 ?

Quoted 5 lines
drivers/nvme/target/nvmet.h +0 -0
index dafee1af4829..5a19f64de891 100644
@@ -202,6 +202,10 @@ struct nvmet_ctrl {
Quoted 8 lines

??
??????? 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.

author
#8 lore raw

PATCH 4/12 nvmet: add interface to update error-log page

In reply to 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(-)
drivers/nvme/target/core.c +30 -1
index 4ed0022b8450..4fe70fc322b4 100644
@@ -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);
drivers/nvme/target/nvmet.h +0 -5
index 5a19f64de891..662def80ff23 100644
@@ -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);
Signature · 1 line
2.17.0
maintainer
#9 lore raw

PATCH 4/12 nvmet: add interface to update error-log page

Quoted 4 lines
drivers/nvme/target/core.c +3 -0
+static void nvmet_set_error(struct nvmet_req *req, u16 status)
+{
+	struct nvmet_ctrl *ctrl = req->sq->ctrl;

drivers/nvme/target/core.c static void nvmet_update_sq_head(struct nvmet_req *req) · line 626

Quoted 10 lines
drivers/nvme/target/core.c +10 -0
+	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.

author
#10 lore raw

PATCH 4/12 nvmet: add interface to update error-log page

Sounds good.

From: Sagi Grimberg <[email protected]>
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 ?

Review

Quoted 13 lines
(quoted diff) +13 -0
+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.

author
#11 lore raw

PATCH 5/12 nvmet: add error log support in the core

In reply to 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(-)
drivers/nvme/target/core.c +23 -8
index 4fe70fc322b4..5f0b281945e4 100644
@@ -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);
Signature · 1 line
2.17.0
maintainer
#12 lore raw

PATCH 5/12 nvmet: add error log support in the core

Looks good,

Reviewed-by Sagi Grimberg <sagi at grimberg.me>
author
#13 lore raw

PATCH 6/12 nvmet: add error log support for bdev backend

In reply to 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(-)
drivers/nvme/target/io-cmd-bdev.c +76 -8
index c1cb2ed5531c..33b548240568 100644
@@ -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;
 	}
 }
Signature · 1 line
2.17.0
maintainer
#14 lore raw

PATCH 6/12 nvmet: add error log support for bdev backend

Quoted 30 lines
drivers/nvme/target/io-cmd-bdev.c +29 -0
+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:

drivers/nvme/target/io-cmd-bdev.c void nvmet_bdev_ns_disable(struct nvmet_ns *ns) · line 85

Quoted 10 lines
drivers/nvme/target/io-cmd-bdev.c +10 -0
+			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...

Quoted 9 lines
drivers/nvme/target/io-cmd-bdev.c +1 -0
@@ -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);

drivers/nvme/target/io-cmd-bdev.c static void nvmet_bdev_execute_write_zeroes(struct nvmet_req *req) · line 283

Quoted 5 lines
drivers/nvme/target/io-cmd-bdev.c +2 -0
@@ -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?

author
#15 lore raw

PATCH 6/12 nvmet: add error log support for bdev backend

From: Sagi Grimberg <[email protected]>
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 ?

Quoted 29 lines
(quoted diff) +29 -0
+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:

Review

Quoted 10 lines
(quoted diff) +10 -0
+???????????????????? 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.

Quoted 11 lines
(quoted diff) +1 -0
@@ -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);

(quoted diff) +0 -0
@@ -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;
Quoted 3 lines

??
+???? 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.

author
#16 lore raw

PATCH 7/12 nvmet: add error log support for file backend

In reply to 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(-)
drivers/nvme/target/core.c +42 -0
index 5f0b281945e4..dee691f6e3b3 100644
@@ -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);
drivers/nvme/target/io-cmd-file.c +20 -15
index 12eaa8ddc248..517522305e5c 100644
@@ -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;
 	}
 }
drivers/nvme/target/nvmet.h +2 -0
index 662def80ff23..03f91cd3c24d 100644
@@ -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 */
Signature · 1 line
2.17.0
maintainer
#17 lore raw

PATCH 7/12 nvmet: add error log support for file backend

drivers/nvme/target/core.c u32 nvmet_ana_group_enabled[NVMET_MAX_ANAGRPS + 1]; · line 78

Quoted 4 lines
drivers/nvme/target/core.c +3 -0
+	case -EILSEQ:
+		req->error_loc = offsetof(struct nvme_rw_command, nsid);
+		status = NVME_SC_INVALID_PI;

Same here...

author
#18 lore raw

PATCH 7/12 nvmet: add error log support for file backend

From: Sagi Grimberg <[email protected]>
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 ?

Quoted 3 lines

+???? 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.

author
#19 lore raw

PATCH 8/12 nvmet: add error log support for fabrics-cmd

In reply to 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(-)
drivers/nvme/target/discovery.c +10 -0
index e1bb254671de..d2cb71a0b419 100644
@@ -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;
 	}
drivers/nvme/target/fabrics-cmd.c +37 -13

107 lines not on this page · raw message

Signature · 1 line
2.17.0
maintainer
#20 lore raw

PATCH 8/12 nvmet: add error log support for fabrics-cmd

Reviewed-by Sagi Grimberg <sagi at grimberg.me>
author
#21 lore raw

PATCH 9/12 nvmet: add error log support for rdma backend

In reply to 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(-)
drivers/nvme/target/rdma.c +9 -1
index 932242e27b03..b15c6915fb40 100644
@@ -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;
 	}
 }
Signature · 1 line
2.17.0
maintainer
#22 lore raw

PATCH 9/12 nvmet: add error log support for rdma backend

Signed-off-by Sagi Grimberg <sagi at grimberg.me>
author
#23 lore raw

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

In reply to 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(-)
drivers/nvme/target/admin-cmd.c +18 -4
index 721b041a6b3b..fa62db7a5e9e 100644
@@ -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;
 }
Signature · 1 line
2.17.0
maintainer
#24 lore raw

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

drivers/nvme/target/admin-cmd.c static void nvmet_execute_identify_ns(struct nvmet_req *req) · line 385

Quoted 7 lines
drivers/nvme/target/admin-cmd.c +1 -0
@@ -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>
author
#25 lore raw

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

In reply to 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(-)
drivers/nvme/target/admin-cmd.c +29 -7
index fa62db7a5e9e..f22c0ff8f163 100644
@@ -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;
Signature · 1 line
2.17.0
author
#26 lore raw

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

In reply to 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(+)
drivers/nvme/target/admin-cmd.c +5 -0
index f22c0ff8f163..2824c3edb96e 100644
@@ -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);
Signature · 1 line
2.17.0

A message that is not in the archive (sent off-list or not yet received): BYAPR04MB4502206C5362955EF2F2014686A50@BYAPR04MB4502.namprd04.prod.outlook.com

maintainer
#28 lore raw

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

Quoted 7 lines · Chaitanya Kulkarni

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.

maintainer
#29 lore raw

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

Quoted 9 lines
Quoted 8 lines

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.

Quoted 7 lines

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...

author
#30 lore raw

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

From: Sagi Grimberg <[email protected]>
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 ?

Quoted 8 lines
Quoted 7 lines

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.

Quoted 7 lines

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.