linux-block.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* please revert the UFS HPB support
@ 2021-10-21 14:42 Christoph Hellwig
  2021-10-21 15:13 ` Jens Axboe
  2021-10-23  1:54 ` Martin K. Petersen
  0 siblings, 2 replies; 11+ messages in thread
From: Christoph Hellwig @ 2021-10-21 14:42 UTC (permalink / raw)
  To: martin.petersen; +Cc: Jens Axboe, linux-scsi, linux-block

Hi Martin,

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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: please revert the UFS HPB support
  2021-10-21 14:42 please revert the UFS HPB support Christoph Hellwig
@ 2021-10-21 15:13 ` Jens Axboe
  2021-10-21 15:15   ` Christoph Hellwig
  2021-10-23  1:54 ` Martin K. Petersen
  1 sibling, 1 reply; 11+ messages in thread
From: Jens Axboe @ 2021-10-21 15:13 UTC (permalink / raw)
  To: Christoph Hellwig, martin.petersen; +Cc: linux-scsi, linux-block

On 10/21/21 8:42 AM, Christoph Hellwig wrote:
> Hi Martin,
> 
> 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)?

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: please revert the UFS HPB support
  2021-10-21 15:13 ` Jens Axboe
@ 2021-10-21 15:15   ` Christoph Hellwig
  2021-10-21 15:17     ` Christoph Hellwig
  0 siblings, 1 reply; 11+ messages in thread
From: Christoph Hellwig @ 2021-10-21 15:15 UTC (permalink / raw)
  To: Jens Axboe; +Cc: Christoph Hellwig, martin.petersen, linux-scsi, linux-block

On Thu, Oct 21, 2021 at 09:13:30AM -0600, Jens Axboe wrote:
> On 10/21/21 8:42 AM, Christoph Hellwig wrote:
> > Hi Martin,
> > 
> > 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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: please revert the UFS HPB support
  2021-10-21 15:15   ` Christoph Hellwig
@ 2021-10-21 15:17     ` Christoph Hellwig
  2021-10-21 16:22       ` Bart Van Assche
  0 siblings, 1 reply; 11+ messages in thread
From: Christoph Hellwig @ 2021-10-21 15:17 UTC (permalink / raw)
  To: Jens Axboe; +Cc: Christoph Hellwig, martin.petersen, linux-scsi, linux-block

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: please revert the UFS HPB support
  2021-10-21 15:17     ` Christoph Hellwig
@ 2021-10-21 16:22       ` Bart Van Assche
  2021-10-23 15:43         ` James Bottomley
       [not found]         ` <CGME20211023154316epcas2p208f95cf1e4a87a4b61d2daf1a2b3c725@epcms2p3>
  0 siblings, 2 replies; 11+ messages in thread
From: Bart Van Assche @ 2021-10-21 16:22 UTC (permalink / raw)
  To: Christoph Hellwig, Jens Axboe
  Cc: martin.petersen, linux-scsi, linux-block, Daejun Park, Jaegeuk Kim

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: please revert the UFS HPB support
  2021-10-21 14:42 please revert the UFS HPB support Christoph Hellwig
  2021-10-21 15:13 ` Jens Axboe
@ 2021-10-23  1:54 ` Martin K. Petersen
  2021-10-23  4:40   ` Christoph Hellwig
  1 sibling, 1 reply; 11+ messages in thread
From: Martin K. Petersen @ 2021-10-23  1:54 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: martin.petersen, Jens Axboe, linux-scsi, linux-block, Greg KH


Hi Christoph,

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

The series went through 40 iterations on the list prior to being merged.
I don't recall a better approach to reconcile the HPB model with the
stack being offered during that process?

As much as I don't like HPB, all the major UFS subsystem stakeholders
are behind it. The hardware is shipping and various device stacks
already adopted support for it. At this stage I don't think dropping the
code is a way forward. I am much more in favor of having a productive
discussion about how to go about addressing the problems with the
queuing model...

-- 
Martin K. Petersen	Oracle Linux Engineering

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: please revert the UFS HPB support
  2021-10-23  1:54 ` Martin K. Petersen
@ 2021-10-23  4:40   ` Christoph Hellwig
  0 siblings, 0 replies; 11+ messages in thread
From: Christoph Hellwig @ 2021-10-23  4:40 UTC (permalink / raw)
  To: Martin K. Petersen
  Cc: Christoph Hellwig, Jens Axboe, linux-scsi, linux-block, Greg KH

On Fri, Oct 22, 2021 at 09:54:44PM -0400, Martin K. Petersen wrote:
> The series went through 40 iterations on the list prior to being merged.
> I don't recall a better approach to reconcile the HPB model with the
> stack being offered during that process?
> 
> As much as I don't like HPB, all the major UFS subsystem stakeholders
> are behind it. The hardware is shipping and various device stacks
> already adopted support for it. At this stage I don't think dropping the
> code is a way forward. I am much more in favor of having a productive
> discussion about how to go about addressing the problems with the
> queuing model...

This is іs not about HPB about a feature beeing a bad idea.  I explained
that at least a dozend times, and there's been no sensible counter
arguments to that but it has been merged anyway.  (FYI Zoned devices
are the obvious and old answer).

But this implementation is simply completely broken from the block
layer POV and we must not release a kernel with that brokenness in it.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: please revert the UFS HPB support
  2021-10-21 16:22       ` Bart Van Assche
@ 2021-10-23 15:43         ` James Bottomley
       [not found]         ` <CGME20211023154316epcas2p208f95cf1e4a87a4b61d2daf1a2b3c725@epcms2p3>
  1 sibling, 0 replies; 11+ messages in thread
From: James Bottomley @ 2021-10-23 15:43 UTC (permalink / raw)
  To: Bart Van Assche, Christoph Hellwig, Jens Axboe
  Cc: martin.petersen, linux-scsi, linux-block, Daejun Park, Jaegeuk Kim

On Thu, 2021-10-21 at 09:22 -0700, Bart Van Assche wrote:
> 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>
> > 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.

Just to give urgency to Bart's request: we have two or three weeks
before the kernel is due to go final.  Can the problems identified by
Christoph be fixed within that timeframe?

Specifically, looking at the paper you reference, it only uses READ
BUFFER for the host cache sharing.  Since the JDEC standard appears to
be proprietary, I have no method of understanding why the driver now
uses WRITE BUFFER as well, but it appears to be a simple optimization. 
If you can cut out the WRITE BUFFER code, blk_insert_cloned_request()
will also be gone and thus the API abuse.  Can you get us a simple
patch doing this ASAP so we don't have to revert the driver?

James



^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: please revert the UFS HPB support
       [not found]         ` <CGME20211023154316epcas2p208f95cf1e4a87a4b61d2daf1a2b3c725@epcms2p3>
@ 2021-10-25  5:16           ` Daejun Park
  2021-10-25  7:07             ` Christoph Hellwig
  2021-10-25 11:39             ` James Bottomley
  0 siblings, 2 replies; 11+ messages in thread
From: Daejun Park @ 2021-10-25  5:16 UTC (permalink / raw)
  To: James Bottomley, Bart Van Assche, Christoph Hellwig, Jens Axboe
  Cc: martin.petersen, linux-scsi, linux-block, Daejun Park, Jaegeuk Kim

> On Thu, 2021-10-21 at 09:22 -0700, Bart Van Assche wrote:
> > 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>
> > > 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.
>  
> Just to give urgency to Bart's request: we have two or three weeks
> before the kernel is due to go final.  Can the problems identified by
> Christoph be fixed within that timeframe?

I'm checking to see if I can replace blk_execute_rq_nowait with
blk_insert_cloned_request in the HPB code.

>  
> Specifically, looking at the paper you reference, it only uses READ
> BUFFER for the host cache sharing.  Since the JDEC standard appears to
> be proprietary, I have no method of understanding why the driver now
> uses WRITE BUFFER as well, but it appears to be a simple optimization. 
> If you can cut out the WRITE BUFFER code, blk_insert_cloned_request()
> will also be gone and thus the API abuse.  Can you get us a simple
> patch doing this ASAP so we don't have to revert the driver?

If WRITE BUFFER is not used, only READs with a size of 32KB or less can be
changed to HPB READs. This becomes a limiting factor in how READ
performance can be improved by the HPB.

Thanks,
Daejun

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: please revert the UFS HPB support
  2021-10-25  5:16           ` Daejun Park
@ 2021-10-25  7:07             ` Christoph Hellwig
  2021-10-25 11:39             ` James Bottomley
  1 sibling, 0 replies; 11+ messages in thread
From: Christoph Hellwig @ 2021-10-25  7:07 UTC (permalink / raw)
  To: Daejun Park
  Cc: James Bottomley, Bart Van Assche, Christoph Hellwig, Jens Axboe,
	martin.petersen, linux-scsi, linux-block, Jaegeuk Kim

On Mon, Oct 25, 2021 at 02:16:54PM +0900, Daejun Park wrote:
> > before the kernel is due to go final.  Can the problems identified by
> > Christoph be fixed within that timeframe?
> 
> I'm checking to see if I can replace blk_execute_rq_nowait with
> blk_insert_cloned_request in the HPB code.

blk_insert_cloned_request is part of the problem.  You can't just fan
out one request into two on the same queue.

FYI, 5.15 is supposed to be released end of this week (unless an -rc8)
happens.  I don't think we can sit this out, we need to revert (or at
least disable) the code now.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: please revert the UFS HPB support
  2021-10-25  5:16           ` Daejun Park
  2021-10-25  7:07             ` Christoph Hellwig
@ 2021-10-25 11:39             ` James Bottomley
  1 sibling, 0 replies; 11+ messages in thread
From: James Bottomley @ 2021-10-25 11:39 UTC (permalink / raw)
  To: daejun7.park, Bart Van Assche, Christoph Hellwig, Jens Axboe
  Cc: martin.petersen, linux-scsi, linux-block, Jaegeuk Kim

On Mon, 2021-10-25 at 14:16 +0900, Daejun Park wrote:
> > On Thu, 2021-10-21 at 09:22 -0700, Bart Van Assche wrote:
> > > 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>
> > > > 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.
> >  
> > Just to give urgency to Bart's request: we have two or three weeks
> > before the kernel is due to go final.  Can the problems identified
> > by Christoph be fixed within that timeframe?
> 
> I'm checking to see if I can replace blk_execute_rq_nowait with
> blk_insert_cloned_request in the HPB code.

That's not going to help: removing blk_insert_cloned_request() is what
we need.
 
> > Specifically, looking at the paper you reference, it only uses READ
> > BUFFER for the host cache sharing.  Since the JDEC standard appears
> > to be proprietary, I have no method of understanding why the driver
> > now uses WRITE BUFFER as well, but it appears to be a simple
> > optimization.  If you can cut out the WRITE BUFFER code,
> > blk_insert_cloned_request() will also be gone and thus the API
> > abuse.  Can you get us a simple patch doing this ASAP so we don't
> > have to revert the driver?
> 
> If WRITE BUFFER is not used, only READs with a size of 32KB or less
> can be changed to HPB READs. This becomes a limiting factor in how
> READ performance can be improved by the HPB.

Well, precisely: it's an optimization.  It can be removed now and you
can work out how to add the code back without the problem API later. 
We're running critically short of time to fix this, so unless you have
a different proposal, it's either this hunk of code or the entire
driver.

James



^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2021-10-25 11:39 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-21 14:42 please revert the UFS HPB support Christoph Hellwig
2021-10-21 15:13 ` Jens Axboe
2021-10-21 15:15   ` Christoph Hellwig
2021-10-21 15:17     ` Christoph Hellwig
2021-10-21 16:22       ` Bart Van Assche
2021-10-23 15:43         ` James Bottomley
     [not found]         ` <CGME20211023154316epcas2p208f95cf1e4a87a4b61d2daf1a2b3c725@epcms2p3>
2021-10-25  5:16           ` Daejun Park
2021-10-25  7:07             ` Christoph Hellwig
2021-10-25 11:39             ` James Bottomley
2021-10-23  1:54 ` Martin K. Petersen
2021-10-23  4:40   ` Christoph Hellwig

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).