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:
signature.asc
Description: OpenPGP digital signature
