From nobody Mon Feb 19 15:18:48 2024 X-Original-To: dev-commits-src-main@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4TdmQl0XS1z5C02N for ; Mon, 19 Feb 2024 15:18:55 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-lf1-x130.google.com (mail-lf1-x130.google.com [IPv6:2a00:1450:4864:20::130]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1D4" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4TdmQj4TDjz4HV4 for ; Mon, 19 Feb 2024 15:18:53 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Authentication-Results: mx1.freebsd.org; none Received: by mail-lf1-x130.google.com with SMTP id 2adb3069b0e04-5120ecfd75cso6572906e87.0 for ; Mon, 19 Feb 2024 07:18:53 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bsdimp-com.20230601.gappssmtp.com; s=20230601; t=1708355931; x=1708960731; darn=freebsd.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=ywsswnEXsq0zSd4Remvb5qKzn/MeXXvMxtYKC19c+QM=; b=QYht2iZ27tVh0kLWCxTZEcHiFFWj2FiznSUuxr2TLwmj860WgY3ZoAICPgsBrrd0J8 c2BcC8gTel72BsMj6vGhmStlK1ZmN8g8Sy7jSdAjJXgLNm/UV2Nu84LCjxXnNvd0Ne2c R4ox0b2JaVGDLQPfu8KXl/6iMINgE1qcPDrgJE8dZ46l2wrBcv4Xm3zLcmLJ7vooyOve TfVNQ/WfOn8GL6G6Kv6KaeNp5qWaC1GeqI5YE+kP2L3xdL/zEXpNO+nW9guxVpM0Ixky 1YkvpEh1vrAXs4eYKotqgVU6LCEsRGVSMJfqcqOCmYsF7umdNHiFer/WLm9xOHFj2Ha6 NMIQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1708355931; x=1708960731; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=ywsswnEXsq0zSd4Remvb5qKzn/MeXXvMxtYKC19c+QM=; b=ItPzfuKkGvthJ/MF/LeywJK8mriKo23Ub+0LRUZcVYdTCKkezUouDkDZBSNZ95gIt0 rdJffpOCHSy+ar3n3wgg5BGEGuXHuVS9IKoueOhxDfOWvtmuAiP9rJRI3wobCUa7syEF IUsHHya50OiK+ZlNn1JjMQ8UD4L9SiEgm1Vq0xvwkqRTuN4Gb2CehYcUPGbg3uPxC6hA ZbOcgQPZJkI4kLQc8gZi7JqCNeqjD9PZ9xDtnjShWaNIPwUd81Mz87P7hL+JVEg3bVSc goocbDBleUqZV87pj95Khw1nvUD2wtrLWoqxCSBXR8RX0hzzPwQxOLy50HIsADUxNvcl vPtg== X-Forwarded-Encrypted: i=1; AJvYcCXR0KRs8c8eVMuHjWbFWcol7ev3vwFtVuSRXKyHBWqaTX3AvEuKzUuxIE9Xk6G1nt0ndgAgmYUEzyTbTC5UVw8R6eLuN/5QjiqhCVBJn2ccIQ== X-Gm-Message-State: AOJu0YwVgfZufL+ab0KdMVQ5larSlkWFcBr5f1OY2bo27/c7o7kpio4l L8FVGHrShdhxREMr24ib6/GZ5i+OQyVFtic8/LxreYNuQ/whbAOpb0X1x9JuQILZJS19ED088vZ sTpiUAl3WKzC73H/7cgE1kBzJI3TGeeHL1PpatFm3N8dnX6OjtoI= X-Google-Smtp-Source: AGHT+IEZWX/okq2QAfWYujcH/Wg8q3+gNdH1d4Iup1LIQzidb0ChpIZTmARv9DyZeo6Mee0+fkldkIjblf5jXxS/npk= X-Received: by 2002:a05:6512:4026:b0:512:a009:6e21 with SMTP id br38-20020a056512402600b00512a0096e21mr5134562lfb.31.1708355930798; Mon, 19 Feb 2024 07:18:50 -0800 (PST) List-Id: Commit messages for the main branch of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-main List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-main@freebsd.org X-BeenThere: dev-commits-src-main@freebsd.org MIME-Version: 1.0 References: In-Reply-To: From: Warner Losh Date: Mon, 19 Feb 2024 08:18:48 -0700 Message-ID: Subject: Re: git: c01af41c3c8f - main - ata_da: add quirk to disable NCQ TRIM for Samsung 860/870 SSDs To: cglogic Cc: Andriy Gapon , src-committers , "" , "" Content-Type: multipart/alternative; boundary="00000000000067d1ca0611bd9cb8" X-Spamd-Bar: ---- X-Rspamd-Queue-Id: 4TdmQj4TDjz4HV4 X-Rspamd-Pre-Result: action=no action; module=replies; Message is reply to one we originated X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[]; ASN(0.00)[asn:15169, ipnet:2a00:1450::/32, country:US] --00000000000067d1ca0611bd9cb8 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Mon, Feb 19, 2024, 4:29=E2=80=AFAM cglogic wrot= e: > On Monday, February 19th, 2024 at 1:03 PM, Andriy Gapon > wrote: > > > On 19/02/2024 12:47, cglogic wrote: > > > > > Hello Andriy, > > > > > > I use ZFS with autotrim enabled on Samsung 860 PRO connected to Intel > AHCI SATA controller for 9 years without any issue. > > > > > > I think that it was released in 2018? > > You are right, it's 9 years old computer, SSD was added later. > > > > > > Can I disable this quirk locally or have I revert the patch to > continue use NCQ TRIM with this SSD? > > > > > > I don't think that there is a way to disable the quirk, so you'll have > to revert. > > Note that ATA TRIM is still enabled, it's only NCQ TRIM that got > disabled. > > From my understanding ATA TRIM is non-queued TRIM as opposed to NCQ TRIM. > With only ATA TRIM enabled, drive will have increased latency when large > amount of files deleted. > > However I'm not sure how ZFS handles TRIMs, maybe it groups several TRIMs > and then sends when drive is idle. > Another question is how this affects TRIM enabled swap partition. > > In general, if I'm correct here, disabling NCQ TRIM should not be an issu= e > for desktop usage, except for large git repository updates, etc. > In general, it won't matter a ton. Unless you are seeing stalls because of read/write I/O waiting behind trims that have to go to the drive one at a time, the effects are in the noise. If you are seeing that, though, the effects can be quite helpful in terms of latency... assuming the drive is cooperative. The benefits of it vary a lot from drive to drive as well. Some early drives still serialize the trims, so the latency benefit is not so great. Some enterprise drives do trims instantly and keep some internal state that is helped a lot by them. But if NCQ trims are unstable, we likely should err on the side of disabling. The errors that Andriy was seeing typically are cable or controller issues. In his case, it was a controller issue (apparently, and surprisingly, paired with a specific type of drive). Linux has a specific workaround for the pair, which is unusual. But there's two issues at play. The first issue is that these drives should have NCQ Trim turned off, according to Linux's commit log. This may be firmware revision based or may just be some people are luckier with newer firmware (it's hard to say from the mixed reports, but they read more like luck than version for the sampling I did). So Andriy's change is good. The second issue, and one I was confused on earlier, is that Linux goes a step further and disables NCQ entirely for these old controllers for 860 and 870 drives. This isn't too surprising. We have a workaround for PMP for these controllers already.... One can disable NCQ entirely by setting the tags to 0... Given the specificity of the problem, and the age of the controllers, I'm inclined to not implement the Linux workaround for this specific pair. Warner > > > > On Monday, February 19th, 2024 at 12:09 PM, Andriy Gapon > avg@FreeBSD.org wrote: > > > > > > > The branch main has been updated by avg: > > > > > > > > URL: > https://cgit.FreeBSD.org/src/commit/?id=3Dc01af41c3c8fdd570764ff9b6bfbad6= ac9ca1664 > > > > > > > > commit c01af41c3c8fdd570764ff9b6bfbad6ac9ca1664 > > > > Author: Andriy Gapon avg@FreeBSD.org > > > > > > > > AuthorDate: 2024-02-19 10:08:12 +0000 > > > > Commit: Andriy Gapon avg@FreeBSD.org > > > > > > > > CommitDate: 2024-02-19 10:08:12 +0000 > > > > > > > > ata_da: add quirk to disable NCQ TRIM for Samsung 860/870 SSDs > > > > > > > > NCQ TRIM for Samsung 860/870 SSDs results in data corruption on > systems > > > > with some SATA controllers. > > > > > > > > This can be easily reproduced using ZFS which uses TRIM and is able > to > > > > detect block content changes. > > > > > > > > Linux bug report for this issue: > > > > https://bugzilla.kernel.org/show_bug.cgi?id=3D201693 > > > > > > > > Since at present we can not limit a quirk based on the contorller / > SIM, > > > > apply the quirk in all cases. > > > > > > > > Reviewed by: imp > > > > MFC after: 2 weeks > > > > Differential Revision: https://reviews.freebsd.org/D43961 > > > > --- > > > > sys/cam/ata/ata_da.c | 16 ++++++++++++++++ > > > > 1 file changed, 16 insertions(+) > > > > > > > > diff --git a/sys/cam/ata/ata_da.c b/sys/cam/ata/ata_da.c > > > > index f5d3aeca9329..d4a591943307 100644 > > > > --- a/sys/cam/ata/ata_da.c > > > > +++ b/sys/cam/ata/ata_da.c > > > > @@ -727,6 +727,22 @@ static struct ada_quirk_entry ada_quirk_table[= ] > =3D > > > > { T_DIRECT, SIP_MEDIA_FIXED, "", "Samsung SSD 850", "" }, > > > > /quirks/ADA_Q_4K | ADA_Q_NCQ_TRIM_BROKEN > > > > }, > > > > + { > > > > + / > > > > + * Samsung 860 SSDs > > > > + * 4k optimised, NCQ TRIM broken (normal TRIM fine) > > > > + / > > > > + { T_DIRECT, SIP_MEDIA_FIXED, "", "Samsung SSD 860*", "" }, > > > > + /quirks/ADA_Q_4K | ADA_Q_NCQ_TRIM_BROKEN > > > > + }, > > > > + { > > > > + / > > > > + * Samsung 870 SSDs > > > > + * 4k optimised, NCQ TRIM broken (normal TRIM fine) > > > > + / > > > > + { T_DIRECT, SIP_MEDIA_FIXED, "", "Samsung SSD 870*", "" }, > > > > + /quirks/ADA_Q_4K | ADA_Q_NCQ_TRIM_BROKEN > > > > + }, > > > > { > > > > / > > > > * Samsung SM863 Series SSDs (MZ7KM*) > > > > > > -- > > Andriy Gapon > --00000000000067d1ca0611bd9cb8 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Mon, Feb 19, 2024, 4:29=E2=80=AFAM= cglogic <cg= logic@protonmail.com> wrote:
On Monday, February 19th, 2024 at 1:03 PM, Andriy Gapon <avg@FreeBSD.or= g> wrote:

> On 19/02/2024 12:47, cglogic wrote:
>
> > Hello Andriy,
> >
> > I use ZFS with autotrim enabled on Samsung 860 PRO connected to I= ntel AHCI SATA controller for 9 years without any issue.
>
>
> I think that it was released in 2018?

You are right, it's 9 years old computer, SSD was added later.

>
> > Can I disable this quirk locally or have I revert the patch to co= ntinue use NCQ TRIM with this SSD?
>
>
> I don't think that there is a way to disable the quirk, so you'= ;ll have to revert.
> Note that ATA TRIM is still enabled, it's only NCQ TRIM that got d= isabled.

From my understanding ATA TRIM is non-queued TRIM as opposed to NCQ TRIM. With only ATA TRIM enabled, drive will have increased latency when large am= ount of files deleted.

However I'm not sure how ZFS handles TRIMs, maybe it groups several TRI= Ms and then sends when drive is idle.
Another question is how this affects TRIM enabled swap partition.

In general, if I'm correct here, disabling NCQ TRIM should not be an is= sue for desktop usage, except for large git repository updates, etc.

In general, it won't = matter a ton. Unless you are seeing stalls because of read/write I/O waitin= g behind trims that have to go to the drive one at a time, the effects are = in the noise. If you are seeing that, though, the effects can be quite help= ful in terms of latency... assuming the drive is cooperative. The benefits = of it vary a lot from drive to drive as well. Some early drives still seria= lize the trims, so the latency benefit is not so great. Some enterprise dri= ves do trims instantly and keep some internal state that is helped a lot by= them. But if NCQ trims are unstable, we likely should err on the side of d= isabling.

The errors that Andriy = was seeing typically are cable or controller issues. In his case, it was a = controller issue (apparently, and surprisingly, paired with a specific type= of drive). Linux has a specific workaround for the pair, which is unusual.= But there's two issues at play.

The first= issue is that these drives should have NCQ Trim turned off, according to L= inux's commit log. This may be firmware revision based or may just be s= ome people are luckier with newer firmware (it's hard to say from the m= ixed reports, but they read more like luck than version for the sampling I = did). So Andriy's change is good. The second issue, and one I was confu= sed on earlier, is that Linux goes a step further and disables NCQ entirely= for these old controllers for 860 and 870 drives. This isn't too surpr= ising. We have a workaround for PMP for these controllers already.... One c= an disable NCQ entirely by setting the tags to 0... Given the specificity o= f the problem, and the age of the controllers, I'm inclined to not impl= ement the Linux workaround for this specific pair.

=
Warner

>
> > On Monday, February 19th, 2024 at 12:09 PM, Andriy Gapon avg@Free= BSD.org wrote:
> >
> > > The branch main has been updated by avg:
> > >
> > > URL: https://cgit.FreeBSD.org/src/commit/?id=3Dc01af41c3c8fdd570764= ff9b6bfbad6ac9ca1664
> > >
> > > commit c01af41c3c8fdd570764ff9b6bfbad6ac9ca1664
> > > Author: Andriy Gapon avg@FreeBSD.org
> > >
> > > AuthorDate: 2024-02-19 10:08:12 +0000
> > > Commit: Andriy Gapon avg@FreeBSD.org
> > >
> > > CommitDate: 2024-02-19 10:08:12 +0000
> > >
> > > ata_da: add quirk to disable NCQ TRIM for Samsung 860/870 SS= Ds
> > >
> > > NCQ TRIM for Samsung 860/870 SSDs results in data corruption= on systems
> > > with some SATA controllers.
> > >
> > > This can be easily reproduced using ZFS which uses TRIM and = is able to
> > > detect block content changes.
> > >
> > > Linux bug report for this issue:
> > > https://bugzilla.kerne= l.org/show_bug.cgi?id=3D201693
> > >
> > > Since at present we can not limit a quirk based on the conto= rller / SIM,
> > > apply the quirk in all cases.
> > >
> > > Reviewed by: imp
> > > MFC after: 2 weeks
> > > Differential Revision: https://reviews.f= reebsd.org/D43961
> > > ---
> > > sys/cam/ata/ata_da.c | 16 ++++++++++++++++
> > > 1 file changed, 16 insertions(+)
> > >
> > > diff --git a/sys/cam/ata/ata_da.c b/sys/cam/ata/ata_da.c
> > > index f5d3aeca9329..d4a591943307 100644
> > > --- a/sys/cam/ata/ata_da.c
> > > +++ b/sys/cam/ata/ata_da.c
> > > @@ -727,6 +727,22 @@ static struct ada_quirk_entry ada_quirk= _table[] =3D
> > > { T_DIRECT, SIP_MEDIA_FIXED, "", "Samsung SSD= 850", "" },
> > > /quirks/ADA_Q_4K | ADA_Q_NCQ_TRIM_BROKEN
> > > },
> > > + {
> > > + /
> > > + * Samsung 860 SSDs
> > > + * 4k optimised, NCQ TRIM broken (normal TRIM fine)
> > > + /
> > > + { T_DIRECT, SIP_MEDIA_FIXED, "", "Samsung S= SD 860*", "" },
> > > + /quirks/ADA_Q_4K | ADA_Q_NCQ_TRIM_BROKEN
> > > + },
> > > + {
> > > + /
> > > + * Samsung 870 SSDs
> > > + * 4k optimised, NCQ TRIM broken (normal TRIM fine)
> > > + /
> > > + { T_DIRECT, SIP_MEDIA_FIXED, "", "Samsung S= SD 870*", "" },
> > > + /quirks/ADA_Q_4K | ADA_Q_NCQ_TRIM_BROKEN
> > > + },
> > > {
> > > /
> > > * Samsung SM863 Series SSDs (MZ7KM*)
>
>
> --
> Andriy Gapon
--00000000000067d1ca0611bd9cb8--