* [PATCH] nvme/scsi: Consider LBA format in IO splitting calculation @ 2017-04-25 0:02 Jon Derrick 2017-04-25 4:00 ` Jens Axboe 2017-04-25 17:57 ` Christoph Hellwig 0 siblings, 2 replies; 6+ messages in thread From: Jon Derrick @ 2017-04-25 0:02 UTC (permalink / raw) To: axboe Cc: linux-nvme, linux-scsi, linux-block, keith.busch, hch, sagi, 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 <jonathan.derrick@intel.com> --- drivers/nvme/host/scsi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/nvme/host/scsi.c b/drivers/nvme/host/scsi.c index f49ae27..988da61 100644 --- a/drivers/nvme/host/scsi.c +++ b/drivers/nvme/host/scsi.c @@ -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); -- 2.9.3 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] nvme/scsi: Consider LBA format in IO splitting calculation 2017-04-25 0:02 [PATCH] nvme/scsi: Consider LBA format in IO splitting calculation Jon Derrick @ 2017-04-25 4:00 ` Jens Axboe 2017-04-25 4:27 ` Damien Le Moal 2017-04-25 17:57 ` Christoph Hellwig 1 sibling, 1 reply; 6+ messages in thread From: Jens Axboe @ 2017-04-25 4:00 UTC (permalink / raw) To: Jon Derrick; +Cc: linux-nvme, linux-scsi, linux-block, keith.busch, hch, sagi On Mon, Apr 24 2017, Jon Derrick wrote: > 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@intel.com> > --- > drivers/nvme/host/scsi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/nvme/host/scsi.c b/drivers/nvme/host/scsi.c > index f49ae27..988da61 100644 > --- a/drivers/nvme/host/scsi.c > +++ b/drivers/nvme/host/scsi.c > @@ -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 <axboe@fb.com> -- Jens Axboe ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] nvme/scsi: Consider LBA format in IO splitting calculation 2017-04-25 4:00 ` Jens Axboe @ 2017-04-25 4:27 ` Damien Le Moal 2017-04-25 4:32 ` Jens Axboe 0 siblings, 1 reply; 6+ messages in thread From: Damien Le Moal @ 2017-04-25 4:27 UTC (permalink / raw) To: Jens Axboe, Jon Derrick Cc: linux-nvme, linux-scsi, linux-block, keith.busch, hch, sagi Jon, On 4/25/17 13:00, Jens Axboe wrote: > On Mon, Apr 24 2017, Jon Derrick wrote: >> 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@intel.com> >> --- >> drivers/nvme/host/scsi.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/nvme/host/scsi.c b/drivers/nvme/host/scsi.c >> index f49ae27..988da61 100644 >> --- a/drivers/nvme/host/scsi.c >> +++ b/drivers/nvme/host/scsi.c >> @@ -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 <axboe@fb.com> May be replace 9 with SECTOR_SHIFT ? 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. Best regards. -- Damien Le Moal, Ph.D. Sr. Manager, System Software Research Group, Western Digital Corporation Damien.LeMoal@wdc.com (+81) 0466-98-3593 (ext. 513593) 1 kirihara-cho, Fujisawa, Kanagawa, 252-0888 Japan www.wdc.com, www.hgst.com ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] nvme/scsi: Consider LBA format in IO splitting calculation 2017-04-25 4:27 ` Damien Le Moal @ 2017-04-25 4:32 ` Jens Axboe 2017-04-25 4:43 ` Damien Le Moal 0 siblings, 1 reply; 6+ messages in thread From: Jens Axboe @ 2017-04-25 4:32 UTC (permalink / raw) To: Damien Le Moal, Jon Derrick Cc: linux-nvme, linux-scsi, linux-block, keith.busch, hch, sagi On 04/24/2017 09:27 PM, Damien Le Moal wrote: > Jon, > > On 4/25/17 13:00, Jens Axboe wrote: >> On Mon, Apr 24 2017, Jon Derrick wrote: >>> 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@intel.com> >>> --- >>> drivers/nvme/host/scsi.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/nvme/host/scsi.c b/drivers/nvme/host/scsi.c >>> index f49ae27..988da61 100644 >>> --- a/drivers/nvme/host/scsi.c >>> +++ b/drivers/nvme/host/scsi.c >>> @@ -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 <axboe@fb.com> > > May be replace 9 with SECTOR_SHIFT ? > > 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. -- Jens Axboe ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] nvme/scsi: Consider LBA format in IO splitting calculation 2017-04-25 4:32 ` Jens Axboe @ 2017-04-25 4:43 ` Damien Le Moal 0 siblings, 0 replies; 6+ messages in thread From: Damien Le Moal @ 2017-04-25 4:43 UTC (permalink / raw) To: Jens Axboe, Jon Derrick Cc: linux-nvme, linux-scsi, linux-block, keith.busch, hch, sagi Jens, On 4/25/17 13:32, Jens Axboe wrote: >> 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. Thank you for the clarification. I will try to send something later. Best regards. -- Damien Le Moal, Western Digital ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] nvme/scsi: Consider LBA format in IO splitting calculation 2017-04-25 0:02 [PATCH] nvme/scsi: Consider LBA format in IO splitting calculation Jon Derrick 2017-04-25 4:00 ` Jens Axboe @ 2017-04-25 17:57 ` Christoph Hellwig 1 sibling, 0 replies; 6+ messages in thread From: Christoph Hellwig @ 2017-04-25 17:57 UTC (permalink / raw) To: Jon Derrick Cc: axboe, keith.busch, sagi, linux-scsi, hch, linux-nvme, linux-block Applied to nvme-4.12. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-04-25 17:58 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-04-25 0:02 [PATCH] nvme/scsi: Consider LBA format in IO splitting calculation Jon Derrick 2017-04-25 4:00 ` Jens Axboe 2017-04-25 4:27 ` Damien Le Moal 2017-04-25 4:32 ` Jens Axboe 2017-04-25 4:43 ` Damien Le Moal 2017-04-25 17:57 ` Christoph Hellwig
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox