Re: [RFC PATCH] mm: use nth_page() for all memmap (struct page) position operations.

[Matthew Wilcox <willy@infradead.org>]

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.