* [RFC v3 0/1] nvme: add passthrough error logging opt-in
@ 2023-03-31 22:18 Alan Adamson
2023-03-31 22:18 ` [RFC v3 1/1] " Alan Adamson
0 siblings, 1 reply; 14+ messages in thread
From: Alan Adamson @ 2023-03-31 22:18 UTC (permalink / raw)
To: linux-nvme; +Cc: alan.adamson, kbusch, hch, sagi
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* [RFC v3 1/1] nvme: add passthrough error logging opt-in
2023-03-31 22:18 [RFC v3 0/1] nvme: add passthrough error logging opt-in Alan Adamson
@ 2023-03-31 22:18 ` Alan Adamson
[not found] ` <CGME20230403083443eucas1p225993f889610c6c070dcb24e232aa526@eucas1p2.samsung.com>
2023-04-03 10:17 ` Sagi Grimberg
0 siblings, 2 replies; 14+ messages in thread
From: Alan Adamson @ 2023-03-31 22:18 UTC (permalink / raw)
To: linux-nvme; +Cc: alan.adamson, kbusch, hch, sagi
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [RFC v3 1/1] nvme: add passthrough error logging opt-in
[not found] ` <CGME20230403083443eucas1p225993f889610c6c070dcb24e232aa526@eucas1p2.samsung.com>
@ 2023-04-03 8:26 ` Pankaj Raghav
2023-04-05 22:35 ` alan.adamson
[not found] ` <CGME20230403083443eucas1p225993f889610c6c070dcb24e232aa526@epcms2p1>
0 siblings, 2 replies; 14+ messages in thread
From: Pankaj Raghav @ 2023-04-03 8:26 UTC (permalink / raw)
To: Alan Adamson; +Cc: linux-nvme, kbusch, hch, sagi, p.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.
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC v3 1/1] nvme: add passthrough error logging opt-in
2023-03-31 22:18 ` [RFC v3 1/1] " Alan Adamson
[not found] ` <CGME20230403083443eucas1p225993f889610c6c070dcb24e232aa526@eucas1p2.samsung.com>
@ 2023-04-03 10:17 ` Sagi Grimberg
2023-04-03 22:20 ` alan.adamson
1 sibling, 1 reply; 14+ messages in thread
From: Sagi Grimberg @ 2023-04-03 10:17 UTC (permalink / raw)
To: Alan Adamson, linux-nvme; +Cc: kbusch, hch
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;
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC v3 1/1] nvme: add passthrough error logging opt-in
2023-04-03 10:17 ` Sagi Grimberg
@ 2023-04-03 22:20 ` alan.adamson
2023-04-03 22:38 ` Sagi Grimberg
0 siblings, 1 reply; 14+ messages in thread
From: alan.adamson @ 2023-04-03 22:20 UTC (permalink / raw)
To: Sagi Grimberg, linux-nvme; +Cc: kbusch, hch
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC v3 1/1] nvme: add passthrough error logging opt-in
2023-04-03 22:20 ` alan.adamson
@ 2023-04-03 22:38 ` Sagi Grimberg
2023-04-03 22:54 ` alan.adamson
0 siblings, 1 reply; 14+ messages in thread
From: Sagi Grimberg @ 2023-04-03 22:38 UTC (permalink / raw)
To: alan.adamson, linux-nvme; +Cc: kbusch, hch
>>> + 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?
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC v3 1/1] nvme: add passthrough error logging opt-in
2023-04-03 22:38 ` Sagi Grimberg
@ 2023-04-03 22:54 ` alan.adamson
2023-04-03 23:29 ` Sagi Grimberg
0 siblings, 1 reply; 14+ messages in thread
From: alan.adamson @ 2023-04-03 22:54 UTC (permalink / raw)
To: Sagi Grimberg, linux-nvme; +Cc: kbusch, hch
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC v3 1/1] nvme: add passthrough error logging opt-in
2023-04-03 22:54 ` alan.adamson
@ 2023-04-03 23:29 ` Sagi Grimberg
2023-04-04 16:20 ` alan.adamson
0 siblings, 1 reply; 14+ messages in thread
From: Sagi Grimberg @ 2023-04-03 23:29 UTC (permalink / raw)
To: alan.adamson, linux-nvme; +Cc: kbusch, hch
>>>>> + 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...
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC v3 1/1] nvme: add passthrough error logging opt-in
2023-04-03 23:29 ` Sagi Grimberg
@ 2023-04-04 16:20 ` alan.adamson
2023-04-05 15:07 ` Christoph Hellwig
0 siblings, 1 reply; 14+ messages in thread
From: alan.adamson @ 2023-04-04 16:20 UTC (permalink / raw)
To: Sagi Grimberg, linux-nvme; +Cc: kbusch, hch
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC v3 1/1] nvme: add passthrough error logging opt-in
2023-04-04 16:20 ` alan.adamson
@ 2023-04-05 15:07 ` Christoph Hellwig
0 siblings, 0 replies; 14+ messages in thread
From: Christoph Hellwig @ 2023-04-05 15:07 UTC (permalink / raw)
To: alan.adamson; +Cc: Sagi Grimberg, linux-nvme, kbusch, hch
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.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC v3 1/1] nvme: add passthrough error logging opt-in
2023-04-03 8:26 ` Pankaj Raghav
@ 2023-04-05 22:35 ` alan.adamson
[not found] ` <CGME20230403083443eucas1p225993f889610c6c070dcb24e232aa526@epcms2p1>
1 sibling, 0 replies; 14+ messages in thread
From: alan.adamson @ 2023-04-05 22:35 UTC (permalink / raw)
To: Pankaj Raghav; +Cc: linux-nvme, kbusch, hch, sagi
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.
>>
^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [RFC v3 1/1] nvme: add passthrough error logging opt-in
[not found] ` <CGME20230403083443eucas1p225993f889610c6c070dcb24e232aa526@epcms2p1>
@ 2023-04-06 4:13 ` Minwoo Im
2023-04-06 4:24 ` Chaitanya Kulkarni
[not found] ` <CGME20230403083443eucas1p225993f889610c6c070dcb24e232aa526@epcms2p5>
0 siblings, 2 replies; 14+ messages in thread
From: Minwoo Im @ 2023-04-06 4:13 UTC (permalink / raw)
To: alan.adamson, Pankaj Raghav Partha Sarathy; +Cc: linux-nvme, kbusch, hch, sagi
> -----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].
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [RFC v3 1/1] nvme: add passthrough error logging opt-in
2023-04-06 4:13 ` Minwoo Im
@ 2023-04-06 4:24 ` Chaitanya Kulkarni
[not found] ` <CGME20230403083443eucas1p225993f889610c6c070dcb24e232aa526@epcms2p5>
1 sibling, 0 replies; 14+ messages in thread
From: Chaitanya Kulkarni @ 2023-04-06 4:24 UTC (permalink / raw)
To: minwoo.im, alan.adamson, Pankaj Raghav Partha Sarathy
Cc: linux-nvme, kbusch, hch, sagi
>>> 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
^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [RFC v3 1/1] nvme: add passthrough error logging opt-in
[not found] ` <CGME20230403083443eucas1p225993f889610c6c070dcb24e232aa526@epcms2p5>
@ 2023-04-06 4:27 ` Minwoo Im
0 siblings, 0 replies; 14+ messages in thread
From: Minwoo Im @ 2023-04-06 4:27 UTC (permalink / raw)
To: Chaitanya Kulkarni, alan.adamson, Pankaj Raghav Partha Sarathy
Cc: linux-nvme, kbusch, hch, sagi
> -----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
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2023-04-06 4:27 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-31 22:18 [RFC v3 0/1] nvme: add passthrough error logging opt-in Alan Adamson
2023-03-31 22:18 ` [RFC v3 1/1] " Alan Adamson
[not found] ` <CGME20230403083443eucas1p225993f889610c6c070dcb24e232aa526@eucas1p2.samsung.com>
2023-04-03 8:26 ` Pankaj Raghav
2023-04-05 22:35 ` alan.adamson
[not found] ` <CGME20230403083443eucas1p225993f889610c6c070dcb24e232aa526@epcms2p1>
2023-04-06 4:13 ` Minwoo Im
2023-04-06 4:24 ` Chaitanya Kulkarni
[not found] ` <CGME20230403083443eucas1p225993f889610c6c070dcb24e232aa526@epcms2p5>
2023-04-06 4:27 ` Minwoo Im
2023-04-03 10:17 ` Sagi Grimberg
2023-04-03 22:20 ` alan.adamson
2023-04-03 22:38 ` Sagi Grimberg
2023-04-03 22:54 ` alan.adamson
2023-04-03 23:29 ` Sagi Grimberg
2023-04-04 16:20 ` alan.adamson
2023-04-05 15:07 ` Christoph Hellwig
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.