Messages in this thread | | | Subject | Re: ubifs: fix page_count in ->ubifs_migrate_page() | From | zhangjun <> | Date | Fri, 14 Dec 2018 14:15:18 +0800 |
| |
On 2018/12/14 上午6:57, Dave Chinner wrote: > On Thu, Dec 13, 2018 at 03:23:37PM +0100, Richard Weinberger wrote: >> Hello zhangjun, >> >> thanks a lot for bringing this up! >> >> Am Mittwoch, 12. Dezember 2018, 15:13:57 CET schrieb zhangjun: >>> Because the PagePrivate() in UBIFS is different meanings, >>> alloc_cma() will fail when one dirty page cache located in >>> the type of MIGRATE_CMA >>> >>> If not adjust the 'extra_count' for dirty page, >>> ubifs_migrate_page() -> migrate_page_move_mapping() will >>> always return -EAGAIN for: >>> expected_count += page_has_private(page) >>> This causes the migration to fail until the page cache is cleaned >>> >>> In general, PagePrivate() indicates that buff_head is already bound >>> to this page, and at the same time page_count() will also increase. > > That's an invalid assumption. > > We should not be trying to infer what PagePrivate() means in code > that has no business using looking at it i.e. page->private is private > information for the owner of the page, and it's life cycle and > intent are unknown to anyone other than the page owner. > > e.g. on XFS, a page cache page's page->private /might/ contain a > struct iomap_page, or it might be NULL. Assigning a struct > iomap_page to the page does not change the reference count on the > page. IOWs, the page needs to be handled exactly the same > way by external code regardless of whether there is somethign > attached to page->private or not. > > Hence it looks to me like the migration code is making invalid > assumptions about PagePrivate inferring reference counts and so the > migration code needs to be fixed. Requiring filesystems to work > around invalid assumptions in the migration code is a sure recipe > for problems with random filesystems using page->private for their > own internal purposes.... > > Cheers, > > Dave. I agree with your main point of view, but for the buffer_head based file system this assumption is no problem, and the parameters and comments from the migrate_page_move_mapping() function: * 3 for pages with a mapping and PagePrivate/PagePrivate2 set. This assumption has been explained. Or to accurately say that the migrate system does not currently have a generic function for this case. Since you call the function implemented for buffer_head, you should follow its rules.
| |