Re: configurable discard parameters

["Martin K. Petersen" <martin.petersen@oracle.com>]

>>>>> "Tom" == Tom Yan <tom.ty89@gmail.com> writes:

Tom> total untrimmed sectors: (310635 - 1 - 2410) / 32 = 9632 total
Tom> ranges: (43 GiB * (1024 ^ 3) / 512) / 65535 ~= 1376 average
Tom> untrimmed sectors per range: (9632 / 1376) = 7 = (65535 % 8)

Every type of drive has its own internal restrictions. Unless the drive
guarantees zero after TRIM it is perfectly normal for heads, tails or
random pieces in-between to be left untouched.

I am surprised that the common case of contiguous range entries was not
handled properly by your drive. Most of them deal with that just fine
(regardless of their internal granularity and alignment constraints). It
is normally only the partial block tracking between command invocations
that causes grief.

In any case. Unless discard_zeroes_data is 1 (and that requires
guarantees above and beyond what can be discovered via the ATA Command
Set) all bets are off wrt. dependable behavior.

The code below is an untested proof of concept to see what it would take
to alleviate your particular issue. I am, however, not at all convinced
that introducing such a change is worth the risk. I know of at least a
couple of devices that would blow up as a result of this patch...

-- 
Martin K. Petersen	Oracle Linux Engineering

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 3131adcc1f87..9c270a303f0b 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -2238,8 +2238,24 @@ static unsigned int ata_scsiop_inq_b0(struct ata_scsi_args *args, u8 *rbuf)
 	 * with the unmap bit set.
 	 */
 	if (ata_id_has_trim(args->id)) {
-		put_unaligned_be64(65535 * 512 / 8, &rbuf[36]);
-		put_unaligned_be32(1, &rbuf[28]);
+		/*
+		 * If the drive supports reliable zero after trim we set
+		 * the granularity to 1 and the blocks per range to
+		 * 0xffff. Otherwise we set set the granularity to 8 and
+		 * restrict the blocks per range to 0xfff8. This is done
+		 * to accommodate older drives which prefer 4K-alignment.
+		 */
+
+		if (ata_id_has_zero_after_trim(args->id) &&
+		    args->dev->horkage & ATA_HORKAGE_ZERO_AFTER_TRIM) {
+			put_unaligned_be64(ATA_MAX_TRIM_RANGE * 512 / 8,
+					   &rbuf[36]);
+			put_unaligned_be32(1, &rbuf[28]);
+		} else {
+			put_unaligned_be64(ATA_ALIGNED_TRIM_RANGE * 512 / 8,
+					   &rbuf[36]);
+			put_unaligned_be32(8, &rbuf[28]);
+		}
 	}
 
 	return 0;
@@ -3168,7 +3184,14 @@ static unsigned int ata_scsi_write_same_xlat(struct ata_queued_cmd *qc)
 		goto invalid_fld;
 
 	buf = page_address(sg_page(scsi_sglist(scmd)));
-	size = ata_set_lba_range_entries(buf, 512, block, n_block);
+
+	if (ata_id_has_zero_after_trim(dev->id) &&
+	    dev->horkage & ATA_HORKAGE_ZERO_AFTER_TRIM)
+		size = ata_set_lba_range_entries(buf, 512, block, n_block,
+						 ATA_MAX_TRIM_RANGE);
+	else
+		size = ata_set_lba_range_entries(buf, 512, block, n_block,
+						 ATA_ALIGNED_TRIM_RANGE);
 
 	if (ata_ncq_enabled(dev) && ata_fpdma_dsm_supported(dev)) {
 		/* Newer devices support queued TRIM commands */
diff --git a/include/linux/ata.h b/include/linux/ata.h
index fed36418dd1c..8a17fa22cdbe 100644
--- a/include/linux/ata.h
+++ b/include/linux/ata.h
@@ -47,6 +47,8 @@ enum {
 	ATA_MAX_SECTORS		= 256,
 	ATA_MAX_SECTORS_LBA48	= 65535,/* TODO: 65536? */
 	ATA_MAX_SECTORS_TAPE	= 65535,
+	ATA_MAX_TRIM_RANGE	= 0xffff,
+	ATA_ALIGNED_TRIM_RANGE	= 0xfff8,
 
 	ATA_ID_WORDS		= 256,
 	ATA_ID_CONFIG		= 0,
@@ -1012,19 +1014,20 @@ static inline void ata_id_to_hd_driveid(u16 *id)
  * TO NV CACHE PINNED SET.
  */
 static inline unsigned ata_set_lba_range_entries(void *_buffer,
-		unsigned buf_size, u64 sector, unsigned long count)
+		unsigned buf_size, u64 sector, unsigned long count,
+		u16 bpe)
 {
 	__le64 *buffer = _buffer;
 	unsigned i = 0, used_bytes;
 
 	while (i < buf_size / 8 ) { /* 6-byte LBA + 2-byte range per entry */
 		u64 entry = sector |
-			((u64)(count > 0xffff ? 0xffff : count) << 48);
+			((u64)(count > bpe ? bpe : count) << 48);
 		buffer[i++] = __cpu_to_le64(entry);
-		if (count <= 0xffff)
+		if (count <= bpe)
 			break;
-		count -= 0xffff;
-		sector += 0xffff;
+		count -= bpe;
+		sector += bpe;
 	}
 
 	used_bytes = ALIGN(i * 8, 512);