| Message ID | 1591810159-240929-12-git-send-email-john.garry@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | blk-mq/scsi: Provide hostwide shared tags for SCSI HBAs | expand |
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. > + } > } > Thanks, John
On 14/07/2020 14:16, 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). > Sorry, I missed that you use i = blk_mq_unique_tag_to_tag(blk_tag) + limit for regular command. But we still set next_io_request_slot for regular command and maybe then read and use it for reserved command. Thanks
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@suse.de +49 911 74053 688 SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg HRB 36809 (AG Nürnberg), GF: Felix Imendörffer
Subject: Re: [PATCH RFC v7 11/12] smartpqi: enable host tagset Both the driver author and myself do not want to change how commands are processed in the smartpqi driver. Nak.
On 14/07/2020 19:16, Don.Brace@microchip.com wrote: > Subject: Re: [PATCH RFC v7 11/12] smartpqi: enable host tagset > > > Both the driver author and myself do not want to change how commands are processed in the smartpqi driver. > > Nak. > ok, your call. But it still seems that the driver should set the host_tagset flag. Anyway, can you also let us know your stance on the hpsa change in this series? https://lore.kernel.org/linux-scsi/1591810159-240929-1-git-send-email-john.garry@huawei.com/T/#m1057b8a8c23a9bf558a048817a1cda4a576291b2 thanks
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
diff --git a/drivers/scsi/smartpqi/smartpqi_init.c b/drivers/scsi/smartpqi/smartpqi_init.c index cd157f11eb22..1f4de4c2d876 100644 --- a/drivers/scsi/smartpqi/smartpqi_init.c +++ b/drivers/scsi/smartpqi/smartpqi_init.c @@ -575,17 +575,29 @@ static inline void pqi_reinit_io_request(struct pqi_io_request *io_request) } 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]; - 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; + } } /* benignly racy */ @@ -4075,7 +4087,7 @@ static int pqi_submit_raid_request_synchronous(struct pqi_ctrl_info *ctrl_info, atomic_inc(&ctrl_info->sync_cmds_outstanding); - io_request = pqi_alloc_io_request(ctrl_info); + io_request = pqi_alloc_io_request(ctrl_info, NULL); put_unaligned_le16(io_request->index, &(((struct pqi_raid_path_request *)request)->request_id)); @@ -5032,7 +5044,9 @@ static inline int pqi_raid_submit_scsi_cmd(struct pqi_ctrl_info *ctrl_info, { struct pqi_io_request *io_request; - io_request = pqi_alloc_io_request(ctrl_info); + io_request = pqi_alloc_io_request(ctrl_info, scmd); + if (!io_request) + return SCSI_MLQUEUE_HOST_BUSY; return pqi_raid_submit_scsi_cmd_with_io_request(ctrl_info, io_request, device, scmd, queue_group); @@ -5230,7 +5244,10 @@ static int pqi_aio_submit_io(struct pqi_ctrl_info *ctrl_info, struct pqi_io_request *io_request; struct pqi_aio_path_request *request; - io_request = pqi_alloc_io_request(ctrl_info); + io_request = pqi_alloc_io_request(ctrl_info, scmd); + if (!io_request) + return SCSI_MLQUEUE_HOST_BUSY; + io_request->io_complete_callback = pqi_aio_io_complete; io_request->scmd = scmd; io_request->raid_bypass = raid_bypass; @@ -5657,7 +5674,7 @@ static int pqi_lun_reset(struct pqi_ctrl_info *ctrl_info, DECLARE_COMPLETION_ONSTACK(wait); struct pqi_task_management_request *request; - io_request = pqi_alloc_io_request(ctrl_info); + io_request = pqi_alloc_io_request(ctrl_info, NULL); io_request->io_complete_callback = pqi_lun_reset_complete; io_request->context = &wait; @@ -6504,6 +6521,7 @@ static struct scsi_host_template pqi_driver_template = { .map_queues = pqi_map_queues, .sdev_attrs = pqi_sdev_attrs, .shost_attrs = pqi_shost_attrs, + .host_tagset = 1, }; static int pqi_register_scsi(struct pqi_ctrl_info *ctrl_info)