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

[Max Gurtovoy <mgurtovoy@nvidia.com>]

On 11/9/2021 10:09 AM, Christoph Hellwig wrote:
> On Mon, Nov 08, 2021 at 08:45:11AM -0800, Keith Busch wrote:
>> On Mon, Nov 08, 2021 at 04:46:57PM +0200, Max Gurtovoy wrote:
>>> 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.

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