On 14/07/2020 15:02, Hannes Reinecke wrote:
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.
Here is the code:
----8<----
unsigned int limit = PQI_RESERVED_IO_SLOTS;
u16 i = ctrl_info->next_io_request_slot; /* benignly racy */
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];
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;
}
}
/* benignly racy */
ctrl_info->next_io_request_slot = (i + 1) % ctrl_info->max_io_slots;
---->8----
Is how we set ctrl_info->next_io_request_slot ok? Should it be:
ctrl_info->next_io_request_slot = (i + 1) % limit;
And also moved into 'else' leg for good measure.
Thanks,
John
