Skip to contentSkip to conversation

nvme/scsi: Consider LBA format in IO splitting calculation

PATCH Applied Jon Derrick 4 participants 6 messages Apr 25, 2017

Subsystems (3)
SCSI Block layer NVMe
lore
Share & tools
lore

Jon Derrick

author
#0 lore raw

PATCH 1/1 nvme/scsi: Consider LBA format in IO splitting calculation

The current command submission code uses a sector-based value when considering the maximum number of blocks per command. With a 4k-formatted namespace and a command exceeding max hardware limits, this calculation doesn't split IOs which should be split and fails in the nvme layer. This patch fixes that calculation and enables IO splitting in these circumstances.

Signed-off-by Jon Derrick <jonathan.derrick at intel.com>
---
 drivers/nvme/host/scsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/nvme/host/scsi.c +1 -1
index f49ae27..988da61 100644
@@ -1609,7 +1609,7 @@ static int nvme_trans_do_nvme_io(struct nvme_ns *ns, struct sg_io_hdr *hdr,
 	struct nvme_command c;
 	u8 opcode = (is_write ? nvme_cmd_write : nvme_cmd_read);
 	u16 control;
-	u32 max_blocks = queue_max_hw_sectors(ns->queue);
+	u32 max_blocks = queue_max_hw_sectors(ns->queue) >> (ns->lba_shift - 9);
 
 	num_cmds = nvme_trans_io_get_num_cmds(hdr, cdb_info, max_blocks);
Signature · 1 line
2.9.3
maintainer Reviewed-by on v1 1/1
#1 lore raw
Quoted 17 lines · Jon Derrick

The current command submission code uses a sector-based value when considering the maximum number of blocks per command. With a 4k-formatted namespace and a command exceeding max hardware limits, this calculation doesn't split IOs which should be split and fails in the nvme layer. This patch fixes that calculation and enables IO splitting in these circumstances.

Signed-off-by Jon Derrick <[email protected]>
---
 drivers/nvme/host/scsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/nvme/host/scsi.c +0 -0
index f49ae27..988da61 100644

drivers/nvme/host/scsi.c static int nvme_trans_do_nvme_io(struct nvme_ns *ns, struct sg_io_hdr *hdr, · line 1614

Quoted 7 lines
drivers/nvme/host/scsi.c +1 -1
@@ -1609,7 +1609,7 @@ static int nvme_trans_do_nvme_io(struct nvme_ns *ns, struct sg_io_hdr *hdr,
 	struct nvme_command c;
 	u8 opcode = (is_write ? nvme_cmd_write : nvme_cmd_read);
 	u16 control;
-	u32 max_blocks = queue_max_hw_sectors(ns->queue);
+	u32 max_blocks = queue_max_hw_sectors(ns->queue) >> (ns->lba_shift - 9);
 
 	num_cmds = nvme_trans_io_get_num_cmds(hdr, cdb_info, max_blocks);

Patch looks correct to me, as we always consider the hw sectors settings in units of 512b blocks.

Reviewed-by Jens Axboe <[email protected]>
Signature · 1 line
Jens Axboe
#2 lore raw

Sm9uLAoKT24gNC8yNS8xNyAxMzowMCwgSmVucyBBeGJvZSB3cm90ZToKPiBPbiBNb24sIEFwciAy NCAyMDE3LCBKb24gRGVycmljayB3cm90ZToKPj4gVGhlIGN1cnJlbnQgY29tbWFuZCBzdWJtaXNz aW9uIGNvZGUgdXNlcyBhIHNlY3Rvci1iYXNlZCB2YWx1ZSB3aGVuCj4+IGNvbnNpZGVyaW5nIHRo ZSBtYXhpbXVtIG51bWJlciBvZiBibG9ja3MgcGVyIGNvbW1hbmQuIFdpdGggYQo+PiA0ay1mb3Jt YXR0ZWQgbmFtZXNwYWNlIGFuZCBhIGNvbW1hbmQgZXhjZWVkaW5nIG1heCBoYXJkd2FyZSBsaW1p dHMsIHRoaXMKPj4gY2FsY3VsYXRpb24gZG9lc24ndCBzcGxpdCBJT3Mgd2hpY2ggc2hvdWxkIGJl IHNwbGl0IGFuZCBmYWlscyBpbiB0aGUKPj4gbnZtZSBsYXllci4gVGhpcyBwYXRjaCBmaXhlcyB0 aGF0IGNhbGN1bGF0aW9uIGFuZCBlbmFibGVzIElPIHNwbGl0dGluZwo+PiBpbiB0aGVzZSBjaXJj dW1zdGFuY2VzLgo+Pgo+PiBTaWduZWQtb2ZmLWJ5OiBKb24gRGVycmljayA8am9uYXRoYW4uZGVy cmlja0BpbnRlbC5jb20+Cj4+IC0tLQo+PiAgZHJpdmVycy9udm1lL2hvc3Qvc2NzaS5jIHwgMiAr LQo+PiAgMSBmaWxlIGNoYW5nZWQsIDEgaW5zZXJ0aW9uKCspLCAxIGRlbGV0aW9uKC0pCj4+Cj4+ IGRpZmYgLS1naXQgYS9kcml2ZXJzL252bWUvaG9zdC9zY3NpLmMgYi9kcml2ZXJzL252bWUvaG9z dC9zY3NpLmMKPj4gaW5kZXggZjQ5YWUyNy4uOTg4ZGE2MSAxMDA2NDQKPj4gLS0tIGEvZHJpdmVy cy9udm1lL2hvc3Qvc2NzaS5jCj4+ICsrKyBiL2RyaXZlcnMvbnZtZS9ob3N0L3Njc2kuYwo+PiBA QCAtMTYwOSw3ICsxNjA5LDcgQEAgc3RhdGljIGludCBudm1lX3RyYW5zX2RvX252bWVfaW8oc3Ry dWN0IG52bWVfbnMgKm5zLCBzdHJ1Y3Qgc2dfaW9faGRyICpoZHIsCj4+ICAJc3RydWN0IG52bWVf Y29tbWFuZCBjOwo+PiAgCXU4IG9wY29kZSA9IChpc193cml0ZSA/IG52bWVfY21kX3dyaXRlIDog bnZtZV9jbWRfcmVhZCk7Cj4+ICAJdTE2IGNvbnRyb2w7Cj4+IC0JdTMyIG1heF9ibG9ja3MgPSBx dWV1ZV9tYXhfaHdfc2VjdG9ycyhucy0+cXVldWUpOwo+PiArCXUzMiBtYXhfYmxvY2tzID0gcXVl dWVfbWF4X2h3X3NlY3RvcnMobnMtPnF1ZXVlKSA+PiAobnMtPmxiYV9zaGlmdCAtIDkpOwo+PiAg Cj4+ICAJbnVtX2NtZHMgPSBudm1lX3RyYW5zX2lvX2dldF9udW1fY21kcyhoZHIsIGNkYl9pbmZv LCBtYXhfYmxvY2tzKTsKPiAKPiBQYXRjaCBsb29rcyBjb3JyZWN0IHRvIG1lLCBhcyB3ZSBhbHdh eXMgY29uc2lkZXIgdGhlIGh3IHNlY3RvcnMgc2V0dGluZ3MKPiBpbiB1bml0cyBvZiA1MTJiIGJs b2Nrcy4KPiAKPiBSZXZpZXdlZC1ieTogSmVucyBBeGJvZSA8YXhib2VAZmIuY29tPgoKTWF5IGJl IHJlcGxhY2UgOSB3aXRoIFNFQ1RPUl9TSElGVCA/CgpKZW5zLAoKSSBqdXN0IHJlYWxpemVkIHRo YXQgdGhpcyBtYWNybyBpcyBkZWZpbmVkIGluIGxpbnV4L2RldmljZS1tYXBwZXIuaCwKd2hpY2gg ZG9lcyBub3Qgc2VlbSBsaWtlIHRvIGJlc3QgcGxhY2UgdG8gaGF2ZSBpdC4gV2h5IG5vdCBibGtk ZXYuaCA/CkFueSBwYXJ0aWN1bGFyIHJlYXNvbiA/IFRoaXMgbGVhZHMgdG8gc29tZSBzdHJhbmdl IGluY2x1ZGUgZGVwZW5kZW5jaWVzLApsaWtlIG1hbnkgbmZzL2Jsb2NrbGF5b3V0LyBmaWxlcyBp bmNsdWRpbmcgZGV2aWNlLW1hcHBlci5oIGp1c3QgdG8gZ2V0CnRoYXQgZGVmaW5pdGlvbi4KCkJl c3QgcmVnYXJkcy4KCi0tIApEYW1pZW4gTGUgTW9hbCwgUGguRC4KU3IuIE1hbmFnZXIsIFN5c3Rl bSBTb2Z0d2FyZSBSZXNlYXJjaCBHcm91cCwKV2VzdGVybiBEaWdpdGFsIENvcnBvcmF0aW9uCkRh bWllbi5MZU1vYWxAd2RjLmNvbQooKzgxKSAwNDY2LTk4LTM1OTMgKGV4dC4gNTEzNTkzKQoxIGtp cmloYXJhLWNobywgRnVqaXNhd2EsCkthbmFnYXdhLCAyNTItMDg4OCBKYXBhbgp3d3cud2RjLmNv bSwgd3d3Lmhnc3QuY29tCldlc3Rlcm4gRGlnaXRhbCBDb3Jwb3JhdGlvbiAoYW5kIGl0cyBzdWJz aWRpYXJpZXMpIEUtbWFpbCBDb25maWRlbnRpYWxpdHkgTm90aWNlICYgRGlzY2xhaW1lcjoKClRo aXMgZS1tYWlsIGFuZCBhbnkgZmlsZXMgdHJhbnNtaXR0ZWQgd2l0aCBpdCBtYXkgY29udGFpbiBj b25maWRlbnRpYWwgb3IgbGVnYWxseSBwcml2aWxlZ2VkIGluZm9ybWF0aW9uIG9mIFdEQyBhbmQv b3IgaXRzIGFmZmlsaWF0ZXMsIGFuZCBhcmUgaW50ZW5kZWQgc29sZWx5IGZvciB0aGUgdXNlIG9m IHRoZSBpbmRpdmlkdWFsIG9yIGVudGl0eSB0byB3aGljaCB0aGV5IGFyZSBhZGRyZXNzZWQuIElm IHlvdSBhcmUgbm90IHRoZSBpbnRlbmRlZCByZWNpcGllbnQsIGFueSBkaXNjbG9zdXJlLCBjb3B5 aW5nLCBkaXN0cmlidXRpb24gb3IgYW55IGFjdGlvbiB0YWtlbiBvciBvbWl0dGVkIHRvIGJlIHRh a2VuIGluIHJlbGlhbmNlIG9uIGl0LCBpcyBwcm9oaWJpdGVkLiBJZiB5b3UgaGF2ZSByZWNlaXZl ZCB0aGlzIGUtbWFpbCBpbiBlcnJvciwgcGxlYXNlIG5vdGlmeSB0aGUgc2VuZGVyIGltbWVkaWF0 ZWx5IGFuZCBkZWxldGUgdGhlIGUtbWFpbCBpbiBpdHMgZW50aXJldHkgZnJvbSB5b3VyIHN5c3Rl bS4K

maintainer
#3 lore raw
Quoted 36 lines · Damien Le Moal

Jon,

Quoted 24 lines · Jens Axboe
Quoted 23 lines · Jon Derrick

The current command submission code uses a sector-based value when considering the maximum number of blocks per command. With a 4k-formatted namespace and a command exceeding max hardware limits, this calculation doesn't split IOs which should be split and fails in the nvme layer. This patch fixes that calculation and enables IO splitting in these circumstances.

Signed-off-by Jon Derrick <[email protected]>
---
 drivers/nvme/host/scsi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/nvme/host/scsi.c +1 -1
index f49ae27..988da61 100644
@@ -1609,7 +1609,7 @@ static int nvme_trans_do_nvme_io(struct nvme_ns *ns, struct sg_io_hdr *hdr,
 	struct nvme_command c;
 	u8 opcode = (is_write ? nvme_cmd_write : nvme_cmd_read);
 	u16 control;
-	u32 max_blocks = queue_max_hw_sectors(ns->queue);
+	u32 max_blocks = queue_max_hw_sectors(ns->queue) >> (ns->lba_shift - 9);
Quoted 6 lines

drivers/nvme/host/scsi.c static int nvme_trans_do_nvme_io(struct nvme_ns *ns, struct sg_io_hdr *hdr, · line 1614

Quoted 1 line
drivers/nvme/host/scsi.c +0 -0
 	num_cmds = nvme_trans_io_get_num_cmds(hdr, cdb_info, max_blocks);

Patch looks correct to me, as we always consider the hw sectors settings in units of 512b blocks.

Reviewed-by Jens Axboe <[email protected]>

May be replace 9 with SECTOR_SHIFT ?

Quoted 7 lines

Jens,

I just realized that this macro is defined in linux/device-mapper.h, which does not seem like to best place to have it. Why not blkdev.h ? Any particular reason ? This leads to some strange include dependencies, like many nfs/blocklayout/ files including device-mapper.h just to get that definition.

I'm fine with moving it and using it everywhere. Right now we don't in the block core at all, 9 is always hard coded. While that change would be fine, it should be done independently of this patch, which is a real bug fix.

Signature · 1 line
Jens Axboe
#4 lore raw

SmVucywKCk9uIDQvMjUvMTcgMTM6MzIsIEplbnMgQXhib2Ugd3JvdGU6Cj4+IEkganVzdCByZWFs aXplZCB0aGF0IHRoaXMgbWFjcm8gaXMgZGVmaW5lZCBpbiBsaW51eC9kZXZpY2UtbWFwcGVyLmgs Cj4+IHdoaWNoIGRvZXMgbm90IHNlZW0gbGlrZSB0byBiZXN0IHBsYWNlIHRvIGhhdmUgaXQuIFdo eSBub3QgYmxrZGV2LmggPwo+PiBBbnkgcGFydGljdWxhciByZWFzb24gPyBUaGlzIGxlYWRzIHRv IHNvbWUgc3RyYW5nZSBpbmNsdWRlIGRlcGVuZGVuY2llcywKPj4gbGlrZSBtYW55IG5mcy9ibG9j a2xheW91dC8gZmlsZXMgaW5jbHVkaW5nIGRldmljZS1tYXBwZXIuaCBqdXN0IHRvIGdldAo+PiB0 aGF0IGRlZmluaXRpb24uCj4gCj4gSSdtIGZpbmUgd2l0aCBtb3ZpbmcgaXQgYW5kIHVzaW5nIGl0 IGV2ZXJ5d2hlcmUuIFJpZ2h0IG5vdyB3ZSBkb24ndCBpbgo+IHRoZSBibG9jayBjb3JlIGF0IGFs bCwgOSBpcyBhbHdheXMgaGFyZCBjb2RlZC4gV2hpbGUgdGhhdCBjaGFuZ2Ugd291bGQKPiBiZSBm aW5lLCBpdCBzaG91bGQgYmUgZG9uZSBpbmRlcGVuZGVudGx5IG9mIHRoaXMgcGF0Y2gsIHdoaWNo IGlzIGEgcmVhbAo+IGJ1ZyBmaXguCgpUaGFuayB5b3UgZm9yIHRoZSBjbGFyaWZpY2F0aW9uLiBJ IHdpbGwgdHJ5IHRvIHNlbmQgc29tZXRoaW5nIGxhdGVyLgoKQmVzdCByZWdhcmRzLgoKLS0gCkRh bWllbiBMZSBNb2FsLApXZXN0ZXJuIERpZ2l0YWwKV2VzdGVybiBEaWdpdGFsIENvcnBvcmF0aW9u IChhbmQgaXRzIHN1YnNpZGlhcmllcykgRS1tYWlsIENvbmZpZGVudGlhbGl0eSBOb3RpY2UgJiBE aXNjbGFpbWVyOgoKVGhpcyBlLW1haWwgYW5kIGFueSBmaWxlcyB0cmFuc21pdHRlZCB3aXRoIGl0 IG1heSBjb250YWluIGNvbmZpZGVudGlhbCBvciBsZWdhbGx5IHByaXZpbGVnZWQgaW5mb3JtYXRp b24gb2YgV0RDIGFuZC9vciBpdHMgYWZmaWxpYXRlcywgYW5kIGFyZSBpbnRlbmRlZCBzb2xlbHkg Zm9yIHRoZSB1c2Ugb2YgdGhlIGluZGl2aWR1YWwgb3IgZW50aXR5IHRvIHdoaWNoIHRoZXkgYXJl IGFkZHJlc3NlZC4gSWYgeW91IGFyZSBub3QgdGhlIGludGVuZGVkIHJlY2lwaWVudCwgYW55IGRp c2Nsb3N1cmUsIGNvcHlpbmcsIGRpc3RyaWJ1dGlvbiBvciBhbnkgYWN0aW9uIHRha2VuIG9yIG9t aXR0ZWQgdG8gYmUgdGFrZW4gaW4gcmVsaWFuY2Ugb24gaXQsIGlzIHByb2hpYml0ZWQuIElmIHlv dSBoYXZlIHJlY2VpdmVkIHRoaXMgZS1tYWlsIGluIGVycm9yLCBwbGVhc2Ugbm90aWZ5IHRoZSBz ZW5kZXIgaW1tZWRpYXRlbHkgYW5kIGRlbGV0ZSB0aGUgZS1tYWlsIGluIGl0cyBlbnRpcmV0eSBm cm9tIHlvdXIgc3lzdGVtLgo=