-
Notifications
You must be signed in to change notification settings - Fork 6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bluestore/NVMEDevice.cc: fix ceph_assert() when enable SPDK with 64KB kernel page size #24817
Conversation
d0484d1
to
beceb29
Compare
src/os/bluestore/NVMEDevice.cc
Outdated
@@ -99,7 +101,7 @@ class SharedDriverData { | |||
ctrlr(c), | |||
ns(ns_) { | |||
sector_size = spdk_nvme_ns_get_sector_size(ns); | |||
block_size = std::max(CEPH_PAGE_SIZE, sector_size); | |||
block_size = std::max<uint64_t>(CEPH_NVME_BLK_SIZE, sector_size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably we can just use the spdk_nvme_ns_get_extended_sector_size(ns)
for block size, because in this case we are performing i/o on an NVMe device, the block size is sector + meta data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tchaikov yes, we can assign block_size with the spdk_nvme_ns_get_extended_sector_size(ns), and remove sector_size parameter. Thanks for the comments. I will update the change soon.
beceb29
to
608f458
Compare
src/os/bluestore/NVMEDevice.cc
Outdated
block_size = std::max(CEPH_PAGE_SIZE, sector_size); | ||
size = ((uint64_t)sector_size) * spdk_nvme_ns_get_num_sectors(ns); | ||
block_size = spdk_nvme_ns_get_extended_sector_size(ns); | ||
size = block_size * spdk_nvme_ns_get_num_sectors(ns); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tone-zhang sorry for the confusion. i don't think we can calc the size using block_size * spdk_nvme_ns_get_num_sectors(ns);
, instead, we can either use
size = spdk_nvme_ns_get_size(ns)
or
size = sector_size * spdk_nvme_ns_get_num_sectors(ns);
as block size is the unit for addressing, which takes the overhead (metadata) into consideration, while the "size" here is the total size we can use for as data storage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I think spdk_nvme_ns_get_size(ns) is better. I will update the code.
… kernel page size When enable SPDK in Ceph with 64KB kernel page size, observed the ceph_assert() in NVMEDevice. In SPDK NVME driver, the block size should be the sector size of the NVMe device, not system page size. Get the correct sector size by SPDK API. This patch corrects the NVME block size and fixes the problem. Fixes: http://tracker.ceph.com/issues/36624 Signed-off-by: tone.zhang <tone.zhang@arm.com>
In SharedDriverData, the element sector_size is redundant. After pick up the latest verison of SPDK 18.07, the element block_size takes the same role, then remove sector_size. Signed-off-by: tone.zhang <tone.zhang@arm.com>
608f458
to
7529084
Compare
@tchaikov Kefu, thanks a lot! ;) |
When enable SPDK in Ceph with 64KB kernel page size, observed the
ceph_assert() in NVMEDevice.
This patch fixes the problem.
Fixes: http://tracker.ceph.com/issues/36624
Signed-off-by: tone.zhang tone.zhang@arm.com