On 7/14/20 3:16 PM, John Garry wrote:
> Hi Hannes,
>
>> static struct pqi_io_request *pqi_alloc_io_request(
>> - struct pqi_ctrl_info *ctrl_info)
>> + struct pqi_ctrl_info *ctrl_info, struct scsi_cmnd *scmd)
>> {
>> struct pqi_io_request *io_request;
>> + unsigned int limit = PQI_RESERVED_IO_SLOTS;
>> u16 i = ctrl_info->next_io_request_slot; /* benignly racy */
>> - while (1) {
>> + if (scmd) {
>> + u32 blk_tag = blk_mq_unique_tag(scmd->request);
>> +
>> + i = blk_mq_unique_tag_to_tag(blk_tag) + limit;
>> io_request = &ctrl_info->io_request_pool[i];
>
> This looks ok
>
>> - if (atomic_inc_return(&io_request->refcount) == 1)
>> - break;
>> - atomic_dec(&io_request->refcount);
>> - i = (i + 1) % ctrl_info->max_io_slots;
>> + if (WARN_ON(atomic_inc_return(&io_request->refcount) > 1)) {
>> + atomic_dec(&io_request->refcount);
>> + return NULL;
>> + }
>> + } else {
>> + while (1) {
>> + io_request = &ctrl_info->io_request_pool[i];
>> + if (atomic_inc_return(&io_request->refcount) == 1)
>> + break;
>> + atomic_dec(&io_request->refcount);
>> + i = (i + 1) % limit;
>
> To me, the range we use here looks incorrect. I would assume we should
> restrict range to [max_io_slots - PQI_RESERVED_IO_SLOTS, max_io_slots).
>
> But then your reserved commands support would solve that.
>
This branch of the 'if' condition will only be taken for internal
commands, for which we only allow up to PQI_RESERVED_IO_SLOTS.
And we set the 'normal' I/O commands above at an offset, so we're fine here.
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@xxxxxxx +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer
