LinuxLists.cc - [PATCH] scsi: ufs: Fix proper API to send HPB pre-request

[permalink] [raw]

Subject: Re: [PATCH] scsi: ufs: Fix proper API to send HPB pre-request

On 10/27/21 15:36, Daejun Park wrote:
> This patch addresses the issue of using the wrong API to create a
> pre_request for HPB READ.
> HPB READ candidate that require a pre-request will try to allocate a
> pre-request only during request_timeout_ms (default: 0). Otherwise, it is
> passed as normal READ, so deadlock problem can be resolved.

Reviewed-by: Bart Van Assche <[email protected]>

2021-10-28 14:29:30

[permalink] [raw]

Subject: Re: [PATCH] scsi: ufs: Fix proper API to send HPB pre-request

On Thu, 2021-10-28 at 07:36 +0900, Daejun Park wrote:
> This patch addresses the issue of using the wrong API to create a
> pre_request for HPB READ.
> HPB READ candidate that require a pre-request will try to allocate a
> pre-request only during request_timeout_ms (default: 0). Otherwise,
> it is
> passed as normal READ, so deadlock problem can be resolved.
>
> Signed-off-by: Daejun Park <[email protected]>
> ---
> drivers/scsi/ufs/ufshpb.c | 11 +++++------
> drivers/scsi/ufs/ufshpb.h | 1 +
> 2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
> index 02fb51ae8b25..3117bd47d762 100644
> --- a/drivers/scsi/ufs/ufshpb.c
> +++ b/drivers/scsi/ufs/ufshpb.c
> @@ -548,8 +548,7 @@ static int ufshpb_execute_pre_req(struct
> ufshpb_lu *hpb, struct scsi_cmnd *cmd,
> read_id);
> rq->cmd_len = scsi_command_size(rq->cmd);
>
> - if (blk_insert_cloned_request(q, req) != BLK_STS_OK)
> - return -EAGAIN;
> + blk_execute_rq_nowait(NULL, req, true,
> ufshpb_pre_req_compl_fn);
>
> hpb->stats.pre_req_cnt++;
>
> @@ -2315,19 +2314,19 @@ struct attribute_group
> ufs_sysfs_hpb_param_group = {
> static int ufshpb_pre_req_mempool_init(struct ufshpb_lu *hpb)
> {
> struct ufshpb_req *pre_req = NULL, *t;
> - int qd = hpb->sdev_ufs_lu->queue_depth / 2;
> int i;
>
> INIT_LIST_HEAD(&hpb->lh_pre_req_free);
>
> - hpb->pre_req = kcalloc(qd, sizeof(struct ufshpb_req),
> GFP_KERNEL);
> - hpb->throttle_pre_req = qd;
> + hpb->pre_req = kcalloc(HPB_INFLIGHT_PRE_REQ, sizeof(struct
> ufshpb_req),
> + GFP_KERNEL);
> + hpb->throttle_pre_req = HPB_INFLIGHT_PRE_REQ;
> hpb->num_inflight_pre_req = 0;
>
> if (!hpb->pre_req)
> goto release_mem;
>
> - for (i = 0; i < qd; i++) {
> + for (i = 0; i < HPB_INFLIGHT_PRE_REQ; i++) {
> pre_req = hpb->pre_req + i;
> INIT_LIST_HEAD(&pre_req->list_req);
> pre_req->req = NULL;
> diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
> index a79e07398970..411a6d625f53 100644
> --- a/drivers/scsi/ufs/ufshpb.h
> +++ b/drivers/scsi/ufs/ufshpb.h
> @@ -50,6 +50,7 @@
> #define HPB_RESET_REQ_RETRIES 10
> #define HPB_MAP_REQ_RETRIES 5
> #define HPB_REQUEUE_TIME_MS 0
> +#define HPB_INFLIGHT_PRE_REQ 4

If the block people are happy with this, then I'm OK with it, but it
doesn't look like you've solved the fanout deadlock problem because
this new mechanism is still going to allocate a new tag.

James

2021-10-28 15:23:51

[permalink] [raw]

Subject: Re: [PATCH] scsi: ufs: Fix proper API to send HPB pre-request

+Jens, Christoph and linux-block

On Thu, 2021-10-28 at 07:36 +0900, Daejun Park wrote:
> This patch addresses the issue of using the wrong API to create a
> pre_request for HPB READ.
> HPB READ candidate that require a pre-request will try to allocate a
> pre-request only during request_timeout_ms (default: 0). Otherwise,
> it is
> passed as normal READ, so deadlock problem can be resolved.
>
> Signed-off-by: Daejun Park <[email protected]>
> ---
> drivers/scsi/ufs/ufshpb.c | 11 +++++------
> drivers/scsi/ufs/ufshpb.h | 1 +
> 2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
> index 02fb51ae8b25..3117bd47d762 100644
> --- a/drivers/scsi/ufs/ufshpb.c
> +++ b/drivers/scsi/ufs/ufshpb.c
> @@ -548,8 +548,7 @@ static int ufshpb_execute_pre_req(struct
> ufshpb_lu *hpb, struct scsi_cmnd *cmd,
> read_id);
> rq->cmd_len = scsi_command_size(rq->cmd);
>
> - if (blk_insert_cloned_request(q, req) != BLK_STS_OK)
> - return -EAGAIN;
> + blk_execute_rq_nowait(NULL, req, true,
> ufshpb_pre_req_compl_fn);
>
> hpb->stats.pre_req_cnt++;
>
> @@ -2315,19 +2314,19 @@ struct attribute_group
> ufs_sysfs_hpb_param_group = {
> static int ufshpb_pre_req_mempool_init(struct ufshpb_lu *hpb)
> {
> struct ufshpb_req *pre_req = NULL, *t;
> - int qd = hpb->sdev_ufs_lu->queue_depth / 2;
> int i;
>
> INIT_LIST_HEAD(&hpb->lh_pre_req_free);
>
> - hpb->pre_req = kcalloc(qd, sizeof(struct ufshpb_req),
> GFP_KERNEL);
> - hpb->throttle_pre_req = qd;
> + hpb->pre_req = kcalloc(HPB_INFLIGHT_PRE_REQ, sizeof(struct
> ufshpb_req),
> + GFP_KERNEL);
> + hpb->throttle_pre_req = HPB_INFLIGHT_PRE_REQ;
> hpb->num_inflight_pre_req = 0;
>
> if (!hpb->pre_req)
> goto release_mem;
>
> - for (i = 0; i < qd; i++) {
> + for (i = 0; i < HPB_INFLIGHT_PRE_REQ; i++) {
> pre_req = hpb->pre_req + i;
> INIT_LIST_HEAD(&pre_req->list_req);
> pre_req->req = NULL;
> diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
> index a79e07398970..411a6d625f53 100644
> --- a/drivers/scsi/ufs/ufshpb.h
> +++ b/drivers/scsi/ufs/ufshpb.h
> @@ -50,6 +50,7 @@
> #define HPB_RESET_REQ_RETRIES 10
> #define HPB_MAP_REQ_RETRIES 5
> #define HPB_REQUEUE_TIME_MS 0
> +#define HPB_INFLIGHT_PRE_REQ 4
>
> #define HPB_SUPPORT_VERSION 0x200
> #define HPB_SUPPORT_LEGACY_VERSION 0x100

2021-10-28 15:30:01

by Christoph Hellwig

[permalink] [raw]

Subject: Re: [PATCH] scsi: ufs: Fix proper API to send HPB pre-request

On Thu, Oct 28, 2021 at 10:28:01AM -0400, James Bottomley wrote:
> If the block people are happy with this, then I'm OK with it, but it
> doesn't look like you've solved the fanout deadlock problem because
> this new mechanism is still going to allocate a new tag.

Yes, same problem as before.

2021-10-28 16:00:54

[permalink] [raw]

Subject: Re: [PATCH] scsi: ufs: Fix proper API to send HPB pre-request

On 10/28/21 7:28 AM, James Bottomley wrote:
> If the block people are happy with this, then I'm OK with it, but it
> doesn't look like you've solved the fanout deadlock problem because
> this new mechanism is still going to allocate a new tag.

(+Jens, Christoph and linux-block)

Hi James,

My understanding is that the UFS HPB code makes ufshcd_queuecommand()
return SCSI_MLQUEUE_HOST_BUSY if the pool with pre-allocated requests is
exhausted. This will make the SCSI core reissue a SCSI command until
completion of another command has freed up one of the pre-allocated
requests. This is not the most efficient approach but should not trigger
a deadlock.

Thanks,

Bart.

2021-10-28 17:09:36

[permalink] [raw]

Subject: Re: [PATCH] scsi: ufs: Fix proper API to send HPB pre-request

On 10/28/21 8:27 AM, Christoph Hellwig wrote:
> On Thu, Oct 28, 2021 at 10:28:01AM -0400, James Bottomley wrote:
>> If the block people are happy with this, then I'm OK with it, but it
>> doesn't look like you've solved the fanout deadlock problem because
>> this new mechanism is still going to allocate a new tag.
>
> Yes, same problem as before.

Hi Christoph,

I spent some time looking around for other examples of allocating and
inserting a request from inside block layer callbacks. I only found one
such example, namely in the NVMe core. nvme_timeout() calls
nvme_alloc_request() and blk_execute_rq_nowait(). The difference between
what the UFS HPB code is doing and what nvme_timeout() does doesn't seem
that big to me. For clarity, I don't like the UFS HPB protocol nor how
support for that protocol has been implemented. However, I don't see how
the UFS HPB implementation would complicate maintenance of the block
layer core. Am I perhaps missing something?

Thanks,

Bart.

2021-10-28 17:12:58

by Christoph Hellwig

[permalink] [raw]

Subject: Re: [PATCH] scsi: ufs: Fix proper API to send HPB pre-request

On Thu, Oct 28, 2021 at 10:07:52AM -0700, Bart Van Assche wrote:
> I spent some time looking around for other examples of allocating and
> inserting a request from inside block layer callbacks. I only found one
> such example, namely in the NVMe core. nvme_timeout() calls
> nvme_alloc_request() and blk_execute_rq_nowait(). The difference between
> what the UFS HPB code is doing and what nvme_timeout() does doesn't seem
> that big to me.

The difference is that nvme_timeout allocates a request on the
admin queue, and only does so for commands on the I/O queues.

2021-10-28 19:15:17

[permalink] [raw]

Subject: Re: [PATCH] scsi: ufs: Fix proper API to send HPB pre-request

On Thu, 2021-10-28 at 08:59 -0700, Bart Van Assche wrote:
> On 10/28/21 7:28 AM, James Bottomley wrote:
> > If the block people are happy with this, then I'm OK with it, but
> > it
> > doesn't look like you've solved the fanout deadlock problem because
> > this new mechanism is still going to allocate a new tag.
>
> (+Jens, Christoph and linux-block)
>
> Hi James,
>
> My understanding is that the UFS HPB code makes ufshcd_queuecommand()
> return SCSI_MLQUEUE_HOST_BUSY if the pool with pre-allocated requests
> is exhausted. This will make the SCSI core reissue a SCSI command
> until completion of another command has freed up one of the pre-
> allocated requests. This is not the most efficient approach but
> should not trigger a deadlock.

I think the deadlock is triggered if the system is down to its last
reserved request on the memory clearing device and the next entry in
the queue for this device is one which does a fanout so we can't
service it with the single reserved request we have left for the
purposes of making forward progress. Sending it back doesn't help,
assuming this is the only memory clearing path, because retrying it
won't help ... we have to succeed with a request on this path to move
forward with clearing memory.

I think this problem could be solved by processing the WRITE BUFFER and
the request serially by hijacking the request sent down, but we can't
solve it if we try to allocate a new request.

James

2021-10-28 20:06:28

[permalink] [raw]

Subject: Re: [PATCH] scsi: ufs: Fix proper API to send HPB pre-request

On 10/28/21 12:12 PM, James Bottomley wrote:
> I think the deadlock is triggered if the system is down to its last
> reserved request on the memory clearing device and the next entry in
> the queue for this device is one which does a fanout so we can't
> service it with the single reserved request we have left for the
> purposes of making forward progress. Sending it back doesn't help,
> assuming this is the only memory clearing path, because retrying it
> won't help ... we have to succeed with a request on this path to move
> forward with clearing memory.
>
> I think this problem could be solved by processing the WRITE BUFFER and
> the request serially by hijacking the request sent down, but we can't
> solve it if we try to allocate a new request.

Hi James,

How about fixing the abuse of blk_insert_cloned_request() in the UFS HPB
before the v5.16 SCSI pull request is sent to Linus and implementing the
proposal from your email at a later time? I'm proposing to defer further
UFS HPB rework since the issue described above only affects UFS HPB users
and does not obstruct maintenance or refactoring of the block layer core.

Thanks,

Bart.

2021-10-28 20:14:36

[permalink] [raw]

Subject: Re: [PATCH] scsi: ufs: Fix proper API to send HPB pre-request

On Thu, 2021-10-28 at 13:04 -0700, Bart Van Assche wrote:
> On 10/28/21 12:12 PM, James Bottomley wrote:
> > I think the deadlock is triggered if the system is down to its last
> > reserved request on the memory clearing device and the next entry
> > in the queue for this device is one which does a fanout so we can't
> > service it with the single reserved request we have left for the
> > purposes of making forward progress. Sending it back doesn't help,
> > assuming this is the only memory clearing path, because retrying it
> > won't help ... we have to succeed with a request on this path to
> > move forward with clearing memory.
> >
> > I think this problem could be solved by processing the WRITE BUFFER
> > and the request serially by hijacking the request sent down, but we
> > can't solve it if we try to allocate a new request.
>
> Hi James,
>
> How about fixing the abuse of blk_insert_cloned_request() in the UFS
> HPB before the v5.16 SCSI pull request is sent to Linus and
> implementing the proposal from your email at a later time? I'm
> proposing to defer further UFS HPB rework since the issue described
> above only affects UFS HPB users and does not obstruct maintenance or
> refactoring of the block layer core.

Well, yes, I'm already on record as saying we need to do that and add
the functionality back compatibly in a later release. I think excising
the WRITE BUFFER path, which is simply an optimization and will affect
performance but not function, solves the above issue (and the clone API
problem as well) completely but I haven't heard the patch I proposed
has actually been tested yet.

James

2021-10-28 21:22:10

[permalink] [raw]

Subject: RE: [PATCH] scsi: ufs: Fix proper API to send HPB pre-request

Hi James,

> On Thu, 2021-10-28 at 08:59 -0700, Bart Van Assche wrote:
> > On 10/28/21 7:28 AM, James Bottomley wrote:
> > > If the block people are happy with this, then I'm OK with it, but
> > > it
> > > doesn't look like you've solved the fanout deadlock problem because
> > > this new mechanism is still going to allocate a new tag.
> >
> > (+Jens, Christoph and linux-block)
> >
> > Hi James,
> >
> > My understanding is that the UFS HPB code makes ufshcd_queuecommand()
> > return SCSI_MLQUEUE_HOST_BUSY if the pool with pre-allocated requests
> > is exhausted. This will make the SCSI core reissue a SCSI command
> > until completion of another command has freed up one of the pre-
> > allocated requests. This is not the most efficient approach but
> > should not trigger a deadlock.
>
> I think the deadlock is triggered if the system is down to its last
> reserved request on the memory clearing device and the next entry in
> the queue for this device is one which does a fanout so we can't
> service it with the single reserved request we have left for the
> purposes of making forward progress. Sending it back doesn't help,
> assuming this is the only memory clearing path, because retrying it
> won't help ... we have to succeed with a request on this path to move
> forward with clearing memory.
The above approach can retry several times (before the HPB timeout) but, it
gives up to allocate pre-request and it sends as just READ. So deadlock can
be avoided.

Thanks,
Daejun

2021-10-29 01:37:15

by Ming Lei

[permalink] [raw]

Subject: Re: [PATCH] scsi: ufs: Fix proper API to send HPB pre-request

On Thu, Oct 28, 2021 at 07:36:19AM +0900, Daejun Park wrote:
> This patch addresses the issue of using the wrong API to create a
> pre_request for HPB READ.
> HPB READ candidate that require a pre-request will try to allocate a
> pre-request only during request_timeout_ms (default: 0). Otherwise, it is

Can you explain about 'only during request_timeout_ms'?

From the following code in ufshpb_prep(), the pre-request is allocated
for each READ IO in case of (!ufshpb_is_legacy(hba) && ufshpb_is_required_wb(hpb,
transfer_len)).

if (!ufshpb_is_legacy(hba) &&
ufshpb_is_required_wb(hpb, transfer_len)) {
err = ufshpb_issue_pre_req(hpb, cmd, &read_id);

> passed as normal READ, so deadlock problem can be resolved.
>
> Signed-off-by: Daejun Park <[email protected]>
> ---
> drivers/scsi/ufs/ufshpb.c | 11 +++++------
> drivers/scsi/ufs/ufshpb.h | 1 +
> 2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
> index 02fb51ae8b25..3117bd47d762 100644
> --- a/drivers/scsi/ufs/ufshpb.c
> +++ b/drivers/scsi/ufs/ufshpb.c
> @@ -548,8 +548,7 @@ static int ufshpb_execute_pre_req(struct ufshpb_lu *hpb, struct scsi_cmnd *cmd,
> read_id);
> rq->cmd_len = scsi_command_size(rq->cmd);
>
> - if (blk_insert_cloned_request(q, req) != BLK_STS_OK)
> - return -EAGAIN;
> + blk_execute_rq_nowait(NULL, req, true, ufshpb_pre_req_compl_fn);

Be care with above change, blk_insert_cloned_request() allocates
driver tag and issues the request to LLD directly, then returns the
result. If anything fails in the code path, -EAGAIN is returned.

But blk_execute_rq_nowait() simply queued the request in block layer,
and run hw queue. It doesn't allocate driver tag, and doesn't issue it
to LLD.

So ufshpb_execute_pre_req() may think the pre-request is issued to LLD
successfully, but actually not, maybe never. What will happen after the
READ IO is issued to device, but the pre-request(write buffer) isn't
sent to device?

>
> hpb->stats.pre_req_cnt++;
>
> @@ -2315,19 +2314,19 @@ struct attribute_group ufs_sysfs_hpb_param_group = {
> static int ufshpb_pre_req_mempool_init(struct ufshpb_lu *hpb)
> {
> struct ufshpb_req *pre_req = NULL, *t;
> - int qd = hpb->sdev_ufs_lu->queue_depth / 2;
> int i;
>
> INIT_LIST_HEAD(&hpb->lh_pre_req_free);
>
> - hpb->pre_req = kcalloc(qd, sizeof(struct ufshpb_req), GFP_KERNEL);
> - hpb->throttle_pre_req = qd;
> + hpb->pre_req = kcalloc(HPB_INFLIGHT_PRE_REQ, sizeof(struct ufshpb_req),
> + GFP_KERNEL);
> + hpb->throttle_pre_req = HPB_INFLIGHT_PRE_REQ;
> hpb->num_inflight_pre_req = 0;
>
> if (!hpb->pre_req)
> goto release_mem;
>
> - for (i = 0; i < qd; i++) {
> + for (i = 0; i < HPB_INFLIGHT_PRE_REQ; i++) {
> pre_req = hpb->pre_req + i;
> INIT_LIST_HEAD(&pre_req->list_req);
> pre_req->req = NULL;
> diff --git a/drivers/scsi/ufs/ufshpb.h b/drivers/scsi/ufs/ufshpb.h
> index a79e07398970..411a6d625f53 100644
> --- a/drivers/scsi/ufs/ufshpb.h
> +++ b/drivers/scsi/ufs/ufshpb.h
> @@ -50,6 +50,7 @@
> #define HPB_RESET_REQ_RETRIES 10
> #define HPB_MAP_REQ_RETRIES 5
> #define HPB_REQUEUE_TIME_MS 0
> +#define HPB_INFLIGHT_PRE_REQ 4

Can you explain how this change solves the deadlock?

Thanks,
Ming

2021-10-29 01:52:10

[permalink] [raw]

Subject: RE: [PATCH] scsi: ufs: Fix proper API to send HPB pre-request

> On Thu, Oct 28, 2021 at 07:36:19AM +0900, Daejun Park wrote:
> > This patch addresses the issue of using the wrong API to create a
> > pre_request for HPB READ.
> > HPB READ candidate that require a pre-request will try to allocate a
> > pre-request only during request_timeout_ms (default: 0). Otherwise, it is
>
> Can you explain about 'only during request_timeout_ms'?
>
> From the following code in ufshpb_prep(), the pre-request is allocated
> for each READ IO in case of (!ufshpb_is_legacy(hba) && ufshpb_is_required_wb(hpb,
> transfer_len)).
>
> if (!ufshpb_is_legacy(hba) &&
> ufshpb_is_required_wb(hpb, transfer_len)) {
> err = ufshpb_issue_pre_req(hpb, cmd, &read_id);
>
> > passed as normal READ, so deadlock problem can be resolved.
> >
> > Signed-off-by: Daejun Park <[email protected]>
> > ---
> > drivers/scsi/ufs/ufshpb.c | 11 +++++------
> > drivers/scsi/ufs/ufshpb.h | 1 +
> > 2 files changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
> > index 02fb51ae8b25..3117bd47d762 100644
> > --- a/drivers/scsi/ufs/ufshpb.c
> > +++ b/drivers/scsi/ufs/ufshpb.c
> > @@ -548,8 +548,7 @@ static int ufshpb_execute_pre_req(struct ufshpb_lu *hpb, struct scsi_cmnd *cmd,
> > read_id);
> > rq->cmd_len = scsi_command_size(rq->cmd);
> >
> > - if (blk_insert_cloned_request(q, req) != BLK_STS_OK)
> > - return -EAGAIN;
> > + blk_execute_rq_nowait(NULL, req, true, ufshpb_pre_req_compl_fn);
>
> Be care with above change, blk_insert_cloned_request() allocates
> driver tag and issues the request to LLD directly, then returns the
> result. If anything fails in the code path, -EAGAIN is returned.
>
> But blk_execute_rq_nowait() simply queued the request in block layer,
> and run hw queue. It doesn't allocate driver tag, and doesn't issue it
> to LLD.
>
> So ufshpb_execute_pre_req() may think the pre-request is issued to LLD
> successfully, but actually not, maybe never. What will happen after the
> READ IO is issued to device, but the pre-request(write buffer) isn't
> sent to device?

In that case, the HPB READ cannot get benefit from pre-request. But it is not
common case.

> Can you explain how this change solves the deadlock?

The deadlock is happen when the READ waiting allocation of pre-request. But
the timeout code makes to stop waiting after given time later.

Thanks,
Daejun

2021-10-29 02:13:43

by Ming Lei

[permalink] [raw]

Subject: Re: [PATCH] scsi: ufs: Fix proper API to send HPB pre-request

On Fri, Oct 29, 2021 at 10:50:15AM +0900, Daejun Park wrote:
> > On Thu, Oct 28, 2021 at 07:36:19AM +0900, Daejun Park wrote:
> > > This patch addresses the issue of using the wrong API to create a
> > > pre_request for HPB READ.
> > > HPB READ candidate that require a pre-request will try to allocate a
> > > pre-request only during request_timeout_ms (default: 0). Otherwise, it is
> >
> > Can you explain about 'only during request_timeout_ms'?
> >
> > From the following code in ufshpb_prep(), the pre-request is allocated
> > for each READ IO in case of (!ufshpb_is_legacy(hba) && ufshpb_is_required_wb(hpb,
> > transfer_len)).
> >
> > if (!ufshpb_is_legacy(hba) &&
> > ufshpb_is_required_wb(hpb, transfer_len)) {
> > err = ufshpb_issue_pre_req(hpb, cmd, &read_id);
> >
> > > passed as normal READ, so deadlock problem can be resolved.
> > >
> > > Signed-off-by: Daejun Park <[email protected]>
> > > ---
> > > drivers/scsi/ufs/ufshpb.c | 11 +++++------
> > > drivers/scsi/ufs/ufshpb.h | 1 +
> > > 2 files changed, 6 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
> > > index 02fb51ae8b25..3117bd47d762 100644
> > > --- a/drivers/scsi/ufs/ufshpb.c
> > > +++ b/drivers/scsi/ufs/ufshpb.c
> > > @@ -548,8 +548,7 @@ static int ufshpb_execute_pre_req(struct ufshpb_lu *hpb, struct scsi_cmnd *cmd,
> > > read_id);
> > > rq->cmd_len = scsi_command_size(rq->cmd);
> > >
> > > - if (blk_insert_cloned_request(q, req) != BLK_STS_OK)
> > > - return -EAGAIN;
> > > + blk_execute_rq_nowait(NULL, req, true, ufshpb_pre_req_compl_fn);
> >
> > Be care with above change, blk_insert_cloned_request() allocates
> > driver tag and issues the request to LLD directly, then returns the
> > result. If anything fails in the code path, -EAGAIN is returned.
> >
> > But blk_execute_rq_nowait() simply queued the request in block layer,
> > and run hw queue. It doesn't allocate driver tag, and doesn't issue it
> > to LLD.
> >
> > So ufshpb_execute_pre_req() may think the pre-request is issued to LLD
> > successfully, but actually not, maybe never. What will happen after the
> > READ IO is issued to device, but the pre-request(write buffer) isn't
> > sent to device?
>
> In that case, the HPB READ cannot get benefit from pre-request. But it is not
> common case.

OK, so the device will ignore the pre-request if it isn't received in
time, not sure it is common or not, since blk_execute_rq_nowait()
doesn't provide any feedback. Here looks blk_insert_cloned_request()
is better.

>
> > Can you explain how this change solves the deadlock?
>
> The deadlock is happen when the READ waiting allocation of pre-request. But
> the timeout code makes to stop waiting after given time later.

If you mean blk-mq timeout code will be triggered, I think it won't.
Meantime, LLD may see nothing to timeout too.

For example, in case of none io sched, the queue depth is 128, and 128 READ
IOs are sent to ufshcd_queuecommand() at the same time, and all these 128 IOs
require to allocate pre-request, but no one can move on because 128 tags are
used up.

So no request can be sent to device and BLK_STS_RESOURCE is always returned
from ufshcd_queuecommand(), also when blk-mq timeout is triggered, all in-flight
request's state may just be updated as IDLE, so blk-mq may find nothing to expire.

In case of real io scheduler, request->tag is released when BLK_STS_RESOURCE
is returned from ufshcd_queuecommand(), but it doesn't mean that
pre-request can get tag always.

The approach[1] of reserving one slot for pre-request should provide
forward progress guarantee.

[1] https://lore.kernel.org/linux-block/YXoF59XeZ5KS0jZj@T590/

Thanks,
Ming

2021-10-29 02:52:04

[permalink] [raw]

Subject: RE: [PATCH] scsi: ufs: Fix proper API to send HPB pre-request

> On Fri, Oct 29, 2021 at 10:50:15AM +0900, Daejun Park wrote:
> > > On Thu, Oct 28, 2021 at 07:36:19AM +0900, Daejun Park wrote:
> > > > This patch addresses the issue of using the wrong API to create a
> > > > pre_request for HPB READ.
> > > > HPB READ candidate that require a pre-request will try to allocate a
> > > > pre-request only during request_timeout_ms (default: 0). Otherwise, it is
> > >
> > > Can you explain about 'only during request_timeout_ms'?
> > >
> > > From the following code in ufshpb_prep(), the pre-request is allocated
> > > for each READ IO in case of (!ufshpb_is_legacy(hba) && ufshpb_is_required_wb(hpb,
> > > transfer_len)).
> > >
> > > if (!ufshpb_is_legacy(hba) &&
> > > ufshpb_is_required_wb(hpb, transfer_len)) {
> > > err = ufshpb_issue_pre_req(hpb, cmd, &read_id);
> > >
> > > > passed as normal READ, so deadlock problem can be resolved.
> > > >
> > > > Signed-off-by: Daejun Park <[email protected]>
> > > > ---
> > > > drivers/scsi/ufs/ufshpb.c | 11 +++++------
> > > > drivers/scsi/ufs/ufshpb.h | 1 +
> > > > 2 files changed, 6 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
> > > > index 02fb51ae8b25..3117bd47d762 100644
> > > > --- a/drivers/scsi/ufs/ufshpb.c
> > > > +++ b/drivers/scsi/ufs/ufshpb.c
> > > > @@ -548,8 +548,7 @@ static int ufshpb_execute_pre_req(struct ufshpb_lu *hpb, struct scsi_cmnd *cmd,
> > > > read_id);
> > > > rq->cmd_len = scsi_command_size(rq->cmd);
> > > >
> > > > - if (blk_insert_cloned_request(q, req) != BLK_STS_OK)
> > > > - return -EAGAIN;
> > > > + blk_execute_rq_nowait(NULL, req, true, ufshpb_pre_req_compl_fn);
> > >
> > > Be care with above change, blk_insert_cloned_request() allocates
> > > driver tag and issues the request to LLD directly, then returns the
> > > result. If anything fails in the code path, -EAGAIN is returned.
> > >
> > > But blk_execute_rq_nowait() simply queued the request in block layer,
> > > and run hw queue. It doesn't allocate driver tag, and doesn't issue it
> > > to LLD.
> > >
> > > So ufshpb_execute_pre_req() may think the pre-request is issued to LLD
> > > successfully, but actually not, maybe never. What will happen after the
> > > READ IO is issued to device, but the pre-request(write buffer) isn't
> > > sent to device?
> >
> > In that case, the HPB READ cannot get benefit from pre-request. But it is not
> > common case.
>
> OK, so the device will ignore the pre-request if it isn't received in
> time, not sure it is common or not, since blk_execute_rq_nowait()
> doesn't provide any feedback. Here looks blk_insert_cloned_request()
> is better.

Yor're right.

> >
> > > Can you explain how this change solves the deadlock?
> >
> > The deadlock is happen when the READ waiting allocation of pre-request. But
> > the timeout code makes to stop waiting after given time later.
>
> If you mean blk-mq timeout code will be triggered, I think it won't.
> Meantime, LLD may see nothing to timeout too.

I mean timeout of the HPB code. Please refer following code:

if (!ufshpb_is_legacy(hba) &&
ufshpb_is_required_wb(hpb, transfer_len)) {
err = ufshpb_issue_pre_req(hpb, cmd, &read_id);
if (err) {
unsigned long timeout;

timeout = cmd->jiffies_at_alloc + msecs_to_jiffies(
hpb->params.requeue_timeout_ms);

if (time_before(jiffies, timeout))
return -EAGAIN;

hpb->stats.miss_cnt++;
return 0;
}
}

Although the return value of ufshpb_issue_pre_req() is -EAGAIN, the code
ignores the return value and issues READ not HPB READ.

Thanks,
Daejun

2021-10-29 03:17:46

by Ming Lei

[permalink] [raw]

Subject: Re: [PATCH] scsi: ufs: Fix proper API to send HPB pre-request

On Fri, Oct 29, 2021 at 11:50:12AM +0900, Daejun Park wrote:
> > On Fri, Oct 29, 2021 at 10:50:15AM +0900, Daejun Park wrote:
> > > > On Thu, Oct 28, 2021 at 07:36:19AM +0900, Daejun Park wrote:
> > > > > This patch addresses the issue of using the wrong API to create a
> > > > > pre_request for HPB READ.
> > > > > HPB READ candidate that require a pre-request will try to allocate a
> > > > > pre-request only during request_timeout_ms (default: 0). Otherwise, it is
> > > >
> > > > Can you explain about 'only during request_timeout_ms'?
> > > >
> > > > From the following code in ufshpb_prep(), the pre-request is allocated
> > > > for each READ IO in case of (!ufshpb_is_legacy(hba) && ufshpb_is_required_wb(hpb,
> > > > transfer_len)).
> > > >
> > > > if (!ufshpb_is_legacy(hba) &&
> > > > ufshpb_is_required_wb(hpb, transfer_len)) {
> > > > err = ufshpb_issue_pre_req(hpb, cmd, &read_id);
> > > >
> > > > > passed as normal READ, so deadlock problem can be resolved.
> > > > >
> > > > > Signed-off-by: Daejun Park <[email protected]>
> > > > > ---
> > > > > drivers/scsi/ufs/ufshpb.c | 11 +++++------
> > > > > drivers/scsi/ufs/ufshpb.h | 1 +
> > > > > 2 files changed, 6 insertions(+), 6 deletions(-)
> > > > >
> > > > > diff --git a/drivers/scsi/ufs/ufshpb.c b/drivers/scsi/ufs/ufshpb.c
> > > > > index 02fb51ae8b25..3117bd47d762 100644
> > > > > --- a/drivers/scsi/ufs/ufshpb.c
> > > > > +++ b/drivers/scsi/ufs/ufshpb.c
> > > > > @@ -548,8 +548,7 @@ static int ufshpb_execute_pre_req(struct ufshpb_lu *hpb, struct scsi_cmnd *cmd,
> > > > > read_id);
> > > > > rq->cmd_len = scsi_command_size(rq->cmd);
> > > > >
> > > > > - if (blk_insert_cloned_request(q, req) != BLK_STS_OK)
> > > > > - return -EAGAIN;
> > > > > + blk_execute_rq_nowait(NULL, req, true, ufshpb_pre_req_compl_fn);
> > > >
> > > > Be care with above change, blk_insert_cloned_request() allocates
> > > > driver tag and issues the request to LLD directly, then returns the
> > > > result. If anything fails in the code path, -EAGAIN is returned.
> > > >
> > > > But blk_execute_rq_nowait() simply queued the request in block layer,
> > > > and run hw queue. It doesn't allocate driver tag, and doesn't issue it
> > > > to LLD.
> > > >
> > > > So ufshpb_execute_pre_req() may think the pre-request is issued to LLD
> > > > successfully, but actually not, maybe never. What will happen after the
> > > > READ IO is issued to device, but the pre-request(write buffer) isn't
> > > > sent to device?
> > >
> > > In that case, the HPB READ cannot get benefit from pre-request. But it is not
> > > common case.
> >
> > OK, so the device will ignore the pre-request if it isn't received in
> > time, not sure it is common or not, since blk_execute_rq_nowait()
> > doesn't provide any feedback. Here looks blk_insert_cloned_request()
> > is better.
>
> Yor're right.
>
> > >
> > > > Can you explain how this change solves the deadlock?
> > >
> > > The deadlock is happen when the READ waiting allocation of pre-request. But
> > > the timeout code makes to stop waiting after given time later.
> >
> > If you mean blk-mq timeout code will be triggered, I think it won't.
> > Meantime, LLD may see nothing to timeout too.
>
> I mean timeout of the HPB code. Please refer following code:
>
> if (!ufshpb_is_legacy(hba) &&
> ufshpb_is_required_wb(hpb, transfer_len)) {
> err = ufshpb_issue_pre_req(hpb, cmd, &read_id);
> if (err) {
> unsigned long timeout;
>
> timeout = cmd->jiffies_at_alloc + msecs_to_jiffies(
> hpb->params.requeue_timeout_ms);
>
> if (time_before(jiffies, timeout))
> return -EAGAIN;
>
> hpb->stats.miss_cnt++;
> return 0;
> }
> }
>
> Although the return value of ufshpb_issue_pre_req() is -EAGAIN, the code
> ignores the return value and issues READ not HPB READ.

OK, got it, this way should avoid the deadlock. But just be curious why
you change hpb->throttle_pre_req to 4, seems it isn't necessary for
avoiding the deadlock?

Thanks,
Ming

2021-10-29 04:51:05