| Message ID | 20200430131904.5847-38-hare@suse.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | scsi: enable reserved commands for LLDDs | expand |
On 30/04/2020 14:19, Hannes Reinecke wrote: > All block layer commands now have a tag, so we should be storing > it in the sas_task structure for easier lookup. This seems like a decent idea, to put the tag here, so that we don't need to do the lookup in the LLDD queuecommand. However it feels safer to use a sas_task scsicmd->request->tag in the LLDD directly, if we can make that possible. More below. > > Signed-off-by: Hannes Reinecke <hare@suse.de> > --- > drivers/scsi/libsas/sas_ata.c | 4 ++++ > drivers/scsi/libsas/sas_init.c | 2 ++ > drivers/scsi/libsas/sas_scsi_host.c | 2 +- > include/scsi/libsas.h | 2 ++ > 4 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c > index 5d716d388707..897007343b3d 100644 > --- a/drivers/scsi/libsas/sas_ata.c > +++ b/drivers/scsi/libsas/sas_ata.c > @@ -211,6 +211,10 @@ static unsigned int sas_ata_qc_issue(struct ata_queued_cmd *qc) > > task->data_dir = qc->dma_dir; > task->scatter = qc->sg; > + if (qc->scsicmd) > + task->tag = qc->scsicmd->request->tag; > + else > + task->tag = qc->tag; I think that this tag comes from ata_sas_allocate_tag(), and would be managed from yet another bitmap for ATA commands tags. Maybe we can allocate a sas slow task here, instead of using this tag. Needs more checking... > task->ata_task.retry_count = 1; > task->task_state_flags = SAS_TASK_STATE_PENDING; > qc->lldd_task = task; > diff --git a/drivers/scsi/libsas/sas_init.c b/drivers/scsi/libsas/sas_init.c > index 5aa8593b88b5..0d32cb49d0af 100644 > --- a/drivers/scsi/libsas/sas_init.c > +++ b/drivers/scsi/libsas/sas_init.c > @@ -53,6 +53,7 @@ struct sas_task *sas_alloc_slow_task(struct sas_ha_struct *ha, > if (!slow) > goto out_err_slow; > > + task->tag = -1; > if (shost->nr_reserved_cmds) { > struct scsi_device *sdev; > > @@ -66,6 +67,7 @@ struct sas_task *sas_alloc_slow_task(struct sas_ha_struct *ha, > slow->scmd = scsi_get_reserved_cmd(sdev, DMA_NONE, false); > if (!slow->scmd) > goto out_err_scmd; > + task->tag = slow->scmd->request->tag; > ASSIGN_SAS_TASK(slow->scmd, task); > } > > diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c > index c5a430e3fa2d..585e0df5fce2 100644 > --- a/drivers/scsi/libsas/sas_scsi_host.c > +++ b/drivers/scsi/libsas/sas_scsi_host.c > @@ -149,7 +149,7 @@ static struct sas_task *sas_create_task(struct scsi_cmnd *cmd, > memcpy(task->ssp_task.LUN, &lun.scsi_lun, 8); > task->ssp_task.task_attr = TASK_ATTR_SIMPLE; > task->ssp_task.cmd = cmd; > - > + task->tag = cmd->request->tag; > task->scatter = scsi_sglist(cmd); > task->num_scatter = scsi_sg_count(cmd); > task->total_xfer_len = scsi_bufflen(cmd); > diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h > index c927228019c9..af864f68b5cc 100644 > --- a/include/scsi/libsas.h > +++ b/include/scsi/libsas.h > @@ -594,6 +594,8 @@ struct sas_task { > u32 total_xfer_len; > u8 data_dir:2; /* Use PCI_DMA_... */ > > + u32 tag; unsigned, yet we assign it -1? > + > struct task_status_struct task_status; > void (*task_done)(struct sas_task *); > > Thanks!
On 5/1/20 12:26 PM, John Garry wrote: > On 30/04/2020 14:19, Hannes Reinecke wrote: >> All block layer commands now have a tag, so we should be storing >> it in the sas_task structure for easier lookup. > > This seems like a decent idea, to put the tag here, so that we don't > need to do the lookup in the LLDD queuecommand. > > However it feels safer to use a sas_task scsicmd->request->tag in the > LLDD directly, if we can make that possible. More below. > >> >> Signed-off-by: Hannes Reinecke <hare@suse.de> >> --- >> drivers/scsi/libsas/sas_ata.c | 4 ++++ >> drivers/scsi/libsas/sas_init.c | 2 ++ >> drivers/scsi/libsas/sas_scsi_host.c | 2 +- >> include/scsi/libsas.h | 2 ++ >> 4 files changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/scsi/libsas/sas_ata.c >> b/drivers/scsi/libsas/sas_ata.c >> index 5d716d388707..897007343b3d 100644 >> --- a/drivers/scsi/libsas/sas_ata.c >> +++ b/drivers/scsi/libsas/sas_ata.c >> @@ -211,6 +211,10 @@ static unsigned int sas_ata_qc_issue(struct >> ata_queued_cmd *qc) >> task->data_dir = qc->dma_dir; >> task->scatter = qc->sg; >> + if (qc->scsicmd) >> + task->tag = qc->scsicmd->request->tag; >> + else >> + task->tag = qc->tag; > > I think that this tag comes from ata_sas_allocate_tag(), and would be > managed from yet another bitmap for ATA commands tags. Maybe we can > allocate a sas slow task here, instead of using this tag. Needs more > checking... > That is actually not a bad idea. non-scsi ATA commands are typically internal commands, so for them a slow task should be sufficient. Plus ultimately the tags have to come from the same pool than the normal FS I/O, as they still do touch the same hardware. So indeed, well spotted. I'll need to revisit this and see if one can't be using slow tasks here. >> task->ata_task.retry_count = 1; >> task->task_state_flags = SAS_TASK_STATE_PENDING; >> qc->lldd_task = task; >> diff --git a/drivers/scsi/libsas/sas_init.c >> b/drivers/scsi/libsas/sas_init.c >> index 5aa8593b88b5..0d32cb49d0af 100644 >> --- a/drivers/scsi/libsas/sas_init.c >> +++ b/drivers/scsi/libsas/sas_init.c >> @@ -53,6 +53,7 @@ struct sas_task *sas_alloc_slow_task(struct >> sas_ha_struct *ha, >> if (!slow) >> goto out_err_slow; >> + task->tag = -1; >> if (shost->nr_reserved_cmds) { >> struct scsi_device *sdev; >> @@ -66,6 +67,7 @@ struct sas_task *sas_alloc_slow_task(struct >> sas_ha_struct *ha, >> slow->scmd = scsi_get_reserved_cmd(sdev, DMA_NONE, false); >> if (!slow->scmd) >> goto out_err_scmd; >> + task->tag = slow->scmd->request->tag; >> ASSIGN_SAS_TASK(slow->scmd, task); >> } >> diff --git a/drivers/scsi/libsas/sas_scsi_host.c >> b/drivers/scsi/libsas/sas_scsi_host.c >> index c5a430e3fa2d..585e0df5fce2 100644 >> --- a/drivers/scsi/libsas/sas_scsi_host.c >> +++ b/drivers/scsi/libsas/sas_scsi_host.c >> @@ -149,7 +149,7 @@ static struct sas_task *sas_create_task(struct >> scsi_cmnd *cmd, >> memcpy(task->ssp_task.LUN, &lun.scsi_lun, 8); >> task->ssp_task.task_attr = TASK_ATTR_SIMPLE; >> task->ssp_task.cmd = cmd; >> - >> + task->tag = cmd->request->tag; >> task->scatter = scsi_sglist(cmd); >> task->num_scatter = scsi_sg_count(cmd); >> task->total_xfer_len = scsi_bufflen(cmd); >> diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h >> index c927228019c9..af864f68b5cc 100644 >> --- a/include/scsi/libsas.h >> +++ b/include/scsi/libsas.h >> @@ -594,6 +594,8 @@ struct sas_task { >> u32 total_xfer_len; >> u8 data_dir:2; /* Use PCI_DMA_... */ >> + u32 tag; > > unsigned, yet we assign it -1? > Yeah, that's how the block layer does internally, too. Maybe we should export SCSI_NO_TAG and use it here. Cheers, Hannes
>>> - >>> + task->tag = cmd->request->tag; >>> task->scatter = scsi_sglist(cmd); >>> task->num_scatter = scsi_sg_count(cmd); >>> task->total_xfer_len = scsi_bufflen(cmd); >>> diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h >>> index c927228019c9..af864f68b5cc 100644 >>> --- a/include/scsi/libsas.h >>> +++ b/include/scsi/libsas.h >>> @@ -594,6 +594,8 @@ struct sas_task { >>> u32 total_xfer_len; >>> u8 data_dir:2; /* Use PCI_DMA_... */ >>> + u32 tag; >> >> unsigned, yet we assign it -1? >> > Yeah, that's how the block layer does internally, too. > Maybe we should export SCSI_NO_TAG and use it here. > I think it's better that the LLDD would not have to deal with "no tag" scenario (pm8001 driver has to handle it in this series). Rather libsas can handle that, and fail an allocation of a slow_task to the LLDD instead. Thanks, John
On 5/4/20 9:49 AM, John Garry wrote: >>>> - >>>> + task->tag = cmd->request->tag; >>>> task->scatter = scsi_sglist(cmd); >>>> task->num_scatter = scsi_sg_count(cmd); >>>> task->total_xfer_len = scsi_bufflen(cmd); >>>> diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h >>>> index c927228019c9..af864f68b5cc 100644 >>>> --- a/include/scsi/libsas.h >>>> +++ b/include/scsi/libsas.h >>>> @@ -594,6 +594,8 @@ struct sas_task { >>>> u32 total_xfer_len; >>>> u8 data_dir:2; /* Use PCI_DMA_... */ >>>> + u32 tag; >>> >>> unsigned, yet we assign it -1? >>> >> Yeah, that's how the block layer does internally, too. >> Maybe we should export SCSI_NO_TAG and use it here. >> > > I think it's better that the LLDD would not have to deal with "no tag" > scenario (pm8001 driver has to handle it in this series). Rather libsas > can handle that, and fail an allocation of a slow_task to the LLDD instead. > I fully agree. The 'no tag' scenario should never happen with libsas with this patchset. But for that to happen we need to: - Ensure that even ATA devices on libsas always have a domain device (I think it's true nowadays, but we'll need to check). - Add a host port to libsas, such that sas_alloc_target() will have a valid target/port upon lookup (and your patch won't be needed anymore) - Move ATA non-SCSI commands over to using sas slow tasks. This shouldn't be much of a problem as really the only non-SCSI ATA command which will need to be sent from within an I/O stream is the 'read log' command, and that is handled internally by the drivers anyway. All other commands are sent during device discovery or from out-of-band things like ioctl, so they should be fine using slow tasks. Cheers, Hannes
On 04/05/2020 09:00, Hannes Reinecke wrote: > On 5/4/20 9:49 AM, John Garry wrote: >>>>> - >>>>> + task->tag = cmd->request->tag; >>>>> task->scatter = scsi_sglist(cmd); >>>>> task->num_scatter = scsi_sg_count(cmd); >>>>> task->total_xfer_len = scsi_bufflen(cmd); >>>>> diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h >>>>> index c927228019c9..af864f68b5cc 100644 >>>>> --- a/include/scsi/libsas.h >>>>> +++ b/include/scsi/libsas.h >>>>> @@ -594,6 +594,8 @@ struct sas_task { >>>>> u32 total_xfer_len; >>>>> u8 data_dir:2; /* Use PCI_DMA_... */ >>>>> + u32 tag; >>>> >>>> unsigned, yet we assign it -1? >>>> >>> Yeah, that's how the block layer does internally, too. >>> Maybe we should export SCSI_NO_TAG and use it here. >>> >> >> I think it's better that the LLDD would not have to deal with "no tag" >> scenario (pm8001 driver has to handle it in this series). Rather >> libsas can handle that, and fail an allocation of a slow_task to the >> LLDD instead. >> > I fully agree. The 'no tag' scenario should never happen with libsas > with this patchset. > But for that to happen we need to: > - Ensure that even ATA devices on libsas always have a domain device (I > think it's true nowadays, but we'll need to check). They should do. See references to sas_ata_init(), which is at the 2x points of ATA dev discovery - root port and expander remote port. > - Add a host port to libsas, such that sas_alloc_target() will have a > valid target/port upon lookup (and your patch won't be needed anymore) I think that they should already have for ATA devices. > - Move ATA non-SCSI commands over to using sas slow tasks. This > shouldn't be much of a problem as really the only non-SCSI ATA command > which will need to be sent from within an I/O stream is the 'read log' > command, and that is handled internally by the drivers anyway. ok > All other commands are sent during device discovery or from out-of-band > things like ioctl, so they should be fine using slow tasks. > ok Cheers, John
diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c index 5d716d388707..897007343b3d 100644 --- a/drivers/scsi/libsas/sas_ata.c +++ b/drivers/scsi/libsas/sas_ata.c @@ -211,6 +211,10 @@ static unsigned int sas_ata_qc_issue(struct ata_queued_cmd *qc) task->data_dir = qc->dma_dir; task->scatter = qc->sg; + if (qc->scsicmd) + task->tag = qc->scsicmd->request->tag; + else + task->tag = qc->tag; task->ata_task.retry_count = 1; task->task_state_flags = SAS_TASK_STATE_PENDING; qc->lldd_task = task; diff --git a/drivers/scsi/libsas/sas_init.c b/drivers/scsi/libsas/sas_init.c index 5aa8593b88b5..0d32cb49d0af 100644 --- a/drivers/scsi/libsas/sas_init.c +++ b/drivers/scsi/libsas/sas_init.c @@ -53,6 +53,7 @@ struct sas_task *sas_alloc_slow_task(struct sas_ha_struct *ha, if (!slow) goto out_err_slow; + task->tag = -1; if (shost->nr_reserved_cmds) { struct scsi_device *sdev; @@ -66,6 +67,7 @@ struct sas_task *sas_alloc_slow_task(struct sas_ha_struct *ha, slow->scmd = scsi_get_reserved_cmd(sdev, DMA_NONE, false); if (!slow->scmd) goto out_err_scmd; + task->tag = slow->scmd->request->tag; ASSIGN_SAS_TASK(slow->scmd, task); } diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c index c5a430e3fa2d..585e0df5fce2 100644 --- a/drivers/scsi/libsas/sas_scsi_host.c +++ b/drivers/scsi/libsas/sas_scsi_host.c @@ -149,7 +149,7 @@ static struct sas_task *sas_create_task(struct scsi_cmnd *cmd, memcpy(task->ssp_task.LUN, &lun.scsi_lun, 8); task->ssp_task.task_attr = TASK_ATTR_SIMPLE; task->ssp_task.cmd = cmd; - + task->tag = cmd->request->tag; task->scatter = scsi_sglist(cmd); task->num_scatter = scsi_sg_count(cmd); task->total_xfer_len = scsi_bufflen(cmd); diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h index c927228019c9..af864f68b5cc 100644 --- a/include/scsi/libsas.h +++ b/include/scsi/libsas.h @@ -594,6 +594,8 @@ struct sas_task { u32 total_xfer_len; u8 data_dir:2; /* Use PCI_DMA_... */ + u32 tag; + struct task_status_struct task_status; void (*task_done)(struct sas_task *);
All block layer commands now have a tag, so we should be storing it in the sas_task structure for easier lookup. Signed-off-by: Hannes Reinecke <hare@suse.de> --- drivers/scsi/libsas/sas_ata.c | 4 ++++ drivers/scsi/libsas/sas_init.c | 2 ++ drivers/scsi/libsas/sas_scsi_host.c | 2 +- include/scsi/libsas.h | 2 ++ 4 files changed, 9 insertions(+), 1 deletion(-)