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@xxxxxxxxxx>
>
> Reviewed-by: John Garry <john.g.garry@xxxxxxxxxx>
>
> 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@xxxxxxxxxx/
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.
--
Damien Le Moal
Western Digital Research
