Re: [PATCH] scsi: ufs: mark HPB support as BROKEN

[Jens Axboe <axboe@kernel.dk>]

On 10/26/21 12:05 PM, Bart Van Assche wrote:
> On 10/26/21 10:25 AM, Jens Axboe wrote:
>> On 10/26/21 11:19 AM, James Bottomley wrote:
>>> On Tue, 2021-10-26 at 09:36 -0700, Bart Van Assche wrote:
>>>> On 10/26/21 12:12 AM, Christoph Hellwig wrote:
>>>>> The HPB support added this merge window is fundanetally flawed as
>>>>> it
>>>>                                                ^^^^^^^^^^^^
>>>>                                                fundanetally ->
>>>> fundamentally
>>>>
>>>> Since the implementation can be reworked not to use
>>>> blk_insert_cloned_request() I'm not sure using the word
>>>> "fundamentally" is appropriate.
>>>
>>> I'm not so sure about that.  The READ BUFFER implementation runs from a
>>> work queue and looks fine.  The WRITE BUFFER implementation is trying
>>> to spawn a second command to precede the queued command which is a
>>> fundamental problem for the block API.  It's not clear to me that the
>>> WRITE BUFFER can be fixed because of the tying to the sent command ...
>>> but like I said, the standard is proprietary so I can't look at it to
>>> see if there are alternative ways of achieving the same effect.
>>
>> Is there a model in which this can actually work? If not, or if we
>> aren't sure, I think we'd be better off just reverting the parts
>> involved with that block layer misuse. Simply marking it broken is a
>> half measure that doesn't really solve anything (except send a message).
>>
>> IMHO, it should be reverted and the clone usage we currently export be
>> moved into dm for now. That'll prevent further abuse of this in the
>> future.
> 
> Hi Jens and James,
> 
> This is what I found in the HPB 2.0 specification (the spec is
> copyrighted but I assume that I have the right to quote small parts of
> that spec):
> 
> <quote>
> 6.3 HPB WRITE BUFFER Command
> 
> HPB WRITE BUFFER command have following 3 different function depending
> on the value of BUFFER_ID field.
> 1) Inactivating an HPB Region (supported in host control mode only)
> 2) prefetching HPB Entries from the host to the device (supported in any
>     control mode)
> 3) Inactivating all HPB Regions, except for Provisioning Pinned Region
>     in the host (supported in device control mode only)
> </quote>
> 
> Reverting only the problematic code (HPB 2.0) sounds reasonable to me
> because reworking the HPB 2.0 code will be nontrivial.

Then let's please go ahead and do that. I'm assuming this is a smaller
set than what Christoph originally posted, who's taking on the job of
lining it up?

> Using BLK_MQ_F_BLOCKING might be a viable approach. However, I don't
> want to see that flag enabled in the UFS driver if HPB is not used
> because of the negative performance effects of that flag.

Agree, and if we do just the problematic revert, then the decision on
whether BLK_MQ_F_BLOCKING is useful or not belongs to whoever reworks
the WRITE BUFFER code and reposts that support.

-- 
Jens Axboe