Messages in this thread | | | Subject | Re: [f2fs-dev] [PATCH] f2fs: initialize page->private when using for our internal use | From | Chao Yu <> | Date | Tue, 6 Jul 2021 08:16:11 +0800 |
| |
On 2021/7/6 2:06, Jaegeuk Kim wrote: > On 07/06, Chao Yu wrote: >> On 2021/7/5 19:47, Matthew Wilcox wrote: >>> On Mon, Jul 05, 2021 at 07:33:35PM +0800, Chao Yu wrote: >>>> On 2021/7/5 16:56, Jaegeuk Kim wrote: >>>>> On 07/05, Chao Yu wrote: >>>>>> On 2021/7/5 13:22, Jaegeuk Kim wrote: >>>>>>> We need to guarantee it's initially zero. Otherwise, it'll hurt entire flag >>>>>>> operations. >>>>>> >>>>>> Oops, I didn't get the point, shouldn't .private be zero after page was >>>>>> just allocated by filesystem? What's the case we will encounter stall >>>>>> private data left in page? >>>>> >>>>> I'm seeing f2fs_migrate_page() has the newpage with some value without Private >>>>> flag. That causes a kernel panic later due to wrong private flag used in f2fs. >>>> >>>> I'm not familiar with that part of codes, so Cc mm mailing list for help. >>>> >>>> My question is newpage in .migrate_page() may contain non-zero value in .private >>>> field but w/o setting PagePrivate flag, is it a normal case? >>> >>> I think freshly allocated pages have a page->private of 0. ie this >>> code in mm/page_alloc.c: >>> >>> page = rmqueue(ac->preferred_zoneref->zone, zone, order, >>> gfp_mask, alloc_flags, ac->migratetype); >>> if (page) { >>> prep_new_page(page, order, gfp_mask, alloc_flags); >>> >>> where prep_new_page() calls post_alloc_hook() which contains: >>> set_page_private(page, 0); >>> >>> Now, I do see in __buffer_migrate_page() (mm/migrate.c): >>> >>> attach_page_private(newpage, detach_page_private(page)); >>> >>> but as far as I can tell, f2fs doesn't call any of the >>> buffer_migrate_page() paths. So I'm not sure why you're seeing >>> a non-zero page->private. >> >> Well, that's strange. >> >> Jaegeuk, let's add a BUGON in f2fs to track the call path where newpage >> has non-zero private value? if this issue is reproducible. > > We can debug anything tho, this issue is blocking the production, and I'd > like to get this in this merge windows. Could you please check the patch > has any holes?
The code looks good to me,
Reviewed-by: Chao Yu <chao@kernel.org>
Thanks,
> >> >> Thanks, >> >>>
| |