please revert the UFS HPB support
Subsystems (2)
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.
Jens Axboe
maintainerQuoted 9 lines · Christoph Hellwig
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_requestfor 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)?
Signature · 1 line
Jens Axboe
Quoted 11 lines · Jens Axboe
Quoted 9 lines · Christoph Hellwig
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_requestfor 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.
Quoted 1 line
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.
Quoted 8 lines · Christoph Hellwig
Quoted 8 lines
Quoted 7 lines
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_requestfor 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.
Quoted 4 lines
Quoted 1 line
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 <[email protected]> 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.
Quoted 24 lines · Christoph Hellwig
Quoted 8 lines · Christoph Hellwig
Quoted 8 lines
Quoted 7 lines
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_requestfor 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.Quoted 4 lines
Quoted 1 line
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
41d8a9333cc96f5ad4dd7a52786585338257d9f1Author: Daejun Park <[email protected]> Date: Mon Jul 12 18:00:25 2021 +0900scsi: ufs: ufshpb: Add HPB 2.0 support
Version 2.0 of HBP supports reads of varying sizes from 4KB to 1MB.
Quoted 7 lines
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_msentry 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.
James Bottomley
maintainerQuoted 41 lines · Bart Van Assche
Quoted 26 lines · Christoph Hellwig
Quoted 8 lines · Christoph Hellwig
Quoted 8 lines
Quoted 7 lines
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_requestfor 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.Quoted 5 lines
Quoted 2 lines
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
41d8a9333cc96f5ad4dd7a52786585338257d9f1Author: Daejun Park <[email protected]> Date: Mon Jul 12 18:00:25 2021 +0900scsi: ufs: ufshpb: Add HPB 2.0 support
Version 2.0 of HBP supports reads of varying sizes from 4KB to
1MB.Quoted 13 lines
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 withrequeue_timeout_msentry in
sysfs.
Quoted 5 lines
(+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
Quoted 44 lines · Bart Van Assche
Quoted 43 lines · Christoph Hellwig
Quoted 26 lines · Christoph Hellwig
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
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_requestfor 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.Quoted 5 lines
Quoted 2 lines
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
41d8a9333cc96f5ad4dd7a52786585338257d9f1Author: Daejun Park <[email protected]> Date: Mon Jul 12 18:00:25 2021 +0900scsi: ufs: ufshpb: Add HPB 2.0 support
Version 2.0 of HBP supports reads of varying sizes from 4KB to
1MB.Quoted 13 lines
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 withrequeue_timeout_msentry in
sysfs.(+Daejun)
Quoted 7 lines
Quoted 3 lines
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.
Quoted 8 lines
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
Quoted 5 lines · Daejun Park
Quoted 2 lines
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_nowaitwithblk_insert_cloned_requestin 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.
James Bottomley
maintainerQuoted 50 lines · Daejun Park
Quoted 50 lines · Bart Van Assche
Quoted 48 lines · Christoph Hellwig
Quoted 28 lines
Quoted 9 lines · Christoph Hellwig
Quoted 9 lines
Quoted 8 lines
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_requestfor 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.Quoted 5 lines
Quoted 2 lines
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
41d8a9333cc96f5ad4dd7a52786585338257d9f1Author: Daejun Park <[email protected]> Date: Mon Jul 12 18:00:25 2021 +0900scsi: ufs: ufshpb: Add HPB 2.0 support
Version 2.0 of HBP supports reads of varying sizes from
4KB to 1MB.Quoted 13 lines
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 withrequeue_timeout_msentry 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.
Quoted 6 lines
Quoted 3 lines
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_nowaitwithblk_insert_cloned_requestin the HPB code.
That's not going to help: removing blk_insert_cloned_request() is what we need.
Quoted 9 lines
Quoted 8 lines
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?
Quoted 3 lines
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
Quoted 48 lines · Daejun Park
Quoted 48 lines · Bart Van Assche
Quoted 46 lines · Christoph Hellwig
Quoted 26 lines · Christoph Hellwig
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
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_requestfor 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.Quoted 5 lines
Quoted 2 lines
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
41d8a9333cc96f5ad4dd7a52786585338257d9f1Author: Daejun Park <[email protected]> Date: Mon Jul 12 18:00:25 2021 +0900scsi: ufs: ufshpb: Add HPB 2.0 support
Version 2.0 of HBP supports reads of varying sizes from 4KB to
1MB.Quoted 13 lines
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 withrequeue_timeout_msentry 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.
Quoted 6 lines
Quoted 3 lines
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_nowaitwithblk_insert_cloned_requestin the HPB code.
In case it is of any use, there is an example of sending a REQUEST_SENSE command directly here:
https://lore.kernel.org/all/[email protected]
Quoted 17 lines
Quoted 9 lines
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
Hi Christoph,
Quoted 6 lines
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_requestfor 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...
Signature · 1 line
Martin K. Petersen Oracle Linux Engineering
Quoted 10 lines · Martin K. Petersen
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.