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

[Zi Yan <ziy@nvidia.com>]

[-- Attachment #1: Type: text/plain, Size: 3275 bytes --]

On 22 Aug 2023, at 23:27, Matthew Wilcox wrote:

> 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.

Sure. Will break up the patch and rebase it against -next.

>
>> +++ 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.

Got it.

>
> 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.

Yes, just checked the definition of struct bio_vec and confirmed it. I will drop
the changes to block layer.

>> +++ 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.

Right. So most likely only mm code can see hugetlb would need nth_page() instead
of direct struct page offset operations.

>
>> +++ 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.

I was trying to be on the safe side. I also get your point that I probably should
only convert only if necessary. I will check my changes and drop unnecessary ones.

Thank you for the review.

--
Best Regards,
Yan, Zi

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 854 bytes --]