[RFC v3 0/1] nvme: add passthrough error logging opt-in

[RFC v3 0/1] nvme: add passthrough error logging opt-in

[Alan Adamson]

v3:
- Log a passthrough specific message that dumps CDW* contents.
- Enable/disable vis sysfs rather than debugfs.

v2:
- Included Pankaj Raghav's patch 'nvme: ignore starting sector while error logging for passthrough requests'
  with a couple changes.
- Moved error_logging flag to nvme_ctrl structure
- The entire nvme-debugfs.c does not need to be guarded by #ifdef CONFIG_FAULT_INJECTION_DEBUG_FS.
- Use IS_ENABLED((CONFIG_NVME_ERROR_LOGGING_DEBUG_FS)) to determine if error logging should be
  initialized.
- Various other nits.

[RFC v3 1/1] nvme: add passthrough error logging opt-in

[Alan Adamson]

Commit d7ac8dca938c ("nvme: quiet user passthrough command errors") disabled error
logging for user passthrough commands.  This commit adds the ability to opt-in
to passthrough error logging.

To enable passthrough error logging:
        echo 1 > /sys/class/nvme/nvme0/passthrough_logging

To disable passthrough error logging:
        echo 0 > /sys/class/nvme/nvme0/passthrough_logging

By default, passthrough error logging will remain disabled.

Signed-off-by: Alan Adamson <alan.adamson@oracle.com>
---
 drivers/nvme/host/core.c | 93 ++++++++++++++++++++++++++++++++++++++--
 drivers/nvme/host/nvme.h |  1 +
 2 files changed, 90 insertions(+), 4 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 53ef028596c6..82d4f8235a8f 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -337,6 +337,49 @@ static void nvme_log_error(struct request *req)
 			   nr->status & NVME_SC_DNR  ? "DNR "  : "");
 }
 
+static void nvme_log_error_passthrough(struct request *req)
+{
+	struct nvme_ns *ns = req->q->queuedata;
+	struct nvme_request *nr = nvme_req(req);
+
+	if (ns) {
+		pr_err_ratelimited("%s: %s(0x%x), %s (sct 0x%x / sc 0x%x) %s%s"
+			"cdw10=0x%x cdw11=0x%x cdw12=0x%x cdw13=0x%x cdw14=0x%x cdw15=0x%x\n",
+			ns->disk ? ns->disk->disk_name : "?",
+			nvme_get_opcode_str(nr->cmd->common.opcode),
+			nr->cmd->common.opcode,
+			nvme_get_error_status_str(nr->status),
+			nr->status >> 8 & 7,	/* Status Code Type */
+			nr->status & 0xff,	/* Status Code */
+			nr->status & NVME_SC_MORE ? "MORE " : "",
+			nr->status & NVME_SC_DNR  ? "DNR "  : "",
+			nr->cmd->common.cdw10,
+			nr->cmd->common.cdw11,
+			nr->cmd->common.cdw12,
+			nr->cmd->common.cdw13,
+			nr->cmd->common.cdw14,
+			nr->cmd->common.cdw14);
+		return;
+	}
+
+	pr_err_ratelimited("%s: %s(0x%x), %s (sct 0x%x / sc 0x%x) %s%s"
+			"cdw10=0x%x cdw11=0x%x cdw12=0x%x cdw13=0x%x cdw14=0x%x cdw15=0x%x\n",
+			dev_name(nr->ctrl->device),
+			nvme_get_admin_opcode_str(nr->cmd->common.opcode),
+			nr->cmd->common.opcode,
+			nvme_get_error_status_str(nr->status),
+			nr->status >> 8 & 7,	/* Status Code Type */
+			nr->status & 0xff,	/* Status Code */
+			nr->status & NVME_SC_MORE ? "MORE " : "",
+			nr->status & NVME_SC_DNR  ? "DNR "  : "",
+			nr->cmd->common.cdw10,
+			nr->cmd->common.cdw11,
+			nr->cmd->common.cdw12,
+			nr->cmd->common.cdw13,
+			nr->cmd->common.cdw14,
+			nr->cmd->common.cdw14);
+}
+
 enum nvme_disposition {
 	COMPLETE,
 	RETRY,
@@ -381,8 +424,12 @@ static inline void nvme_end_req(struct request *req)
 {
 	blk_status_t status = nvme_error_status(nvme_req(req)->status);
 
-	if (unlikely(nvme_req(req)->status && !(req->rq_flags & RQF_QUIET)))
-		nvme_log_error(req);
+	if (unlikely(nvme_req(req)->status && !(req->rq_flags & RQF_QUIET))) {
+		if (blk_rq_is_passthrough(req))
+			nvme_log_error_passthrough(req);
+		else
+			nvme_log_error(req);
+	}
 	nvme_end_req_zoned(req);
 	nvme_trace_bio_complete(req);
 	if (req->cmd_flags & REQ_NVME_MPATH)
@@ -666,6 +713,8 @@ static inline void nvme_clear_nvme_request(struct request *req)
 /* initialize a passthrough request */
 void nvme_init_request(struct request *req, struct nvme_command *cmd)
 {
+	struct nvme_request *nr = nvme_req(req);
+
 	if (req->q->queuedata)
 		req->timeout = NVME_IO_TIMEOUT;
 	else /* no queuedata implies admin queue */
@@ -678,8 +727,10 @@ void nvme_init_request(struct request *req, struct nvme_command *cmd)
 	if (req->mq_hctx->type == HCTX_TYPE_POLL)
 		req->cmd_flags |= REQ_POLLED;
 	nvme_clear_nvme_request(req);
-	req->rq_flags |= RQF_QUIET;
-	memcpy(nvme_req(req)->cmd, cmd, sizeof(*cmd));
+	if (!nr->ctrl->error_logging)
+		req->rq_flags |= RQF_QUIET;
+
+	memcpy(nr->cmd, cmd, sizeof(*cmd));
 }
 EXPORT_SYMBOL_GPL(nvme_init_request);
 
@@ -3418,6 +3469,38 @@ static ssize_t nvme_sysfs_rescan(struct device *dev,
 }
 static DEVICE_ATTR(rescan_controller, S_IWUSR, NULL, nvme_sysfs_rescan);
 
+static ssize_t nvme_passthrough_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
+
+	if (ctrl->error_logging)
+		return sysfs_emit(buf, "on\n");
+	else
+		return sysfs_emit(buf, "off\n");
+}
+
+static ssize_t nvme_passthrough_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
+	int passthrough_enable, err;
+
+	err = kstrtoint(buf, 10, &passthrough_enable);
+	if (err)
+		return -EINVAL;
+
+	if (passthrough_enable)
+		ctrl->error_logging = true;
+	else
+		ctrl->error_logging = false;
+
+	return count;
+}
+
+static DEVICE_ATTR(passthrough_logging, S_IRUGO | S_IWUSR,
+	nvme_passthrough_show, nvme_passthrough_store);
+
 static inline struct nvme_ns_head *dev_to_ns_head(struct device *dev)
 {
 	struct gendisk *disk = dev_to_disk(dev);
@@ -3926,6 +4009,7 @@ static struct attribute *nvme_dev_attrs[] = {
 	&dev_attr_dhchap_secret.attr,
 	&dev_attr_dhchap_ctrl_secret.attr,
 #endif
+	&dev_attr_passthrough_logging.attr,
 	NULL
 };
 
@@ -5125,6 +5209,7 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
 	int ret;
 
 	ctrl->state = NVME_CTRL_NEW;
+	ctrl->error_logging = false;
 	clear_bit(NVME_CTRL_FAILFAST_EXPIRED, &ctrl->flags);
 	spin_lock_init(&ctrl->lock);
 	mutex_init(&ctrl->scan_lock);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index bf46f122e9e1..dce5e6f7260c 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -248,6 +248,7 @@ struct nvme_ctrl {
 	bool comp_seen;
 	enum nvme_ctrl_state state;
 	bool identified;
+	bool error_logging;
 	spinlock_t lock;
 	struct mutex scan_lock;
 	const struct nvme_ctrl_ops *ops;
-- 
2.31.1

Re: [RFC v3 1/1] nvme: add passthrough error logging opt-in

[Pankaj Raghav]

On Fri, Mar 31, 2023 at 03:18:23PM -0700, Alan Adamson wrote:
> Commit d7ac8dca938c ("nvme: quiet user passthrough command errors") disabled error
> logging for user passthrough commands.  This commit adds the ability to opt-in
> to passthrough error logging.
> 
> To enable passthrough error logging:
>         echo 1 > /sys/class/nvme/nvme0/passthrough_logging
passthrough_logging is a bit misleading as it doesn't indicate it is
going to log only passthrough errors. `passthrough_error_logging` is a bit
verbose but it is more apt for what this sysfs entry is doing. Maybe
an abbreviation for passthrough_error_logging?
> 
> To disable passthrough error logging:
>         echo 0 > /sys/class/nvme/nvme0/passthrough_logging
> 
> By default, passthrough error logging will remain disabled.
> 

Re: [RFC v3 1/1] nvme: add passthrough error logging opt-in

[alan.adamson]

On 4/3/23 1:26 AM, Pankaj Raghav wrote:
> On Fri, Mar 31, 2023 at 03:18:23PM -0700, Alan Adamson wrote:
>> Commit d7ac8dca938c ("nvme: quiet user passthrough command errors") disabled error
>> logging for user passthrough commands.  This commit adds the ability to opt-in
>> to passthrough error logging.
>>
>> To enable passthrough error logging:
>>          echo 1 > /sys/class/nvme/nvme0/passthrough_logging
> passthrough_logging is a bit misleading as it doesn't indicate it is
> going to log only passthrough errors. `passthrough_error_logging` is a bit
> verbose but it is more apt for what this sysfs entry is doing. Maybe
> an abbreviation for passthrough_error_logging?

Since we are looking at only opting in to passthrough admin commands, 
how about we call it:

/sys/class/nvme/nvme0/pt_admin_error_logging


Alan



>> To disable passthrough error logging:
>>          echo 0 > /sys/class/nvme/nvme0/passthrough_logging
>>
>> By default, passthrough error logging will remain disabled.
>>

RE: [RFC v3 1/1] nvme: add passthrough error logging opt-in

[Minwoo Im]

> -----Original Message-----
> From: Linux-nvme <linux-nvme-bounces@lists.infradead.org> On Behalf Of
> alan.adamson@oracle.com
> Sent: Thursday, April 6, 2023 7:35 AM
> To: Pankaj Raghav <p.raghav@samsung.com>
> Cc: linux-nvme@lists.infradead.org; kbusch@kernel.org; hch@lst.de;
> sagi@grimberg.me
> Subject: Re: [RFC v3 1/1] nvme: add passthrough error logging opt-in
>
>
> On 4/3/23 1:26 AM, Pankaj Raghav wrote:
> > On Fri, Mar 31, 2023 at 03:18:23PM -0700, Alan Adamson wrote:
> >> Commit d7ac8dca938c ("nvme: quiet user passthrough command errors") disabled
> error
> >> logging for user passthrough commands.  This commit adds the ability to opt-in
> >> to passthrough error logging.
> >>
> >> To enable passthrough error logging:
> >>          echo 1 > /sys/class/nvme/nvme0/passthrough_logging
> > passthrough_logging is a bit misleading as it doesn't indicate it is
> > going to log only passthrough errors. `passthrough_error_logging` is a bit
> > verbose but it is more apt for what this sysfs entry is doing. Maybe
> > an abbreviation for passthrough_error_logging?
>
> Since we are looking at only opting in to passthrough admin commands,
> how about we call it:
>
> /sys/class/nvme/nvme0/pt_admin_error_logging

I would make `passthrough` short to `passthu` as it looks like we don't use
`pt` as a short for passthrough in drivers/nvme/[host|target].

Re: [RFC v3 1/1] nvme: add passthrough error logging opt-in

[Chaitanya Kulkarni]

>>> passthrough_logging is a bit misleading as it doesn't indicate it is
>>> going to log only passthrough errors. `passthrough_error_logging` is a bit
>>> verbose but it is more apt for what this sysfs entry is doing. Maybe
>>> an abbreviation for passthrough_error_logging?
>> Since we are looking at only opting in to passthrough admin commands,
>> how about we call it:
>>
>> /sys/class/nvme/nvme0/pt_admin_error_logging
> I would make `passthrough` short to `passthu` as it looks like we don't use
> `pt` as a short for passthrough in drivers/nvme/[host|target].
>

right it's "passthru" not "passthu", see :-

/sys/kernel/config/nvmet/subsystems/tests
├── allowed_hosts
├── attr_allow_any_host
├── attr_cntlid_max
├── attr_cntlid_min
├── attr_firmware
├── attr_ieee_oui
├── attr_model
├── attr_pi_enable
├── attr_qid_max
├── attr_serial
├── attr_version
├── namespaces
└── passthru
     ├── admin_timeout
     ├── clear_ids
     ├── device_path
     ├── enable
     └── io_timeout


passthru_admin_err_logging should to the trick ...

-ck

RE: [RFC v3 1/1] nvme: add passthrough error logging opt-in

[Minwoo Im]

> -----Original Message-----
> From: Chaitanya Kulkarni <chaitanyak@nvidia.com>
> Sent: Thursday, April 6, 2023 1:24 PM
> To: minwoo.im@samsung.com; alan.adamson@oracle.com; Pankaj Raghav Partha
> Sarathy <p.raghav@samsung.com>
> Cc: linux-nvme@lists.infradead.org; kbusch@kernel.org; hch@lst.de;
> sagi@grimberg.me
> Subject: Re: [RFC v3 1/1] nvme: add passthrough error logging opt-in
>
>
> >>> passthrough_logging is a bit misleading as it doesn't indicate it isa
> >>> going to log only passthrough errors. `passthrough_error_logging` is a bit
> >>> verbose but it is more apt for what this sysfs entry is doing. Maybe
> >>> an abbreviation for passthrough_error_logging?
> >> Since we are looking at only opting in to passthrough admin commands,
> >> how about we call it:
> >>
> >> /sys/class/nvme/nvme0/pt_admin_error_logging
> > I would make `passthrough` short to `passthu` as it looks like we don't use
> > `pt` as a short for passthrough in drivers/nvme/[host|target].
> >
>
> right it's "passthru" not "passthu", see :-

Yes, indeed.  I missed out 'r' ;)  Thanks for fixing this!

> /sys/kernel/config/nvmet/subsystems/tests
> ├── allowed_hosts
> ├── attr_allow_any_host
> ├── attr_cntlid_max
> ├── attr_cntlid_min
> ├── attr_firmware
> ├── attr_ieee_oui
> ├── attr_model
> ├── attr_pi_enable
> ├── attr_qid_max
> ├── attr_serial
> ├── attr_version
> ├── namespaces
> └── passthru
>      ├── admin_timeout
>      ├── clear_ids
>      ├── device_path
>      ├── enable
>      └── io_timeout
>
>
> passthru_admin_err_logging should to the trick ...
> -ck

Re: [RFC v3 1/1] nvme: add passthrough error logging opt-in

[Sagi Grimberg]

On 4/1/23 01:18, Alan Adamson wrote:
> Commit d7ac8dca938c ("nvme: quiet user passthrough command errors") disabled error
> logging for user passthrough commands.  This commit adds the ability to opt-in
> to passthrough error logging.
> 
> To enable passthrough error logging:
>          echo 1 > /sys/class/nvme/nvme0/passthrough_logging
> 
> To disable passthrough error logging:
>          echo 0 > /sys/class/nvme/nvme0/passthrough_logging
> 
> By default, passthrough error logging will remain disabled.
> 
> Signed-off-by: Alan Adamson <alan.adamson@oracle.com>
> ---
>   drivers/nvme/host/core.c | 93 ++++++++++++++++++++++++++++++++++++++--
>   drivers/nvme/host/nvme.h |  1 +
>   2 files changed, 90 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 53ef028596c6..82d4f8235a8f 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -337,6 +337,49 @@ static void nvme_log_error(struct request *req)
>   			   nr->status & NVME_SC_DNR  ? "DNR "  : "");
>   }
>   
> +static void nvme_log_error_passthrough(struct request *req)
> +{
> +	struct nvme_ns *ns = req->q->queuedata;
> +	struct nvme_request *nr = nvme_req(req);
> +
> +	if (ns) {
> +		pr_err_ratelimited("%s: %s(0x%x), %s (sct 0x%x / sc 0x%x) %s%s"
> +			"cdw10=0x%x cdw11=0x%x cdw12=0x%x cdw13=0x%x cdw14=0x%x cdw15=0x%x\n",
> +			ns->disk ? ns->disk->disk_name : "?",
> +			nvme_get_opcode_str(nr->cmd->common.opcode),
> +			nr->cmd->common.opcode,
> +			nvme_get_error_status_str(nr->status),
> +			nr->status >> 8 & 7,	/* Status Code Type */
> +			nr->status & 0xff,	/* Status Code */
> +			nr->status & NVME_SC_MORE ? "MORE " : "",
> +			nr->status & NVME_SC_DNR  ? "DNR "  : "",
> +			nr->cmd->common.cdw10,
> +			nr->cmd->common.cdw11,
> +			nr->cmd->common.cdw12,
> +			nr->cmd->common.cdw13,
> +			nr->cmd->common.cdw14,
> +			nr->cmd->common.cdw14);
> +		return;
> +	}
> +
> +	pr_err_ratelimited("%s: %s(0x%x), %s (sct 0x%x / sc 0x%x) %s%s"
> +			"cdw10=0x%x cdw11=0x%x cdw12=0x%x cdw13=0x%x cdw14=0x%x cdw15=0x%x\n",
> +			dev_name(nr->ctrl->device),
> +			nvme_get_admin_opcode_str(nr->cmd->common.opcode),
> +			nr->cmd->common.opcode,
> +			nvme_get_error_status_str(nr->status),
> +			nr->status >> 8 & 7,	/* Status Code Type */
> +			nr->status & 0xff,	/* Status Code */
> +			nr->status & NVME_SC_MORE ? "MORE " : "",
> +			nr->status & NVME_SC_DNR  ? "DNR "  : "",
> +			nr->cmd->common.cdw10,
> +			nr->cmd->common.cdw11,
> +			nr->cmd->common.cdw12,
> +			nr->cmd->common.cdw13,
> +			nr->cmd->common.cdw14,
> +			nr->cmd->common.cdw14);
> +}
> +
>   enum nvme_disposition {
>   	COMPLETE,
>   	RETRY,
> @@ -381,8 +424,12 @@ static inline void nvme_end_req(struct request *req)
>   {
>   	blk_status_t status = nvme_error_status(nvme_req(req)->status);
>   
> -	if (unlikely(nvme_req(req)->status && !(req->rq_flags & RQF_QUIET)))
> -		nvme_log_error(req);
> +	if (unlikely(nvme_req(req)->status && !(req->rq_flags & RQF_QUIET))) {
> +		if (blk_rq_is_passthrough(req))
> +			nvme_log_error_passthrough(req);
> +		else
> +			nvme_log_error(req);
> +	}
>   	nvme_end_req_zoned(req);
>   	nvme_trace_bio_complete(req);
>   	if (req->cmd_flags & REQ_NVME_MPATH)
> @@ -666,6 +713,8 @@ static inline void nvme_clear_nvme_request(struct request *req)
>   /* initialize a passthrough request */
>   void nvme_init_request(struct request *req, struct nvme_command *cmd)
>   {
> +	struct nvme_request *nr = nvme_req(req);
> +
>   	if (req->q->queuedata)
>   		req->timeout = NVME_IO_TIMEOUT;
>   	else /* no queuedata implies admin queue */
> @@ -678,8 +727,10 @@ void nvme_init_request(struct request *req, struct nvme_command *cmd)
>   	if (req->mq_hctx->type == HCTX_TYPE_POLL)
>   		req->cmd_flags |= REQ_POLLED;
>   	nvme_clear_nvme_request(req);
> -	req->rq_flags |= RQF_QUIET;
> -	memcpy(nvme_req(req)->cmd, cmd, sizeof(*cmd));
> +	if (!nr->ctrl->error_logging)
> +		req->rq_flags |= RQF_QUIET;
> +
> +	memcpy(nr->cmd, cmd, sizeof(*cmd));

Question, if we already introduce granularity to this setting, why
not per-ns? why only per controller? I'd think it makes more
sense to do this per ns. Will also remove access to ctrl in the hot
path...

>   }
>   EXPORT_SYMBOL_GPL(nvme_init_request);
>   
> @@ -3418,6 +3469,38 @@ static ssize_t nvme_sysfs_rescan(struct device *dev,
>   }
>   static DEVICE_ATTR(rescan_controller, S_IWUSR, NULL, nvme_sysfs_rescan);
>   
> +static ssize_t nvme_passthrough_show(struct device *dev,
> +		struct device_attribute *attr, char *buf)
> +{
> +	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
> +
> +	if (ctrl->error_logging)
> +		return sysfs_emit(buf, "on\n");
> +	else
> +		return sysfs_emit(buf, "off\n");
> +}
> +
> +static ssize_t nvme_passthrough_store(struct device *dev,
> +		struct device_attribute *attr, const char *buf, size_t count)
> +{
> +	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
> +	int passthrough_enable, err;
> +
> +	err = kstrtoint(buf, 10, &passthrough_enable);
> +	if (err)
> +		return -EINVAL;
> +
> +	if (passthrough_enable)
> +		ctrl->error_logging = true;
> +	else
> +		ctrl->error_logging = false;
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR(passthrough_logging, S_IRUGO | S_IWUSR,
> +	nvme_passthrough_show, nvme_passthrough_store);
> +
>   static inline struct nvme_ns_head *dev_to_ns_head(struct device *dev)
>   {
>   	struct gendisk *disk = dev_to_disk(dev);
> @@ -3926,6 +4009,7 @@ static struct attribute *nvme_dev_attrs[] = {
>   	&dev_attr_dhchap_secret.attr,
>   	&dev_attr_dhchap_ctrl_secret.attr,
>   #endif
> +	&dev_attr_passthrough_logging.attr,
>   	NULL
>   };
>   
> @@ -5125,6 +5209,7 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
>   	int ret;
>   
>   	ctrl->state = NVME_CTRL_NEW;
> +	ctrl->error_logging = false;
>   	clear_bit(NVME_CTRL_FAILFAST_EXPIRED, &ctrl->flags);
>   	spin_lock_init(&ctrl->lock);
>   	mutex_init(&ctrl->scan_lock);
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index bf46f122e9e1..dce5e6f7260c 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -248,6 +248,7 @@ struct nvme_ctrl {
>   	bool comp_seen;
>   	enum nvme_ctrl_state state;
>   	bool identified;
> +	bool error_logging;
>   	spinlock_t lock;
>   	struct mutex scan_lock;
>   	const struct nvme_ctrl_ops *ops;

Re: [RFC v3 1/1] nvme: add passthrough error logging opt-in

[alan.adamson]

On 4/3/23 3:17 AM, Sagi Grimberg wrote:
>
>
> On 4/1/23 01:18, Alan Adamson wrote:
>> Commit d7ac8dca938c ("nvme: quiet user passthrough command errors") 
>> disabled error
>> logging for user passthrough commands.  This commit adds the ability 
>> to opt-in
>> to passthrough error logging.
>>
>> To enable passthrough error logging:
>>          echo 1 > /sys/class/nvme/nvme0/passthrough_logging
>>
>> To disable passthrough error logging:
>>          echo 0 > /sys/class/nvme/nvme0/passthrough_logging
>>
>> By default, passthrough error logging will remain disabled.
>>
>> Signed-off-by: Alan Adamson <alan.adamson@oracle.com>
>> ---
>>   drivers/nvme/host/core.c | 93 ++++++++++++++++++++++++++++++++++++++--
>>   drivers/nvme/host/nvme.h |  1 +
>>   2 files changed, 90 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>> index 53ef028596c6..82d4f8235a8f 100644
>> --- a/drivers/nvme/host/core.c
>> +++ b/drivers/nvme/host/core.c
>> @@ -337,6 +337,49 @@ static void nvme_log_error(struct request *req)
>>                  nr->status & NVME_SC_DNR  ? "DNR "  : "");
>>   }
>>   +static void nvme_log_error_passthrough(struct request *req)
>> +{
>> +    struct nvme_ns *ns = req->q->queuedata;
>> +    struct nvme_request *nr = nvme_req(req);
>> +
>> +    if (ns) {
>> +        pr_err_ratelimited("%s: %s(0x%x), %s (sct 0x%x / sc 0x%x) %s%s"
>> +            "cdw10=0x%x cdw11=0x%x cdw12=0x%x cdw13=0x%x cdw14=0x%x 
>> cdw15=0x%x\n",
>> +            ns->disk ? ns->disk->disk_name : "?",
>> +            nvme_get_opcode_str(nr->cmd->common.opcode),
>> +            nr->cmd->common.opcode,
>> +            nvme_get_error_status_str(nr->status),
>> +            nr->status >> 8 & 7,    /* Status Code Type */
>> +            nr->status & 0xff,    /* Status Code */
>> +            nr->status & NVME_SC_MORE ? "MORE " : "",
>> +            nr->status & NVME_SC_DNR  ? "DNR "  : "",
>> +            nr->cmd->common.cdw10,
>> +            nr->cmd->common.cdw11,
>> +            nr->cmd->common.cdw12,
>> +            nr->cmd->common.cdw13,
>> +            nr->cmd->common.cdw14,
>> +            nr->cmd->common.cdw14);
>> +        return;
>> +    }
>> +
>> +    pr_err_ratelimited("%s: %s(0x%x), %s (sct 0x%x / sc 0x%x) %s%s"
>> +            "cdw10=0x%x cdw11=0x%x cdw12=0x%x cdw13=0x%x cdw14=0x%x 
>> cdw15=0x%x\n",
>> +            dev_name(nr->ctrl->device),
>> + nvme_get_admin_opcode_str(nr->cmd->common.opcode),
>> +            nr->cmd->common.opcode,
>> +            nvme_get_error_status_str(nr->status),
>> +            nr->status >> 8 & 7,    /* Status Code Type */
>> +            nr->status & 0xff,    /* Status Code */
>> +            nr->status & NVME_SC_MORE ? "MORE " : "",
>> +            nr->status & NVME_SC_DNR  ? "DNR "  : "",
>> +            nr->cmd->common.cdw10,
>> +            nr->cmd->common.cdw11,
>> +            nr->cmd->common.cdw12,
>> +            nr->cmd->common.cdw13,
>> +            nr->cmd->common.cdw14,
>> +            nr->cmd->common.cdw14);
>> +}
>> +
>>   enum nvme_disposition {
>>       COMPLETE,
>>       RETRY,
>> @@ -381,8 +424,12 @@ static inline void nvme_end_req(struct request 
>> *req)
>>   {
>>       blk_status_t status = nvme_error_status(nvme_req(req)->status);
>>   -    if (unlikely(nvme_req(req)->status && !(req->rq_flags & 
>> RQF_QUIET)))
>> -        nvme_log_error(req);
>> +    if (unlikely(nvme_req(req)->status && !(req->rq_flags & 
>> RQF_QUIET))) {
>> +        if (blk_rq_is_passthrough(req))
>> +            nvme_log_error_passthrough(req);
>> +        else
>> +            nvme_log_error(req);
>> +    }
>>       nvme_end_req_zoned(req);
>>       nvme_trace_bio_complete(req);
>>       if (req->cmd_flags & REQ_NVME_MPATH)
>> @@ -666,6 +713,8 @@ static inline void nvme_clear_nvme_request(struct 
>> request *req)
>>   /* initialize a passthrough request */
>>   void nvme_init_request(struct request *req, struct nvme_command *cmd)
>>   {
>> +    struct nvme_request *nr = nvme_req(req);
>> +
>>       if (req->q->queuedata)
>>           req->timeout = NVME_IO_TIMEOUT;
>>       else /* no queuedata implies admin queue */
>> @@ -678,8 +727,10 @@ void nvme_init_request(struct request *req, 
>> struct nvme_command *cmd)
>>       if (req->mq_hctx->type == HCTX_TYPE_POLL)
>>           req->cmd_flags |= REQ_POLLED;
>>       nvme_clear_nvme_request(req);
>> -    req->rq_flags |= RQF_QUIET;
>> -    memcpy(nvme_req(req)->cmd, cmd, sizeof(*cmd));
>> +    if (!nr->ctrl->error_logging)
>> +        req->rq_flags |= RQF_QUIET;
>> +
>> +    memcpy(nr->cmd, cmd, sizeof(*cmd));
>
> Question, if we already introduce granularity to this setting, why
> not per-ns? why only per controller? I'd think it makes more
> sense to do this per ns. Will also remove access to ctrl in the hot
> path...


Meaning we should have both:

/sys/class/nvme/nvme0/passthrough_logging

/sys/class/nvme/nvme0/nvme0n1/passthrough_logging


Alan

Re: [RFC v3 1/1] nvme: add passthrough error logging opt-in

[Sagi Grimberg]

>>> +    if (!nr->ctrl->error_logging)
>>> +        req->rq_flags |= RQF_QUIET;
>>> +
>>> +    memcpy(nr->cmd, cmd, sizeof(*cmd));
>>
>> Question, if we already introduce granularity to this setting, why
>> not per-ns? why only per controller? I'd think it makes more
>> sense to do this per ns. Will also remove access to ctrl in the hot
>> path...
> 
> 
> Meaning we should have both:
> 
> /sys/class/nvme/nvme0/passthrough_logging
> 
> /sys/class/nvme/nvme0/nvme0n1/passthrough_logging

No, just the namespace logging, why would we need the
controller as well?

Re: [RFC v3 1/1] nvme: add passthrough error logging opt-in

[alan.adamson]

On 4/3/23 3:38 PM, Sagi Grimberg wrote:
>
>>>> +    if (!nr->ctrl->error_logging)
>>>> +        req->rq_flags |= RQF_QUIET;
>>>> +
>>>> +    memcpy(nr->cmd, cmd, sizeof(*cmd));
>>>
>>> Question, if we already introduce granularity to this setting, why
>>> not per-ns? why only per controller? I'd think it makes more
>>> sense to do this per ns. Will also remove access to ctrl in the hot
>>> path...
>>
>>
>> Meaning we should have both:
>>
>> /sys/class/nvme/nvme0/passthrough_logging
>>
>> /sys/class/nvme/nvme0/nvme0n1/passthrough_logging
>
> No, just the namespace logging, why would we need the
> controller as well?

For Admin Commands?

Alan

Re: [RFC v3 1/1] nvme: add passthrough error logging opt-in

[Sagi Grimberg]

>>>>> +    if (!nr->ctrl->error_logging)
>>>>> +        req->rq_flags |= RQF_QUIET;
>>>>> +
>>>>> +    memcpy(nr->cmd, cmd, sizeof(*cmd));
>>>>
>>>> Question, if we already introduce granularity to this setting, why
>>>> not per-ns? why only per controller? I'd think it makes more
>>>> sense to do this per ns. Will also remove access to ctrl in the hot
>>>> path...
>>>
>>>
>>> Meaning we should have both:
>>>
>>> /sys/class/nvme/nvme0/passthrough_logging
>>>
>>> /sys/class/nvme/nvme0/nvme0n1/passthrough_logging
>>
>> No, just the namespace logging, why would we need the
>> controller as well?
> 
> For Admin Commands?

Yea.... you have those...

Re: [RFC v3 1/1] nvme: add passthrough error logging opt-in

[alan.adamson]

On 4/3/23 4:29 PM, Sagi Grimberg wrote:
>
>>>>>> +    if (!nr->ctrl->error_logging)
>>>>>> +        req->rq_flags |= RQF_QUIET;
>>>>>> +
>>>>>> +    memcpy(nr->cmd, cmd, sizeof(*cmd));
>>>>>
>>>>> Question, if we already introduce granularity to this setting, why
>>>>> not per-ns? why only per controller? I'd think it makes more
>>>>> sense to do this per ns. Will also remove access to ctrl in the hot
>>>>> path...
>>>>
>>>>
>>>> Meaning we should have both:
>>>>
>>>> /sys/class/nvme/nvme0/passthrough_logging
>>>>
>>>> /sys/class/nvme/nvme0/nvme0n1/passthrough_logging
>>>
>>> No, just the namespace logging, why would we need the
>>> controller as well?
>>
>> For Admin Commands?
>
> Yea.... you have those...

For namespace errors (read, write, etc), passthrough initiated or not, 
shouldn't they always be logged?


Alan

Re: [RFC v3 1/1] nvme: add passthrough error logging opt-in

[Christoph Hellwig]

On Tue, Apr 04, 2023 at 09:20:40AM -0700, alan.adamson@oracle.com wrote:
> For namespace errors (read, write, etc), passthrough initiated or not,
> shouldn't they always be logged?

We could start with that.  If we see complaints we can add the
per-namespace attribute.