Re: [PATCH v3 04/26] scsi: core: Bypass the queue limit checks for reserved commands

[John Garry <john.g.garry@oracle.com>]

On 04/09/2025 20:25, Bart Van Assche wrote:
> On 9/4/25 2:49 AM, John Garry wrote:
>> On 27/08/2025 01:06, Bart Van Assche wrote:
>>> [ bvanassche: modified patch title and patch description. 
>>
>> it's an odd name now... I don't know which queue limits we are bypassing
> 
> I can change "queue" into "SCSI host" in the patch title.
> 
>>> --- a/drivers/scsi/scsi_lib.c
>>> +++ b/drivers/scsi/scsi_lib.c
>>> @@ -1539,6 +1539,14 @@ 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)) {
>>> +        /* Only pass-through requests are supported in this code 
>>> path. */
>>> +        WARN_ON_ONCE(!blk_rq_is_passthrough(scsi_cmd_to_rq(cmd)));
>>
>> eh, do we really have passthough reserved command?
> 
> All reserved commands that end up in scsi_complete() should be
> pass-through commands (REQ_OP_DRV_IN / REQ_OP_DRV_OUT), isn't it?
> I don't think that we should allow other request types for reserved
> commands.

ok, I suppose, but it seems odd to say that a reserved command is a 
passthrough command.

>>> -    if (unlikely(sdev->sdev_state != SDEV_RUNNING)) {
>>> -        ret = scsi_device_state_check(sdev, req);
>>> -        if (ret != BLK_STS_OK)
>>> -            goto out_put_budget;
>>> -    }
>>> +    if (!blk_mq_is_reserved_rq(req)) {
>>> +        /*
>>> +         * If the device is not in running state we will reject some or
>>> +         * all commands.
>>> +         */
>>> +        if (unlikely(sdev->sdev_state != SDEV_RUNNING)) {
>>
>> I am curious about this. I mentioned previously if we only send 
>> reserved commands to the psuedo sdev (in this seris). If so, would the 
>> psuedo sdev not always be running state?
> 
> Has the above code change perhaps been misread? The above change causes
> the sdev->sdev_state check to be skipped for pseudo SCSI devices.
> 

Maybe I misread. I was just curious if we are doing this sdev state 
check for a psuedo sdev, which does not seem to be the case.

Thanks,
John