Re: [PATCH 3/3] scsi: core: Improve IOPS in case of host-wide tags

[Bart Van Assche <bvanassche@acm.org>]

On 9/11/25 1:15 AM, John Garry wrote:
> On 10/09/2025 22:32, Bart Van Assche wrote:
>> -    sbitmap_resize(&sdev->budget_map, sdev->queue_depth);
>> +    if (shost->host_tagset && depth >= shost->can_queue)
>> +        sbitmap_free(&sdev->budget_map);
> 
> eh, what happens if we call this twice?

I have checked that calling sbitmap_free() twice is safe.

>> +    else
>> +        sbitmap_resize(&sdev->budget_map, sdev->queue_depth);
> 
> what if we set queue_depth = shost->can_queue (and free the budget map) 
> and then later set lower than shost->can_queue (and try to reference the 
> budget map)?

I will modify scsi_change_queue_depth() such that it allocates a budget
map if sdev->budget_map.map == NULL.

>> +static bool scsi_device_check_in_flight(struct request *rq, void *data)
> 
> so this does not check the cmd state (like scsi_host_check_in_flight() 
> does), but it uses the same naming (scsi_xxx_check_in_flight)

I will rename this function.

>> @@ -1358,11 +1400,13 @@ scsi_device_state_check(struct scsi_device 
>> *sdev, struct request *req)
>>   static inline int scsi_dev_queue_ready(struct request_queue *q,
>>                     struct scsi_device *sdev)
>>   {
>> -    int token;
>> +    int token = INT_MAX;
>> -    token = sbitmap_get(&sdev->budget_map);
>> -    if (token < 0)
>> -        return -1;
>> +    if (sdev->budget_map.map) {
> 
> this can race with a call to scsi_change_queue_depth() (which may free 
> sdev->budget_map.map), right?
> 
> scsi_change_queue_depth() does not seem to do any queue freezing.

Agreed, and I think that's a longstanding bug in the upstream kernel.
scsi_change_queue_depth() should freeze the request queue before it
modifies the budget map.

Thanks,

Bart.