On Tue, Aug 22, 2023 at 11:06:22PM -0400, Zi Yan wrote:
> With sparsemem and without vmemmap, memmap (struct page) array might not be
> contiguous all the time. Thus, memmap position operations like page + N,
> page++, might not give a valid struct page. Use nth_page() to properly
> operate on struct page position changes.
This is too big to be a single patch; you need to break it up by
subsystem at least. And it's not against current -next; just the first
one I'm looking at is wrecked by "block: move the bi_size update out of
__bio_try_merge_page" from July 24th.
> +++ b/block/bio.c
> @@ -923,7 +923,7 @@ static inline bool page_is_mergeable(const struct bio_vec *bv,
> return true;
> else if (IS_ENABLED(CONFIG_KMSAN))
> return false;
> - return (bv->bv_page + bv_end / PAGE_SIZE) == (page + off / PAGE_SIZE);
> + return nth_page(bv->bv_page, bv_end / PAGE_SIZE) == nth_page(page, off / PAGE_SIZE);
I think this one is actually wrong. We already checked the addresses were
physically contiguous earlier in the function:
phys_addr_t vec_end_addr = page_to_phys(bv->bv_page) + bv_end - 1;
phys_addr_t page_addr = page_to_phys(page);
if (vec_end_addr + 1 != page_addr + off)
return false;
so this line is checking whether the struct pages are virtually contiguous.
That makes me suspicious of the other changes in the block layer,
because a bvec is defined to not cross a virtual discontiguity in
memmap.
> +++ b/fs/hfs/btree.c
> @@ -270,7 +270,7 @@ struct hfs_bnode *hfs_bmap_alloc(struct hfs_btree *tree)
> off = off16;
>
> off += node->page_offset;
> - pagep = node->page + (off >> PAGE_SHIFT);
> + pagep = nth_page(node->page, (off >> PAGE_SHIFT));
Are normal filesystems ever going to see folios that cross memmap
discontiguities? I think hugetlb is the only way to see such things.
> +++ b/mm/compaction.c
> @@ -362,7 +362,7 @@ __reset_isolation_pfn(struct zone *zone, unsigned long pfn, bool check_source,
> return true;
> }
>
> - page += (1 << PAGE_ALLOC_COSTLY_ORDER);
> + page = nth_page(page, (1 << PAGE_ALLOC_COSTLY_ORDER));
> } while (page <= end_page);
>
> return false;
Isn't this within a single page block?
> +++ b/mm/debug.c
> @@ -67,7 +67,7 @@ static void __dump_page(struct page *page)
> int mapcount;
> char *type = "";
>
> - if (page < head || (page >= head + MAX_ORDER_NR_PAGES)) {
> + if (page < head || (page >= nth_page(head, MAX_ORDER_NR_PAGES))) {
It's kind of right there in the name. MAX_ORDER_NR_PAGES.
