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

James Bottomley
2021-10-27T14:43:52Z

On Wed, 2021-10-27 at 08:38 -0600, Jens Axboe wrote:
> On 10/27/21 8:12 AM, Keith Busch wrote:
> > On Wed, Oct 27, 2021 at 06:16:19AM -0700, Bart Van Assche wrote:
> > > On 10/26/21 10:27 PM, Christoph Hellwig wrote:
> > > > On Tue, Oct 26, 2021 at 01:10:47PM -0700, Bart Van Assche
> > > > wrote:
> > > > > If blk_insert_cloned_request() is moved into the device
> > > > > mapper then I think that blk_mq_request_issue_directly() will
> > > > > need to be exported.
> > > >
> > > > Which is even worse.
> > > >
> > > > > How
> > > > > about the (totally untested) patch below for removing the
> > > > > blk_insert_cloned_request() call from the UFS-HPB code?
> > > >
> > > > Which again doesn't fix anything. The problem is that it fans
> > > > out one request into two on the same queue, not the specific
> > > > interface used.
> > >
> > > That patch fixes the reported issue, namely removing the
> > > additional accounting caused by calling
> > > blk_insert_cloned_request(). Please explain why it is considered
> > > wrong to fan out one request into two. That code could be
> > > reworked such that the block layer is not involved as Adrian
> > > Hunter explained. However, before someone spends time on making
> > > these changes I think that someone should provide more
> > > information about why it is considered wrong to fan out one
> > > request into two.
> >
> > The original request consumes a tag from that queue's tagset. If
> > the lifetime of that tag depends on that same queue having another
> > free tag, you can deadlock.
>
> And to expand on that, one potential solution would be to require as
> many reserved tags as normal tags, and have the cloned/direct insert
> grab requests out of the reserved pool. That guarantees the forward
> progress that is currently violated by randomly requiring an extra
> tag.

Another potential way of doing this might be to transmit the buffer
information as a separate scatterlist, a bit like DIF/DIX. At the
bottom of the stack, the write buffer has to complete before the
dependent command so we could actually enforce that right at the driver
level using the same tag since the two commands are sequential.

James