2016 Changes to nvme.h

2016 Changes to nvme.h

[Wilson, Ellis]

Hi all,

I am looking for further insight into the change, which just caused some 
issues with code I'm working with when we moved to a new kernel.  It is 
identified on this page:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9d99a8dda154f38307d43d9c9aa504bd3703d596

In essence this pulls 90+% of the structs/enums/etc into the kernel 
sources, and leaves only the most basic interface in nvme_uapi.h.  This 
forces one to privately re-implement the structs/enums/etc that 
previously existed in nvme.h, which previously was available in the 
normal include path.  Fortunately all such data structures are described 
clearly in the NVMe specs, but it still feels like work without much 
reason.  This is especially the case since the author calls out that the 
only public user of the old header file (nvme cli) already had a copy of 
it in their sources locally, so it wouldn't break them.  This points out 
the reliance/expectation of external users on the structs expected to be 
passed through the uapi.

Is there rationale behind this that's not obvious to me?  Any insight is 
appreciated.  I admit I don't live in kernel code on a daily basis so 
it's possible I'm being naive about something -- apologies in advance if 
that's the case.

Thanks!

ellis

2016 Changes to nvme.h

[Keith Busch]

On Thu, Jan 24, 2019@04:59:58PM +0000, Wilson, Ellis wrote:
> Hi all,
> 
> I am looking for further insight into the change, which just caused some 
> issues with code I'm working with when we moved to a new kernel.  It is 
> identified on this page:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9d99a8dda154f38307d43d9c9aa504bd3703d596
> 
> In essence this pulls 90+% of the structs/enums/etc into the kernel 
> sources, and leaves only the most basic interface in nvme_uapi.h.  This 
> forces one to privately re-implement the structs/enums/etc that 
> previously existed in nvme.h, which previously was available in the 
> normal include path.  Fortunately all such data structures are described 
> clearly in the NVMe specs, but it still feels like work without much 
> reason.  This is especially the case since the author calls out that the 
> only public user of the old header file (nvme cli) already had a copy of 
> it in their sources locally, so it wouldn't break them.  This points out 
> the reliance/expectation of external users on the structs expected to be 
> passed through the uapi.
> 
> Is there rationale behind this that's not obvious to me?  Any insight is 
> appreciated.  I admit I don't live in kernel code on a daily basis so 
> it's possible I'm being naive about something -- apologies in advance if 
> that's the case.

As the spec defines new fields and user tooling wants to user them,
it is just not a good idea to rely on the kernel to own updating the
structures when the kernel has no internal need for it. It's better
for user space to control its own needs independent from the kernel's.

This could really be a user space library. I had a request to separate
nvme-cli parts into a library that included the spec structures, but I
admit I dropped the ball on that...

2016 Changes to nvme.h

[Wilson, Ellis]

On 1/24/19 12:21 PM, Keith Busch wrote:
> On Thu, Jan 24, 2019@04:59:58PM +0000, Wilson, Ellis wrote:
>> Hi all,
>>
>> I am looking for further insight into the change, which just caused some
>> issues with code I'm working with when we moved to a new kernel.  It is
>> identified on this page:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9d99a8dda154f38307d43d9c9aa504bd3703d596
>>
>> In essence this pulls 90+% of the structs/enums/etc into the kernel
>> sources, and leaves only the most basic interface in nvme_uapi.h.  This
>> forces one to privately re-implement the structs/enums/etc that
>> previously existed in nvme.h, which previously was available in the
>> normal include path.  Fortunately all such data structures are described
>> clearly in the NVMe specs, but it still feels like work without much
>> reason.  This is especially the case since the author calls out that the
>> only public user of the old header file (nvme cli) already had a copy of
>> it in their sources locally, so it wouldn't break them.  This points out
>> the reliance/expectation of external users on the structs expected to be
>> passed through the uapi.
>>
>> Is there rationale behind this that's not obvious to me?  Any insight is
>> appreciated.  I admit I don't live in kernel code on a daily basis so
>> it's possible I'm being naive about something -- apologies in advance if
>> that's the case.
> 
> As the spec defines new fields and user tooling wants to user them,
> it is just not a good idea to rely on the kernel to own updating the
> structures when the kernel has no internal need for it. It's better
> for user space to control its own needs independent from the kernel's.
> 
> This could really be a user space library. I had a request to separate
> nvme-cli parts into a library that included the spec structures, but I
> admit I dropped the ball on that...

Hi Keith,

Thank you for the response.  I think I understand the intent now.  Your 
comment about a userspace library makes total sense.  I still disagree 
with one thing.  You say, "it is just not a good idea to rely on the 
kernel to own updating the structures when the kernel has no internal 
need for it."  And yet, those very structures were moved further 
/inside/, rather than being deleted wholesale, so the action doesn't 
quite match up with your explanation from what I can tell.

I dug into the code and I see pci.c and scsi.c both rely on many of 
these structs and enums, which explains why it was pulled in rather than 
being removed entirely.  I think I follow now, and agree it makes sense 
to disentangle the userspace code from the kernel code to prevent 
forcing everybody to move forward in lock-step in the face of changing 
spec structures.  Both need and rely on these structures, but can be 
off-version with regard to one another without mishap.

Thanks for your time,

ellis

2016 Changes to nvme.h

[Keith Busch]

On Thu, Jan 24, 2019@09:38:11PM +0000, Wilson, Ellis wrote:
> Thank you for the response.  I think I understand the intent now.  Your 
> comment about a userspace library makes total sense.  I still disagree 
> with one thing.  You say, "it is just not a good idea to rely on the 
> kernel to own updating the structures when the kernel has no internal 
> need for it."  And yet, those very structures were moved further 
> /inside/, rather than being deleted wholesale, so the action doesn't 
> quite match up with your explanation from what I can tell.

We do have in-kernel uses for nvme structures, hence the relocation to
an internal definition. We just don't want to export it for user space,
which would (and did) create undue maintenance to non-kernel usage.