Re: [PATCH] scsi: ufs: revert HPB support

Re: [PATCH] scsi: ufs: revert HPB support

[Bart Van Assche]

On 10/21/21 11:20 PM, Christoph Hellwig wrote:
> The HPB support added this merge window is fundanetally flawed as it
> uses blk_insert_cloned_request to insert a cloned request onto the same
> queue as the one that the original request came from, leading to all
> kinds of issues in blk-mq accounting (in addition to this API being
> a special case for dm-mpath that should not see other users).

I do not agree with removing the UFS HPB code from the upstream kernel 
at this time.

One of the HPB users promised to look into removing the 
blk_insert_cloned_request() call from the UFS HPB code. Shouldn't that 
person be given the chance to come up with a patch before removal of the 
UFS HPB code is considered?

Additionally, removing the UFS HPB code from the upstream kernel won't 
affect UFS users much. As far as I know all HPB users use Android. UFS 
HPB is supported by Android 12 and will also be supported by Android 13. 
Whether or not this patch is goes upstream won't affect the Android 
kernel. I am not aware of any plans to make any changes in the Android 
kernel UFS HPB code if this patch would be integrated in the upstream 
kernel.

Bart.

Re: [PATCH] scsi: ufs: revert HPB support

[Greg Kroah-Hartman]

On Mon, Oct 25, 2021 at 09:12:14AM -0700, Bart Van Assche wrote:
> On 10/21/21 11:20 PM, Christoph Hellwig wrote:
> > The HPB support added this merge window is fundanetally flawed as it
> > uses blk_insert_cloned_request to insert a cloned request onto the same
> > queue as the one that the original request came from, leading to all
> > kinds of issues in blk-mq accounting (in addition to this API being
> > a special case for dm-mpath that should not see other users).
> 
> I do not agree with removing the UFS HPB code from the upstream kernel at
> this time.
> 
> One of the HPB users promised to look into removing the
> blk_insert_cloned_request() call from the UFS HPB code. Shouldn't that
> person be given the chance to come up with a patch before removal of the UFS
> HPB code is considered?
> 
> Additionally, removing the UFS HPB code from the upstream kernel won't
> affect UFS users much. As far as I know all HPB users use Android. UFS HPB
> is supported by Android 12 and will also be supported by Android 13. Whether
> or not this patch is goes upstream won't affect the Android kernel. I am not
> aware of any plans to make any changes in the Android kernel UFS HPB code if
> this patch would be integrated in the upstream kernel.

Under this line of reasoning, why would upstream take the code at all?

{sigh}

Is there a link to where the HPB developer said they would look into
this?  Perhaps until that happens this should be marked as BROKEN?

thanks,

greg k-h

Re: [PATCH] scsi: ufs: revert HPB support

[Bart Van Assche]

On 10/25/21 11:26 AM, Greg Kroah-Hartman wrote:
> Is there a link to where the HPB developer said they would look into
> this?  Perhaps until that happens this should be marked as BROKEN?

Hi Greg,

Daejun wrote the following on Thursday: "I will find out how to make
the HPB code without blk_insert_cloned_request() API." Unfortunately
that email was sent as MIME-encoded so it was only received by the
people Cc-ed on that email and has not been archived by any of the
websites that archives linux-scsi or linux-block emails. The email
that I received is available below. I think it is fine to make this
email public given the presence of two mailing lists in the Cc-list.

Thanks,

Bart.



-------- Forwarded Message --------
Subject: 	Re: please revert the UFS HPB support
Date: 	Fri, 22 Oct 2021 07:41:43 +0900
From: 	박대준 <pdaejun@gmail.com>
To: 	Bart Van Assche <bvanassche@acm.org>
CC: 	Christoph Hellwig <hch@lst.de>, Jens Axboe <axboe@kernel.dk>, martin.petersen@oracle.com, linux-scsi@vger.kernel.org, linux-block@vger.kernel.org, Daejun Park <daejun7.park@samsung.com>, Jaegeuk Kim <jaegeuk@kernel.org>



Hi Bart,

2021년 10월 22일 (금) 오전 1:23, Bart Van Assche <bvanassche@acm.org <mailto:bvanassche@acm.org>>님이 작성:

     On 10/21/21 8:17 AM, Christoph Hellwig wrote:
      > On Thu, Oct 21, 2021 at 05:15:20PM +0200, Christoph Hellwig wrote:
      >>>> I just noticed the UFS HPB support landed in 5.15, and just as
      >>>> before it is completely broken by allocating another request on
      >>>> the same device and then reinserting it in the queue.  It is bad
      >>>> enough that we have to live with blk_insert_cloned_request for
      >>>> dm-mpath, but this is too big of an API abuse to make it into
      >>>> a release.  We need to drop this code ASAP, and I can prepare
      >>>> a patch for that.
      >>>
      >>> That sounds awful, do you have a link to the offending commit(s)?
      >>
      >> I'll need to look for it, busy in calls right now, but just grep for
      >> blk_insert_cloned_request.
      >
      > Might as well finish the git blame:
      >
      > commit 41d8a9333cc96f5ad4dd7a52786585338257d9f1
      > Author: Daejun Park <daejun7.park@samsung.com <mailto:daejun7.park@samsung.com>>
      > Date:   Mon Jul 12 18:00:25 2021 +0900
      >
      >      scsi: ufs: ufshpb: Add HPB 2.0 support
      >
      >      Version 2.0 of HBP supports reads of varying sizes from 4KB to 1MB.
      >
      >      A read operation <= 32KB is supported as single HPB read. A read between
      >      36KB and 1MB is supported by a combination of write buffer command and HPB
      >      read command to deliver more PPN. The write buffer commands may not be
      >      issued immediately due to busy tags. To use HPB read more aggressively, the
      >      driver can requeue the write buffer command. The requeue threshold is
      >      implemented as timeout and can be modified with requeue_timeout_ms entry in
      >      sysfs.

     (+Daejun)

     Daejun, can the HPB code be reworked such that it does not use
     blk_insert_cloned_request()? I'm concerned that if the HPB code is not
     reworked that it will be removed from the upstream kernel.

     Thanks,

     Bart.


I will find out how to make the HPB code without blk_insert_cloned_request() API.

Thanks,
Daejun

Re: [PATCH] scsi: ufs: revert HPB support

[Martin K. Petersen]

Greg,

> Under this line of reasoning, why would upstream take the code at all?
>
> {sigh}

Sigh indeed.

> Is there a link to where the HPB developer said they would look into
> this?  Perhaps until that happens this should be marked as BROKEN?

I say we just mark it broken for now.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] scsi: ufs: revert HPB support

[Christoph Hellwig]

On Tue, Oct 26, 2021 at 12:13:46AM -0400, Martin K. Petersen wrote:
> 
> Greg,
> 
> > Under this line of reasoning, why would upstream take the code at all?
> >
> > {sigh}
> 
> Sigh indeed.
> 
> > Is there a link to where the HPB developer said they would look into
> > this?  Perhaps until that happens this should be marked as BROKEN?
> 
> I say we just mark it broken for now.

I've sent a patch.