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

Bart Van Assche
2025-09-11T15:59:54Z

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.