Re: [PATCH] libata: add horkage for M88V29

[Damien Le Moal]

On 6/23/22 19:12, Böszörményi Zoltán wrote:
> 2022. 06. 23. 11:32 keltezéssel, Böszörményi Zoltán írta:
>> 2022. 06. 23. 10:46 keltezéssel, Damien Le Moal írta:
>>> On 6/23/22 17:38, Böszörményi Zoltán wrote:
>>>> 2022. 06. 23. 10:22 keltezéssel, Damien Le Moal írta:
>>>>> On 6/23/22 16:47, Böszörményi Zoltán wrote:
>>>>>> 2022. 02. 08. 9:07 keltezéssel, Damien Le Moal írta:
>>>>>>> On 2/4/22 21:57, zboszor@xxxxx wrote:
>>>>>>>> From: Zoltán Böszörményi <zboszor@xxxxxxxxx>
>>>>>>>>
>>>>>>>> This device is a CF card, or possibly an SSD in CF form factor.
>>>>>>>> It supports NCQ and high speed DMA.
>>>>>>>>
>>>>>>>> While it also advertises TRIM support, I/O errors are reported
>>>>>>>> when the discard mount option fstrim is used. TRIM also fails
>>>>>>>> when disabling NCQ and not just as an NCQ command.
>>>>>>>>
>>>>>>>> TRIM must be disabled for this device.
>>>>>>>>
>>>>>>>> Signed-off-by: Zoltán Böszörményi <zboszor@xxxxxxxxx>
>>>>>>>> ---
>>>>>>>>     drivers/ata/libata-core.c | 1 +
>>>>>>>>     1 file changed, 1 insertion(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>>>>>>>> index 67f88027680a..4a7f58fcc411 100644
>>>>>>>> --- a/drivers/ata/libata-core.c
>>>>>>>> +++ b/drivers/ata/libata-core.c
>>>>>>>> @@ -4028,6 +4028,7 @@ static const struct ata_blacklist_entry ata_device_blacklist 
>>>>>>>> [] = {
>>>>>>>>             /* devices that don't properly handle TRIM commands */
>>>>>>>>         { "SuperSSpeed S238*",        NULL, ATA_HORKAGE_NOTRIM, },
>>>>>>>> +    { "M88V29*",            NULL, ATA_HORKAGE_NOTRIM, },
>>>>>>>>             /*
>>>>>>>>          * As defined, the DRAT (Deterministic Read After Trim) and RZAT
>>>>>>> Applied to for-5.17-fixes. Thanks !
>>>>>> Thank you. However, I have second thoughts about this patch.
>>>>>> The device advertises this:
>>>>>>
>>>>>> # hdparm -iI /dev/sda
>>>>>> ...
>>>>>>     Enabled Supported
>>>>>>        *    Data Set Management TRIM supported (limit 1 block)
>>>>>> ...
>>>>>>
>>>>>> but the I/O failures always reported higher number of blocks,
>>>>>> IIRC the attempted number of block was 8 or so.
>>>>>>
>>>>>> Can the kernel limit or split TRIM commands according to the
>>>>>> advertised limit? If not (or not yet) then the quirk is good for now.
>>>>> Yes, the kernel does that. See the sysfs queue attributes
>>>>> discard_max_bytes and discard_max_hw_bytes. What are the values for your
>>>>> device ? I think that the "limit 1 block" indicated by hdparm is simply to
>>>>> say that the DSM command (to trim the device) accept only at most a 1
>>>>> block (512 B) list of sectors to trim. That is not the actual trim limit
>>>>> for each sector range in that list.
>>>> With the quirk in effect (TRIM disabled) I have these:
>>>>
>>>> [root@chef queue]# pwd
>>>> /sys/block/sda/queue
>>>> [root@chef queue]# cat discard_granularity
>>>> 0
>>>> [root@chef queue]# cat discard_max_bytes
>>>> 0
>>>> [root@chef queue]# cat discard_max_hw_bytes
>>>> 0
>>> Yes, expected. What are the values without the quirk applied ?
>>
>> I built 5.18.6 with removing the quirk.
>>
>> [root@chef queue]# pwd
>> /sys/block/sda/queue/
>> [root@chef queue]# cat discard_granularity
>> 512
>> [root@chef queue]# cat discard_max_bytes
>> 2147450880
>> [root@chef queue]# cat discard_max_hw_bytes
>> 2147450880
>> [root@chef queue]# cat max_discard_segments
>> 1
> 
> "echo 512 >discard_max_hw_bytes" says permission denied.

That is normal. This is a hardware characteristic so this is read only.

> "echo 512 >discard_max_bytes" can be set

Yes, this is the soft limit that can be used to limit trim command size.

> But with or without libata.force=noncqtrim, running
> "fstrim /boot" (which is ext4) goes into an infinite loop
> dumping a lot of I/O errors into dmesg.
> 
> Interestingly, after setting discard_max_bytes=512,
> in both cases (with or without libata.force=noncqrtim)
> running "fstrim /" (which is f2fs) there is no error in
> dmesg and fstrim returns after a small delay.

Which would tend to indicate that the drive only likes single sector trims...

> 
> So I guess TRIM does work but ext4 seems to be misbehaving.

I do not think so. The ext4 is going to issue whatever trim request for
free blocks it has and the block layer will split these request into at
most discard_max_bytes trim commands. You can check this behavior using
blktrace.

> FWIW "mount" shows "discard" for the big f2fs partition but
> it doesn't for ext4 but it's in the default mount option AFAIK.
> "mount /boot -o remount.discard" doesn't make a difference.
> the machine dumps a lot of errors into dmesg with "fstrim /boot".

If you have an empty partition, you could experiment using blkdiscard
command to remove the fs.

-- 
Damien Le Moal
Western Digital Research