Re: [PATCH] ata: libata-sata: Simplify ata_change_queue_depth()

[Damien Le Moal <dlemoal@kernel.org>]

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.

-- 
Damien Le Moal
Western Digital Research