* Re: [PATCH] nvme/scsi: Consider LBA format in IO splitting calculation
@ 2017-04-25 4:27 ` Damien Le Moal
0 siblings, 0 replies; 15+ 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] 15+ messages in thread
* [PATCH] nvme/scsi: Consider LBA format in IO splitting calculation
@ 2017-04-25 4:27 ` Damien Le Moal
0 siblings, 0 replies; 15+ messages in thread
From: Damien Le Moal @ 2017-04-25 4:27 UTC (permalink / raw)
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 at 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 at 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 at 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] 15+ 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
-1 siblings, 0 replies; 15+ 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] 15+ messages in thread
* [PATCH] nvme/scsi: Consider LBA format in IO splitting calculation
@ 2017-04-25 4:32 ` Jens Axboe
0 siblings, 0 replies; 15+ messages in thread
From: Jens Axboe @ 2017-04-25 4:32 UTC (permalink / raw)
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 at 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 at 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] 15+ 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
-1 siblings, 0 replies; 15+ 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
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=
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] nvme/scsi: Consider LBA format in IO splitting calculation
@ 2017-04-25 4:43 ` Damien Le Moal
0 siblings, 0 replies; 15+ 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] 15+ messages in thread
* [PATCH] nvme/scsi: Consider LBA format in IO splitting calculation
@ 2017-04-25 4:43 ` Damien Le Moal
0 siblings, 0 replies; 15+ messages in thread
From: Damien Le Moal @ 2017-04-25 4:43 UTC (permalink / raw)
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] 15+ messages in thread