[PATCH V2] scsi: core: use blk_mq_requeue_request in __scsi_queue_insert

[PATCH V2] scsi: core: use blk_mq_requeue_request in __scsi_queue_insert

[Jianchao Wang]

In scsi core, __scsi_queue_insert should just put request back on
the queue and retry using the same command as before. However, for
blk-mq, scsi_mq_requeue_cmd is employed here which will unprepare
the request. To align with the semantics of __scsi_queue_insert,
use blk_mq_requeue_request with kick_requeue_list == true and put
the reference of scsi_device.

V1 -> V2:
 - add put_device on scsi_device->sdev_gendev

Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Jianchao Wang <jianchao.w.wang@oracle.com>
---
 drivers/scsi/scsi_lib.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index a86df9c..6fa7b0c 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -191,7 +191,8 @@ static void __scsi_queue_insert(struct scsi_cmnd *cmd, int reason, bool unbusy)
 	 */
 	cmd->result = 0;
 	if (q->mq_ops) {
-		scsi_mq_requeue_cmd(cmd);
+		blk_mq_requeue_request(cmd->request, true);
+		put_device(&device->sdev_gendev);
 		return;
 	}
 	spin_lock_irqsave(q->queue_lock, flags);
-- 
2.7.4

Re: [PATCH V2] scsi: core: use blk_mq_requeue_request in __scsi_queue_insert

[Bart Van Assche]

On Wed, 2018-02-28 at 16:55 +0800, Jianchao Wang wrote:
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index a86df9c..6fa7b0c 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -191,7 +191,8 @@ static void __scsi_queue_insert(struct scsi_cmnd *cmd, int reason, bool unbusy)
>  	 */
>  	cmd->result = 0;
>  	if (q->mq_ops) {
> -		scsi_mq_requeue_cmd(cmd);
> +		blk_mq_requeue_request(cmd->request, true);
> +		put_device(&device->sdev_gendev);
>  		return;
>  	}
>  	spin_lock_irqsave(q->queue_lock, flags);

Anyone who sees the put_device() call that follows the blk_mq_requeue_request()
call will wonder why that call occurs there. So I think we need a comment above
that call that explains where the matching get_device() call is.

For the legacy code path, there is a get_device() call in scsi_prep_fn() but no
put_device() call in scsi_unprep_fn() - the matching put_device() calls occur in
scsi_end_request() and after blk_requeue_request().

For scsi-mq however there is a get_device() call in scsi_mq_get_budget() and a
put_device() call in scsi_mq_put_budget(). So why do we need the put_device()
calls after blk_mq_requeue_request() and in the mq path for scsi_end_request()?

Thanks,

Bart.

Re: [PATCH V2] scsi: core: use blk_mq_requeue_request in __scsi_queue_insert

[jianchao.wang]

Hi Bart

Thanks for your precious time to review this and kindly detailed response.

On 03/01/2018 01:52 AM, Bart Van Assche wrote:
> On Wed, 2018-02-28 at 16:55 +0800, Jianchao Wang wrote:
>> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
>> index a86df9c..6fa7b0c 100644
>> --- a/drivers/scsi/scsi_lib.c
>> +++ b/drivers/scsi/scsi_lib.c
>> @@ -191,7 +191,8 @@ static void __scsi_queue_insert(struct scsi_cmnd *cmd, int reason, bool unbusy)
>>  	 */
>>  	cmd->result = 0;
>>  	if (q->mq_ops) {
>> -		scsi_mq_requeue_cmd(cmd);
>> +		blk_mq_requeue_request(cmd->request, true);
>> +		put_device(&device->sdev_gendev);
>>  		return;
>>  	}
>>  	spin_lock_irqsave(q->queue_lock, flags);
> 
> Anyone who sees the put_device() call that follows the blk_mq_requeue_request()
> call will wonder why that call occurs there. So I think we need a comment above
> that call that explains where the matching get_device() call is.

Yes, I will add this.

> For the legacy code path, there is a get_device() call in scsi_prep_fn() but no
> put_device() call in scsi_unprep_fn() - the matching put_device() calls occur in
> scsi_end_request() and after blk_requeue_request().
> 
> For scsi-mq however there is a get_device() call in scsi_mq_get_budget() and a
> put_device() call in scsi_mq_put_budget(). So why do we need the put_device()
> calls after blk_mq_requeue_request() and in the mq path for scsi_end_request()?
> 

>From the source code, we know the scsi_mq_get_budget will be invoked every time when we issue a request.
But scsi_mq_put_budget is just in the fail path.

scsi_queue_rq // if any error
  -> scsi_mq_put_budget

blk_mq_dispatch_rq_list // if no driver tags
  -> blk_mq_put_dispatch_budget
    -> scsi_mq_put_budget
blk_mq_do_dispatch_sched/blk_mq_do_dispatch_ctx // if no requests
  -> blk_mq_put_dispatch_budget
    -> scsi_mq_put_budget

So we have to add put_device after  blk_mq_requeue_request() and in scsi_end_request() to match the
scsi_mq_get_budget.

Thanks
Jianchao

Re: [PATCH V2] scsi: core: use blk_mq_requeue_request in __scsi_queue_insert

[Bart Van Assche]

On Thu, 2018-03-01 at 09:57 +0800, jianchao.wang wrote:
> On 03/01/2018 01:52 AM, Bart Van Assche wrote:
> > On Wed, 2018-02-28 at 16:55 +0800, Jianchao Wang wrote:
> > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> > > index a86df9c..6fa7b0c 100644
> > > --- a/drivers/scsi/scsi_lib.c
> > > +++ b/drivers/scsi/scsi_lib.c
> > > @@ -191,7 +191,8 @@ static void __scsi_queue_insert(struct scsi_cmnd *cmd, int reason, bool unbusy)
> > >  	 */
> > >  	cmd->result = 0;
> > >  	if (q->mq_ops) {
> > > -		scsi_mq_requeue_cmd(cmd);
> > > +		blk_mq_requeue_request(cmd->request, true);
> > > +		put_device(&device->sdev_gendev);
> > >  		return;
> > >  	}
> > >  	spin_lock_irqsave(q->queue_lock, flags);
> > 
> > Anyone who sees the put_device() call that follows the blk_mq_requeue_request()
> > call will wonder why that call occurs there. So I think we need a comment above
> > that call that explains where the matching get_device() call is.
> 
> Yes, I will add this.
> 
> > For the legacy code path, there is a get_device() call in scsi_prep_fn() but no
> > put_device() call in scsi_unprep_fn() - the matching put_device() calls occur in
> > scsi_end_request() and after blk_requeue_request().
> > 
> > For scsi-mq however there is a get_device() call in scsi_mq_get_budget() and a
> > put_device() call in scsi_mq_put_budget(). So why do we need the put_device()
> > calls after blk_mq_requeue_request() and in the mq path for scsi_end_request()?
> > 
> 
> From the source code, we know the scsi_mq_get_budget will be invoked every time
> when we issue a request. But scsi_mq_put_budget is just in the fail path.
> 
> scsi_queue_rq // if any error
>   -> scsi_mq_put_budget
> 
> blk_mq_dispatch_rq_list // if no driver tags
>   -> blk_mq_put_dispatch_budget
>     -> scsi_mq_put_budget
> blk_mq_do_dispatch_sched/blk_mq_do_dispatch_ctx // if no requests
>   -> blk_mq_put_dispatch_budget
>     -> scsi_mq_put_budget
> 
> So we have to add put_device after blk_mq_requeue_request() and in
> scsi_end_request() to match the scsi_mq_get_budget.

Hello Jianchao,

Yes, the block layer core guarantees that scsi_mq_get_budget() will be called
before scsi_queue_rq(). I think the full picture is as follows:
* Before scsi_queue_rq() calls .queuecommand(), get_device() is called for the
  SCSI device and the device, target and host busy counters are incremented.
* If the SCSI core decides to requeue a command, scsi_queue_insert() causes
  __scsi_queue_insert() to call scsi_device_unbusy(). That last function
  decreases the device, target and host busy counters but not put_device(sdev).
  Hence the need for a separate put_device() call after requeuing.

It's unfortunate that the SCSI core became so asymmetric. Anyway, since I am
now convinced that this patch is correct, feel free to add:

Reviewed-by: Bart Van Assche <bart.vanassche@wdc.com>

Re: [PATCH V2] scsi: core: use blk_mq_requeue_request in __scsi_queue_insert

[Martin K. Petersen]

Jianchao,

> Yes, the block layer core guarantees that scsi_mq_get_budget() will be
> called before scsi_queue_rq(). I think the full picture is as follows:

> * Before scsi_queue_rq() calls .queuecommand(), get_device() is called for the
>   SCSI device and the device, target and host busy counters are incremented.
> * If the SCSI core decides to requeue a command, scsi_queue_insert() causes
>   __scsi_queue_insert() to call scsi_device_unbusy(). That last function
>   decreases the device, target and host busy counters but not put_device(sdev).
>   Hence the need for a separate put_device() call after requeuing.
>
> It's unfortunate that the SCSI core became so asymmetric. Anyway,
> since I am now convinced that this patch is correct, feel free to add:

Please add something akin to Bart's explanation as a comment and repost.

Thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH V2] scsi: core: use blk_mq_requeue_request in __scsi_queue_insert

[Martin K. Petersen]

Jianchao,

> Yes, the block layer core guarantees that scsi_mq_get_budget() will be
> called before scsi_queue_rq(). I think the full picture is as follows:

> * Before scsi_queue_rq() calls .queuecommand(), get_device() is called for the
>   SCSI device and the device, target and host busy counters are incremented.
> * If the SCSI core decides to requeue a command, scsi_queue_insert() causes
>   __scsi_queue_insert() to call scsi_device_unbusy(). That last function
>   decreases the device, target and host busy counters but not put_device(sdev).
>   Hence the need for a separate put_device() call after requeuing.
>
> It's unfortunate that the SCSI core became so asymmetric. Anyway,
> since I am now convinced that this patch is correct, feel free to add:

Please add something akin to Bart's explanation as a comment and repost.

Thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH V2] scsi: core: use blk_mq_requeue_request in __scsi_queue_insert

[Martin K. Petersen]

Jianchao,

> In scsi core, __scsi_queue_insert should just put request back on the
> queue and retry using the same command as before. However, for blk-mq,
> scsi_mq_requeue_cmd is employed here which will unprepare the
> request. To align with the semantics of __scsi_queue_insert, use
> blk_mq_requeue_request with kick_requeue_list == true and put the
> reference of scsi_device.
>
> V1 -> V2:
>  - add put_device on scsi_device->sdev_gendev

Also, please put changelog after the --- delimiter.

> Cc: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Jianchao Wang <jianchao.w.wang@oracle.com>
> ---
>  drivers/scsi/scsi_lib.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index a86df9c..6fa7b0c 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -191,7 +191,8 @@ static void __scsi_queue_insert(struct scsi_cmnd *cmd, int reason, bool unbusy)
>  	 */
>  	cmd->result = 0;
>  	if (q->mq_ops) {
> -		scsi_mq_requeue_cmd(cmd);
> +		blk_mq_requeue_request(cmd->request, true);
> +		put_device(&device->sdev_gendev);
>  		return;
>  	}
>  	spin_lock_irqsave(q->queue_lock, flags);

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH V2] scsi: core: use blk_mq_requeue_request in __scsi_queue_insert

[jianchao.wang]

Hi Bart

Thanks for your precious time and detailed summary.

On 03/02/2018 01:43 AM, Bart Van Assche wrote:
> Yes, the block layer core guarantees that scsi_mq_get_budget() will be called
> before scsi_queue_rq(). I think the full picture is as follows:
> * Before scsi_queue_rq() calls .queuecommand(), get_device() is called for the
>   SCSI device and the device, target and host busy counters are incremented.

Supply some details here:
scsi_mq_get_budget before calling .queuecommand get_device and increase device_busy.
scsi_queue_rq increases target_busy and host_busy.

> * If the SCSI core decides to requeue a command, scsi_queue_insert() causes
>   __scsi_queue_insert() to call scsi_device_unbusy(). That last function
>   decreases the device, target and host busy counters but not put_device(sdev).
>   Hence the need for a separate put_device() call after requeuing.
> 
> It's unfortunate that the SCSI core became so asymmetric. Anyway, since I am
> now convinced that this patch is correct, feel free to add:
> 
> Reviewed-by: Bart Van Assche <bart.vanassche@wdc.com>


Sincerely
Jianchao

Re: [PATCH V2] scsi: core: use blk_mq_requeue_request in __scsi_queue_insert

[jianchao.wang]

Hi martin

Thanks for your kindly response.

On 03/02/2018 09:43 AM, Martin K. Petersen wrote:
> 
> Jianchao,
> 
>> Yes, the block layer core guarantees that scsi_mq_get_budget() will be
>> called before scsi_queue_rq(). I think the full picture is as follows:
> 
>> * Before scsi_queue_rq() calls .queuecommand(), get_device() is called for the
>>   SCSI device and the device, target and host busy counters are incremented.
>> * If the SCSI core decides to requeue a command, scsi_queue_insert() causes
>>   __scsi_queue_insert() to call scsi_device_unbusy(). That last function
>>   decreases the device, target and host busy counters but not put_device(sdev).
>>   Hence the need for a separate put_device() call after requeuing.
>>
>> It's unfortunate that the SCSI core became so asymmetric. Anyway,
>> since I am now convinced that this patch is correct, feel free to add:
> 
> Please add something akin to Bart's explanation as a comment and repost.

yes, sure.


Thanks
Jianchao

Re: [PATCH V2] scsi: core: use blk_mq_requeue_request in __scsi_queue_insert

[jianchao.wang]

Hi martin

On 03/02/2018 09:44 AM, Martin K. Petersen wrote:
>> In scsi core, __scsi_queue_insert should just put request back on the
>> queue and retry using the same command as before. However, for blk-mq,
>> scsi_mq_requeue_cmd is employed here which will unprepare the
>> request. To align with the semantics of __scsi_queue_insert, use
>> blk_mq_requeue_request with kick_requeue_list == true and put the
>> reference of scsi_device.
>>
>> V1 -> V2:
>>  - add put_device on scsi_device->sdev_gendev
> Also, please put changelog after the --- delimiter.
> 

Yes, I will modify this next version.

Thanks 
Jianchao