* [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