Project

General

Profile

Actions

Bug #12070

closed

sata SSDs attached to sata ports can't trim

Added by Bill Sommerfeld almost 2 years ago. Updated almost 2 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
driver - device drivers
Start date:
Due date:
% Done:

100%

Estimated time:
Difficulty:
Medium
Tags:
zfs, sata, ssd, trim
Gerrit CR:

Description

I attempted a little testing with TRIM enabled on one of my systems (with a couple Crucial MX500's connected to motherboard SATA ports) and discovered some questionable code in uts/common/io/sata/impl/sata.c's implementation of READ CAPACITY 16.

If I'm reading the spec fragments I've found correctly, SATA devices advertise "trim" capability via the low order bit of "word 169" (which sata.c refers to as "satadrv_id.ad_dsm"), and make commitments about the behavior of reads after trim with two bits in "word 69" (satadrv_id.ad_addsupported) -- either "always zero", "always the same thing", or "subject to change at any time").

As best as I can tell, none of this additional detail matters to ZFS -- everything it writes is checksummed, and it's never going to attempt to read a block that's been trimmed until it's been rewritten with new contents.

Meanwhile, the SCSI command set communicates a subset of this information in the high order two bits of one of the bytes of the READ CAPACITY 16 response - the high order bit for TPE ("thin provisioning enabled"?) and the next one for "TPRZ" ("thin provisioning read zero"?).

Above this code, sd.c looks only for the TPE bit and ignores the TPRZ bit in implementing the DKIOC_CANFREE and DKIOCFREE ioctls.

However when we look at the code in common/io/sata/impl/sata.c's sata_txlt_read_capacity16(), which fills in the READ CAPACITY 16 response we find:

if (sdinfo->satadrv_id.ai_addsupported &
SATA_DETERMINISTIC_READ) {
if (sdinfo->satadrv_id.ai_addsupported &
SATA_READ_ZERO) {
rbuf[14] |= TPRZ;
} else {
rbuf[14] |= TPE;
}
}

This only sets TPE (which is what sd.c is looking for) if SATA_DETERMINISTIC_READ is set and SATA_READ_ZERO is clear!

I'd think that to meet ZFS's minimal expectations for TRIM, it should set TPE if the low order bit of ai_dsm is set, and then add in TPRZ only if all three bits (ai_dsm&1, SATA_DETERMINISTIC_READ, and SATA_READ_ZERO) are set, and when I tweak the code accordingly, zpool trim appears to be functional for the SATA-connected ssd's mentioned above.

Actions #1

Updated by Jerry Jelinek almost 2 years ago

  • Assignee set to Jerry Jelinek
Actions #2

Updated by Jerry Jelinek almost 2 years ago

I have re-tested zfs trim with this fix on the SATA SSD I currently have access to

ATA      INTEL SSDSC2BX01 1490.42 GiB   

This SSD already was working with zfs trim so my test just confirms no regression.

Bill Sommerfeld also tested on his SSDs which were exhibiting the problem and he confirms that this fix resolved the problem

Crucial MX500 SSD:

Fix works for me on my hardware (and otherwise looks good; feel free to list me as a code reviewer).

Actions #3

Updated by Electric Monk almost 2 years ago

  • Status changed from New to Closed
  • % Done changed from 0 to 100

git commit 8b35e52344673c75ba6a446ced1fb5c36b52a242

commit  8b35e52344673c75ba6a446ced1fb5c36b52a242
Author: Jerry Jelinek <jerry.jelinek@joyent.com>
Date:   2019-12-18T17:58:16.000Z

    12070 sata SSDs attached to sata ports can't trim
    Reviewed by: Dan McDonald <danmcd@joyent.com>
    Reviewed by: Bill Sommerfeld <sommerfeld@hamachi.org>
    Approved by: Robert Mustacchi <rm@fingolfin.org>

Actions

Also available in: Atom PDF