[PATCH V2 0/2] nvme: Support user mode processing of ZNS Zone Changed AEN

[PATCH V2 0/2] nvme: Support user mode processing of ZNS Zone Changed AEN

[clay.mayers]

From: Clay Mayers <clay.mayers@kioxia.com>

This adds support for the ZNS Zone-Descriptor-Changes AEN, which is of
type notify and uses CQE.DW1 to convey the NSID of the log page to read.

Patch 1 adds non-zero NVME_AEN_DATA with the value of CQE.DW1 to all
NVME_AEN uevents.

Patch 2 generates a uevent for all unhandled AEN notify results instead
of issuing a dev warning.

This support is planned to be used by both zone based applications and
another unreleased device with an alternate command set.

Changes since v1
- Break up into two patches
- Only inlucde AEN_DATA if CQE.DW1 is non-zero

Clay Mayers (2):
  nvme: Include AEN CQE.DW1 in NVME_AEN uevents
  nvme: All AENs of type notify generate an NVME_AEN uevent

 drivers/nvme/host/core.c | 21 +++++++++++++--------
 drivers/nvme/host/nvme.h |  1 +
 2 files changed, 14 insertions(+), 8 deletions(-)

-- 
2.27.0

[PATCH V2 1/2] nvme: Include AEN CQE.DW1 in NVME_AEN uevents

[clay.mayers]

From: Clay Mayers <clay.mayers@kioxia.com>

There are AENs from alternate command sets that include
extra data in their AEN's CQE.DW1.  For example, the ZNS
Zone-Descriptor-Changed AEN uses it to indicate the NSID
of the event's log page.

NVME_AEN uevent now includes the value of CQE.DW1 as a new
property.  It is only included when non-zero to keep previously
existing uevents unmodified.

Signed-off-by: Clay Mayers <clay.mayers@kioxia.com>
---
 drivers/nvme/host/core.c | 17 ++++++++++++-----
 drivers/nvme/host/nvme.h |  1 +
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 66446f1e06cf..4fcc7aec5a8c 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -4615,17 +4615,22 @@ static void nvme_change_uevent(struct nvme_ctrl *ctrl, char *envdata)
 
 static void nvme_aen_uevent(struct nvme_ctrl *ctrl)
 {
-	char *envp[2] = { NULL, NULL };
+	char *envp[3] = { NULL, NULL, NULL };
 	u32 aen_result = ctrl->aen_result;
+	u32 aen_data = ctrl->aen_data;
 
 	ctrl->aen_result = 0;
+	ctrl->aen_data = 0;
 	if (!aen_result)
 		return;
 
 	envp[0] = kasprintf(GFP_KERNEL, "NVME_AEN=%#08x", aen_result);
-	if (!envp[0])
-		return;
-	kobject_uevent_env(&ctrl->device->kobj, KOBJ_CHANGE, envp);
+	if (aen_data)
+		envp[1] = kasprintf(GFP_KERNEL, "NVME_AEN_DATA=%#08x", aen_data);
+	if (envp[0] && (envp[1] || !aen_data))
+		kobject_uevent_env(&ctrl->device->kobj, KOBJ_CHANGE, envp);
+
+	kfree(envp[1]);
 	kfree(envp[0]);
 }
 
@@ -4799,8 +4804,10 @@ void nvme_complete_async_event(struct nvme_ctrl *ctrl, __le16 status,
 		break;
 	}
 
-	if (requeue)
+	if (requeue) {
+		ctrl->aen_data = le64_to_cpu(res->u64) >> 32;
 		queue_work(nvme_wq, &ctrl->async_event_work);
+	}
 }
 EXPORT_SYMBOL_GPL(nvme_complete_async_event);
 
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 1bdf714dcd9e..a1bab447dc65 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -299,6 +299,7 @@ struct nvme_ctrl {
 	u16 cctemp;
 	u32 oaes;
 	u32 aen_result;
+	u32 aen_data;
 	u32 ctratt;
 	unsigned int shutdown_timeout;
 	unsigned int kato;
-- 
2.27.0

Re: [PATCH V2 1/2] nvme: Include AEN CQE.DW1 in NVME_AEN uevents

[Christoph Hellwig]

On Thu, Sep 29, 2022 at 03:39:54PM -0700, clay.mayers@kioxia.com wrote:
> From: Clay Mayers <clay.mayers@kioxia.com>
> 
> There are AENs from alternate command sets that include
> extra data in their AEN's CQE.DW1.  For example, the ZNS
> Zone-Descriptor-Changed AEN uses it to indicate the NSID
> of the event's log page.
> 
> NVME_AEN uevent now includes the value of CQE.DW1 as a new
> property.  It is only included when non-zero to keep previously
> existing uevents unmodified.

I don't think we can actually do this.  If we ever want to handle
Zone Excursions or any other event where the device can chane the
Zone Descriptor we need to handle this AEN in the kernel and thus
control the clearing of the even by reading the associated log page,
so we can't just send it on to userspace.  This is a bit of a sad
state of affairs but unavoidable due to the NVMe AEN design.

RE: [PATCH V2 1/2] nvme: Include AEN CQE.DW1 in NVME_AEN uevents

[Clay Mayers]

> From: Christoph Hellwig <hch@lst.de>
> Sent: Monday, October 17, 2022 6:24 AM
> 
> On Thu, Sep 29, 2022 at 03:39:54PM -0700, clay.mayers@kioxia.com wrote:
> > From: Clay Mayers <clay.mayers@kioxia.com>
> >
> > There are AENs from alternate command sets that include
> > extra data in their AEN's CQE.DW1.  For example, the ZNS
> > Zone-Descriptor-Changed AEN uses it to indicate the NSID
> > of the event's log page.
> >
> > NVME_AEN uevent now includes the value of CQE.DW1 as a new
> > property.  It is only included when non-zero to keep previously
> > existing uevents unmodified.
> 
> I don't think we can actually do this.  If we ever want to handle
> Zone Excursions or any other event where the device can chane the
> Zone Descriptor we need to handle this AEN in the kernel and thus
> control the clearing of the even by reading the associated log page,
> so we can't just send it on to userspace.  This is a bit of a sad
> state of affairs but unavoidable due to the NVMe AEN design.

What happens today is a warning is logged and the log page is left
unread.  The patch closes that gap allowing ZDC AENs to be enable
and handled in user space for things like RocksDB's ZenFS.  Kernel
clients will also need a way to handle them, but can't that be a
different patch series?

The limitation is, the ZDC AEN is enabled/disabled at the controller
level but it's essentially a namespace level event.  Once on for one
namespace, zone descriptor changes have to be handled as an AEN
for all namespaces.

Re: [PATCH V2 1/2] nvme: Include AEN CQE.DW1 in NVME_AEN uevents

[Christoph Hellwig]

On Tue, Oct 18, 2022 at 08:12:59PM +0000, Clay Mayers wrote:
> What happens today is a warning is logged and the log page is left
> unread.  The patch closes that gap allowing ZDC AENs to be enable
> and handled in user space for things like RocksDB's ZenFS.  Kernel
> clients will also need a way to handle them, but can't that be a
> different patch series?

The problem is how the NVMe AENs work - the are raised and then the
AEN command completion is delivered and they are cleared by reading
the log page.  But if we deliver them to userspace, we lose the pending
state of the AEN for the kernel.  Right now that is not an issue,
but we lose all chance of ever making use of that information in
the kernel.  So maybe the answer is to handle the AEN in the kernel,
read the changed zones log in the kernel, and then send an uevent
for all changes zones to userspace.

Re: [PATCH V2 1/2] nvme: Include AEN CQE.DW1 in NVME_AEN uevents

[Sagi Grimberg]

>> What happens today is a warning is logged and the log page is left
>> unread.  The patch closes that gap allowing ZDC AENs to be enable
>> and handled in user space for things like RocksDB's ZenFS.  Kernel
>> clients will also need a way to handle them, but can't that be a
>> different patch series?
> 
> The problem is how the NVMe AENs work - the are raised and then the
> AEN command completion is delivered and they are cleared by reading
> the log page.  But if we deliver them to userspace, we lose the pending
> state of the AEN for the kernel.  Right now that is not an issue,
> but we lose all chance of ever making use of that information in
> the kernel.  So maybe the answer is to handle the AEN in the kernel,
> read the changed zones log in the kernel, and then send an uevent
> for all changes zones to userspace.

That is what I was thinking... I feel we had this discussion before
on a different AEN (maybe it was something that wanted to get ANA
info to userspace)...

RE: [PATCH V2 1/2] nvme: Include AEN CQE.DW1 in NVME_AEN uevents

[Clay Mayers]

> From: Sagi Grimberg <sagi@grimberg.me>
> Sent: Tuesday, October 25, 2022 9:00 AM
> >> What happens today is a warning is logged and the log page is left
> >> unread.  The patch closes that gap allowing ZDC AENs to be enable
> >> and handled in user space for things like RocksDB's ZenFS.  Kernel
> >> clients will also need a way to handle them, but can't that be a
> >> different patch series?
> >
> > The problem is how the NVMe AENs work - the are raised and then the
> > AEN command completion is delivered and they are cleared by reading
> > the log page.  But if we deliver them to userspace, we lose the pending
> > state of the AEN for the kernel.  Right now that is not an issue,
> > but we lose all chance of ever making use of that information in
> > the kernel.  So maybe the answer is to handle the AEN in the kernel,
> > read the changed zones log in the kernel, and then send an uevent
> > for all changes zones to userspace.
> 
> That is what I was thinking... I feel we had this discussion before
> on a different AEN (maybe it was something that wanted to get ANA
> info to userspace)...

If you're thinking of this: https://lore.kernel.org/linux-nvme/BL0PR13MB4291371B4FD44B93D187B2319C419@BL0PR13MB4291.namprd13.prod.outlook.com/
I share the concern of both user and kernel reading the AEN referenced
log page.   The solution should look more like what Christoph outlined
above with the kernel reading the log page and routing results.

However, the ZDC AEN is not at the controller level.  It supplies the
NSID so only the code writing to the namespace can be notified and
only it will read the log page.  Host-aware zoned devices can't be
partitioned so responsibility wouldn't be split across user mode,
kernel or multiple applications.

Yes, the kernel doesn't get to look at my namespace's ZDC entries and
It has no reason to.  In the future when the kernel is processing ZDC
entries for a namespace, I shouldn’t expect a uevent or to see its ZDC
entries just like two apps using different namespaces.

[PATCH V2 2/2] nvme: All AENs of type notify generate an NVME_AEN uevent

[clay.mayers]

From: Clay Mayers <clay.mayers@kioxia.com>

This enables user mode processing of all unhandled AENs of type NOTIFY.

Signed-off-by: Clay Mayers <clay.mayers@kioxia.com>
---
 drivers/nvme/host/core.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 4fcc7aec5a8c..a3d8e6476af3 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -4753,11 +4753,9 @@ static bool nvme_handle_aen_notice(struct nvme_ctrl *ctrl, u32 result)
 		queue_work(nvme_wq, &ctrl->ana_work);
 		break;
 #endif
-	case NVME_AER_NOTICE_DISC_CHANGED:
+	default:
 		ctrl->aen_result = result;
 		break;
-	default:
-		dev_warn(ctrl->device, "async event result %08x\n", result);
 	}
 	return requeue;
 }
-- 
2.27.0

Re: [PATCH V2 0/2] nvme: Support user mode processing of ZNS Zone Changed AEN

[Chaitanya Kulkarni]

On 9/29/22 15:39, clay.mayers@kioxia.com wrote:
> From: Clay Mayers <clay.mayers@kioxia.com>
> 
> This adds support for the ZNS Zone-Descriptor-Changes AEN, which is of
> type notify and uses CQE.DW1 to convey the NSID of the log page to read.
> 
> Patch 1 adds non-zero NVME_AEN_DATA with the value of CQE.DW1 to all
> NVME_AEN uevents.
> 
> Patch 2 generates a uevent for all unhandled AEN notify results instead
> of issuing a dev warning.
> 
> This support is planned to be used by both zone based applications and
> another unreleased device with an alternate command set.
> 
>

Please write a testcase in the blktest to this will get tested every
release.

-ck

RE: [PATCH V2 0/2] nvme: Support user mode processing of ZNS Zone Changed AEN

[Clay Mayers]

> From: Chaitanya Kulkarni <chaitanyak@nvidia.com>
> Sent: Monday, October 3, 2022 6:49 PM
> Subject: Re: [PATCH V2 0/2] nvme: Support user mode processing of ZNS Zone
> Changed AEN
> 
> On 9/29/22 15:39, clay.mayers@kioxia.com wrote:
> > From: Clay Mayers <clay.mayers@kioxia.com>
> >
> > This adds support for the ZNS Zone-Descriptor-Changes AEN, which is of
> > type notify and uses CQE.DW1 to convey the NSID of the log page to read.
> >
> > Patch 1 adds non-zero NVME_AEN_DATA with the value of CQE.DW1 to all
> > NVME_AEN uevents.
> >
> > Patch 2 generates a uevent for all unhandled AEN notify results instead
> > of issuing a dev warning.
> >
> > This support is planned to be used by both zone based applications and
> > another unreleased device with an alternate command set.
> >
> >
> 
> Please write a testcase in the blktest to this will get tested every
> release.
> 
> -ck

Ok - I'll give it a go to implement that but AENs by definition aren't a
response to a request (mostly) - they just happen.  The case that I know
of that creates a zone excursion AEN takes many hours to manifest and
that duration would be FLASH-type specific.  I don't know how you could
use a real device.

This patch was tested under QEMU with our custom emulator.  The
current QEMU NVMe/ZNS device doesn't emulate zone excursions so I'll
have to start there unless someone knows of a better way.

Re: [PATCH V2 0/2] nvme: Support user mode processing of ZNS Zone Changed AEN

[Chaitanya Kulkarni]

>>> This support is planned to be used by both zone based applications and
>>> another unreleased device with an alternate command set.
>>>
>>>
>>
>> Please write a testcase in the blktest to this will get tested every
>> release.
>>
>> -ck
> 
> Ok - I'll give it a go to implement that but AENs by definition aren't a
> response to a request (mostly) - they just happen.  The case that I know

and this is exactly the main reason we need a testcase since it is
sporadic and side effect of the controller's behavior that cannot be 
predicted (untill it hits the condition) by the requests.
Lack of the testcase will leave this code untested unlike
other code paths which are getting exercised predictably.

> of that creates a zone excursion AEN takes many hours to manifest and
> that duration would be FLASH-type specific.  I don't know how you could
> use a real device.
> 

For this using real device is not an option since everyone should be
able to test that with ease in blktests.

> This patch was tested under QEMU with our custom emulator.  The
> current QEMU NVMe/ZNS device doesn't emulate zone excursions so I'll
> have to start there unless someone knows of a better way.
> 

Please add the support our QEMU NVMe developers are very helpful to get 
new features in.

-ck

Re: [PATCH V2 0/2] nvme: Support user mode processing of ZNS Zone Changed AEN

[Sagi Grimberg]

> This adds support for the ZNS Zone-Descriptor-Changes AEN, which is of
> type notify and uses CQE.DW1 to convey the NSID of the log page to read.
> 
> Patch 1 adds non-zero NVME_AEN_DATA with the value of CQE.DW1 to all
> NVME_AEN uevents.
> 
> Patch 2 generates a uevent for all unhandled AEN notify results instead
> of issuing a dev warning.
> 
> This support is planned to be used by both zone based applications and
> another unreleased device with an alternate command set.
> 
> Changes since v1
> - Break up into two patches
> - Only inlucde AEN_DATA if CQE.DW1 is non-zero

What about the other comments that were given on v1?

--
 >   #endif
 > -    case NVME_AER_NOTICE_DISC_CHANGED:
 > +    default:
 >           ctrl->aen_result = result;
 >           break;
 > -    default:
 > -        dev_warn(ctrl->device, "async event result %08x\n", result);

I'd keep the log... and also don't know if we want to pass any unknown
possible spam to userspace... not sure that being blindly forward
compatible is the right choice here.

 >       }
 >       return requeue;
 >   }
 > @@ -4799,8 +4800,10 @@ void nvme_complete_async_event(struct 
nvme_ctrl *ctrl, __le16 status,
 >           break;
 >       }
 >   -    if (requeue)
 > +    if (requeue) {
 > +        ctrl->aen_data = le64_to_cpu(res->u64) >> 32;

Please use a helper for that.
aen_data is not a great name... maybe use a union for that for the
specific aen subtype?

Maybe it'd be better that zoned namespace aen is handled explicitly
and passes a AEN_NSID env veriable to the uevent.

RE: [PATCH V2 0/2] nvme: Support user mode processing of ZNS Zone Changed AEN

[Clay Mayers]

> From: Sagi Grimberg <sagi@grimberg.me>
> Sent: Thursday, October 6, 2022 4:34 AM
> To: Clay Mayers <Clay.Mayers@kioxia.com>; linux-nvme@lists.infradead.org
> Cc: Keith Busch <kbusch@kernel.org>; Jens Axboe <axboe@fb.com>; Christoph
> Hellwig <hch@lst.de>
> Subject: Re: [PATCH V2 0/2] nvme: Support user mode processing of ZNS Zone
> Changed AEN
> 
> 
> > This adds support for the ZNS Zone-Descriptor-Changes AEN, which is of
> > type notify and uses CQE.DW1 to convey the NSID of the log page to read.
> >
> > Patch 1 adds non-zero NVME_AEN_DATA with the value of CQE.DW1 to all
> > NVME_AEN uevents.
> >
> > Patch 2 generates a uevent for all unhandled AEN notify results instead
> > of issuing a dev warning.
> >
> > This support is planned to be used by both zone based applications and
> > another unreleased device with an alternate command set.
> >
> > Changes since v1
> > - Break up into two patches
> > - Only inlucde AEN_DATA if CQE.DW1 is non-zero
> 
> What about the other comments that were given on v1?

My mistake

> 
> --
>  >   #endif
>  > -    case NVME_AER_NOTICE_DISC_CHANGED:
>  > +    default:
>  >           ctrl->aen_result = result;
>  >           break;
>  > -    default:
>  > -        dev_warn(ctrl->device, "async event result %08x\n", result);
> 
> I'd keep the log... and also don't know if we want to pass any unknown
> possible spam to userspace... not sure that being blindly forward
> compatible is the right choice here.

I'll put it back in.  I went back and forth on this.  My reasoning for
removing is that the other types of AEN don't warn and always generate
a uevent.  Only the NOTICE type conditionally sets ctrl->aen_result
before queueing async_event_work.

The real control over AENs is the AEN config feature.  When an AEN is
generated and unhandled, all the AENs of that type will no longer be
sent.  They can't be on by default without breaking the system so
there won't be a flood of meaningless NVME_AEN uevents.

The ones to suppress are those that don't go through default and
are handled in the kernel.  That's unchanged.

> 
>  >       }
>  >       return requeue;
>  >   }
>  > @@ -4799,8 +4800,10 @@ void nvme_complete_async_event(struct
> nvme_ctrl *ctrl, __le16 status,
>  >           break;
>  >       }
>  >   -    if (requeue)
>  > +    if (requeue) {
>  > +        ctrl->aen_data = le64_to_cpu(res->u64) >> 32;
> 
> Please use a helper for that.
> aen_data is not a great name... maybe use a union for that for the
> specific aen subtype?
> 
> Maybe it'd be better that zoned namespace aen is handled explicitly
> and passes a AEN_NSID env veriable to the uevent.

I'll respin with ZNS specific handling and naming with a union. I
think it's valuable to keep generating uevents with AEN_DATA when
not ZNS or NOTICE.  Some recent work has allowed all alternate
command sets to be used through an ng device without changes to
the core.  This fills the gap of AEN processing.  The down side is it's
difficult to test the non-ZNS path when ZNS is special cased.