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

From: Bart Van Assche <bvanassche@acm.org>
To: Jens Axboe <axboe@kernel.dk>,
	James Bottomley <James.Bottomley@HansenPartnership.com>,
	Christoph Hellwig <hch@lst.de>,
	martin.petersen@oracle.com, Jaegeuk Kim <jaegeuk@kernel.org>
Cc: alim.akhtar@samsung.com, avri.altman@wdc.com,
	linux-scsi@vger.kernel.org, linux-block@vger.kernel.org
Subject: Re: [PATCH] scsi: ufs: mark HPB support as BROKEN
Date: Tue, 26 Oct 2021 11:05:33 -0700	[thread overview]
Message-ID: <4438ab72-7da0-33de-ecc9-91c3c179eca7@acm.org> (raw)
In-Reply-To: <870e986c-08dd-2fa2-a593-0f97e10d6df5@kernel.dk>

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

Thanks,

Bart.