[PATCH] ata: Disable queued TRIM for Samsung 860 SSDs

[PATCH] ata: Disable queued TRIM for Samsung 860 SSDs

[Simon Arlott]

Despite the unsubstantiated claim from Samsung that "the improved
queued trim enhances Linux compatibility" this does not appear to be
true, even on Intel SATA controllers:

Bug 203475 - Samsung 860 EVO queued TRIM issues
https://bugzilla.kernel.org/show_bug.cgi?id=203475

Disable queued TRIM for all Samsung 860 SSDs. Only the EVO has been
reported as having this problem, but the original justification for
enabling appears to be based on marketing material with no explanation
of what has been changed to make the 860 work properly when the earlier
840 and 850 both have the same issue.

Signed-off-by: Simon Arlott <simon@octiron.net>
Cc: Park Ju Hyung <qkrwngud825@gmail.com>
Cc: stable@vger.kernel.org
Fixes: ca6bfcb2f6d9 ("libata: Enable queued TRIM for Samsung SSD 860")
---
 drivers/ata/libata-core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index b1cd4d97bc2a..02e861aac5ec 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -3951,6 +3951,8 @@ static const struct ata_blacklist_entry ata_device_blacklist [] = {
 						ATA_HORKAGE_ZERO_AFTER_TRIM, },
 	{ "Samsung SSD 850*",		NULL,	ATA_HORKAGE_NO_NCQ_TRIM |
 						ATA_HORKAGE_ZERO_AFTER_TRIM, },
+	{ "Samsung SSD 860*",		NULL,	ATA_HORKAGE_NO_NCQ_TRIM |
+						ATA_HORKAGE_ZERO_AFTER_TRIM, },
 	{ "FCCT*M500*",			NULL,	ATA_HORKAGE_NO_NCQ_TRIM |
 						ATA_HORKAGE_ZERO_AFTER_TRIM, },
 
-- 
2.17.1

-- 
Simon Arlott

Re: [PATCH] ata: Disable queued TRIM for Samsung 860 SSDs

[Ju Hyung Park]

Hi Simon,

On Wed, Jul 15, 2020 at 8:13 PM Simon Arlott <simon@octiron.net> wrote:
> the original justification for
> enabling appears to be based on marketing material with no explanation
> of what has been changed to make the 860 work properly when the earlier
> 840 and 850 both have the same issue.

Yes, this was why I sent out the patch.

With that said though, I've tested 860 EVO at that time(on i5-6200U,
so it's an Intel SATA controller) for a few weeks both with
asynchronous trim via f2fs and manual synchronous trim via fstrim.
(Since I was also using TLP, the SATA controller and the SSD was
constantly switching between LPM mode and non-LPM.)
My unit did not have any problem whereas both my previous 850 PRO and
850 EVO suffered from issues.

My 860 EVO was just fine with no problem for about 1.5 year until
later I switched to NVMe.

Given how late the bug reports were made after my patch went into
mainline, I wonder if this is firmware-specific.

Thanks.

>
> Signed-off-by: Simon Arlott <simon@octiron.net>
> Cc: Park Ju Hyung <qkrwngud825@gmail.com>
> Cc: stable@vger.kernel.org
> Fixes: ca6bfcb2f6d9 ("libata: Enable queued TRIM for Samsung SSD 860")
> ---
>  drivers/ata/libata-core.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index b1cd4d97bc2a..02e861aac5ec 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -3951,6 +3951,8 @@ static const struct ata_blacklist_entry ata_device_blacklist [] = {
>                                                 ATA_HORKAGE_ZERO_AFTER_TRIM, },
>         { "Samsung SSD 850*",           NULL,   ATA_HORKAGE_NO_NCQ_TRIM |
>                                                 ATA_HORKAGE_ZERO_AFTER_TRIM, },
> +       { "Samsung SSD 860*",           NULL,   ATA_HORKAGE_NO_NCQ_TRIM |
> +                                               ATA_HORKAGE_ZERO_AFTER_TRIM, },
>         { "FCCT*M500*",                 NULL,   ATA_HORKAGE_NO_NCQ_TRIM |
>                                                 ATA_HORKAGE_ZERO_AFTER_TRIM, },
>
> --
> 2.17.1
>
> --
> Simon Arlott

Re: [PATCH] ata: Disable queued TRIM for Samsung 860 SSDs

[Martin K. Petersen]

Hi Simon!

> Despite the unsubstantiated claim from Samsung that "the improved
> queued trim enhances Linux compatibility" this does not appear to be
> true, even on Intel SATA controllers:

I am aware of several people using 860 drives with queued TRIM. And I
haven't heard any complaints outside of the bug you referenced.

Also, I have tested both 860 2.5" Pro and 860 mSATA EVO on a few
different systems in my lab without any problems. See:

    https://lore.kernel.org/stable/yq1h87du82d.fsf@oracle.com/T/

I really wish we had some more data to work with :(

Lacking a proper heuristic I guess we don't have any choice to disable
the feature. But that's sad news for the people who currently don't have
problems since their performance will inevitably suffer.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] ata: Disable queued TRIM for Samsung 860 SSDs

[Simon Arlott]

On 15/07/2020 21:53, Martin K. Petersen wrote:
>> Despite the unsubstantiated claim from Samsung that "the improved
>> queued trim enhances Linux compatibility" this does not appear to be
>> true, even on Intel SATA controllers:
> 
> I am aware of several people using 860 drives with queued TRIM. And I
> haven't heard any complaints outside of the bug you referenced.
> 
> Also, I have tested both 860 2.5" Pro and 860 mSATA EVO on a few
> different systems in my lab without any problems. See:
> 
>     https://lore.kernel.org/stable/yq1h87du82d.fsf@oracle.com/T/

I've just checked and it happens on both my SATA 860s:
  860 EVO 2TB (RVT04B6Q) on Intel Z170
  860 PRO 1TB (RVM02B6Q) on Intel H170

> I really wish we had some more data to work with :(

Is there a reliable way of reproducing this?

I have a Marvell 88SE9235 that I could try with the EVO.

> Lacking a proper heuristic I guess we don't have any choice to disable
> the feature. But that's sad news for the people who currently don't have
> problems since their performance will inevitably suffer.

Samsung need to identify what the problem is before claiming that their
Linux support is better, especially if specific chipsets are known to
have issues...

-- 
Simon Arlott

Re: [PATCH] ata: Disable queued TRIM for Samsung 860 SSDs

[Ju Hyung Park]

Hi Martin,

On Thu, Jul 16, 2020 at 5:53 AM Martin K. Petersen
<martin.petersen@oracle.com> wrote:
> I really wish we had some more data to work with :(
>
> Lacking a proper heuristic I guess we don't have any choice to disable
> the feature. But that's sad news for the people who currently don't have
> problems since their performance will inevitably suffer.

It seems like the latest 860 EVO's firmware, RVT24B6Q, is fairly recent.
The earliest reference that I could find on Google is this from Jan,
2020: https://smarthdd.com/database/Samsung-SSD-860-EVO-M.2-500GB/RVT24B6Q/
and an Amazon review.

Earlier reports seem to be related to ASMedia's chipsets and NCQ quirks.

AFAIK, no reports were made in 2018.
IIRC the last time we went through this with the 850 series, a bunch
of people reported data corruptions, sometimes even filesystem's
superblock.
Surely, we would have gotten reports of it pretty soon if the drives
were indeed faulty.

Maybe the latest firmware is to blame?

Also, I don't think queued trim is all that crucial to performance
imho, at least in the context of Linux.

In my experience, regular R/W I/Os were still severely blocked when
fstrim is undergoing even with queued trim was in use(which, to my
understanding, is exactly what queued trim tried to resolve in the
first place?). Probably file-system's implementation is at partial
fault too with it sending ERASE commands with too big granularity. I
believe f2fs' default discard_granularity of 4KB is what tries to
mitigate this.

Linux distros are not using the "discard" mount flag by default and
defers to periodic fstrim on idle.
Android has been doing this since 4.3(2013), and doesn't even use SATA.
f2fs avoids this problem entirely by sending ERASE commands only when
the drive is idle.

All in all, I don't think we should pull out hairs trying to figure
out how to do this properly.
I'm yet to be convinced that queued trim solves practical performance issues.

If we can't figure this out quickly, I agree on blacklisting 860s again.

Thanks.