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
