nvme: set physical block size to value discovered in Identify Namespace

In old implementation physical block size exported by NVMe driver defaulted to logical block size. Existing NVMe SSDs very often
Musze naprawic zeby brac pod uwage relative performance.

---
 drivers/nvme/host/core.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

index 37046ac2c441..e20c5c6517fc 100644
@@ -1194,11 +1194,36 @@ static int nvme_revalidate_ns(struct nvme_ns *ns, struct nvme_id_ns **id)
 	return 0;
 }
 
+static u32 calc_pbs(struct nvme_id_ns *id)
+{
+	struct nvme_lbaf *lbaf_used;
+	int i, max_id = 0;
+	u8 ds = 0;
+
+	lbaf_used = &id->lbaf[id->flbas & NVME_NS_FLBAS_LBA_MASK];
+	if (lbaf_used->rp == NVME_LBAF_RP_BEST)
+		return (((u32)1) << lbaf_used->ds);
+
+	for (i = 0; i < id->nlbaf; i++) {
+		if (id->lbaf[i].ms == 0 && id->lbaf[i].ds > ds &&
+			id->lbaf[i].rp < lbaf_used->rp && id->lbaf[i].ds <= PAGE_SHIFT)
+			max_id = i;
+
+		ds = id->lbaf[i].ds;
+	}
+
+	if (id->lbaf[max_id].ds)
+		return (((u32)1) << id->lbaf[max_id].ds);
+	else
+		return 0;
+}
+
 static void __nvme_revalidate_disk(struct gendisk *disk, struct nvme_id_ns *id)
 {
 	struct nvme_ns *ns = disk->private_data;
 	struct nvme_ctrl *ctrl = ns->ctrl;
 	u16 bs;
+	u32 pbs;
 
 	/*
 	 * If identify namespace failed, use default 512 byte block size so

@@ -1208,6 +1233,9 @@ static void __nvme_revalidate_disk(struct gendisk *disk, struct nvme_id_ns *id)
 	if (ns->lba_shift == 0)
 		ns->lba_shift = 9;
 	bs = 1 << ns->lba_shift;
+	pbs = calc_pbs(id);
+	if (pbs == 0)
+		pbs = bs;
 	ns->noiob = le16_to_cpu(id->noiob);
 
 	blk_mq_freeze_queue(disk->queue);

@@ -1215,6 +1243,8 @@ static void __nvme_revalidate_disk(struct gendisk *disk, struct nvme_id_ns *id)
 	if (ctrl->ops->flags & NVME_F_METADATA_SUPPORTED)
 		nvme_prep_integrity(disk, id, bs);
 	blk_queue_logical_block_size(ns->queue, bs);
+	blk_queue_physical_block_size(ns->queue, pbs);
+
 	if (ns->noiob)
 		nvme_set_chunk_size(ns);
 	if (ns->ms && !blk_get_integrity(disk) && !ns->ext)

2.13.5

Dziekuje for the patch, but the change logs gotta be in English. :)

I think what you're wanting to say is:

The physical block size will default to the logical block size unless otherwise specified. While NVMe doesn't provide a way to discover the physical block size, the format with best relative performance is a good indicator as to the underlying block size.

{
	int i;
	struct nvme_lbaf *best = &id->lbaf[id->flbas & NVME_NS_FLBAS_LBA_MASK];

	for (i = 0; i < id->nlbaf; i++)
		if (id->lbaf[i].rp <= best->rp &&
				id->lbaf[i].ds > best->lbaf.ds &&
				id->lbaf[i].ds <= PAGE_SHIFT)
			best = &id->lbaf[i];

	return 1 << best->ds;
}

I don't like this at all. It's a really nasty guesswork. If you need this to get reasonable performance out of a specific device please quirk it.

NVMe doesn't have the legacy issues that require 4k physical sectors and 512 logical like ATA, so if a device really prefers 4k sectors it should skip with 4k format by default instead of doing the logical / physical dance. Especially that a "physical" blocksize that Linux could actually deal with (<= 4k) is a complete lie for flash anyway.

I don't think it's about "reasonable" performance; it's about getting extra relative performance. What else can the best performing LBAF indicate other than the device's preferred access alignment/granularity? The spec provides this hint, so it's not really a guess, but maybe there's a better way to make use of it instead of considering it to be the physical block size? io_opt?

On a slightly related topic, I think we should fix the consistency in what's reported in the queue's attributes after reformatting the namespace. Check out the following for what happens today:

Start with a 512b format:

# cat /sys/block/nvme0n1/queue/{minimum_io_size,logical_block_size,physical_block_size}
512
512
512

Format it to 4k:

# cat /sys/block/nvme0n1/queue/{minimum_io_size,logical_block_size,physical_block_size}
4096
4096
4096

Format it back to 512b:

# cat /sys/block/nvme0n1/queue/{minimum_io_size,logical_block_size,physical_block_size}
4096
512
4096

The first and last are the exact same NVMe format, but they're reported differently.

Keith,

It should really report io_min and not physical_block_size. pbs is a special case for spinning media.

I agree it's a bit weird to walk the format list as a heuristic. Would be nicer to have a smallest preferred I/O size in the protocol.

Yeah, we should set pbs/io_min as well.

Martin K. Petersen	Oracle Linux Engineering

What:           /sys/block/<disk>/queue/physical_block_size
Date:           May 2009
Contact:        Martin K. Petersen <martin.petersen at oracle.com>

Description:
This is the smallest unit a physical storage device can write atomically. It is usually the same as the logical block size but may be bigger. One example is SATA drives with 4KB sectors that expose a 512-byte logical block size to the operating system. For stacked block devices the physical_block_size variable contains the maximum physical_block_size of the component devices.

The best performing format certainly isn't related to that at all. If we'd really want to set a physical_block_size we'd have to based it on top of AWUN/AWUPF (although I still struggle how you define a global value based on LBAs if the device supports different LBA formats) or NAWUN/NABSN/NABSPF if supported.

The closest to what you seem to want above would be io_min:

What:           /sys/block/<disk>/queue/minimum_io_size
Date:           April 2009
Contact:        Martin K. Petersen <martin.petersen at oracle.com>

Description:
Storage devices may report a granularity or preferred minimum I/O size which is the smallest request the device can perform without incurring a performance penalty. For disk drives this is often the physical block size. For RAID arrays it is often the stripe chunk size. A properly aligned multiple of minimum_io_size is the preferred request size for workloads where a high number of I/O operations is desired.

I think we just need to opt into always getting the Namespace Attribute Notices AER and this should be taken care of automatically? I was going to send a patch for that for other reasons eventually.