On 9/28/22 01:09, John Garry wrote: > 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@xxxxxxxxxxxxxxxxxx> >>> >>> Tested-by: John Garry <john.garry@xxxxxxxxxx> >>> >>> 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. Gotcha. This needs to be changed to: if (sdev->queue_depth == queue_depth) return queue_depth; And the non-sensical error will go away. Will send another patch for that. > >> >>> 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 > -- Damien Le Moal Western Digital Research
