On Sun, Aug 15, 2021 at 02:23:03PM +0800, Baolin Wang wrote:
> We've got the expected count for anonymous page or file page by
> expected_page_refs() at the beginning of migrate_page_move_mapping(),
> thus we should move the page count validation a little forward to
> reduce duplicated code.
>
> Moreover the i_pages lock is not used to guarantee the page refcount
> safety in migrate_page_move_mapping(), so we can move the getting page
> count out of the i_pages lock. Since now the migration page has
> established a migration pte under the page lock now, with the page
> refcount freezing validation, to ensure that the page references
> meet the migration requirement.
I remain unconvinced by this.
Looking at folio_migrate_mapping() a little more deeply, I don't
understand why we first check folio_ref_count() and then attempt
to free the refcount. Why not just try to freeze it directly?
ie instead of your patch, this:
+++ b/mm/migrate.c
@@ -403,13 +403,8 @@ int folio_migrate_mapping(struct address_space *mapping,
newzone = folio_zone(newfolio);
xas_lock_irq(&xas);
- if (folio_ref_count(folio) != expected_count ||
- xas_load(&xas) != folio) {
- xas_unlock_irq(&xas);
- return -EAGAIN;
- }
-
- if (!folio_ref_freeze(folio, expected_count)) {
+ if (xas_load(&xas) != folio ||
+ !folio_ref_freeze(folio, expected_count)) {
xas_unlock_irq(&xas);
return -EAGAIN;
}
And since we've got the lock on the page, how can somebody else be
removing it from the page cache? I think that xas_load() can be
removed too. So even more simply,
+++ b/mm/migrate.c
@@ -403,12 +403,6 @@ int folio_migrate_mapping(struct address_space *mapping,
newzone = folio_zone(newfolio);
xas_lock_irq(&xas);
- if (folio_ref_count(folio) != expected_count ||
- xas_load(&xas) != folio) {
- xas_unlock_irq(&xas);
- return -EAGAIN;
- }
-
if (!folio_ref_freeze(folio, expected_count)) {
xas_unlock_irq(&xas);
return -EAGAIN;
but I'm not really set up to test page migration. Does your test suite
test migrating file-backed pages?
