Skip to content

fix page size - #123

Closed
James23rc wants to merge 1 commit into
MoatLab:masterfrom
James23rc:master
Closed

fix page size#123
James23rc wants to merge 1 commit into
MoatLab:masterfrom
James23rc:master

Conversation

@James23rc

Copy link
Copy Markdown

Size of a page is 4096 bytes, which is 8 * 512 bytes, not 4096 * 512 bytes(which is 2MB).

@James23rc

Copy link
Copy Markdown
Author

@huaicheng

@inhoinno

inhoinno commented Oct 14, 2023

Copy link
Copy Markdown
Contributor

Hello James23rc,
Thanks for your valid point out. I acknowledged your commit.

However, I have little worries about hardcoding the numbers such as /4096, /8. (I know /4096 things are we did, but still)
So I'm considering not to take your commit, at the moment.
Please see the reason below.

Size of the page in SSD is also a configurable parameter(Scalability)
and hardcoding such as ( lba / 8 ) hinders understanding the code.(Code visibility)

Using macro something like this is better, this is just an example

#define _4KB 4096 
#define LBA_TO_LPN (SSD_PAGE_SIZE / HOST_LBA_SIZE)

Again, appreciate for your commit, we will reflect your point soon :)

@huaicheng

Copy link
Copy Markdown
Contributor

Thanks, and apologies this sat so long.

You were right about the bug: zns_zone_idx() takes an LBA, so multiplying an LPN by 4096 rather than by the sectors-per-page was wrong, as was dividing an LBA by 4096 to get an LPN.

The ZNS datapath has since been reworked and the functions this patches (lpn_to_ppa, advance_read_pointer) no longer exist. The conversion is now derived as LOGICAL_PAGE_SIZE / lbasz rather than a hardcoded constant, so it is correct for any configured block size instead of only 512 B — the same bug, fixed more generally.

@huaicheng

Copy link
Copy Markdown
Contributor

Thanks for catching this — your diagnosis was right, an LBA-to-page conversion was using 4096 where the page is 8 sectors of 512 B.

That code has since been rewritten: zns_zone_idx() is now called with the LBA directly and the sizes come from lbasz = 1 << zns_ns_lbads(ns) rather than a hardcoded constant, so the lpn * 4096 and slba / 4096 lines this PR targeted no longer exist on master. The branch therefore conflicts and I am closing it, but the bug you reported is gone.

Sorry it sat here so long.

@huaicheng huaicheng closed this Aug 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants