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

[Clay Mayers <Clay.Mayers@kioxia.com>]

> 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.