| Message ID | 546516bb62c91671b1464e6a7c68eebc8422efe5.1418928090.git.shli@fb.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
Ping! On Thu, Dec 18, 2014 at 10:46:22AM -0800, Shaohua Li wrote: > libata uses its own tag management which is duplication and the > implementation is poor. And if we switch to blk-mq, tag is build-in. > It's time to switch to generic taging. > > The SAS driver has its own tag management, and looks we can't directly > map the host controler tag to SATA tag. So I just bypassed the SAS case. > > Cc: Jens Axboe <axboe@fb.com> > Cc: Tejun Heo <tj@kernel.org> > Cc: Christoph Hellwig <hch@infradead.org> > Signed-off-by: Shaohua Li <shli@fb.com> > --- > drivers/ata/libata-core.c | 20 ++++++++++++++------ > drivers/ata/libata-scsi.c | 4 +++- > drivers/ata/libata.h | 2 +- > include/linux/libata.h | 1 + > 4 files changed, 19 insertions(+), 8 deletions(-) > > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c > index 5c84fb5..a27d00f 100644 > --- a/drivers/ata/libata-core.c > +++ b/drivers/ata/libata-core.c > @@ -1585,8 +1585,9 @@ unsigned ata_exec_internal_sg(struct ata_device *dev, > else > tag = 0; > > - if (test_and_set_bit(tag, &ap->qc_allocated)) > - BUG(); > + BUG_ON((!ap->scsi_host && test_and_set_bit(tag, &ap->qc_allocated)) || > + (ap->scsi_host && dev->sdev && > + blk_queue_find_tag(dev->sdev->request_queue, tag))); > qc = __ata_qc_from_tag(ap, tag); > > qc->tag = tag; > @@ -4737,7 +4738,7 @@ void swap_buf_le16(u16 *buf, unsigned int buf_words) > * None. > */ > > -static struct ata_queued_cmd *ata_qc_new(struct ata_port *ap) > +static struct ata_queued_cmd *ata_qc_new(struct ata_port *ap, int blktag) > { > struct ata_queued_cmd *qc = NULL; > unsigned int max_queue = ap->host->n_tags; > @@ -4747,6 +4748,12 @@ static struct ata_queued_cmd *ata_qc_new(struct ata_port *ap) > if (unlikely(ap->pflags & ATA_PFLAG_FROZEN)) > return NULL; > > + if (ap->scsi_host) { > + qc = __ata_qc_from_tag(ap, blktag); > + qc->tag = blktag; > + return qc; > + } > + > for (i = 0, tag = ap->last_tag + 1; i < max_queue; i++, tag++) { > tag = tag < max_queue ? tag : 0; > > @@ -4773,12 +4780,12 @@ static struct ata_queued_cmd *ata_qc_new(struct ata_port *ap) > * None. > */ > > -struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev) > +struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev, int blktag) > { > struct ata_port *ap = dev->link->ap; > struct ata_queued_cmd *qc; > > - qc = ata_qc_new(ap); > + qc = ata_qc_new(ap, blktag); > if (qc) { > qc->scsicmd = NULL; > qc->ap = ap; > @@ -4812,7 +4819,8 @@ void ata_qc_free(struct ata_queued_cmd *qc) > tag = qc->tag; > if (likely(ata_tag_valid(tag))) { > qc->tag = ATA_TAG_POISON; > - clear_bit(tag, &ap->qc_allocated); > + if (!ap->scsi_host) > + clear_bit(tag, &ap->qc_allocated); > } > } > > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c > index e364e86..94339c2 100644 > --- a/drivers/ata/libata-scsi.c > +++ b/drivers/ata/libata-scsi.c > @@ -756,7 +756,7 @@ static struct ata_queued_cmd *ata_scsi_qc_new(struct ata_device *dev, > { > struct ata_queued_cmd *qc; > > - qc = ata_qc_new_init(dev); > + qc = ata_qc_new_init(dev, cmd->request->tag); > if (qc) { > qc->scsicmd = cmd; > qc->scsidone = cmd->scsi_done; > @@ -3666,6 +3666,8 @@ int ata_scsi_add_hosts(struct ata_host *host, struct scsi_host_template *sht) > */ > shost->max_host_blocked = 1; > > + scsi_init_shared_tag_map(shost, host->n_tags); > + > rc = scsi_add_host_with_dma(ap->scsi_host, > &ap->tdev, ap->host->dev); > if (rc) > diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h > index 5f4e0cc..4040513 100644 > --- a/drivers/ata/libata.h > +++ b/drivers/ata/libata.h > @@ -63,7 +63,7 @@ extern struct ata_link *ata_dev_phys_link(struct ata_device *dev); > extern void ata_force_cbl(struct ata_port *ap); > extern u64 ata_tf_to_lba(const struct ata_taskfile *tf); > extern u64 ata_tf_to_lba48(const struct ata_taskfile *tf); > -extern struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev); > +extern struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev, int tag); > extern int ata_build_rw_tf(struct ata_taskfile *tf, struct ata_device *dev, > u64 block, u32 n_block, unsigned int tf_flags, > unsigned int tag); > diff --git a/include/linux/libata.h b/include/linux/libata.h > index 2d18241..5f1c5606 100644 > --- a/include/linux/libata.h > +++ b/include/linux/libata.h > @@ -1344,6 +1344,7 @@ extern struct device_attribute *ata_common_sdev_attrs[]; > .ioctl = ata_scsi_ioctl, \ > .queuecommand = ata_scsi_queuecmd, \ > .can_queue = ATA_DEF_QUEUE, \ > + .tag_alloc_policy = BLK_TAG_ALLOC_RR, \ > .this_id = ATA_SHT_THIS_ID, \ > .cmd_per_lun = ATA_SHT_CMD_PER_LUN, \ > .emulated = ATA_SHT_EMULATED, \ > -- > 1.8.1 > -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jan 09, 2015 at 10:15:29AM -0800, Shaohua Li wrote:
> Ping!
I like the idea but it bothers me that we end up with two separate
ways of allocating command tags. Any chance the old one can be
removed?
Thanks.
On Fri, Jan 09, 2015 at 04:43:07PM -0500, Tejun Heo wrote: > On Fri, Jan 09, 2015 at 10:15:29AM -0800, Shaohua Li wrote: > > Ping! > > I like the idea but it bothers me that we end up with two separate > ways of allocating command tags. Any chance the old one can be > removed? > > Thanks. > > -- > tejun -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jan 09, 2015 at 01:59:19PM -0800, Shaohua Li wrote: > On Fri, Jan 09, 2015 at 04:43:07PM -0500, Tejun Heo wrote: > > On Fri, Jan 09, 2015 at 10:15:29AM -0800, Shaohua Li wrote: > > > Ping! > > > > I like the idea but it bothers me that we end up with two separate > > ways of allocating command tags. Any chance the old one can be > > removed? Oh, sorry, my reply truncated. So I checked with IPR driver guys, the ipr doesn't use ncq, so we can always use sata tag 0, but not sure for libsas. Maybe Dan can answer if there is a way we can map SCSI tag to SATA tag. Thanks, Shaohua -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jan 9, 2015 at 2:12 PM, Shaohua Li <shli@fb.com> wrote: > On Fri, Jan 09, 2015 at 01:59:19PM -0800, Shaohua Li wrote: >> On Fri, Jan 09, 2015 at 04:43:07PM -0500, Tejun Heo wrote: >> > On Fri, Jan 09, 2015 at 10:15:29AM -0800, Shaohua Li wrote: >> > > Ping! >> > >> > I like the idea but it bothers me that we end up with two separate >> > ways of allocating command tags. Any chance the old one can be >> > removed? > > Oh, sorry, my reply truncated. > > So I checked with IPR driver guys, the ipr doesn't use ncq, so we can > always use sata tag 0, but not sure for libsas. > > Maybe Dan can answer if there is a way we can map SCSI tag to SATA tag. > For libsas or for ata drivers? For libsas, iirc, the internal libata tag is ignored and we use the scsi tag for the sas task For ata some drivers want round robin, but others appear to care about using the lowest available: https://bugzilla.kernel.org/show_bug.cgi?id=87101. Is ata using it's own legacy tag ordering scheme getting in the way of other improvements? I'd just as soon recommend letting legacy dogs lie. In other words what do we gain by switching? I need to follow up on bz87101, seems I never reworked the patch as Tejun asked. However, I'm glad a fix like the one proposed in that report can be self-contained to libata and need not worry about supporting ata specific quirks in the block layer tag ordering scheme. -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 14, 2015 at 12:08:26AM -0800, Dan Williams wrote: > On Fri, Jan 9, 2015 at 2:12 PM, Shaohua Li <shli@fb.com> wrote: > > On Fri, Jan 09, 2015 at 01:59:19PM -0800, Shaohua Li wrote: > >> On Fri, Jan 09, 2015 at 04:43:07PM -0500, Tejun Heo wrote: > >> > On Fri, Jan 09, 2015 at 10:15:29AM -0800, Shaohua Li wrote: > >> > > Ping! > >> > > >> > I like the idea but it bothers me that we end up with two separate > >> > ways of allocating command tags. Any chance the old one can be > >> > removed? > > > > Oh, sorry, my reply truncated. > > > > So I checked with IPR driver guys, the ipr doesn't use ncq, so we can > > always use sata tag 0, but not sure for libsas. > > > > Maybe Dan can answer if there is a way we can map SCSI tag to SATA tag. > > > > For libsas or for ata drivers? For libsas, iirc, the internal libata > tag is ignored and we use the scsi tag for the sas task For ata some > drivers want round robin, but others appear to care about using the > lowest available: https://urldefense.proofpoint.com/v1/url?u=https://bugzilla.kernel.org/show_bug.cgi?id%3D87101&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=3RFlL12A7nwmLRXunVJq2g%3D%3D%0A&m=i5Fz0FbsOgTMAtXW2WeXro9juvX%2BRmkKOK%2Fnl5ZZWMw%3D%0A&s=b8747b79ff51a364a841dd8e2ff51814536fcd63eedb7ad8f1de68e6691e5e98. > > Is ata using it's own legacy tag ordering scheme getting in the way of > other improvements? I'd just as soon recommend letting legacy dogs > lie. In other words what do we gain by switching? > > I need to follow up on bz87101, seems I never reworked the patch as > Tejun asked. However, I'm glad a fix like the one proposed in that > report can be self-contained to libata and need not worry about > supporting ata specific quirks in the block layer tag ordering scheme. Basically I'd like using block tag instead of an open coded tag implementation in libata. The block tag implenmentation is more sophisticated, and with blk-mq tag is built-in, so it's great to remove tag implementation in libata. For sata, it's easy to do it. The problem is sas, which implements its own scsi driver/tag. My question is if we can map SCSI tag (of libsas) to ATA tag. So for example, the ipr sas driver doesn't use NCQ, so we can always use ata tag 0 in libata for ipr. I'm wondering if we can do some mapping in libsas too, so we can completely delete the tag code in libata. Thanks, Shaohua -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[ excuse the re-send ] On Wed, Jan 14, 2015 at 8:30 AM, Shaohua Li <shli@fb.com> wrote: > On Wed, Jan 14, 2015 at 12:08:26AM -0800, Dan Williams wrote: >> On Fri, Jan 9, 2015 at 2:12 PM, Shaohua Li <shli@fb.com> wrote: >> > On Fri, Jan 09, 2015 at 01:59:19PM -0800, Shaohua Li wrote: >> >> On Fri, Jan 09, 2015 at 04:43:07PM -0500, Tejun Heo wrote: >> >> > On Fri, Jan 09, 2015 at 10:15:29AM -0800, Shaohua Li wrote: >> >> > > Ping! >> >> > >> >> > I like the idea but it bothers me that we end up with two separate >> >> > ways of allocating command tags. Any chance the old one can be >> >> > removed? >> > >> > Oh, sorry, my reply truncated. >> > >> > So I checked with IPR driver guys, the ipr doesn't use ncq, so we can >> > always use sata tag 0, but not sure for libsas. >> > >> > Maybe Dan can answer if there is a way we can map SCSI tag to SATA tag. >> > >> >> For libsas or for ata drivers? For libsas, iirc, the internal libata >> tag is ignored and we use the scsi tag for the sas task For ata some >> drivers want round robin, but others appear to care about using the >> lowest available: https://urldefense.proofpoint.com/v1/url?u=https://bugzilla.kernel.org/show_bug.cgi?id%3D87101&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=3RFlL12A7nwmLRXunVJq2g%3D%3D%0A&m=i5Fz0FbsOgTMAtXW2WeXro9juvX%2BRmkKOK%2Fnl5ZZWMw%3D%0A&s=b8747b79ff51a364a841dd8e2ff51814536fcd63eedb7ad8f1de68e6691e5e98. >> >> Is ata using it's own legacy tag ordering scheme getting in the way of >> other improvements? I'd just as soon recommend letting legacy dogs >> lie. In other words what do we gain by switching? >> >> I need to follow up on bz87101, seems I never reworked the patch as >> Tejun asked. However, I'm glad a fix like the one proposed in that >> report can be self-contained to libata and need not worry about >> supporting ata specific quirks in the block layer tag ordering scheme. > > Basically I'd like using block tag instead of an open coded tag > implementation in libata. The block tag implenmentation is more > sophisticated, and with blk-mq tag is built-in, so it's great to remove > tag implementation in libata. Is it great to remove? It's not clear what the cost of maintaining it is. How would you handle the case where some ata controllers want the lowest available tag vs others that want round robin. > For sata, it's easy to do it. The problem > is sas, which implements its own scsi driver/tag. My question is if we > can map SCSI tag (of libsas) to ATA tag. So for example, the ipr sas > driver doesn't use NCQ, so we can always use ata tag 0 in libata for > ipr. I'm wondering if we can do some mapping in libsas too, so we can > completely delete the tag code in libata. The problem with using the scsi tag for libsas is that the scsi_host may support 256 commands, but only 32 per ata device. Seems like a problem blk-mq has already solved (controller + per-endpoint tag queues), but I have not looked... -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 14, 2015 at 09:04:24AM -0800, Dan Williams wrote: > On Wed, Jan 14, 2015 at 8:30 AM, Shaohua Li <shli@fb.com> wrote: > > > On Wed, Jan 14, 2015 at 12:08:26AM -0800, Dan Williams wrote: > > > On Fri, Jan 9, 2015 at 2:12 PM, Shaohua Li <shli@fb.com> wrote: > > > > On Fri, Jan 09, 2015 at 01:59:19PM -0800, Shaohua Li wrote: > > > >> On Fri, Jan 09, 2015 at 04:43:07PM -0500, Tejun Heo wrote: > > > >> > On Fri, Jan 09, 2015 at 10:15:29AM -0800, Shaohua Li wrote: > > > >> > > Ping! > > > >> > > > > >> > I like the idea but it bothers me that we end up with two separate > > > >> > ways of allocating command tags. Any chance the old one can be > > > >> > removed? > > > > > > > > Oh, sorry, my reply truncated. > > > > > > > > So I checked with IPR driver guys, the ipr doesn't use ncq, so we can > > > > always use sata tag 0, but not sure for libsas. > > > > > > > > Maybe Dan can answer if there is a way we can map SCSI tag to SATA tag. > > > > > > > > > > For libsas or for ata drivers? For libsas, iirc, the internal libata > > > tag is ignored and we use the scsi tag for the sas task For ata some > > > drivers want round robin, but others appear to care about using the > > > lowest available: > > https://urldefense.proofpoint.com/v1/url?u=https://bugzilla.kernel.org/show_bug.cgi?id%3D87101&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=3RFlL12A7nwmLRXunVJq2g%3D%3D%0A&m=i5Fz0FbsOgTMAtXW2WeXro9juvX%2BRmkKOK%2Fnl5ZZWMw%3D%0A&s=b8747b79ff51a364a841dd8e2ff51814536fcd63eedb7ad8f1de68e6691e5e98 > > . > > > > > > Is ata using it's own legacy tag ordering scheme getting in the way of > > > other improvements? I'd just as soon recommend letting legacy dogs > > > lie. In other words what do we gain by switching? > > > > > > I need to follow up on bz87101, seems I never reworked the patch as > > > Tejun asked. However, I'm glad a fix like the one proposed in that > > > report can be self-contained to libata and need not worry about > > > supporting ata specific quirks in the block layer tag ordering scheme. > > > > Basically I'd like using block tag instead of an open coded tag > > implementation in libata. The block tag implenmentation is more > > sophisticated, and with blk-mq tag is built-in, so it's great to remove > > tag implementation in libata. > > > Is it great to remove? It's not clear what the cost of maintaining it is. > How would you handle the case where some ata controllers want the lowest > available tag vs others that want round robin. I added a flag to blk-tag/blk-mq tag to implement different tag allocation strategy, for example, round robin and fifo. > > For sata, it's easy to do it. The problem > > is sas, which implements its own scsi driver/tag. My question is if we > > can map SCSI tag (of libsas) to ATA tag. So for example, the ipr sas > > driver doesn't use NCQ, so we can always use ata tag 0 in libata for > > ipr. I'm wondering if we can do some mapping in libsas too, so we can > > completely delete the tag code in libata. > > > > The problem with using the scsi tag for libsas is that the scsi_host may > support 256 commands, but only 32 per ata device. Seems like a problem > blk-mq has already solved (controller + per-endpoint tag queues), but I > have not looked... This is exactly the issue I'm talking about. Is there a way to map the 256 (scsi tag) to ata tag for libsas? Like I said, for ipr sas, I'll do scsi tag -> ata tag 0, since it doesn't use NCQ for SATA. Is there something similar or whatever for libsas? From search, ipr and libsas are the only scsi drivers using libata. Thanks, Shaohua -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 14, 2015 at 9:13 AM, Shaohua Li <shli@fb.com> wrote: > On Wed, Jan 14, 2015 at 09:04:24AM -0800, Dan Williams wrote: >> On Wed, Jan 14, 2015 at 8:30 AM, Shaohua Li <shli@fb.com> wrote: >> >> > On Wed, Jan 14, 2015 at 12:08:26AM -0800, Dan Williams wrote: >> > > On Fri, Jan 9, 2015 at 2:12 PM, Shaohua Li <shli@fb.com> wrote: >> > > > On Fri, Jan 09, 2015 at 01:59:19PM -0800, Shaohua Li wrote: >> > > >> On Fri, Jan 09, 2015 at 04:43:07PM -0500, Tejun Heo wrote: >> > > >> > On Fri, Jan 09, 2015 at 10:15:29AM -0800, Shaohua Li wrote: >> > > >> > > Ping! >> > > >> > >> > > >> > I like the idea but it bothers me that we end up with two separate >> > > >> > ways of allocating command tags. Any chance the old one can be >> > > >> > removed? >> > > > >> > > > Oh, sorry, my reply truncated. >> > > > >> > > > So I checked with IPR driver guys, the ipr doesn't use ncq, so we can >> > > > always use sata tag 0, but not sure for libsas. >> > > > >> > > > Maybe Dan can answer if there is a way we can map SCSI tag to SATA tag. >> > > > >> > > >> > > For libsas or for ata drivers? For libsas, iirc, the internal libata >> > > tag is ignored and we use the scsi tag for the sas task For ata some >> > > drivers want round robin, but others appear to care about using the >> > > lowest available: >> > https://urldefense.proofpoint.com/v1/url?u=https://bugzilla.kernel.org/show_bug.cgi?id%3D87101&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=3RFlL12A7nwmLRXunVJq2g%3D%3D%0A&m=i5Fz0FbsOgTMAtXW2WeXro9juvX%2BRmkKOK%2Fnl5ZZWMw%3D%0A&s=b8747b79ff51a364a841dd8e2ff51814536fcd63eedb7ad8f1de68e6691e5e98 >> > . >> > > >> > > Is ata using it's own legacy tag ordering scheme getting in the way of >> > > other improvements? I'd just as soon recommend letting legacy dogs >> > > lie. In other words what do we gain by switching? >> > > >> > > I need to follow up on bz87101, seems I never reworked the patch as >> > > Tejun asked. However, I'm glad a fix like the one proposed in that >> > > report can be self-contained to libata and need not worry about >> > > supporting ata specific quirks in the block layer tag ordering scheme. >> > >> > Basically I'd like using block tag instead of an open coded tag >> > implementation in libata. The block tag implenmentation is more >> > sophisticated, and with blk-mq tag is built-in, so it's great to remove >> > tag implementation in libata. >> >> >> Is it great to remove? It's not clear what the cost of maintaining it is. >> How would you handle the case where some ata controllers want the lowest >> available tag vs others that want round robin. > > I added a flag to blk-tag/blk-mq tag to implement different tag > allocation strategy, for example, round robin and fifo. Right, but you seem to do it at the scsi_host level, if I'm not mistaken. Isn't that too high for ata quirks? >> > For sata, it's easy to do it. The problem >> > is sas, which implements its own scsi driver/tag. My question is if we >> > can map SCSI tag (of libsas) to ATA tag. So for example, the ipr sas >> > driver doesn't use NCQ, so we can always use ata tag 0 in libata for >> > ipr. I'm wondering if we can do some mapping in libsas too, so we can >> > completely delete the tag code in libata. >> > >> >> The problem with using the scsi tag for libsas is that the scsi_host may >> support 256 commands, but only 32 per ata device. Seems like a problem >> blk-mq has already solved (controller + per-endpoint tag queues), but I >> have not looked... > > This is exactly the issue I'm talking about. Is there a way to map the > 256 (scsi tag) to ata tag for libsas? Like I said, for ipr sas, I'll do > scsi tag -> ata tag 0, since it doesn't use NCQ for SATA. Is there > something similar or whatever for libsas? From search, ipr and libsas > are the only scsi drivers using libata. libsas uses the libata allocated tag for NCQ. libsas first asks libata to allocate a tag/qc for ncq, then it creates a sas_task and assigns the scsi tag. To use a generic allocator I assume you would need to flip that ordering to allocate the sas_task tag first and then use that plus another key to allocate a sub-tag for ata to use for NCQ. -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 14, 2015 at 09:37:30AM -0800, Dan Williams wrote: > libsas uses the libata allocated tag for NCQ. libsas first asks > libata to allocate a tag/qc for ncq, then it creates a sas_task and > assigns the scsi tag. To use a generic allocator I assume you would > need to flip that ordering to allocate the sas_task tag first and then > use that plus another key to allocate a sub-tag for ata to use for > NCQ. As far as I can tell all libsas drivers as well as ipr use block layer tagging, so they always get requests/scsi_cmnds with a tag already assigned. For libsas drivers currently use per-LUN tags, although with scsi-mq we will get per-host tags. libata on the other hand at the moment assigns it's own tags, which are only stored in struct ata_queued_cmd, but never in the request. libata assigns tags per ata_host. Each ata_host might have multiple ports, which each get their own Scsi_Host allocated. Given that ATA NCQ has a fairly low queue depth I assume we want to stick to assigning ATA tags per-device, but due to the host per device scheme libata that even works using scsi-mq. (Q: how do tags work when port mulipliers are involved?) So for SAS driver using libata we will need a separate tag allocator, but that one might as well be in libsas instead of keeping it in libata. -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On Thu, Jan 15, 2015 at 01:28:36AM -0800, Christoph Hellwig wrote: > libata on the other hand at the moment assigns it's own tags, > which are only stored in struct ata_queued_cmd, but never in the > request. libata assigns tags per ata_host. Each ata_host Per ata_port. > might have multiple ports, which each get their own Scsi_Host > allocated. > > Given that ATA NCQ has a fairly low queue depth I assume we want > to stick to assigning ATA tags per-device, but due to the host > per device scheme libata that even works using scsi-mq. (Q: how do tags > work when port mulipliers are involved?) It's still per-port. The same number of tags are shared among all devices attached via PMs. Thanks.
On Thu, Jan 15, 2015 at 01:28:36AM -0800, Christoph Hellwig wrote: > On Wed, Jan 14, 2015 at 09:37:30AM -0800, Dan Williams wrote: > > libsas uses the libata allocated tag for NCQ. libsas first asks > > libata to allocate a tag/qc for ncq, then it creates a sas_task and > > assigns the scsi tag. To use a generic allocator I assume you would > > need to flip that ordering to allocate the sas_task tag first and then > > use that plus another key to allocate a sub-tag for ata to use for > > NCQ. > > As far as I can tell all libsas drivers as well as ipr use block > layer tagging, so they always get requests/scsi_cmnds with a tag > already assigned. For libsas drivers currently use per-LUN tags, > although with scsi-mq we will get per-host tags. > > libata on the other hand at the moment assigns it's own tags, > which are only stored in struct ata_queued_cmd, but never in the > request. libata assigns tags per ata_host. Each ata_host > might have multiple ports, which each get their own Scsi_Host > allocated. > > Given that ATA NCQ has a fairly low queue depth I assume we want > to stick to assigning ATA tags per-device, but due to the host > per device scheme libata that even works using scsi-mq. (Q: how do tags > work when port mulipliers are involved?) > > So for SAS driver using libata we will need a separate tag allocator, > but that one might as well be in libsas instead of keeping it in libata. Yep, the ATA NCQ should work well with block tag as each port is a scsi host. So looks it's not easy to map scsi tag to ATA tag for libsas and we eventually will have another tag allocator for libsas. Tejun, I'm afraid I can't work out a patch to delete the old tag allocator for libata. Can we take this patch series first and later we can delete the old tag allocator till libsas has its own tag allocator. This isn't optimal, but I really know nothing about libsas. Thanks, Shaohua -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, Shaohua. On Thu, Jan 15, 2015 at 10:40:08AM -0800, Shaohua Li wrote: > I'm afraid I can't work out a patch to delete the old tag allocator for > libata. Can we take this patch series first and later we can delete the > old tag allocator till libsas has its own tag allocator. This isn't > optimal, but I really know nothing about libsas. As long as it's clear that this is libsas-only and the rest of code doesn't have to care about it, I guess it'll have to do. But please do isolate it clearly. Thanks.
On Thu, Jan 15, 2015 at 10:40 AM, Shaohua Li <shli@fb.com> wrote: > On Thu, Jan 15, 2015 at 01:28:36AM -0800, Christoph Hellwig wrote: >> On Wed, Jan 14, 2015 at 09:37:30AM -0800, Dan Williams wrote: >> > libsas uses the libata allocated tag for NCQ. libsas first asks >> > libata to allocate a tag/qc for ncq, then it creates a sas_task and >> > assigns the scsi tag. To use a generic allocator I assume you would >> > need to flip that ordering to allocate the sas_task tag first and then >> > use that plus another key to allocate a sub-tag for ata to use for >> > NCQ. >> >> As far as I can tell all libsas drivers as well as ipr use block >> layer tagging, so they always get requests/scsi_cmnds with a tag >> already assigned. For libsas drivers currently use per-LUN tags, >> although with scsi-mq we will get per-host tags. >> >> libata on the other hand at the moment assigns it's own tags, >> which are only stored in struct ata_queued_cmd, but never in the >> request. libata assigns tags per ata_host. Each ata_host >> might have multiple ports, which each get their own Scsi_Host >> allocated. >> >> Given that ATA NCQ has a fairly low queue depth I assume we want >> to stick to assigning ATA tags per-device, but due to the host >> per device scheme libata that even works using scsi-mq. (Q: how do tags >> work when port mulipliers are involved?) >> >> So for SAS driver using libata we will need a separate tag allocator, >> but that one might as well be in libsas instead of keeping it in libata. > > Yep, the ATA NCQ should work well with block tag as each port is a > scsi host. So looks it's not easy to map scsi tag to ATA tag for libsas > and we eventually will have another tag allocator for libsas. > > Tejun, > I'm afraid I can't work out a patch to delete the old tag allocator for > libata. Can we take this patch series first and later we can delete the > old tag allocator till libsas has its own tag allocator. This isn't > optimal, but I really know nothing about libsas. I still don't understand what we get by adding this new allocator besides complexity, am I missing something? -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jan 15, 2015 at 10:59:36AM -0800, Dan Williams wrote: > I still don't understand what we get by adding this new allocator > besides complexity, am I missing something? Hmmm... one problem with the existing tag allocator in ata is that it's not very efficient which actually shows up in profile when libata is used with a very zippy SSD. Given that ata needs a different allocation policies anyway maybe the right thing to do is making the existing allocator suck less. Thanks.
On 01/15/2015 11:59 AM, Dan Williams wrote: > On Thu, Jan 15, 2015 at 10:40 AM, Shaohua Li <shli@fb.com> wrote: >> On Thu, Jan 15, 2015 at 01:28:36AM -0800, Christoph Hellwig wrote: >>> On Wed, Jan 14, 2015 at 09:37:30AM -0800, Dan Williams wrote: >>>> libsas uses the libata allocated tag for NCQ. libsas first asks >>>> libata to allocate a tag/qc for ncq, then it creates a sas_task and >>>> assigns the scsi tag. To use a generic allocator I assume you would >>>> need to flip that ordering to allocate the sas_task tag first and then >>>> use that plus another key to allocate a sub-tag for ata to use for >>>> NCQ. >>> >>> As far as I can tell all libsas drivers as well as ipr use block >>> layer tagging, so they always get requests/scsi_cmnds with a tag >>> already assigned. For libsas drivers currently use per-LUN tags, >>> although with scsi-mq we will get per-host tags. >>> >>> libata on the other hand at the moment assigns it's own tags, >>> which are only stored in struct ata_queued_cmd, but never in the >>> request. libata assigns tags per ata_host. Each ata_host >>> might have multiple ports, which each get their own Scsi_Host >>> allocated. >>> >>> Given that ATA NCQ has a fairly low queue depth I assume we want >>> to stick to assigning ATA tags per-device, but due to the host >>> per device scheme libata that even works using scsi-mq. (Q: how do tags >>> work when port mulipliers are involved?) >>> >>> So for SAS driver using libata we will need a separate tag allocator, >>> but that one might as well be in libsas instead of keeping it in libata. >> >> Yep, the ATA NCQ should work well with block tag as each port is a >> scsi host. So looks it's not easy to map scsi tag to ATA tag for libsas >> and we eventually will have another tag allocator for libsas. >> >> Tejun, >> I'm afraid I can't work out a patch to delete the old tag allocator for >> libata. Can we take this patch series first and later we can delete the >> old tag allocator till libsas has its own tag allocator. This isn't >> optimal, but I really know nothing about libsas. > > I still don't understand what we get by adding this new allocator > besides complexity, am I missing something? Two things: - libata tag allocator sucks. Like seriously sucks, it's almost a worst case implementation. - Much better to have a single unified allocator to tweak and tune, than having separate version. #2 is still lacking a bit, but I don't think it'd be impossible to unify it all.
On Thu, Jan 15, 2015 at 11:15 AM, Jens Axboe <axboe@fb.com> wrote: > On 01/15/2015 11:59 AM, Dan Williams wrote: >> I still don't understand what we get by adding this new allocator >> besides complexity, am I missing something? > > > Two things: > > - libata tag allocator sucks. Like seriously sucks, it's almost a worst case > implementation. Not questioning its suckiness, but I thought the SATA suckiness made it moot. Apparently not in all cases... > - Much better to have a single unified allocator to tweak and tune, than > having separate version. > > #2 is still lacking a bit, but I don't think it'd be impossible to unify it > all. https://bugzilla.kernel.org/show_bug.cgi?id=87101 has gone silent, I need to ping it. That's my primary concern with the current proposal, supporting controllers that have weird/unnatural relationships with the value of the tag. -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/15/2015 12:51 PM, Dan Williams wrote: > On Thu, Jan 15, 2015 at 11:15 AM, Jens Axboe <axboe@fb.com> wrote: >> On 01/15/2015 11:59 AM, Dan Williams wrote: >>> I still don't understand what we get by adding this new allocator >>> besides complexity, am I missing something? >> >> >> Two things: >> >> - libata tag allocator sucks. Like seriously sucks, it's almost a worst case >> implementation. > > Not questioning its suckiness, but I thought the SATA suckiness made > it moot. Apparently not in all cases... The laptop I'm typing this from does 145K 4k random read IOPS, it's definitely into the area of it mattering. >> - Much better to have a single unified allocator to tweak and tune, than >> having separate version. >> >> #2 is still lacking a bit, but I don't think it'd be impossible to unify it >> all. > > https://bugzilla.kernel.org/show_bug.cgi?id=87101 has gone silent, I > need to ping it. That's my primary concern with the current proposal, > supporting controllers that have weird/unnatural relationships with > the value of the tag. Unfortunately parts of SATA is as crappy as USB when it comes to things like that. I can understand why some controllers would like to see a natural ordering of the tags (even if it is stupid to require, but AHCI doesn't help there), but it makes very little sense why it would break others. Looks like this particular case was likely a different bug, the ordering just made it show up more easily. And speaking of strict ordering, the blk-mq tagging should actually improve ordering. The libata implementation orders globally, but that'll equally break down on multiple processes accessing the device. For that case, you end up interleaving, and if the drive does strict by-tag ordering of what IO to do, it'll go random pretty quickly. The blk-mq implementation preserves ordering between threads in that case, due to how the last tag is cached. So I would expect to see an improvement in behavior with that for use cases that offload IO to thread pools (like posix aio, or private implementations in programs).
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index 5c84fb5..a27d00f 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -1585,8 +1585,9 @@ unsigned ata_exec_internal_sg(struct ata_device *dev, else tag = 0; - if (test_and_set_bit(tag, &ap->qc_allocated)) - BUG(); + BUG_ON((!ap->scsi_host && test_and_set_bit(tag, &ap->qc_allocated)) || + (ap->scsi_host && dev->sdev && + blk_queue_find_tag(dev->sdev->request_queue, tag))); qc = __ata_qc_from_tag(ap, tag); qc->tag = tag; @@ -4737,7 +4738,7 @@ void swap_buf_le16(u16 *buf, unsigned int buf_words) * None. */ -static struct ata_queued_cmd *ata_qc_new(struct ata_port *ap) +static struct ata_queued_cmd *ata_qc_new(struct ata_port *ap, int blktag) { struct ata_queued_cmd *qc = NULL; unsigned int max_queue = ap->host->n_tags; @@ -4747,6 +4748,12 @@ static struct ata_queued_cmd *ata_qc_new(struct ata_port *ap) if (unlikely(ap->pflags & ATA_PFLAG_FROZEN)) return NULL; + if (ap->scsi_host) { + qc = __ata_qc_from_tag(ap, blktag); + qc->tag = blktag; + return qc; + } + for (i = 0, tag = ap->last_tag + 1; i < max_queue; i++, tag++) { tag = tag < max_queue ? tag : 0; @@ -4773,12 +4780,12 @@ static struct ata_queued_cmd *ata_qc_new(struct ata_port *ap) * None. */ -struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev) +struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev, int blktag) { struct ata_port *ap = dev->link->ap; struct ata_queued_cmd *qc; - qc = ata_qc_new(ap); + qc = ata_qc_new(ap, blktag); if (qc) { qc->scsicmd = NULL; qc->ap = ap; @@ -4812,7 +4819,8 @@ void ata_qc_free(struct ata_queued_cmd *qc) tag = qc->tag; if (likely(ata_tag_valid(tag))) { qc->tag = ATA_TAG_POISON; - clear_bit(tag, &ap->qc_allocated); + if (!ap->scsi_host) + clear_bit(tag, &ap->qc_allocated); } } diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index e364e86..94339c2 100644 --- a/drivers/ata/libata-scsi.c +++ b/drivers/ata/libata-scsi.c @@ -756,7 +756,7 @@ static struct ata_queued_cmd *ata_scsi_qc_new(struct ata_device *dev, { struct ata_queued_cmd *qc; - qc = ata_qc_new_init(dev); + qc = ata_qc_new_init(dev, cmd->request->tag); if (qc) { qc->scsicmd = cmd; qc->scsidone = cmd->scsi_done; @@ -3666,6 +3666,8 @@ int ata_scsi_add_hosts(struct ata_host *host, struct scsi_host_template *sht) */ shost->max_host_blocked = 1; + scsi_init_shared_tag_map(shost, host->n_tags); + rc = scsi_add_host_with_dma(ap->scsi_host, &ap->tdev, ap->host->dev); if (rc) diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h index 5f4e0cc..4040513 100644 --- a/drivers/ata/libata.h +++ b/drivers/ata/libata.h @@ -63,7 +63,7 @@ extern struct ata_link *ata_dev_phys_link(struct ata_device *dev); extern void ata_force_cbl(struct ata_port *ap); extern u64 ata_tf_to_lba(const struct ata_taskfile *tf); extern u64 ata_tf_to_lba48(const struct ata_taskfile *tf); -extern struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev); +extern struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev, int tag); extern int ata_build_rw_tf(struct ata_taskfile *tf, struct ata_device *dev, u64 block, u32 n_block, unsigned int tf_flags, unsigned int tag); diff --git a/include/linux/libata.h b/include/linux/libata.h index 2d18241..5f1c5606 100644 --- a/include/linux/libata.h +++ b/include/linux/libata.h @@ -1344,6 +1344,7 @@ extern struct device_attribute *ata_common_sdev_attrs[]; .ioctl = ata_scsi_ioctl, \ .queuecommand = ata_scsi_queuecmd, \ .can_queue = ATA_DEF_QUEUE, \ + .tag_alloc_policy = BLK_TAG_ALLOC_RR, \ .this_id = ATA_SHT_THIS_ID, \ .cmd_per_lun = ATA_SHT_CMD_PER_LUN, \ .emulated = ATA_SHT_EMULATED, \
libata uses its own tag management which is duplication and the implementation is poor. And if we switch to blk-mq, tag is build-in. It's time to switch to generic taging. The SAS driver has its own tag management, and looks we can't directly map the host controler tag to SATA tag. So I just bypassed the SAS case. Cc: Jens Axboe <axboe@fb.com> Cc: Tejun Heo <tj@kernel.org> Cc: Christoph Hellwig <hch@infradead.org> Signed-off-by: Shaohua Li <shli@fb.com> --- drivers/ata/libata-core.c | 20 ++++++++++++++------ drivers/ata/libata-scsi.c | 4 +++- drivers/ata/libata.h | 2 +- include/linux/libata.h | 1 + 4 files changed, 19 insertions(+), 8 deletions(-)