| Message ID | 20150116221600.GA1099317@devbig257.prn2.facebook.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
Hi Tejun, are you ok to pick up these patches? Thanks, Shaohua On Fri, Jan 16, 2015 at 02:16:00PM -0800, Shaohua Li wrote: > On Fri, Jan 16, 2015 at 12:23:44AM -0800, Christoph Hellwig wrote: > > > +static bool ata_valid_internal_tag(struct ata_port *ap, struct ata_device *dev, > > > + unsigned int tag) > > > +{ > > > + if (!ap->scsi_host) > > > + return !test_and_set_bit(tag, &ap->sas_tag_allocated); > > > + return !dev->sdev || > > > + !blk_queue_find_tag(dev->sdev->request_queue, tag); > > > > How is this supposed to work with blk-mq? > > > > > +} > > > + > > > /** > > > * ata_exec_internal_sg - execute libata internal command > > > * @dev: Device to which the command is sent > > > @@ -1585,8 +1594,7 @@ unsigned ata_exec_internal_sg(struct ata_device *dev, > > > else > > > tag = 0; > > > > > > - if (test_and_set_bit(tag, &ap->qc_allocated)) > > > - BUG(); > > > + BUG_ON(!ata_valid_internal_tag(ap, dev, tag)); > > > > But given that this is just a single assert we might as well just > > remove that whole thing. > > Ok, I delete it > > > > +static struct ata_queued_cmd *ata_qc_new(struct ata_port *ap, int blktag) > > > +{ > > > + struct ata_queued_cmd *qc; > > > + > > > + /* no command while frozen */ > > > + if (unlikely(ap->pflags & ATA_PFLAG_FROZEN)) > > > + return NULL; > > > + > > > + /* SATA will directly use block tag. libsas need its own tag management */ > > > + if (ap->scsi_host) { > > > + qc = __ata_qc_from_tag(ap, blktag); > > > + qc->tag = blktag; > > > + return qc; > > > + } > > > + > > > + return sas_ata_qc_new(ap); > > > > Why don't you merge tis into ata_qc_new_init? > > > > > +static void sas_ata_qc_free(unsigned int tag, struct ata_port *ap) > > > +{ > > > + if (!ap->scsi_host) > > > > Given the sas_ name I'd move the check into the caller. > > > > That beeing said I wonder if you shouldn't do all this at a much > > higher level. > > > > __ata_scsi_queuecmd has a two callers, one for sata, and one for sas. > > If you pass in the tag from that level the low lever code won't > > need any branches for sas vs libata. The only downside would be > > that you now consume a tag for simulated command on SAS, but as that's > > already done for SATA it can't be that bad. > > I can do this in __ata_scsi_queuecmd, but don't see a good place to free > the tag, which really should be done at scsi_done. The whole thing > should be done at a higher level. I moved the ata tag allocation code to > libata-scsi.c to make it clearer. > > > From 125d41a001b8555fac42ea5b35dae45049655ab0 Mon Sep 17 00:00:00 2001 > Message-Id: <125d41a001b8555fac42ea5b35dae45049655ab0.1421446044.git.shli@fb.com> > In-Reply-To: <81b8ef84122544faa1f37a404cf2e7c71791d961.1421446044.git.shli@fb.com> > References: <81b8ef84122544faa1f37a404cf2e7c71791d961.1421446044.git.shli@fb.com> > From: Shaohua Li <shli@fb.com> > Date: Thu, 18 Dec 2014 09:04:25 -0800 > Subject: [PATCH 3/3] libata: use blk taging > > 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. > > I changed the code/variable name for the tag management of libata to > make it self contained. Only sas will use it. Later if libsas implements > its tag management, the tag management code in libata can be deleted > easily. > > 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 | 67 +++++++++++++---------------------------------- > drivers/ata/libata-scsi.c | 30 ++++++++++++++++++++- > drivers/ata/libata.h | 4 ++- > include/linux/libata.h | 5 ++-- > 4 files changed, 53 insertions(+), 53 deletions(-) > > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c > index 5c84fb5..d626605 100644 > --- a/drivers/ata/libata-core.c > +++ b/drivers/ata/libata-core.c > @@ -1585,8 +1585,6 @@ unsigned ata_exec_internal_sg(struct ata_device *dev, > else > tag = 0; > > - if (test_and_set_bit(tag, &ap->qc_allocated)) > - BUG(); > qc = __ata_qc_from_tag(ap, tag); > > qc->tag = tag; > @@ -4726,66 +4724,36 @@ void swap_buf_le16(u16 *buf, unsigned int buf_words) > } > > /** > - * ata_qc_new - Request an available ATA command, for queueing > - * @ap: target port > - * > - * Some ATA host controllers may implement a queue depth which is less > - * than ATA_MAX_QUEUE. So we shouldn't allocate a tag which is beyond > - * the hardware limitation. > + * ata_qc_new_init - Request an available ATA command, and initialize it > + * @dev: Device from whom we request an available command structure > * > * LOCKING: > * None. > */ > > -static struct ata_queued_cmd *ata_qc_new(struct ata_port *ap) > +struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev, int tag) > { > - struct ata_queued_cmd *qc = NULL; > - unsigned int max_queue = ap->host->n_tags; > - unsigned int i, tag; > + struct ata_port *ap = dev->link->ap; > + struct ata_queued_cmd *qc; > > /* no command while frozen */ > if (unlikely(ap->pflags & ATA_PFLAG_FROZEN)) > return NULL; > > - for (i = 0, tag = ap->last_tag + 1; i < max_queue; i++, tag++) { > - tag = tag < max_queue ? tag : 0; > - > - /* the last tag is reserved for internal command. */ > - if (tag == ATA_TAG_INTERNAL) > - continue; > - > - if (!test_and_set_bit(tag, &ap->qc_allocated)) { > - qc = __ata_qc_from_tag(ap, tag); > - qc->tag = tag; > - ap->last_tag = tag; > - break; > - } > + /* libsas case */ > + if (!ap->scsi_host) { > + tag = ata_sas_allocate_tag(ap); > + if (tag < 0) > + return NULL; > } > > - return qc; > -} > - > -/** > - * ata_qc_new_init - Request an available ATA command, and initialize it > - * @dev: Device from whom we request an available command structure > - * > - * LOCKING: > - * None. > - */ > - > -struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev) > -{ > - struct ata_port *ap = dev->link->ap; > - struct ata_queued_cmd *qc; > - > - qc = ata_qc_new(ap); > - if (qc) { > - qc->scsicmd = NULL; > - qc->ap = ap; > - qc->dev = dev; > + qc = __ata_qc_from_tag(ap, tag); > + qc->tag = tag; > + qc->scsicmd = NULL; > + qc->ap = ap; > + qc->dev = dev; > > - ata_qc_reinit(qc); > - } > + ata_qc_reinit(qc); > > return qc; > } > @@ -4812,7 +4780,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) > + ata_sas_free_tag(tag, ap); > } > } > > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c > index e364e86..59c9d72 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,9 @@ int ata_scsi_add_hosts(struct ata_host *host, struct scsi_host_template *sht) > */ > shost->max_host_blocked = 1; > > + if (scsi_init_shared_tag_map(shost, host->n_tags)) > + goto err_add; > + > rc = scsi_add_host_with_dma(ap->scsi_host, > &ap->tdev, ap->host->dev); > if (rc) > @@ -4228,3 +4231,28 @@ int ata_sas_queuecmd(struct scsi_cmnd *cmd, struct ata_port *ap) > return rc; > } > EXPORT_SYMBOL_GPL(ata_sas_queuecmd); > + > +int ata_sas_allocate_tag(struct ata_port *ap) > +{ > + unsigned int max_queue = ap->host->n_tags; > + unsigned int i, tag; > + > + for (i = 0, tag = ap->sas_last_tag + 1; i < max_queue; i++, tag++) { > + tag = tag < max_queue ? tag : 0; > + > + /* the last tag is reserved for internal command. */ > + if (tag == ATA_TAG_INTERNAL) > + continue; > + > + if (!test_and_set_bit(tag, &ap->sas_tag_allocated)) { > + ap->sas_last_tag = tag; > + return tag; > + } > + } > + return -1; > +} > + > +void ata_sas_free_tag(unsigned int tag, struct ata_port *ap) > +{ > + clear_bit(tag, &ap->sas_tag_allocated); > +} > diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h > index 5f4e0cc..8c491cd 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); > @@ -145,6 +145,8 @@ extern void ata_scsi_dev_rescan(struct work_struct *work); > extern int ata_bus_probe(struct ata_port *ap); > extern int ata_scsi_user_scan(struct Scsi_Host *shost, unsigned int channel, > unsigned int id, u64 lun); > +int ata_sas_allocate_tag(struct ata_port *ap); > +void ata_sas_free_tag(unsigned int tag, struct ata_port *ap); > > > /* libata-eh.c */ > diff --git a/include/linux/libata.h b/include/linux/libata.h > index 2d18241..f234547 100644 > --- a/include/linux/libata.h > +++ b/include/linux/libata.h > @@ -821,10 +821,10 @@ struct ata_port { > unsigned int cbl; /* cable type; ATA_CBL_xxx */ > > struct ata_queued_cmd qcmd[ATA_MAX_QUEUE]; > - unsigned long qc_allocated; > + unsigned long sas_tag_allocated; /* for sas tag allocation only */ > unsigned int qc_active; > int nr_active_links; /* #links with active qcs */ > - unsigned int last_tag; /* track next tag hw expects */ > + unsigned int sas_last_tag; /* track next tag hw expects */ > > struct ata_link link; /* host default link */ > struct ata_link *slave_link; /* see ata_slave_link_init() */ > @@ -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 Thu, Jan 22, 2015 at 01:12:20PM -0800, Shaohua Li wrote: > Hi Tejun, > are you ok to pick up these patches? Yeah, looks fine from libata side. Please feel free to add Acked-by: Tejun Heo <tj@kernel.org> and route as you see fit. If these can go through libata, please let me know. Thanks.
On 01/23/2015 01:13 PM, Tejun Heo wrote: > On Thu, Jan 22, 2015 at 01:12:20PM -0800, Shaohua Li wrote: >> Hi Tejun, >> are you ok to pick up these patches? > > Yeah, looks fine from libata side. Please feel free to add > > Acked-by: Tejun Heo <tj@kernel.org> > > and route as you see fit. If these can go through libata, please let > me know. I'll just take it through the block tree, there's the highest risk of conflict there with the tagging changes.
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index 5c84fb5..d626605 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -1585,8 +1585,6 @@ unsigned ata_exec_internal_sg(struct ata_device *dev, else tag = 0; - if (test_and_set_bit(tag, &ap->qc_allocated)) - BUG(); qc = __ata_qc_from_tag(ap, tag); qc->tag = tag; @@ -4726,66 +4724,36 @@ void swap_buf_le16(u16 *buf, unsigned int buf_words) } /** - * ata_qc_new - Request an available ATA command, for queueing - * @ap: target port - * - * Some ATA host controllers may implement a queue depth which is less - * than ATA_MAX_QUEUE. So we shouldn't allocate a tag which is beyond - * the hardware limitation. + * ata_qc_new_init - Request an available ATA command, and initialize it + * @dev: Device from whom we request an available command structure * * LOCKING: * None. */ -static struct ata_queued_cmd *ata_qc_new(struct ata_port *ap) +struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev, int tag) { - struct ata_queued_cmd *qc = NULL; - unsigned int max_queue = ap->host->n_tags; - unsigned int i, tag; + struct ata_port *ap = dev->link->ap; + struct ata_queued_cmd *qc; /* no command while frozen */ if (unlikely(ap->pflags & ATA_PFLAG_FROZEN)) return NULL; - for (i = 0, tag = ap->last_tag + 1; i < max_queue; i++, tag++) { - tag = tag < max_queue ? tag : 0; - - /* the last tag is reserved for internal command. */ - if (tag == ATA_TAG_INTERNAL) - continue; - - if (!test_and_set_bit(tag, &ap->qc_allocated)) { - qc = __ata_qc_from_tag(ap, tag); - qc->tag = tag; - ap->last_tag = tag; - break; - } + /* libsas case */ + if (!ap->scsi_host) { + tag = ata_sas_allocate_tag(ap); + if (tag < 0) + return NULL; } - return qc; -} - -/** - * ata_qc_new_init - Request an available ATA command, and initialize it - * @dev: Device from whom we request an available command structure - * - * LOCKING: - * None. - */ - -struct ata_queued_cmd *ata_qc_new_init(struct ata_device *dev) -{ - struct ata_port *ap = dev->link->ap; - struct ata_queued_cmd *qc; - - qc = ata_qc_new(ap); - if (qc) { - qc->scsicmd = NULL; - qc->ap = ap; - qc->dev = dev; + qc = __ata_qc_from_tag(ap, tag); + qc->tag = tag; + qc->scsicmd = NULL; + qc->ap = ap; + qc->dev = dev; - ata_qc_reinit(qc); - } + ata_qc_reinit(qc); return qc; } @@ -4812,7 +4780,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) + ata_sas_free_tag(tag, ap); } } diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index e364e86..59c9d72 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,9 @@ int ata_scsi_add_hosts(struct ata_host *host, struct scsi_host_template *sht) */ shost->max_host_blocked = 1; + if (scsi_init_shared_tag_map(shost, host->n_tags)) + goto err_add; + rc = scsi_add_host_with_dma(ap->scsi_host, &ap->tdev, ap->host->dev); if (rc) @@ -4228,3 +4231,28 @@ int ata_sas_queuecmd(struct scsi_cmnd *cmd, struct ata_port *ap) return rc; } EXPORT_SYMBOL_GPL(ata_sas_queuecmd); + +int ata_sas_allocate_tag(struct ata_port *ap) +{ + unsigned int max_queue = ap->host->n_tags; + unsigned int i, tag; + + for (i = 0, tag = ap->sas_last_tag + 1; i < max_queue; i++, tag++) { + tag = tag < max_queue ? tag : 0; + + /* the last tag is reserved for internal command. */ + if (tag == ATA_TAG_INTERNAL) + continue; + + if (!test_and_set_bit(tag, &ap->sas_tag_allocated)) { + ap->sas_last_tag = tag; + return tag; + } + } + return -1; +} + +void ata_sas_free_tag(unsigned int tag, struct ata_port *ap) +{ + clear_bit(tag, &ap->sas_tag_allocated); +} diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h index 5f4e0cc..8c491cd 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); @@ -145,6 +145,8 @@ extern void ata_scsi_dev_rescan(struct work_struct *work); extern int ata_bus_probe(struct ata_port *ap); extern int ata_scsi_user_scan(struct Scsi_Host *shost, unsigned int channel, unsigned int id, u64 lun); +int ata_sas_allocate_tag(struct ata_port *ap); +void ata_sas_free_tag(unsigned int tag, struct ata_port *ap); /* libata-eh.c */ diff --git a/include/linux/libata.h b/include/linux/libata.h index 2d18241..f234547 100644 --- a/include/linux/libata.h +++ b/include/linux/libata.h @@ -821,10 +821,10 @@ struct ata_port { unsigned int cbl; /* cable type; ATA_CBL_xxx */ struct ata_queued_cmd qcmd[ATA_MAX_QUEUE]; - unsigned long qc_allocated; + unsigned long sas_tag_allocated; /* for sas tag allocation only */ unsigned int qc_active; int nr_active_links; /* #links with active qcs */ - unsigned int last_tag; /* track next tag hw expects */ + unsigned int sas_last_tag; /* track next tag hw expects */ struct ata_link link; /* host default link */ struct ata_link *slave_link; /* see ata_slave_link_init() */ @@ -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, \