From: Christoph Hellwig <hch@lst.de> Date: 2021-10-26 07:12:32
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).
Mark is as BROKEN as the non-intrusive alternative to a last minute
large scale revert.
Fixes: f02bc9754a68 ("scsi: ufs: ufshpb: Introduce Host Performance Buffer
feature")
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/scsi/ufs/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Hannes Reinecke <hare@suse.de> Date: 2021-10-26 07:18:40
On 10/26/21 9:12 AM, Christoph Hellwig wrote:
quoted hunk
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).
Mark is as BROKEN as the non-intrusive alternative to a last minute
large scale revert.
Fixes: f02bc9754a68 ("scsi: ufs: ufshpb: Introduce Host Performance Buffer
feature")
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/scsi/ufs/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
From: Damien Le Moal <hidden> Date: 2021-10-26 07:24:07
On 2021/10/26 16:12, 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).
Mark is as BROKEN as the non-intrusive alternative to a last minute
From: James Bottomley <James.Bottomley@HansenPartnership.com> Date: 2021-10-26 13:04:20
On Tue, 2021-10-26 at 16:24 +0900, Damien Le Moal wrote:
On 2021/10/26 16:12, Christoph Hellwig wrote:
quoted
The HPB support added this merge window is fundanetally
And s/n/m/ while you're at it: fundamentally
Otherwise:
Reviewed-by: James E.J. Bottomley <redacted>
James
quoted
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).
Mark is as BROKEN as the non-intrusive alternative to a last minute
From: Bart Van Assche <bvanassche@acm.org> Date: 2021-10-26 16:36:48
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.
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).
More detailed information would have been welcome.
I assume that you wanted to refer to commit 41d8a9333cc9 ("scsi: ufs: ufshpb:
Add HPB 2.0 support") instead since that commit is the only commit that
introduced a blk_insert_cloned_request() call in the UFS HPB code?
Bart.
From: James Bottomley <James.Bottomley@HansenPartnership.com> Date: 2021-10-26 17:19:37
On Tue, 2021-10-26 at 09:36 -0700, Bart Van Assche wrote:
On 10/26/21 12:12 AM, Christoph Hellwig wrote:
quoted
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.
James
On Tue, 2021-10-26 at 09:36 -0700, Bart Van Assche wrote:
quoted
On 10/26/21 12:12 AM, Christoph Hellwig wrote:
quoted
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.
--
Jens Axboe
From: Bart Van Assche <bvanassche@acm.org> Date: 2021-10-26 18:05:38
On 10/26/21 10:25 AM, Jens Axboe wrote:
On 10/26/21 11:19 AM, James Bottomley wrote:
quoted
On Tue, 2021-10-26 at 09:36 -0700, Bart Van Assche wrote:
quoted
On 10/26/21 12:12 AM, Christoph Hellwig wrote:
quoted
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. 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.
Thanks,
Bart.
On Tue, 2021-10-26 at 09:36 -0700, Bart Van Assche wrote:
quoted
On 10/26/21 12:12 AM, Christoph Hellwig wrote:
quoted
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
From: James Bottomley <James.Bottomley@HansenPartnership.com> Date: 2021-10-26 18:18:19
On Tue, 2021-10-26 at 12:10 -0600, Jens Axboe wrote:
On 10/26/21 12:05 PM, Bart Van Assche wrote:
[...]
quoted
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?
I was hoping the HPB guys would do this. I think I know how to do it
based on my investigations, but I'd really prefer that someone who
cares about HPB did it. It looks like a fairly simple excision, so I
think we can have this done before the end of the week and if it isn't
we could revert the whole driver.
quoted
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.
Agreed, that was my initial proposed solution: get rid of the write
buffer optimzation now to fix the API abuse and see if we can add it
back in a more acceptable form later.
James
From: Christoph Hellwig <hch@lst.de> Date: 2021-10-29 10:53:56
Given that the discussion is now turning into bikeshedding wether the
non-public UFS spec is mereley completly broken or utterly completely
broken can we please add this patch or the revert before 5.15
goes in? I don't think this mess will be resolved in any reasonable
time.
From: James Bottomley <James.Bottomley@HansenPartnership.com> Date: 2021-10-29 11:39:20
On Fri, 2021-10-29 at 12:53 +0200, Christoph Hellwig wrote:
Given that the discussion is now turning into bikeshedding wether the
non-public UFS spec is mereley completly broken or utterly completely
broken can we please add this patch or the revert before 5.15
goes in? I don't think this mess will be resolved in any reasonable
time.
No. Removing the 2.0 HPB optimization fixes all your complaints about
the block API problems, so there's no need to do a full revert. I just
need someone to test the partial revert ASAP. If no-one can test the
partial revert then we can consider more drastic options.
James
On Fri, 2021-10-29 at 12:53 +0200, Christoph Hellwig wrote:
quoted
Given that the discussion is now turning into bikeshedding wether the
non-public UFS spec is mereley completly broken or utterly completely
broken can we please add this patch or the revert before 5.15 goes in?
I don't think this mess will be resolved in any reasonable time.
No. Removing the 2.0 HPB optimization fixes all your complaints about the
block API problems, so there's no need to do a full revert. I just need
someone to test the partial revert ASAP. If no-one can test the partial revert
then we can consider more drastic options.
I support Daejun's patch, but if this is your final decision, I can test it on Sunday.
Can you refer me to exact patch needed to be tested?
There were quite few suggestions flying around....
Thanks,
Avri
From: James Bottomley <James.Bottomley@HansenPartnership.com> Date: 2021-10-29 13:44:12
On Fri, 2021-10-29 at 13:35 +0000, Avri Altman wrote:
quoted
On Fri, 2021-10-29 at 12:53 +0200, Christoph Hellwig wrote:
quoted
Given that the discussion is now turning into bikeshedding wether
the non-public UFS spec is mereley completly broken or utterly
completely broken can we please add this patch or the revert
before 5.15 goes in? I don't think this mess will be resolved in
any reasonable time.
No. Removing the 2.0 HPB optimization fixes all your complaints
about the block API problems, so there's no need to do a full
revert. I just need someone to test the partial revert ASAP. If
no-one can test the partial revert then we can consider more
drastic options.
I support Daejun's patch, but if this is your final decision, I can
test it on Sunday. Can you refer me to exact patch needed to be
tested?