| Message ID | 20230601222607.263024-1-dlemoal@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | ata: libata-sata: Simplify ata_change_queue_depth() | expand |
On 01/06/2023 23:26, Damien Le Moal wrote: > Commit 141f3d6256e5 ("ata: libata-sata: Fix device queue depth control") > added a struct ata_device argument to ata_change_queue_depth() to > address problems with changing the queue depth of ATA devices managed > through libsas. This was due to problems with ata_scsi_find_dev() which > are now fixed with commit 7f875850f20a ("ata: libata-scsi: Use correct > device no in ata_find_dev()"). > > Undo some of the changes of commit 141f3d6256e5: remove the added struct > ata_device aregument and use again ata_scsi_find_dev() to find the > target ATA device structure. While doing this, also make sure that > ata_scsi_find_dev() is called with ap->lock held, as it should. > > libsas and libata call sites of ata_change_queue_depth() are updated to > match the modified function arguments. > > Signed-off-by: Damien Le Moal <dlemoal@kernel.org> Reviewed-by: John Garry <john.g.garry@oracle.com> Thanks! Just a reminder to all - I'm not asking anyone to fix it - we still have that funky libsas behaviour for attempting to set queue depth at 33: https://lore.kernel.org/linux-scsi/db84e61a-1069-982a-5659-297fcffc14f4@huawei.com/ > --- > drivers/ata/libata-sata.c | 19 ++++++++++--------- > drivers/scsi/libsas/sas_scsi_host.c | 3 +-- > include/linux/libata.h | 4 ++-- > 3 files changed, 13 insertions(+), 13 deletions(-) > ... > + int queue_depth) > { > + struct ata_device *dev; > unsigned long flags; > > - if (!dev || !ata_dev_enabled(dev)) > - return sdev->queue_depth; > + spin_lock_irqsave(ap->lock, flags); > > - if (queue_depth < 1 || queue_depth == sdev->queue_depth) > + dev = ata_scsi_find_dev(ap, sdev); > + if (!dev || queue_depth < 1 || queue_depth == sdev->queue_depth) { > + spin_unlock_irqrestore(ap->lock, flags); > return sdev->queue_depth; > + } > > /* NCQ enabled? */ > - spin_lock_irqsave(ap->lock, flags); > dev->flags &= ~ATA_DFLAG_NCQ_OFF; > if (queue_depth == 1 || !ata_ncq_enabled(dev)) { Apart from this change, should we call ata_ncq_supported() here (instead of ata_ncq_enabled())? ata_ncq_enabled() checks if ATA_DFLAG_NCQ_OFF is not set, which we have already ensured. > dev->flags |= ATA_DFLAG_NCQ_OFF; > queue_depth = 1; > } > +
On 6/2/23 17:02, John Garry wrote: > On 01/06/2023 23:26, Damien Le Moal wrote: >> Commit 141f3d6256e5 ("ata: libata-sata: Fix device queue depth control") >> added a struct ata_device argument to ata_change_queue_depth() to >> address problems with changing the queue depth of ATA devices managed >> through libsas. This was due to problems with ata_scsi_find_dev() which >> are now fixed with commit 7f875850f20a ("ata: libata-scsi: Use correct >> device no in ata_find_dev()"). >> >> Undo some of the changes of commit 141f3d6256e5: remove the added struct >> ata_device aregument and use again ata_scsi_find_dev() to find the >> target ATA device structure. While doing this, also make sure that >> ata_scsi_find_dev() is called with ap->lock held, as it should. >> >> libsas and libata call sites of ata_change_queue_depth() are updated to >> match the modified function arguments. >> >> Signed-off-by: Damien Le Moal <dlemoal@kernel.org> > > Reviewed-by: John Garry <john.g.garry@oracle.com> > > Thanks! > > Just a reminder to all - I'm not asking anyone to fix it - we still have > that funky libsas behaviour for attempting to set queue depth at 33: > > https://lore.kernel.org/linux-scsi/db84e61a-1069-982a-5659-297fcffc14f4@huawei.com/ I checked that again. For a libsas device, with this patch applied, we have: # echo 1 > /sys/block/sdg/device/queue_depth # cat /sys/block/sdg/device/queue_depth 1 # echo 33 > /sys/block/sdg/device/queue_depth; echo $? 0 <-- success ! # cat /sys/block/sdg/device/queue_depth 32 <-- qd was capped # echo 33 > /sys/block/sdg/device/queue_depth; echo $? 1 <-- error ! # cat /sys/block/sdg/device/queue_depth 32 <-- no change For a libata device, we have: # echo 1 > /sys/block/sdc/device/queue_depth # cat /sys/block/sdc/device/queue_depth 1 # echo 33 > /sys/block/sdc/device/queue_depth # echo $? 1 <-- error ! # cat /sys/block/sdc/device/queue_depth 1 <-- no change This is not consistent. The attempt to change from 1 to 33 with libsas should error and not change anything. That is because sdev->host->can_queue is larger than 32 for libsas devices as it indicates the number of commands that the HBA can queue rather than the device max queue depth. >> /* NCQ enabled? */ >> - spin_lock_irqsave(ap->lock, flags); >> dev->flags &= ~ATA_DFLAG_NCQ_OFF; >> if (queue_depth == 1 || !ata_ncq_enabled(dev)) { > > Apart from this change, should we call ata_ncq_supported() here (instead > of ata_ncq_enabled())? ata_ncq_enabled() checks if ATA_DFLAG_NCQ_OFF is > not set, which we have already ensured. Good catch. ata_ncq_enabled() calls ata_ncq_supported(), so this is not really a bug, but it would be indeed cleaner (and less confusing) to call ata_ncq_supported(). I am sending a patch to clean this up.
diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c index f3e7396e3191..e3c9cb617048 100644 --- a/drivers/ata/libata-sata.c +++ b/drivers/ata/libata-sata.c @@ -1023,7 +1023,6 @@ EXPORT_SYMBOL_GPL(dev_attr_sw_activity); /** * ata_change_queue_depth - Set a device maximum queue depth * @ap: ATA port of the target device - * @dev: target ATA device * @sdev: SCSI device to configure queue depth for * @queue_depth: new queue depth * @@ -1031,24 +1030,27 @@ EXPORT_SYMBOL_GPL(dev_attr_sw_activity); * and libata. * */ -int ata_change_queue_depth(struct ata_port *ap, struct ata_device *dev, - struct scsi_device *sdev, int queue_depth) +int ata_change_queue_depth(struct ata_port *ap, struct scsi_device *sdev, + int queue_depth) { + struct ata_device *dev; unsigned long flags; - if (!dev || !ata_dev_enabled(dev)) - return sdev->queue_depth; + spin_lock_irqsave(ap->lock, flags); - if (queue_depth < 1 || queue_depth == sdev->queue_depth) + dev = ata_scsi_find_dev(ap, sdev); + if (!dev || queue_depth < 1 || queue_depth == sdev->queue_depth) { + spin_unlock_irqrestore(ap->lock, flags); return sdev->queue_depth; + } /* NCQ enabled? */ - spin_lock_irqsave(ap->lock, flags); dev->flags &= ~ATA_DFLAG_NCQ_OFF; if (queue_depth == 1 || !ata_ncq_enabled(dev)) { dev->flags |= ATA_DFLAG_NCQ_OFF; queue_depth = 1; } + spin_unlock_irqrestore(ap->lock, flags); /* limit and apply queue depth */ @@ -1082,8 +1084,7 @@ int ata_scsi_change_queue_depth(struct scsi_device *sdev, int queue_depth) { struct ata_port *ap = ata_shost_to_port(sdev->host); - return ata_change_queue_depth(ap, ata_scsi_find_dev(ap, sdev), - sdev, queue_depth); + return ata_change_queue_depth(ap, sdev, queue_depth); } EXPORT_SYMBOL_GPL(ata_scsi_change_queue_depth); diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c index a36fa1c128a8..94c5f14f3c16 100644 --- a/drivers/scsi/libsas/sas_scsi_host.c +++ b/drivers/scsi/libsas/sas_scsi_host.c @@ -872,8 +872,7 @@ int sas_change_queue_depth(struct scsi_device *sdev, int depth) struct domain_device *dev = sdev_to_domain_dev(sdev); if (dev_is_sata(dev)) - return ata_change_queue_depth(dev->sata_dev.ap, - sas_to_ata_dev(dev), sdev, depth); + return ata_change_queue_depth(dev->sata_dev.ap, sdev, depth); if (!sdev->tagged_supported) depth = 1; diff --git a/include/linux/libata.h b/include/linux/libata.h index 01f9fbb69f89..bc756f8586f3 100644 --- a/include/linux/libata.h +++ b/include/linux/libata.h @@ -1144,8 +1144,8 @@ extern int ata_scsi_slave_config(struct scsi_device *sdev); extern void ata_scsi_slave_destroy(struct scsi_device *sdev); extern int ata_scsi_change_queue_depth(struct scsi_device *sdev, int queue_depth); -extern int ata_change_queue_depth(struct ata_port *ap, struct ata_device *dev, - struct scsi_device *sdev, int queue_depth); +extern int ata_change_queue_depth(struct ata_port *ap, struct scsi_device *sdev, + int queue_depth); extern struct ata_device *ata_dev_pair(struct ata_device *adev); extern int ata_do_set_mode(struct ata_link *link, struct ata_device **r_failed_dev); extern void ata_scsi_port_error_handler(struct Scsi_Host *host, struct ata_port *ap);
Commit 141f3d6256e5 ("ata: libata-sata: Fix device queue depth control") added a struct ata_device argument to ata_change_queue_depth() to address problems with changing the queue depth of ATA devices managed through libsas. This was due to problems with ata_scsi_find_dev() which are now fixed with commit 7f875850f20a ("ata: libata-scsi: Use correct device no in ata_find_dev()"). Undo some of the changes of commit 141f3d6256e5: remove the added struct ata_device aregument and use again ata_scsi_find_dev() to find the target ATA device structure. While doing this, also make sure that ata_scsi_find_dev() is called with ap->lock held, as it should. libsas and libata call sites of ata_change_queue_depth() are updated to match the modified function arguments. Signed-off-by: Damien Le Moal <dlemoal@kernel.org> --- drivers/ata/libata-sata.c | 19 ++++++++++--------- drivers/scsi/libsas/sas_scsi_host.c | 3 +-- include/linux/libata.h | 4 ++-- 3 files changed, 13 insertions(+), 13 deletions(-)