diff mbox series

[RFC,v3,04/22] scsi: core: Add support to send reserved commands

Message ID 1666693096-180008-5-git-send-email-john.garry@huawei.com
State New
Headers
Series blk-mq/libata/scsi: SCSI driver tagging improvements Part I |

Commit Message

John Garry Oct. 25, 2022, 10:17 a.m. UTC 
  Add a method to queue reserved commands.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/hosts.c     |  6 ++++++
 drivers/scsi/scsi_lib.c  | 25 +++++++++++++++++++++++++
 include/scsi/scsi_host.h |  1 +
 3 files changed, 32 insertions(+)

Comments

Damien Le Moal Oct. 27, 2022, 1:21 a.m. UTC | #1 
On 10/25/22 19:17, John Garry wrote:
> Add a method to queue reserved commands.
> 
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
>  drivers/scsi/hosts.c     |  6 ++++++
>  drivers/scsi/scsi_lib.c  | 25 +++++++++++++++++++++++++
>  include/scsi/scsi_host.h |  1 +
>  3 files changed, 32 insertions(+)
> 
> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> index db89afc37bc9..78968553089f 100644
> --- a/drivers/scsi/hosts.c
> +++ b/drivers/scsi/hosts.c
> @@ -230,6 +230,12 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
>  		goto fail;
>  	}
>  
> +	if (shost->nr_reserved_cmds && !sht->reserved_queuecommand) {
> +		shost_printk(KERN_ERR, shost,
> +			"nr_reserved_cmds set but no method to queue\n");
> +		goto fail;
> +	}
> +
>  	/* Use min_t(int, ...) in case shost->can_queue exceeds SHRT_MAX */
>  	shost->cmd_per_lun = min_t(int, shost->cmd_per_lun,
>  				   shost->can_queue);
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index a8c4e7c037ae..08015c42c326 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1428,6 +1428,16 @@ static void scsi_complete(struct request *rq)
>  	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(rq);
>  	enum scsi_disposition disposition;
>  
> +	if (blk_mq_is_reserved_rq(rq)) {
> +		struct scsi_device *sdev = cmd->device;

This variable is not really needed. You can call:
		
		scsi_device_unbusy(cmd->device, cmd);

No ?

> +
> +		scsi_mq_uninit_cmd(cmd);
> +		scsi_device_unbusy(sdev, cmd);
> +		__blk_mq_end_request(rq, 0);
> +
> +		return;
> +	}
> +
>  	INIT_LIST_HEAD(&cmd->eh_entry);
>  
>  	atomic_inc(&cmd->device->iodone_cnt);
> @@ -1718,6 +1728,21 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx,
>  	blk_status_t ret;
>  	int reason;
>  
> +	if (blk_mq_is_reserved_rq(req)) {
> +		if (!(req->rq_flags & RQF_DONTPREP)) {
> +			ret = scsi_prepare_cmd(req);
> +			if (ret != BLK_STS_OK)
> +				goto out_dec_host_busy;
> +
> +			req->rq_flags |= RQF_DONTPREP;
> +		} else {
> +			clear_bit(SCMD_STATE_COMPLETE, &cmd->state);
> +		}
> +		blk_mq_start_request(req);
> +
> +		return shost->hostt->reserved_queuecommand(shost, cmd);
> +	}
> +
>  	WARN_ON_ONCE(cmd->budget_token < 0);
>  
>  	/*
> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> index 91678c77398e..a39f36aa0b0d 100644
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -73,6 +73,7 @@ struct scsi_host_template {
>  	 * STATUS: REQUIRED
>  	 */
>  	int (* queuecommand)(struct Scsi_Host *, struct scsi_cmnd *);
> +	int (*reserved_queuecommand)(struct Scsi_Host *, struct scsi_cmnd *);

Nit: This op name sound like something returning a bool... May be a
straight "queue_reserved_command" name would be clearer ?

>  
>  	/*
>  	 * The commit_rqs function is used to trigger a hardware
John Garry Oct. 27, 2022, 9:13 a.m. UTC | #2 
On 27/10/2022 02:21, Damien Le Moal wrote:
>>   
>> +	if (blk_mq_is_reserved_rq(rq)) {
>> +		struct scsi_device *sdev = cmd->device;
> This variable is not really needed. You can call:
> 		
> 		scsi_device_unbusy(cmd->device, cmd);
> 
> No ?

ok, your suggestion is good

> 
>> +
>> +		scsi_mq_uninit_cmd(cmd);
>> +		scsi_device_unbusy(sdev, cmd);
>> +		__blk_mq_end_request(rq, 0);
>> +
>> +		return;
>> +	}
>> +
>>   	INIT_LIST_HEAD(&cmd->eh_entry);
>>   
>>   	atomic_inc(&cmd->device->iodone_cnt);
>> @@ -1718,6 +1728,21 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx,
>>   	blk_status_t ret;
>>   	int reason;
>>   
>> +	if (blk_mq_is_reserved_rq(req)) {
>> +		if (!(req->rq_flags & RQF_DONTPREP)) {
>> +			ret = scsi_prepare_cmd(req);
>> +			if (ret != BLK_STS_OK)
>> +				goto out_dec_host_busy;
>> +
>> +			req->rq_flags |= RQF_DONTPREP;
>> +		} else {
>> +			clear_bit(SCMD_STATE_COMPLETE, &cmd->state);
>> +		}
>> +		blk_mq_start_request(req);
>> +
>> +		return shost->hostt->reserved_queuecommand(shost, cmd);
>> +	}
>> +
>>   	WARN_ON_ONCE(cmd->budget_token < 0);
>>   
>>   	/*
>> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
>> index 91678c77398e..a39f36aa0b0d 100644
>> --- a/include/scsi/scsi_host.h
>> +++ b/include/scsi/scsi_host.h
>> @@ -73,6 +73,7 @@ struct scsi_host_template {
>>   	 * STATUS: REQUIRED
>>   	 */
>>   	int (* queuecommand)(struct Scsi_Host *, struct scsi_cmnd *);
>> +	int (*reserved_queuecommand)(struct Scsi_Host *, struct scsi_cmnd *);
> Nit: This op name sound like something returning a bool... May be a
> straight "queue_reserved_command" name would be clearer ?

or queuecommand_reserved ? I'm just trying to have the name a variant of 
"queuecommand".

> 

thanks,
John
Damien Le Moal Oct. 27, 2022, 9:18 a.m. UTC | #3 
On 10/27/22 18:13, John Garry wrote:
> On 27/10/2022 02:21, Damien Le Moal wrote:
>>>   +    if (blk_mq_is_reserved_rq(rq)) {
>>> +        struct scsi_device *sdev = cmd->device;
>> This variable is not really needed. You can call:
>>        
>>         scsi_device_unbusy(cmd->device, cmd);
>>
>> No ?
> 
> ok, your suggestion is good
> 
>>
>>> +
>>> +        scsi_mq_uninit_cmd(cmd);
>>> +        scsi_device_unbusy(sdev, cmd);
>>> +        __blk_mq_end_request(rq, 0);
>>> +
>>> +        return;
>>> +    }
>>> +
>>>       INIT_LIST_HEAD(&cmd->eh_entry);
>>>         atomic_inc(&cmd->device->iodone_cnt);
>>> @@ -1718,6 +1728,21 @@ static blk_status_t scsi_queue_rq(struct
>>> blk_mq_hw_ctx *hctx,
>>>       blk_status_t ret;
>>>       int reason;
>>>   +    if (blk_mq_is_reserved_rq(req)) {
>>> +        if (!(req->rq_flags & RQF_DONTPREP)) {
>>> +            ret = scsi_prepare_cmd(req);
>>> +            if (ret != BLK_STS_OK)
>>> +                goto out_dec_host_busy;
>>> +
>>> +            req->rq_flags |= RQF_DONTPREP;
>>> +        } else {
>>> +            clear_bit(SCMD_STATE_COMPLETE, &cmd->state);
>>> +        }
>>> +        blk_mq_start_request(req);
>>> +
>>> +        return shost->hostt->reserved_queuecommand(shost, cmd);
>>> +    }
>>> +
>>>       WARN_ON_ONCE(cmd->budget_token < 0);
>>>         /*
>>> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
>>> index 91678c77398e..a39f36aa0b0d 100644
>>> --- a/include/scsi/scsi_host.h
>>> +++ b/include/scsi/scsi_host.h
>>> @@ -73,6 +73,7 @@ struct scsi_host_template {
>>>        * STATUS: REQUIRED
>>>        */
>>>       int (* queuecommand)(struct Scsi_Host *, struct scsi_cmnd *);
>>> +    int (*reserved_queuecommand)(struct Scsi_Host *, struct
>>> scsi_cmnd *);
>> Nit: This op name sound like something returning a bool... May be a
>> straight "queue_reserved_command" name would be clearer ?
> 
> or queuecommand_reserved ? I'm just trying to have the name a variant of
> "queuecommand".

I figured that :)
queuereservedcommand ? (hard to read...)
queuecommand_reserved is OK I guess.

> 
>>
> 
> thanks,
> John
diff mbox series

Patch

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index db89afc37bc9..78968553089f 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -230,6 +230,12 @@  int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
 		goto fail;
 	}
 
+	if (shost->nr_reserved_cmds && !sht->reserved_queuecommand) {
+		shost_printk(KERN_ERR, shost,
+			"nr_reserved_cmds set but no method to queue\n");
+		goto fail;
+	}
+
 	/* Use min_t(int, ...) in case shost->can_queue exceeds SHRT_MAX */
 	shost->cmd_per_lun = min_t(int, shost->cmd_per_lun,
 				   shost->can_queue);
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index a8c4e7c037ae..08015c42c326 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1428,6 +1428,16 @@  static void scsi_complete(struct request *rq)
 	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(rq);
 	enum scsi_disposition disposition;
 
+	if (blk_mq_is_reserved_rq(rq)) {
+		struct scsi_device *sdev = cmd->device;
+
+		scsi_mq_uninit_cmd(cmd);
+		scsi_device_unbusy(sdev, cmd);
+		__blk_mq_end_request(rq, 0);
+
+		return;
+	}
+
 	INIT_LIST_HEAD(&cmd->eh_entry);
 
 	atomic_inc(&cmd->device->iodone_cnt);
@@ -1718,6 +1728,21 @@  static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx,
 	blk_status_t ret;
 	int reason;
 
+	if (blk_mq_is_reserved_rq(req)) {
+		if (!(req->rq_flags & RQF_DONTPREP)) {
+			ret = scsi_prepare_cmd(req);
+			if (ret != BLK_STS_OK)
+				goto out_dec_host_busy;
+
+			req->rq_flags |= RQF_DONTPREP;
+		} else {
+			clear_bit(SCMD_STATE_COMPLETE, &cmd->state);
+		}
+		blk_mq_start_request(req);
+
+		return shost->hostt->reserved_queuecommand(shost, cmd);
+	}
+
 	WARN_ON_ONCE(cmd->budget_token < 0);
 
 	/*
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 91678c77398e..a39f36aa0b0d 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -73,6 +73,7 @@  struct scsi_host_template {
 	 * STATUS: REQUIRED
 	 */
 	int (* queuecommand)(struct Scsi_Host *, struct scsi_cmnd *);
+	int (*reserved_queuecommand)(struct Scsi_Host *, struct scsi_cmnd *);
 
 	/*
 	 * The commit_rqs function is used to trigger a hardware