[PATCH V7 0/1] nvme: allow passthru cmd error logging

[PATCH V7 0/1] nvme: allow passthru cmd error logging

[Alan Adamson]

In nvme_end_req() we only log errors which are for non-passthru
commands. Add a helper function nvme_log_err_passthru() that allows us
to log error for passthru commands by decoding cdw10-cdw15 values of
nvme command.

Below is short testlog :-

* Admin Passsthru error log off, no errors are printed
* Admin Passsthru error log on, errors are printed
* IO Passsthru error log off, errors are printed
* IO Passsthru error log on, errors are printed

v7:
- Changed attribute/flag to passthru_err_log_enabled.
- Use kstrtobool rather than kstrtoint.

v6:
- Reabse, etest nvme-6.5 and add test log for admin and I/O
  passthru error log.

v5:
- Trim down code in the nvme_log_error_passthrough().
  Use following to get the disk name as an arg to
   pr_err_ratelimited() :-
	ns ? ns->disk->disk_name : dev_name(nr->ctrl->device),
  Use following to get the admin vs I/O opcode string as an arg to
  pr_err_ratelimited() :-
       	ns ? nvme_get_opcode_str(nr->cmd->common.opcode) :
       	     nvme_get_admin_opcode_str(nr->cmd->common.opcode),
- Rename nvme_log_error_passthrough() -> nvme_log_err_passthru().
- Remove else and return directly in nvme_passthru_err_log_show().
- Generate error on invalid values of the passthru_enable variable
  in nvme_passthru_log_store().
- Rename passthrough -> passthru.
- Rename sysfs attr from passthru_admin_err_logging -> passthru_log_err.

v4:
- Change sysfs attribute to passthru_admin_err_logging
- Only log passthrough admin commands.  IO passthrough commands will
  always be logged.

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.


Testing
-------
* Admin Passsthru error log off, no errors are printed :-

[root@localhost ~]# echo 0 > /sys/class/nvme/nvme0/passthru_err_log_enabled
[root@localhost ~]# nvme telemetry-log -o /tmp/test /dev/nvme0
NVMe status: INVALID_FIELD: A reserved coded value or an unsupported value in a defined field(0x4002)
Failed to acquire telemetry header 16386!
[root@localhost ~]# dmesg
[root@localhost ~]


* Admin Passsthru error log on, errors are printed :-

[root@localhost ~]# echo 1 > /sys/class/nvme/nvme0/passthru_err_log_enabled
[root@localhost ~]# nvme telemetry-log -o /tmp/test /dev/nvme0
NVMe status: INVALID_FIELD: A reserved coded value or an unsupported value in a defined field(0x4002)
Failed to acquire telemetry header 16386!
[root@localhost ~]# dmesg
[  892.573216] nvme0: Admin Cmd(0x2), I/O Error (sct 0x0 / sc 0x2) DNR cdw10=0x7f8107 cdw11=0x0 cdw12=0x0 cdw13=0x0 cdw14=0x0 cdw15=0x0


* IO Passsthru error log off, errors are printed :-

[root@localhost ~]# echo 0 > /sys/class/nvme/nvme0/passthru_err_log_enabled
[root@localhost ~]# nvme write-zeroes -n 1 -s 0x200000 -c 10 /dev/nvme0
NVMe status: LBA_RANGE: The command references a LBA that exceeds the size of the namespace(0x4080)
[root@localhost ~]# dmesg
[ 1037.307902] nvme nvme0: using deprecated NVME_IOCTL_IO_CMD ioctl on the char device!
[ 1037.307991] nvme0n1: I/O Cmd(0x8), I/O Error (sct 0x0 / sc 0x80) DNR cdw10=0x200000 cdw11=0x0 cdw12=0xa cdw13=0x0 cdw14=0x0 cdw15=0x0
[root@localhost ~]# 


* IO Passsthru error log on, errors are printed :-

[root@localhost ~]# echo 1 > /sys/class/nvme/nvme0/passthru_err_log_enabled
[root@localhost ~]# nvme write-zeroes -n 1 -s 0x200000 -c 10 /dev/nvme0
NVMe status: LBA_RANGE: The command references a LBA that exceeds the size of the namespace(0x4080)
[root@localhost ~]# dmesg
[ 1076.779806] nvme nvme0: using deprecated NVME_IOCTL_IO_CMD ioctl on the char device!
[ 1076.779900] nvme0n1: I/O Cmd(0x8), I/O Error (sct 0x0 / sc 0x80) DNR cdw10=0x200000 cdw11=0x0 cdw12=0xa cdw13=0x0 cdw14=0x0 cdw15=0x0


-- 
2.39.3

[PATCH V7 1/1] nvme: allow passthru cmd error logging

[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 admin error logging. IO
commands initiated as passthrough will always be logged.

The logging output for passthrough commands (Admin and IO) has been
changed to include CDWXX fields.

nvme0n1: Read(0x2), LBA Out of Range (sct 0x0 / sc 0x80) DNR cdw10=0x0 cdw11=0x1
        cdw12=0x70000 cdw13=0x0 cdw14=0x0 cdw15=0x0

Add a helper function nvme_log_err_passthru() which allows us to log
error for passthru commands by decoding cdw10-cdw15 values of nvme
command.

Add a new sysfs attr passthru_err_log_enabled that allows user to conditionally
enable passthru command logging, by default it is disabled.

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

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

Signed-off-by: Alan Adamson <alan.adamson@oracle.com>
[kch] fix sevaral nits and trim down code, details in cover-letter.
Signed-off-by: Chaitanya Kulkarni <kch@nvidia.com>
---
 drivers/nvme/host/core.c  | 43 ++++++++++++++++++++++++++++++++++-----
 drivers/nvme/host/nvme.h  |  1 +
 drivers/nvme/host/sysfs.c | 29 ++++++++++++++++++++++++++
 3 files changed, 68 insertions(+), 5 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index f3a01b79148c..1bf933c886fd 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -337,6 +337,30 @@ static void nvme_log_error(struct request *req)
 			   nr->status & NVME_SC_DNR  ? "DNR "  : "");
 }
 
+static void nvme_log_err_passthru(struct request *req)
+{
+	struct nvme_ns *ns = req->q->queuedata;
+	struct nvme_request *nr = nvme_req(req);
+
+	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 ? ns->disk->disk_name : dev_name(nr->ctrl->device),
+		ns ? nvme_get_opcode_str(nr->cmd->common.opcode) :
+		     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 +405,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_err_passthru(req);
+		else
+			nvme_log_error(req);
+	}
 	nvme_end_req_zoned(req);
 	nvme_trace_bio_complete(req);
 	if (req->cmd_flags & REQ_NVME_MPATH)
@@ -675,10 +703,15 @@ 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 */
+	else { /* no queuedata implies admin queue */
 		req->timeout = NVME_ADMIN_TIMEOUT;
+		if (!nr->ctrl->passthru_err_log_enabled)
+			req->rq_flags |= RQF_QUIET;
+	}
 
 	/* passthru commands should let the driver set the SGL flags */
 	cmd->common.flags &= ~NVME_CMD_SGL_ALL;
@@ -687,8 +720,7 @@ 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));
+	memcpy(nr->cmd, cmd, sizeof(*cmd));
 }
 EXPORT_SYMBOL_GPL(nvme_init_request);
 
@@ -4415,6 +4447,7 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
 	int ret;
 
 	ctrl->state = NVME_CTRL_NEW;
+	ctrl->passthru_err_log_enabled = 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 f35647c470af..7da6285fc30a 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -256,6 +256,7 @@ enum nvme_ctrl_flags {
 struct nvme_ctrl {
 	bool comp_seen;
 	bool identified;
+	bool passthru_err_log_enabled;
 	enum nvme_ctrl_state state;
 	spinlock_t lock;
 	struct mutex scan_lock;
diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
index 212e1b05d298..fbca06a4e4ce 100644
--- a/drivers/nvme/host/sysfs.c
+++ b/drivers/nvme/host/sysfs.c
@@ -35,6 +35,34 @@ static ssize_t nvme_sysfs_rescan(struct device *dev,
 }
 static DEVICE_ATTR(rescan_controller, S_IWUSR, NULL, nvme_sysfs_rescan);
 
+static ssize_t nvme_passthru_err_log_enabled_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
+
+	return sysfs_emit(buf, ctrl->passthru_err_log_enabled ? "on" : "off");
+}
+
+static ssize_t nvme_passthru_err_log_enabled_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
+	int err;
+	bool passthru_err_log_enabled;
+
+	err = kstrtobool(buf, &passthru_err_log_enabled);
+	if (err)
+		return -EINVAL;
+
+	ctrl->passthru_err_log_enabled = passthru_err_log_enabled;
+
+	return count;
+}
+
+static DEVICE_ATTR(passthru_err_log_enabled, S_IRUGO | S_IWUSR,
+	nvme_passthru_err_log_enabled_show,
+	nvme_passthru_err_log_enabled_store);
+
 static inline struct nvme_ns_head *dev_to_ns_head(struct device *dev)
 {
 	struct gendisk *disk = dev_to_disk(dev);
@@ -554,6 +582,7 @@ static struct attribute *nvme_dev_attrs[] = {
 	&dev_attr_dhchap_secret.attr,
 	&dev_attr_dhchap_ctrl_secret.attr,
 #endif
+	&dev_attr_passthru_err_log_enabled.attr,
 	NULL
 };
 
-- 
2.39.3

Re: [PATCH V7 1/1] nvme: allow passthru cmd error logging

[Christoph Hellwig]

>  	if (req->q->queuedata)
>  		req->timeout = NVME_IO_TIMEOUT;
> -	else /* no queuedata implies admin queue */
> +	else { /* no queuedata implies admin queue */
>  		req->timeout = NVME_ADMIN_TIMEOUT;
> +		if (!nr->ctrl->passthru_err_log_enabled)
> +			req->rq_flags |= RQF_QUIET;
> +	}
>  
>  	/* passthru commands should let the driver set the SGL flags */
>  	cmd->common.flags &= ~NVME_CMD_SGL_ALL;
> @@ -687,8 +720,7 @@ 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;

Isn't thisw now dropping the RQF_QUIET flag I/O commands that we
previously set?  While we're at it, why do we only allow passthrough
error logging for admin commands anyway?

Re: [PATCH V7 1/1] nvme: allow passthru cmd error logging

[alan.adamson]

On 9/4/23 11:53 PM, Christoph Hellwig wrote:
>>   	if (req->q->queuedata)
>>   		req->timeout = NVME_IO_TIMEOUT;
>> -	else /* no queuedata implies admin queue */
>> +	else { /* no queuedata implies admin queue */
>>   		req->timeout = NVME_ADMIN_TIMEOUT;
>> +		if (!nr->ctrl->passthru_err_log_enabled)
>> +			req->rq_flags |= RQF_QUIET;
>> +	}
>>   
>>   	/* passthru commands should let the driver set the SGL flags */
>>   	cmd->common.flags &= ~NVME_CMD_SGL_ALL;
>> @@ -687,8 +720,7 @@ 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;
> Isn't thisw now dropping the RQF_QUIET flag I/O commands that we
> previously set?  While we're at it, why do we only allow passthrough
> error logging for admin commands anyway?

While working through this, we decided that for IO commands (passthrough 
or not) error logging would always be enabled. For passthrough admin 
commands, error logging could be enabled or disabled.

Alan

Re: [PATCH V7 1/1] nvme: allow passthru cmd error logging

[Sagi Grimberg]

>>>       if (req->q->queuedata)
>>>           req->timeout = NVME_IO_TIMEOUT;
>>> -    else /* no queuedata implies admin queue */
>>> +    else { /* no queuedata implies admin queue */
>>>           req->timeout = NVME_ADMIN_TIMEOUT;
>>> +        if (!nr->ctrl->passthru_err_log_enabled)
>>> +            req->rq_flags |= RQF_QUIET;
>>> +    }
>>>       /* passthru commands should let the driver set the SGL flags */
>>>       cmd->common.flags &= ~NVME_CMD_SGL_ALL;
>>> @@ -687,8 +720,7 @@ 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;
>> Isn't thisw now dropping the RQF_QUIET flag I/O commands that we
>> previously set?  While we're at it, why do we only allow passthrough
>> error logging for admin commands anyway?
> 
> While working through this, we decided that for IO commands (passthrough 
> or not) error logging would always be enabled. For passthrough admin 
> commands, error logging could be enabled or disabled.

Why shouldn't it apply to both?

Re: [PATCH V7 1/1] nvme: allow passthru cmd error logging

[alan.adamson]

On 9/12/23 4:39 AM, Sagi Grimberg wrote:
>
>>>>       if (req->q->queuedata)
>>>>           req->timeout = NVME_IO_TIMEOUT;
>>>> -    else /* no queuedata implies admin queue */
>>>> +    else { /* no queuedata implies admin queue */
>>>>           req->timeout = NVME_ADMIN_TIMEOUT;
>>>> +        if (!nr->ctrl->passthru_err_log_enabled)
>>>> +            req->rq_flags |= RQF_QUIET;
>>>> +    }
>>>>       /* passthru commands should let the driver set the SGL flags */
>>>>       cmd->common.flags &= ~NVME_CMD_SGL_ALL;
>>>> @@ -687,8 +720,7 @@ 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;
>>> Isn't thisw now dropping the RQF_QUIET flag I/O commands that we
>>> previously set?  While we're at it, why do we only allow passthrough
>>> error logging for admin commands anyway?
>>
>> While working through this, we decided that for IO commands 
>> (passthrough or not) error logging would always be enabled. For 
>> passthrough admin commands, error logging could be enabled or disabled.
>
> Why shouldn't it apply to both?

Currently, we have /sys/class/nvme/nvme0/passthru_err_log_enabled that 
enables/disables logging for admin command only.  Are you saying it 
should also enable/disable for IO commands across all namespaces or 
should there also be:

/sys/class/nvme/nvme0/nvme0n1/passthru_err_log_enabled

/sys/class/nvme/nvme0/nvme0n2/passthru_err_log_enabled

.

.

Alan

Re: [PATCH V7 1/1] nvme: allow passthru cmd error logging

[Chaitanya Kulkarni]

Sagi/Christoph,

On 9/19/23 09:02, alan.adamson@oracle.com wrote:
>
> On 9/12/23 4:39 AM, Sagi Grimberg wrote:
>>
>>>>>       if (req->q->queuedata)
>>>>>           req->timeout = NVME_IO_TIMEOUT;
>>>>> -    else /* no queuedata implies admin queue */
>>>>> +    else { /* no queuedata implies admin queue */
>>>>>           req->timeout = NVME_ADMIN_TIMEOUT;
>>>>> +        if (!nr->ctrl->passthru_err_log_enabled)
>>>>> +            req->rq_flags |= RQF_QUIET;
>>>>> +    }
>>>>>       /* passthru commands should let the driver set the SGL flags */
>>>>>       cmd->common.flags &= ~NVME_CMD_SGL_ALL;
>>>>> @@ -687,8 +720,7 @@ 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;
>>>> Isn't thisw now dropping the RQF_QUIET flag I/O commands that we
>>>> previously set?  While we're at it, why do we only allow passthrough
>>>> error logging for admin commands anyway?
>>>
>>> While working through this, we decided that for IO commands 
>>> (passthrough or not) error logging would always be enabled. For 
>>> passthrough admin commands, error logging could be enabled or disabled.
>>
>> Why shouldn't it apply to both?
>
> Currently, we have /sys/class/nvme/nvme0/passthru_err_log_enabled that 
> enables/disables logging for admin command only.  Are you saying it 
> should also enable/disable for IO commands across all namespaces or 
> should there also be:
>
> /sys/class/nvme/nvme0/nvme0n1/passthru_err_log_enabled
>
> /sys/class/nvme/nvme0/nvme0n2/passthru_err_log_enabled
>
> .
>
> .
>
> Alan
>
>
>

Can we please have resolution on this patch so we can merge it ?

-ck

Re: [PATCH V7 1/1] nvme: allow passthru cmd error logging

[Sagi Grimberg]

> Sagi/Christoph,
> 
> On 9/19/23 09:02, alan.adamson@oracle.com wrote:
>>
>> On 9/12/23 4:39 AM, Sagi Grimberg wrote:
>>>
>>>>>>        if (req->q->queuedata)
>>>>>>            req->timeout = NVME_IO_TIMEOUT;
>>>>>> -    else /* no queuedata implies admin queue */
>>>>>> +    else { /* no queuedata implies admin queue */
>>>>>>            req->timeout = NVME_ADMIN_TIMEOUT;
>>>>>> +        if (!nr->ctrl->passthru_err_log_enabled)
>>>>>> +            req->rq_flags |= RQF_QUIET;
>>>>>> +    }
>>>>>>        /* passthru commands should let the driver set the SGL flags */
>>>>>>        cmd->common.flags &= ~NVME_CMD_SGL_ALL;
>>>>>> @@ -687,8 +720,7 @@ 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;
>>>>> Isn't thisw now dropping the RQF_QUIET flag I/O commands that we
>>>>> previously set?  While we're at it, why do we only allow passthrough
>>>>> error logging for admin commands anyway?
>>>>
>>>> While working through this, we decided that for IO commands
>>>> (passthrough or not) error logging would always be enabled. For
>>>> passthrough admin commands, error logging could be enabled or disabled.
>>>
>>> Why shouldn't it apply to both?
>>
>> Currently, we have /sys/class/nvme/nvme0/passthru_err_log_enabled that
>> enables/disables logging for admin command only.  Are you saying it
>> should also enable/disable for IO commands across all namespaces or
>> should there also be:
>>
>> /sys/class/nvme/nvme0/nvme0n1/passthru_err_log_enabled
>>
>> /sys/class/nvme/nvme0/nvme0n2/passthru_err_log_enabled
>>
>> .
>>
>> .
>>
>> Alan
>>
>>
>>
> 
> Can we please have resolution on this patch so we can merge it ?

I'm in favor of the latter, IO commands enabled per ns, and admin
commands in the controller level.

Re: [PATCH V7 1/1] nvme: allow passthru cmd error logging

[Chaitanya Kulkarni]

Christoph,

On 12/5/23 02:01, Sagi Grimberg wrote:
>
>> Sagi/Christoph,
>>
>> On 9/19/23 09:02, alan.adamson@oracle.com wrote:
>>>
>>> On 9/12/23 4:39 AM, Sagi Grimberg wrote:
>>>>
>>>>>>>        if (req->q->queuedata)
>>>>>>>            req->timeout = NVME_IO_TIMEOUT;
>>>>>>> -    else /* no queuedata implies admin queue */
>>>>>>> +    else { /* no queuedata implies admin queue */
>>>>>>>            req->timeout = NVME_ADMIN_TIMEOUT;
>>>>>>> +        if (!nr->ctrl->passthru_err_log_enabled)
>>>>>>> +            req->rq_flags |= RQF_QUIET;
>>>>>>> +    }
>>>>>>>        /* passthru commands should let the driver set the SGL 
>>>>>>> flags */
>>>>>>>        cmd->common.flags &= ~NVME_CMD_SGL_ALL;
>>>>>>> @@ -687,8 +720,7 @@ 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;
>>>>>> Isn't thisw now dropping the RQF_QUIET flag I/O commands that we
>>>>>> previously set?  While we're at it, why do we only allow passthrough
>>>>>> error logging for admin commands anyway?
>>>>>
>>>>> While working through this, we decided that for IO commands
>>>>> (passthrough or not) error logging would always be enabled. For
>>>>> passthrough admin commands, error logging could be enabled or 
>>>>> disabled.
>>>>
>>>> Why shouldn't it apply to both?
>>>
>>> Currently, we have /sys/class/nvme/nvme0/passthru_err_log_enabled that
>>> enables/disables logging for admin command only.  Are you saying it
>>> should also enable/disable for IO commands across all namespaces or
>>> should there also be:
>>>
>>> /sys/class/nvme/nvme0/nvme0n1/passthru_err_log_enabled
>>>
>>> /sys/class/nvme/nvme0/nvme0n2/passthru_err_log_enabled
>>>
>>> .
>>>
>>> .
>>>
>>> Alan
>>>
>>>
>>>
>>
>> Can we please have resolution on this patch so we can merge it ?
>
> I'm in favor of the latter, IO commands enabled per ns, and admin
> commands in the controller level.

are you okay with above approach ?

-ck