Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <0f9229c3c4c7859524411a47db96a3b53ac89c90.camel@linux.ibm.com>
Subject: Re: [PATCH] scsi: ufs: Fix proper API to send HPB pre-request
From:   James Bottomley <jejb@linux.ibm.com>
Reply-To: jejb@linux.ibm.com
To:     daejun7.park@samsung.com, ALIM AKHTAR <alim.akhtar@samsung.com>,
        "avri.altman@wdc.com" <avri.altman@wdc.com>,
        "martin.petersen@oracle.com" <martin.petersen@oracle.com>,
        "huobean@gmail.com" <huobean@gmail.com>,
        "bvanassche@acm.org" <bvanassche@acm.org>,
        Keoseong Park <keosung.park@samsung.com>
Cc:     "linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Date:   Thu, 28 Oct 2021 10:28:01 -0400
In-Reply-To: <20211027223619epcms2p60bbc74c9ba9757c58709a99acd0892ff@epcms2p6>
References: <CGME20211027223619epcms2p60bbc74c9ba9757c58709a99acd0892ff@epcms2p6>
         <20211027223619epcms2p60bbc74c9ba9757c58709a99acd0892ff@epcms2p6>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.34.4 
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
Precedence: bulk

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 <daejun7.park@samsung.com>
> ---
>  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