Re: [PATCH v2 2/2] ata: libata-sata: Fix device queue depth control

[John Garry <john.garry@huawei.com>]

On 27/09/2022 16:03, Damien Le Moal wrote:
> On 9/27/22 20:51, John Garry wrote:
>> On 26/09/2022 00:08, Damien Le Moal wrote:
>>> The function __ata_change_queue_depth() uses the helper
>>> ata_scsi_find_dev() to get the ata_device structure of a scsi device and
>>> set that device maximum queue depth. However, when the ata device is
>>> managed by libsas, ata_scsi_find_dev() returns NULL, turning
>>> __ata_change_queue_depth() into a nop, which prevents the user from
>>> setting the maximum queue depth of ATA devices used with libsas based
>>> HBAs.
>>>
>>> Fix this by renaming __ata_change_queue_depth() to
>>> ata_change_queue_depth() and adding a pointer to the ata_device
>>> structure of the target device as argument. This pointer is provided by
>>> ata_scsi_change_queue_depth() using ata_scsi_find_dev() in the case of
>>> a libata managed device and by sas_change_queue_depth() using
>>> sas_to_ata_dev() in the case of a libsas managed ata device.
>>>
>>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>>
>> Tested-by: John Garry <john.garry@huawei.com>
>>
>> However - a big however - I will note that this following behaviour is
>> strange for a SATA device for libsas:
>>
>> root@(none)$ echo 33 > 0:0:2:0/device/queue_depth
>> root@(none)$ echo 33 > 0:0:2:0/device/queue_depth
>> sh: echo: write error: Invalid argument
>> root@(none)$
> 
> Weird. I am not getting any error (pm80xx driver). The qd gets capped at
> 32 as expected. Is it something that changes per libsas driver ?

That is with my pm8001 card.

What happens here is that the first store of 33 gets through to 
ata_change_queue_depth() as it does not exceed the SAS shost can_queue, 
which is >> 32, and then we cap this to 32 and store it in 
sdev->queue_depth. And then the 2nd store of 33 also gets through, but 
this following expression not evaluate true in ata_change_queue_depth():

queue_depth < 1 || queue_depth == sdev->queue_depth

So we don't return. However the following subsequent test does evaluate 
true in ata_change_queue_depth():

if (sdev->queue_depth == queue_depth)
	return -EINVAL;

And we error.

> 
>> I also note that setting a value out of range is just rejected for a SAS
>> device, and not capped to the max range (like it is for SATA).
> 
> Not sure where that come from. A quick look does not reveal anything
> obvious. Need to dig into that one.
>>
>> AHCI rejects out of range values it as it exceeds the shost can_queue in
>> sdev_store_queue_depth().
> 
> Indeed:
> 
> echo 1 > /sys/block/sdk/device/queue_depth
> echo 33 > /sys/block/sdk/device/queue_depth

Hmmmm... why no error message? is the printk silenced?

> cat /sys/block/sdk/device/queue_depth
> 1
> 
> But for the libsas SATA device:
> 
> echo 1 > /sys/block/sdd/device/queue_depth
> cat /sys/block/sdd/device/queue_depth
> 1
> echo 33 > /sys/block/sdd/device/queue_depth
> cat /sys/block/sdd/device/queue_depth
> 32
> 
> As one would expect... > Need to dig into that one.
> 

thanks,
John