* Re: [PATCH] nvme: move passthrough logging attribute to head
2024-02-06 17:50 [PATCH] nvme: move passthrough logging attribute to head Keith Busch
@ 2024-02-07 4:02 ` Chaitanya Kulkarni
2024-02-07 5:54 ` Keith Busch
2024-02-07 8:43 ` Tasmiya Nalatwad
` (4 subsequent siblings)
5 siblings, 1 reply; 8+ messages in thread
From: Chaitanya Kulkarni @ 2024-02-07 4:02 UTC (permalink / raw)
To: Keith Busch
Cc: Keith Busch, sagi@grimberg.me, linux-nvme@lists.infradead.org,
hch@lst.de, Tasmiya Nalatwad, Alan Adamson
On 2/6/24 09:50, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
>
> The namespace does not have attributes, but the head does. Move the new
> attribute to that structure instead of dereferncing the wrong type.
>
> And while we're here, fix the reverse-tree coding style.
>
> Fixes: 9f079dda14339e ("nvme: allow passthru cmd error logging")
> Reported-by: Tasmiya Nalatwad <tasmiya@linux.vnet.ibm.com>
> Cc: Alan Adamson <alan.adamson@oracle.com>
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>
Looks good.
Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
BTW I've also posted comment correction patch with Alan's review in this
area, please have a look.
-ck
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] nvme: move passthrough logging attribute to head
2024-02-07 4:02 ` Chaitanya Kulkarni
@ 2024-02-07 5:54 ` Keith Busch
0 siblings, 0 replies; 8+ messages in thread
From: Keith Busch @ 2024-02-07 5:54 UTC (permalink / raw)
To: Chaitanya Kulkarni
Cc: Keith Busch, sagi@grimberg.me, linux-nvme@lists.infradead.org,
hch@lst.de, Tasmiya Nalatwad, Alan Adamson
On Wed, Feb 07, 2024 at 04:02:42AM +0000, Chaitanya Kulkarni wrote:
> On 2/6/24 09:50, Keith Busch wrote:
> > From: Keith Busch <kbusch@kernel.org>
> >
> > The namespace does not have attributes, but the head does. Move the new
> > attribute to that structure instead of dereferncing the wrong type.
> >
> > And while we're here, fix the reverse-tree coding style.
> >
> > Fixes: 9f079dda14339e ("nvme: allow passthru cmd error logging")
> > Reported-by: Tasmiya Nalatwad <tasmiya@linux.vnet.ibm.com>
> > Cc: Alan Adamson <alan.adamson@oracle.com>
> > Signed-off-by: Keith Busch <kbusch@kernel.org>
> > ---
> >
>
> Looks good.
>
> Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
>
> BTW I've also posted comment correction patch with Alan's review in this
> area, please have a look.
Right, I saw that but the comments would need to be updated again after
this fix. I'll take another look tomorrow and maybe just change it when
applying.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] nvme: move passthrough logging attribute to head
2024-02-06 17:50 [PATCH] nvme: move passthrough logging attribute to head Keith Busch
2024-02-07 4:02 ` Chaitanya Kulkarni
@ 2024-02-07 8:43 ` Tasmiya Nalatwad
2024-02-07 18:28 ` alan.adamson
` (3 subsequent siblings)
5 siblings, 0 replies; 8+ messages in thread
From: Tasmiya Nalatwad @ 2024-02-07 8:43 UTC (permalink / raw)
To: Keith Busch, linux-nvme, hch, sagi; +Cc: Keith Busch, Alan Adamson
Thanks for the Patch. I have tested the patch and the issue is fixed
with the patch.
Tested-by: Tasmiya Nalatwad <tasmiya@linux.vnet.ibm.com>
On 2/6/24 23:20, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
>
> The namespace does not have attributes, but the head does. Move the new
> attribute to that structure instead of dereferncing the wrong type.
>
> And while we're here, fix the reverse-tree coding style.
>
> Fixes: 9f079dda14339e ("nvme: allow passthru cmd error logging")
> Reported-by: Tasmiya Nalatwad <tasmiya@linux.vnet.ibm.com>
> Cc: Alan Adamson <alan.adamson@oracle.com>
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
> drivers/nvme/host/core.c | 3 +--
> drivers/nvme/host/nvme.h | 2 +-
> drivers/nvme/host/sysfs.c | 30 +++++++++++++++---------------
> 3 files changed, 17 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 75927e8579857..786bebcfa4f77 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -713,7 +713,7 @@ void nvme_init_request(struct request *req, struct nvme_command *cmd)
> if (req->q->queuedata) {
> struct nvme_ns *ns = req->q->disk->private_data;
>
> - logging_enabled = ns->passthru_err_log_enabled;
> + logging_enabled = ns->head->passthru_err_log_enabled;
> req->timeout = NVME_IO_TIMEOUT;
> } else { /* no queuedata implies admin queue */
> logging_enabled = nr->ctrl->passthru_err_log_enabled;
> @@ -3700,7 +3700,6 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, struct nvme_ns_info *info)
>
> ns->disk = disk;
> ns->queue = disk->queue;
> - ns->passthru_err_log_enabled = false;
>
> if (ctrl->opts && ctrl->opts->data_digest)
> blk_queue_flag_set(QUEUE_FLAG_STABLE_WRITES, ns->queue);
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index 3897334e3950d..7b87763e2f8a6 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -455,6 +455,7 @@ struct nvme_ns_head {
> struct list_head entry;
> struct kref ref;
> bool shared;
> + bool passthru_err_log_enabled;
> int instance;
> struct nvme_effects_log *effects;
> u64 nuse;
> @@ -523,7 +524,6 @@ struct nvme_ns {
> struct device cdev_device;
>
> struct nvme_fault_inject fault_inject;
> - bool passthru_err_log_enabled;
> };
>
> /* NVMe ns supports metadata actions by the controller (generate/strip) */
> diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
> index d099218e494a8..f2832f70e7e0a 100644
> --- a/drivers/nvme/host/sysfs.c
> +++ b/drivers/nvme/host/sysfs.c
> @@ -48,8 +48,8 @@ static ssize_t nvme_adm_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;
> + int err;
>
> err = kstrtobool(buf, &passthru_err_log_enabled);
> if (err)
> @@ -60,25 +60,34 @@ static ssize_t nvme_adm_passthru_err_log_enabled_store(struct device *dev,
> return count;
> }
>
> +static inline struct nvme_ns_head *dev_to_ns_head(struct device *dev)
> +{
> + struct gendisk *disk = dev_to_disk(dev);
> +
> + if (nvme_disk_is_ns_head(disk))
> + return disk->private_data;
> + return nvme_get_ns_from_dev(dev)->head;
> +}
> +
> static ssize_t nvme_io_passthru_err_log_enabled_show(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> - struct nvme_ns *n = dev_get_drvdata(dev);
> + struct nvme_ns_head *head = dev_to_ns_head(dev);
>
> - return sysfs_emit(buf, n->passthru_err_log_enabled ? "on\n" : "off\n");
> + return sysfs_emit(buf, head->passthru_err_log_enabled ? "on\n" : "off\n");
> }
>
> static ssize_t nvme_io_passthru_err_log_enabled_store(struct device *dev,
> struct device_attribute *attr, const char *buf, size_t count)
> {
> - struct nvme_ns *ns = dev_get_drvdata(dev);
> - int err;
> + struct nvme_ns_head *head = dev_to_ns_head(dev);
> bool passthru_err_log_enabled;
> + int err;
>
> err = kstrtobool(buf, &passthru_err_log_enabled);
> if (err)
> return -EINVAL;
> - ns->passthru_err_log_enabled = passthru_err_log_enabled;
> + head->passthru_err_log_enabled = passthru_err_log_enabled;
>
> return count;
> }
> @@ -91,15 +100,6 @@ static struct device_attribute dev_attr_io_passthru_err_log_enabled = \
> __ATTR(passthru_err_log_enabled, S_IRUGO | S_IWUSR, \
> nvme_io_passthru_err_log_enabled_show, nvme_io_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);
> -
> - if (nvme_disk_is_ns_head(disk))
> - return disk->private_data;
> - return nvme_get_ns_from_dev(dev)->head;
> -}
> -
> static ssize_t wwid_show(struct device *dev, struct device_attribute *attr,
> char *buf)
> {
--
Regards,
Tasmiya Nalatwad
IBM Linux Technology Center
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] nvme: move passthrough logging attribute to head
2024-02-06 17:50 [PATCH] nvme: move passthrough logging attribute to head Keith Busch
2024-02-07 4:02 ` Chaitanya Kulkarni
2024-02-07 8:43 ` Tasmiya Nalatwad
@ 2024-02-07 18:28 ` alan.adamson
2024-02-07 18:40 ` Keith Busch
` (2 subsequent siblings)
5 siblings, 0 replies; 8+ messages in thread
From: alan.adamson @ 2024-02-07 18:28 UTC (permalink / raw)
To: Keith Busch, linux-nvme, hch, sagi; +Cc: Keith Busch, Tasmiya Nalatwad
On 2/6/24 9:50 AM, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
>
> The namespace does not have attributes, but the head does. Move the new
> attribute to that structure instead of dereferncing the wrong type.
>
> And while we're here, fix the reverse-tree coding style.
>
> Fixes: 9f079dda14339e ("nvme: allow passthru cmd error logging")
> Reported-by: Tasmiya Nalatwad <tasmiya@linux.vnet.ibm.com>
> Cc: Alan Adamson <alan.adamson@oracle.com>
> Signed-off-by: Keith Busch <kbusch@kernel.org>
Looks good.
Reviewed-by: Alan Adamson <alan.adamson@oracle.com>
I tested it with blktest tests I'm putting together for passthrough errors.
Alan
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] nvme: move passthrough logging attribute to head
2024-02-06 17:50 [PATCH] nvme: move passthrough logging attribute to head Keith Busch
` (2 preceding siblings ...)
2024-02-07 18:28 ` alan.adamson
@ 2024-02-07 18:40 ` Keith Busch
2024-02-08 8:00 ` Tasmiya Nalatwad
2024-02-12 6:57 ` Christoph Hellwig
5 siblings, 0 replies; 8+ messages in thread
From: Keith Busch @ 2024-02-07 18:40 UTC (permalink / raw)
To: Keith Busch; +Cc: linux-nvme, hch, sagi, Tasmiya Nalatwad, Alan Adamson
Applied to nvme-6.8.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] nvme: move passthrough logging attribute to head
2024-02-06 17:50 [PATCH] nvme: move passthrough logging attribute to head Keith Busch
` (3 preceding siblings ...)
2024-02-07 18:40 ` Keith Busch
@ 2024-02-08 8:00 ` Tasmiya Nalatwad
2024-02-12 6:57 ` Christoph Hellwig
5 siblings, 0 replies; 8+ messages in thread
From: Tasmiya Nalatwad @ 2024-02-08 8:00 UTC (permalink / raw)
To: Keith Busch, linux-nvme, hch, sagi; +Cc: Keith Busch, Alan Adamson
Thanks for the Patch. I tested the patch and the reported issue is fixed
with this patch. Thank you
Tested-by: Tasmiya Nalatwad <tasmiya@linux.vnet.ibm.com>
On 2/6/24 23:20, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
>
> The namespace does not have attributes, but the head does. Move the new
> attribute to that structure instead of dereferncing the wrong type.
>
> And while we're here, fix the reverse-tree coding style.
>
> Fixes: 9f079dda14339e ("nvme: allow passthru cmd error logging")
> Reported-by: Tasmiya Nalatwad <tasmiya@linux.vnet.ibm.com>
> Cc: Alan Adamson <alan.adamson@oracle.com>
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
> drivers/nvme/host/core.c | 3 +--
> drivers/nvme/host/nvme.h | 2 +-
> drivers/nvme/host/sysfs.c | 30 +++++++++++++++---------------
> 3 files changed, 17 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 75927e8579857..786bebcfa4f77 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -713,7 +713,7 @@ void nvme_init_request(struct request *req, struct nvme_command *cmd)
> if (req->q->queuedata) {
> struct nvme_ns *ns = req->q->disk->private_data;
>
> - logging_enabled = ns->passthru_err_log_enabled;
> + logging_enabled = ns->head->passthru_err_log_enabled;
> req->timeout = NVME_IO_TIMEOUT;
> } else { /* no queuedata implies admin queue */
> logging_enabled = nr->ctrl->passthru_err_log_enabled;
> @@ -3700,7 +3700,6 @@ static void nvme_alloc_ns(struct nvme_ctrl *ctrl, struct nvme_ns_info *info)
>
> ns->disk = disk;
> ns->queue = disk->queue;
> - ns->passthru_err_log_enabled = false;
>
> if (ctrl->opts && ctrl->opts->data_digest)
> blk_queue_flag_set(QUEUE_FLAG_STABLE_WRITES, ns->queue);
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index 3897334e3950d..7b87763e2f8a6 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -455,6 +455,7 @@ struct nvme_ns_head {
> struct list_head entry;
> struct kref ref;
> bool shared;
> + bool passthru_err_log_enabled;
> int instance;
> struct nvme_effects_log *effects;
> u64 nuse;
> @@ -523,7 +524,6 @@ struct nvme_ns {
> struct device cdev_device;
>
> struct nvme_fault_inject fault_inject;
> - bool passthru_err_log_enabled;
> };
>
> /* NVMe ns supports metadata actions by the controller (generate/strip) */
> diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
> index d099218e494a8..f2832f70e7e0a 100644
> --- a/drivers/nvme/host/sysfs.c
> +++ b/drivers/nvme/host/sysfs.c
> @@ -48,8 +48,8 @@ static ssize_t nvme_adm_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;
> + int err;
>
> err = kstrtobool(buf, &passthru_err_log_enabled);
> if (err)
> @@ -60,25 +60,34 @@ static ssize_t nvme_adm_passthru_err_log_enabled_store(struct device *dev,
> return count;
> }
>
> +static inline struct nvme_ns_head *dev_to_ns_head(struct device *dev)
> +{
> + struct gendisk *disk = dev_to_disk(dev);
> +
> + if (nvme_disk_is_ns_head(disk))
> + return disk->private_data;
> + return nvme_get_ns_from_dev(dev)->head;
> +}
> +
> static ssize_t nvme_io_passthru_err_log_enabled_show(struct device *dev,
> struct device_attribute *attr, char *buf)
> {
> - struct nvme_ns *n = dev_get_drvdata(dev);
> + struct nvme_ns_head *head = dev_to_ns_head(dev);
>
> - return sysfs_emit(buf, n->passthru_err_log_enabled ? "on\n" : "off\n");
> + return sysfs_emit(buf, head->passthru_err_log_enabled ? "on\n" : "off\n");
> }
>
> static ssize_t nvme_io_passthru_err_log_enabled_store(struct device *dev,
> struct device_attribute *attr, const char *buf, size_t count)
> {
> - struct nvme_ns *ns = dev_get_drvdata(dev);
> - int err;
> + struct nvme_ns_head *head = dev_to_ns_head(dev);
> bool passthru_err_log_enabled;
> + int err;
>
> err = kstrtobool(buf, &passthru_err_log_enabled);
> if (err)
> return -EINVAL;
> - ns->passthru_err_log_enabled = passthru_err_log_enabled;
> + head->passthru_err_log_enabled = passthru_err_log_enabled;
>
> return count;
> }
> @@ -91,15 +100,6 @@ static struct device_attribute dev_attr_io_passthru_err_log_enabled = \
> __ATTR(passthru_err_log_enabled, S_IRUGO | S_IWUSR, \
> nvme_io_passthru_err_log_enabled_show, nvme_io_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);
> -
> - if (nvme_disk_is_ns_head(disk))
> - return disk->private_data;
> - return nvme_get_ns_from_dev(dev)->head;
> -}
> -
> static ssize_t wwid_show(struct device *dev, struct device_attribute *attr,
> char *buf)
> {
--
Regards,
Tasmiya Nalatwad
IBM Linux Technology Center
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] nvme: move passthrough logging attribute to head
2024-02-06 17:50 [PATCH] nvme: move passthrough logging attribute to head Keith Busch
` (4 preceding siblings ...)
2024-02-08 8:00 ` Tasmiya Nalatwad
@ 2024-02-12 6:57 ` Christoph Hellwig
5 siblings, 0 replies; 8+ messages in thread
From: Christoph Hellwig @ 2024-02-12 6:57 UTC (permalink / raw)
To: Keith Busch
Cc: linux-nvme, hch, sagi, Keith Busch, Tasmiya Nalatwad,
Alan Adamson
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 8+ messages in thread