Re: [PATCH v1 0/4] Add command id quirk for fabrics

On 11/9/2021 10:09 AM, Christoph Hellwig wrote:

On Mon, Nov 08, 2021 at 08:45:11AM -0800, Keith Busch wrote:

quoted

On Mon, Nov 08, 2021 at 04:46:57PM +0200, Max Gurtovoy wrote:

quoted

Hi all,
Commit a2941f6aa71a ("nvme: add command id quirk for apple controllers")
was merged to fix a regression in apple controllers that was introduced
after merging commit e7006de6c238 ("nvme: code command_id with a genctr
for use-after-free validation").

This series is comming to enable the same quirk for fabrics controllers
that used the command id index in the same way that was probably used in
apple controllers.

If there really are targets behaving this way, then this looks good and
necessary, however unfortunate. A TCP target triggered the need for
valid tag validation in the first place.

What is really unfortunate is that we added this code by default and not 
as a quirk for buggy controllers.

This series is just completing the initial commit for genctr and gives 
it the right flexibility for smart sys-admins.

The genctr patch broke working controllers (such as Apple) and added 
conditions to the IO path in SW.

Also, controllers/devices that did some internal optimizations (such as 
accessing some task context using the cmd id as index in O(1) instead of 
looking the element in linked list in O(N)) to improve performance will 
now suffer. They may work, but performance will be worse.

Similarly, there is a specification for max_nsid and max_supported_ns, 
right ? exactly for these reasons. We might consider same specification 
for cmd_ids.

quoted

Are there really fabrics targets behaving this way, or is this series
anticipating they might exist? Apple disregarding specs is nothing new,
but I would have hoped no other targets would do this since most vendors
care about interop.

Seconded.  We probably also need to document the broken targers in the
nvme-cli documentation.

I don't think there is a list of broken targets, and broken is not the 
only use case as mentioned above.

We also don't know the target that cause the bug in the initial genctr 
commit..